[PATCH] HTTP: Support mixed addr configuration for PROXY protocol in RealIP module

[Maxim Dounin]

Hello!

On Thu, Feb 02, 2023 at 11:06:25PM +0100, Ryan Lahfa wrote:

> # HG changeset patch
> # User Ryan Lahfa <masterancpp at gmail.com>
> # Date 1675214187 -3600
> #      Wed Feb 01 02:16:27 2023 +0100
> # Node ID 53cf9a05e1ae1535166f45582eb4bf5aa34c23ea
> # Parent  106328a70f4ecb32f828d33e5cd66c861e455f92
> HTTP: Support mixed addr configuration for PROXY protocol in RealIP module
> 
> This ensures that under `real_ip_header proxy_protocol`, if a request is
> received on a `listen` block which do not contain `proxy_protocol`, it
> is not rejected by NGINX.

Such requests are not currently rejected by nginx.  Rather, the 
request is ignored by the realip module (which returns 
NGX_DECLINED).

For example, for the following configuration:

    server {
        listen 8080;
        listen 8081 proxy_protocol;

        set_real_ip_from 127.0.0.1/32;
        real_ip_header proxy_protocol;

        return 200 "remote_addr: $remote_addr\n";
    }

will accept requests on port 8080 without PROXY protocol, and on 
port 8081 with PROXY protocol.  Requests on port 8081 will be 
handled by the realip module, and the remote address will be set 
to the one obtained via PROXY protocol (as long as allowed by 
set_real_ip_from).  Requests on port 8080 will be handled without 
additional processing by the realip module.

Could you please clarify what are you trying to fix here?

> 
> This enables the usecase where you want clients to let them access
> directly without going through a "load balancer" or a "proxy" while
> having PROXY protocol for the other clients.
> 
> This do not introduce vulnerability per se, non-PROXY requests
> unsets all the RealIP module context when `real_ip_header` is set to proxy protocol.
> This is akin to not having RealIP module working for that HTTP request.
> 
> diff -r 106328a70f4e -r 53cf9a05e1ae src/http/modules/ngx_http_realip_module.c
> --- a/src/http/modules/ngx_http_realip_module.c	Sat Jan 28 01:29:45 2023 +0300
> +++ b/src/http/modules/ngx_http_realip_module.c	Wed Feb 01 02:16:27 2023 +0100
> @@ -179,10 +179,24 @@
>  
>      case NGX_HTTP_REALIP_PROXY:
>  
> +        // If the address configuration is not PROXY protocol-enabled
> +        // We can ignore this request.
> +        // http_connection is guaranteed to exist as we are
> +        // in a HTTP context.
> +        if (!r->http_connection->addr_conf->proxy_protocol) {
> +            // Unset context to have valid $remote_addr, $remote_port vars.
> +            ngx_http_set_ctx(r, NULL, ngx_http_realip_module);
> +            return NGX_OK;
> +        }
> +
> +        // We are supposed to receive a PROXY protocol-enabled request
> +        // and this is not the case.
> +        // We reject this request.
>          if (r->connection->proxy_protocol == NULL) {
>              return NGX_DECLINED;
>          }

It looks like these changes essentially do nothing, see above.

>  
> +        // This value is now guaranteed to exist.
>          value = &r->connection->proxy_protocol->src_addr;
>          xfwd = NULL;
>  
> @@ -242,6 +256,8 @@
>          return ngx_http_realip_set_addr(r, &addr);
>      }
>  
> +    ngx_log_error(NGX_LOG_ALERT, r->connection->log, 0,
> +             "'%V' is not an allowed proxy, declining request", &c->addr_text);

And that's certainly an incorrect change.  It is quite normal that 
for some requests the realip module returns NGX_DECLINED, since 
this essentially means that the realip module is not configured to 
work with such requests.  There is no need to spam logs with 
alerts about it.

>      return NGX_DECLINED;
>  }
>  

Please also not that it's usually a good idea to follow nginx 
style in patches you submit.  See 
http://nginx.org/en/docs/contributing_changes.html for some basic 
tips.

-- 
Maxim Dounin
http://mdounin.ru/