[PATCH] Detecting duplicate addresses in listen among different modules

[Roman Arutyunyan]

Hi

> On 6 Apr 2024, at 12:32 AM, Sergey Kandaurov <pluknet at nginx.com> wrote:
> 
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1712349104 -14400
> #      Sat Apr 06 00:31:44 2024 +0400
> # Node ID 5fe21225ab3105aeea5e381a5d39ec1d3cfc04e6
> # Parent  47df39ea9e21c4359e0ca96dcc452f2bc8a82c44
> Detecting duplicate addresses in listen among different modules.
> 
> This is used to gracefully reject the following configurations during
> syntax checking with a "duplicate ... address and port pair" error:
> 
>    http {
>        server {
>            listen  127.0.0.1:8080;
>            ...
>        }
>    }
> 
>    stream {
>        server {
>            listen  127.0.0.1:8080;
>            ...
>        }
>    }
> 
> Also for wildcard addresses:
> 
>    stream {
>        server {
>            listen  8080;
>            ...
>        }
>    }
> 
>    mail {
>        server {
>            listen  8080;
>            ...
>        }
>    }
> 
> Notably, this won't catch mixed wildcard and non-wildcard addresses as
> follows, where http server block with a specific address never accepts
> new connections.

Not sure what you mean mean by 'never accepts', but new connections surely
will be accepted by this server, just not directly. You probably mean there's
no designated listen socket for that address.

>  The reason is that http will end up listening on the
> wildcard address, and stream is configured to listen on the specific
> address, which is technically a valid case.
> 
>    http {
>        server {
>            listen  127.0.0.1:8080;
>            ...
>        }
> 
>        server {
>            listen  8080;
>            ...
>        }
>    }
> 
>    stream {
>        server {
>            listen  127.0.0.1:8080;
>            ...
>        }
>    }
> 
> diff --git a/src/core/ngx_connection.c b/src/core/ngx_connection.c
> --- a/src/core/ngx_connection.c
> +++ b/src/core/ngx_connection.c
> @@ -21,10 +21,27 @@ ngx_create_listening(ngx_conf_t *cf, str
>     socklen_t socklen)
> {
>     size_t            len;
> +    ngx_uint_t        i;
>     ngx_listening_t  *ls;
>     struct sockaddr  *sa;
>     u_char            text[NGX_SOCKADDR_STRLEN];
> 
> +    ls = cf->cycle->listening.elts;
> +    for (i = 0; i < cf->cycle->listening.nelts; i++) {
> +
> +        if (ngx_cmp_sockaddr(ls[i].sockaddr, ls[i].socklen,
> +                             sockaddr, socklen, 1)
> +            == NGX_DECLINED)
> +        {
> +            continue;
> +        }
> +
> +        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                           "duplicate \"%V\" address and port pair",
> +                           &ls[i].addr_text);
> +        return NULL;
> +    }
> +
>     ls = ngx_array_push(&cf->cycle->listening);
>     if (ls == NULL) {
>         return NULL;
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

I like the idea of fixing address collisions between modules. However, the patch does not fix the problem entirely and leaves
room for misconfigurations. IMHO we need to think how to properly fix this.

One way for fixing this is rejecting any overlapping listen addresses. We need to evaluate how likely it is that reasonable
configurations will be restricted by this. It doesn't seem likely to me.

----
Roman Arutyunyan
arut at nginx.com




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20240408/05852e0d/attachment.htm>