Hello!

On Thu, Jan 26, 2023 at 08:00:53PM +0400, Sergey Kandaurov wrote:

> > On 26 Jan 2023, at 01:29, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > On Mon, Jan 23, 2023 at 02:21:33PM +0400, Sergey Kandaurov wrote:
> >
> >>> On 31 Dec 2022, at 18:35, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >>>
> >>> # HG changeset patch
> >>> # User Maxim Dounin <mdounin@mdounin.ru>
> >>> # Date 1672497248 -10800
> >>> # Sat Dec 31 17:34:08 2022 +0300
> >>> # Node ID c215d5cf25732ece1819cf1cd48ebb480bb642c7
> >>> # Parent 07b0bee87f32be91a33210bc06973e07c4c1dac9
> >>> Added warning about redefinition of listen socket protocol options.
> >>>
> >>> The "listen" directive in the http module can be used multiple times
> >>> in different server blocks. Originally, it was supposed to be specified
> >>> once with various socket options, and without any parameters in virtual
> >>> server blocks. For example:
> >>>
> >>> server { listen 80 backlog=1024; server_name foo; ... }
> >>> server { listen 80; server_name bar; ... }
> >>> server { listen 80; server_name bazz; ... }
> >>>
> >>> The address part of the syntax ("address[:port]" / "port" / "unix:path")
> >>> uniquely identifies the listening socket, and therefore is enough for
> >>> name-based virtual servers (to let nginx know that the virtual server
> >>> accepts requests on the listening socket in question).
> >>>
> >>> To ensure that listening options do not conflict between virtual servers,
> >>> they were allowed only once. For example, the following configuration
> >>> will be rejected ("duplicate listen options for 0.0.0.0:80 in ..."):
> >>>
> >>> server { listen 80 backlog=1024; server_name foo; ... }
> >>> server { listen 80 backlog=512; server_name bar; ... }
> >>>
> >>> At some point it was, however, noticed, that it is sometimes convenient
> >>> to repeat some options for clarity. In nginx 0.8.51 the "ssl" parameter
> >>> was allowed to be specified multiple times, e.g.:
> >>>
> >>> server { listen 443 ssl backlog=1024; server_name foo; ... }
> >>> server { listen 443 ssl; server_name bar; ... }
> >>> server { listen 443 ssl; server_name bazz; ... }
> >>>
> >>> This approach makes configuration more readable, since SSL sockets are
> >>> immediately visible in the configuration. If this is not needed, just the
> >>> address can still be used.
> >>>
> >>> Later, additional protocol-specific options similar to "ssl" were
> >>> introduced, notably "http2" and "proxy_protocol". With these options,
> >>> one can write:
> >>>
> >>> server { listen 443 ssl backlog=1024; server_name foo; ... }
> >>> server { listen 443 http2; server_name bar; ... }
> >>> server { listen 443 proxy_protocol; server_name bazz; ... }
> >>>
> >>> The resulting socket will use ssl, http2, and proxy_protocol, but this
> >>> is not really obvious from the configuration.
> >>>
> >>> To ensure such misleading configurations are not allowed, nginx now
> >>> warns as long as the "listen" directive is used with options different
> >>
> >> nitpicking:
> >>
> >> "ensure .. are not allowed" and "warns" don't seem to be equally strong.
> >> As such, I'd rewrite to something like:
> >>
> >> To emphasize such misleading configurations are discouraged, nginx now
> >
> > Thanks, changed.
> >
> >>> from the options previously used if these are potentially confusing.
> >>
> >> Not really confident what "these" refers to.
> >>
> >> s/these/they/ ?
> >
> > I don't think that there is much difference between "these" and
> > "they" when it comes to what they refer to. Either way, "if this
> > is potentially confusing" is probably better and unambiguously
> > refers to the fact that the "listen" directive is used with the
> > different options. Changed.
>
> Sounds good as well, tnx.
>
> >
> >>> In particular, the following configurations are allowed:
> >>>
> >>> server { listen 8401 ssl backlog=1024; server_name foo; }
> >>> server { listen 8401 ssl; server_name bar; }
> >>> server { listen 8401 ssl; server_name bazz; }
> >>>
> >>> server { listen 8402 ssl http2 backlog=1024; server_name foo; }
> >>> server { listen 8402 ssl; server_name bar; }
> >>> server { listen 8402 ssl; server_name bazz; }
> >>>
> >>> server { listen 8403 ssl; server_name bar; }
> >>> server { listen 8403 ssl; server_name bazz; }
> >>> server { listen 8403 ssl http2; server_name foo; }
> >>>
> >>> server { listen 8404 ssl http2 backlog=1024; server_name foo; }
> >>> server { listen 8404 http2; server_name bar; }
> >>> server { listen 8404 http2; server_name bazz; }
> >>>
> >>> server { listen 8405 ssl http2 backlog=1024; server_name foo; }
> >>> server { listen 8405 ssl http2; server_name bar; }
> >>> server { listen 8405 ssl http2; server_name bazz; }
> >>>
> >>> server { listen 8406 ssl; server_name foo; }
> >>> server { listen 8406; server_name bar; }
> >>> server { listen 8406; server_name bazz; }
> >>>
> >>> And the following configurations will generate warnings:
> >>>
> >>> server { listen 8501 ssl http2 backlog=1024; server_name foo; }
> >>> server { listen 8501 http2; server_name bar; }
> >>> server { listen 8501 ssl; server_name bazz; }
> >>>
> >>> server { listen 8502 backlog=1024; server_name foo; }
> >>> server { listen 8502 ssl; server_name bar; }
> >>>
> >>> server { listen 8503 ssl; server_name foo; }
> >>> server { listen 8503 http2; server_name bar; }
> >>>
> >>> server { listen 8504 ssl; server_name foo; }
> >>> server { listen 8504 http2; server_name bar; }
> >>> server { listen 8504 proxy_protocol; server_name bazz; }
> >>>
> >>> server { listen 8505 ssl http2 proxy_protocol; server_name foo; }
> >>> server { listen 8505 ssl http2; server_name bar; }
> >>> server { listen 8505 ssl; server_name bazz; }
> >>>
> >>> server { listen 8506 ssl http2; server_name foo; }
> >>> server { listen 8506 ssl; server_name bar; }
> >>> server { listen 8506; server_name bazz; }
> >>>
> >>> server { listen 8507 ssl; server_name bar; }
> >>> server { listen 8507; server_name bazz; }
> >>> server { listen 8507 ssl http2; server_name foo; }
> >>>
> >>> server { listen 8508 ssl; server_name bar; }
> >>> server { listen 8508; server_name bazz; }
> >>> server { listen 8508 ssl backlog=1024; server_name foo; }
> >>>
> >>> server { listen 8509; server_name bazz; }
> >>> server { listen 8509 ssl; server_name bar; }
> >>> server { listen 8509 ssl backlog=1024; server_name foo; }
> >>>
> >>
> >> 15 examples of dos and don'ts looks slightly excessive.
> >> The accurate description (such as provided by you below) allows
> >> to reduce most of them to e.g. four common invalid configurations:
> >>
> >> A lesser option set with socket option:
> >>
> >> server { listen 8443 backlog=1024; server_name foo; }
> >> server { listen 8443 http2; server_name bar; }
> >>
> >> The main option set is repeated at least twice:
> >>
> >> server { listen 127.0.0.1:8443; server_name foo; }
> >> server { listen 127.0.0.1:8443 ssl; server_name bar; }
> >> server { listen 127.0.0.1:8443 ssl; server_name baz; }
> >>
> >> Option sets partially overlap:
> >>
> >> server { listen 127.0.0.1:8443 ssl; server_name foo; }
> >> server { listen 127.0.0.1:8443 http2; server_name bar; }
> >>
> >> More than two option sets:
> >>
> >> server { listen 127.0.0.1:8443 http2 ssl; server_name foo; }
> >> server { listen 127.0.0.1:8443 http2; server_name bar; }
> >> server { listen 127.0.0.1:8443 ssl; server_name baz; }
> >
> > While your approach might be better from documentation point
> > of view, the way it is currently described in the commit log is
> > how it was designed: from the examples of valid and invalid
> > configurations.
> >
> > My current working tests contain 18 valid and 22 invalid
> > configurations, derived from the ones provided in the commit log
> > with additional shuffling. But since these are derived I've
> > decided to avoid providing all of them in the commit log.
> >
> >>> The basic idea is that at most two sets of protocol options are allowed:
> >>> the main one (with socket options, if any), and a shorter one, with options
> >>> being a subset of the main options, repeated for clarity. As long as the
> >>> shorter set of protocol options is used, all listen directives except the
> >>> main one should use it.
> >>
> >> I'd move this paragraph somewhere before examples, as this is the most
> >> specific description of things actually changed.
> >
> > This paragraph summarizes changes made to address the issue
> > described, and I don't think moving it will improve things.
>
> The above looks sane now, thanks for the explanation.
> I won't insist on further adjustments.
>
> >
> >> BTW, while reviewing I caught sort of a bug.
> >> As I understand the above explanation, if there are both full and short
> >> sets present, then at most one listen directive can have the full set,
> >> while shorter sets can be repeated. If so, then with the proposed patch
> >> the next configuration is expectedly invalid:
> >>
> >> server { listen 127.0.0.1:8443 ssl; server_name foo; }
> >> server { listen 127.0.0.1:8443 ssl; server_name bar; }
> >> server { listen 127.0.0.1:8443; server_name baz; }
> >>
> >> This is expected since first two servers with same options are
> >> interpreted as a short form (with full form seen potentially later on),
> >> but 3rd server has lesser options, which is caught by this check:
> >> (addr[i].protocols_set && protocols != addr[i].protocols)
> >> Which is interpreted as:
> >> "this server has a lesser set that doesn't match a a shorter set".
> >>
> >> Now, if 3rd server is moved first, configuration starts to pass:
> >>
> >> server { listen 127.0.0.1:8443; server_name baz; }
> >> server { listen 127.0.0.1:8443 ssl; server_name foo; }
> >> server { listen 127.0.0.1:8443 ssl; server_name bar; }
> >>
> >> This is because after (now) 2nd server, it is parsed as:
> >> 1st server has a short form, and 2nd server has a full form.
> >> Then 3rd server goes to "the same options" case. This also
> >> overwrites the remembered shorter set in addr[i].protocols.
> >>
> >> I guess an additional check should be added to address this.
> >> It is similar to the "options removed" case and ensures that
> >> the repeated options set actually matches a shorter set:
> >>
> >> diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c
> >> --- a/src/http/ngx_http.c
> >> +++ b/src/http/ngx_http.c
> >> @@ -1345,7 +1345,9 @@ ngx_http_add_addresses(ngx_conf_t *cf, n
> >>
> >> /* the same options */
> >>
> >> - if (lsopt->set && addr[i].protocols_changed) {
> >> + if ((lsopt->set && addr[i].protocols_changed)
> >> + || (addr[i].protocols_set && protocols != addr[i].protocols))
> >> + {
> >> ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> >> "protocol options redefined for %V",
> >> &addr[i].opt.addr_text);
> >
> > Thanks for catching this, applied.
> >
> > Updated patch:
> >
> > # HG changeset patch
> > # User Maxim Dounin <mdounin@mdounin.ru>
> > # Date 1674624978 -10800
> > # Wed Jan 25 08:36:18 2023 +0300
> > # Node ID 1619d1563c3231d4283937610ce97aa1e3824fb8
> > # Parent c7e103acb409f0352cb73997c053b3bdbb8dd5db
> > Added warning about redefinition of listen socket protocol options.
> >
> > The "listen" directive in the http module can be used multiple times
> > in different server blocks. Originally, it was supposed to be specified
> > once with various socket options, and without any parameters in virtual
> > server blocks. For example:
> >
> > server { listen 80 backlog=1024; server_name foo; ... }
> > server { listen 80; server_name bar; ... }
> > server { listen 80; server_name bazz; ... }
> >
> > The address part of the syntax ("address[:port]" / "port" / "unix:path")
> > uniquely identifies the listening socket, and therefore is enough for
> > name-based virtual servers (to let nginx know that the virtual server
> > accepts requests on the listening socket in question).
> >
> > To ensure that listening options do not conflict between virtual servers,
> > they were allowed only once. For example, the following configuration
> > will be rejected ("duplicate listen options for 0.0.0.0:80 in ..."):
> >
> > server { listen 80 backlog=1024; server_name foo; ... }
> > server { listen 80 backlog=512; server_name bar; ... }
> >
> > At some point it was, however, noticed, that it is sometimes convenient
> > to repeat some options for clarity. In nginx 0.8.51 the "ssl" parameter
> > was allowed to be specified multiple times, e.g.:
> >
> > server { listen 443 ssl backlog=1024; server_name foo; ... }
> > server { listen 443 ssl; server_name bar; ... }
> > server { listen 443 ssl; server_name bazz; ... }
> >
> > This approach makes configuration more readable, since SSL sockets are
> > immediately visible in the configuration. If this is not needed, just the
> > address can still be used.
> >
> > Later, additional protocol-specific options similar to "ssl" were
> > introduced, notably "http2" and "proxy_protocol". With these options,
> > one can write:
> >
> > server { listen 443 ssl backlog=1024; server_name foo; ... }
> > server { listen 443 http2; server_name bar; ... }
> > server { listen 443 proxy_protocol; server_name bazz; ... }
> >
> > The resulting socket will use ssl, http2, and proxy_protocol, but this
> > is not really obvious from the configuration.
> >
> > To emphasize such misleading configurations are discouraged, nginx now
> > warns as long as the "listen" directive is used with options different
> > from the options previously used if this is potentially confusing.
> >
> > In particular, the following configurations are allowed:
> >
> > server { listen 8401 ssl backlog=1024; server_name foo; }
> > server { listen 8401 ssl; server_name bar; }
> > server { listen 8401 ssl; server_name bazz; }
> >
> > server { listen 8402 ssl http2 backlog=1024; server_name foo; }
> > server { listen 8402 ssl; server_name bar; }
> > server { listen 8402 ssl; server_name bazz; }
> >
> > server { listen 8403 ssl; server_name bar; }
> > server { listen 8403 ssl; server_name bazz; }
> > server { listen 8403 ssl http2; server_name foo; }
> >
> > server { listen 8404 ssl http2 backlog=1024; server_name foo; }
> > server { listen 8404 http2; server_name bar; }
> > server { listen 8404 http2; server_name bazz; }
> >
> > server { listen 8405 ssl http2 backlog=1024; server_name foo; }
> > server { listen 8405 ssl http2; server_name bar; }
> > server { listen 8405 ssl http2; server_name bazz; }
> >
> > server { listen 8406 ssl; server_name foo; }
> > server { listen 8406; server_name bar; }
> > server { listen 8406; server_name bazz; }
> >
> > And the following configurations will generate warnings:
> >
> > server { listen 8501 ssl http2 backlog=1024; server_name foo; }
> > server { listen 8501 http2; server_name bar; }
> > server { listen 8501 ssl; server_name bazz; }
> >
> > server { listen 8502 backlog=1024; server_name foo; }
> > server { listen 8502 ssl; server_name bar; }
> >
> > server { listen 8503 ssl; server_name foo; }
> > server { listen 8503 http2; server_name bar; }
> >
> > server { listen 8504 ssl; server_name foo; }
> > server { listen 8504 http2; server_name bar; }
> > server { listen 8504 proxy_protocol; server_name bazz; }
> >
> > server { listen 8505 ssl http2 proxy_protocol; server_name foo; }
> > server { listen 8505 ssl http2; server_name bar; }
> > server { listen 8505 ssl; server_name bazz; }
> >
> > server { listen 8506 ssl http2; server_name foo; }
> > server { listen 8506 ssl; server_name bar; }
> > server { listen 8506; server_name bazz; }
> >
> > server { listen 8507 ssl; server_name bar; }
> > server { listen 8507; server_name bazz; }
> > server { listen 8507 ssl http2; server_name foo; }
> >
> > server { listen 8508 ssl; server_name bar; }
> > server { listen 8508; server_name bazz; }
> > server { listen 8508 ssl backlog=1024; server_name foo; }
> >
> > server { listen 8509; server_name bazz; }
> > server { listen 8509 ssl; server_name bar; }
> > server { listen 8509 ssl backlog=1024; server_name foo; }
> >
> > The basic idea is that at most two sets of protocol options are allowed:
> > the main one (with socket options, if any), and a shorter one, with options
> > being a subset of the main options, repeated for clarity. As long as the
> > shorter set of protocol options is used, all listen directives except the
> > main one should use it.
> >
> > diff --git a/src/http/ngx_http.c b/src/http/ngx_http.c
> > --- a/src/http/ngx_http.c
> > +++ b/src/http/ngx_http.c
> > @@ -1228,7 +1228,8 @@ static ngx_int_t
> > ngx_http_add_addresses(ngx_conf_t *cf, ngx_http_core_srv_conf_t *cscf,
> > ngx_http_conf_port_t *port, ngx_http_listen_opt_t *lsopt)
> > {
> > - ngx_uint_t i, default_server, proxy_protocol;
> > + ngx_uint_t i, default_server, proxy_protocol,
> > + protocols, protocols_prev;
> > ngx_http_conf_addr_t *addr;
> > #if (NGX_HTTP_SSL)
> > ngx_uint_t ssl;
> > @@ -1264,12 +1265,18 @@ ngx_http_add_addresses(ngx_conf_t *cf, n
> > default_server = addr[i].opt.default_server;
> >
> > proxy_protocol = lsopt->proxy_protocol || addr[i].opt.proxy_protocol;
> > + protocols = lsopt->proxy_protocol;
> > + protocols_prev = addr[i].opt.proxy_protocol;
> >
> > #if (NGX_HTTP_SSL)
> > ssl = lsopt->ssl || addr[i].opt.ssl;
> > + protocols |= lsopt->ssl << 1;
> > + protocols_prev |= addr[i].opt.ssl << 1;
> > #endif
> > #if (NGX_HTTP_V2)
> > http2 = lsopt->http2 || addr[i].opt.http2;
> > + protocols |= lsopt->http2 << 2;
> > + protocols_prev |= addr[i].opt.http2 << 2;
> > #endif
> >
> > if (lsopt->set) {
> > @@ -1299,6 +1306,57 @@ ngx_http_add_addresses(ngx_conf_t *cf, n
> > addr[i].default_server = cscf;
> > }
> >
> > + /* check for conflicting protocol options */
> > +
> > + if ((protocols | protocols_prev) != protocols_prev) {
> > +
> > + /* options added */
> > +
> > + if ((addr[i].opt.set && !lsopt->set)
> > + || addr[i].protocols_changed
> > + || (protocols | protocols_prev) != protocols)
> > + {
> > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> > + "protocol options redefined for %V",
> > + &addr[i].opt.addr_text);
> > + }
> > +
> > + addr[i].protocols = protocols_prev;
> > + addr[i].protocols_set = 1;
> > + addr[i].protocols_changed = 1;
> > +
> > + } else if ((protocols_prev | protocols) != protocols) {
> > +
> > + /* options removed */
> > +
> > + if (lsopt->set
> > + || (addr[i].protocols_set && protocols != addr[i].protocols))
> > + {
> > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> > + "protocol options redefined for %V",
> > + &addr[i].opt.addr_text);
> > + }
> > +
> > + addr[i].protocols = protocols;
> > + addr[i].protocols_set = 1;
> > + addr[i].protocols_changed = 1;
> > +
> > + } else {
> > +
> > + /* the same options */
> > +
> > + if ((lsopt->set && addr[i].protocols_changed)
> > + || (addr[i].protocols_set && protocols != addr[i].protocols))
> > + {
> > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> > + "protocol options redefined for %V",
> > + &addr[i].opt.addr_text);
> > + }
> > +
> > + addr[i].protocols = protocols;
> > + addr[i].protocols_set = 1;
> > + }
> > +
> > addr[i].opt.default_server = default_server;
> > addr[i].opt.proxy_protocol = proxy_protocol;
> > #if (NGX_HTTP_SSL)
> > @@ -1355,6 +1413,9 @@ ngx_http_add_address(ngx_conf_t *cf, ngx
> > }
> >
> > addr->opt = *lsopt;
> > + addr->protocols = 0;
> > + addr->protocols_set = 0;
> > + addr->protocols_changed = 0;
> > addr->hash.buckets = NULL;
> > addr->hash.size = 0;
> > addr->wc_head = NULL;
> > diff --git a/src/http/ngx_http_core_module.h b/src/http/ngx_http_core_module.h
> > --- a/src/http/ngx_http_core_module.h
> > +++ b/src/http/ngx_http_core_module.h
> > @@ -274,6 +274,10 @@ typedef struct {
> > typedef struct {
> > ngx_http_listen_opt_t opt;
> >
> > + unsigned protocols:3;
> > + unsigned protocols_set:1;
> > + unsigned protocols_changed:1;
> > +
> > ngx_hash_t hash;
> > ngx_hash_wildcard_t *wc_head;
> > ngx_hash_wildcard_t *wc_tail;
> >
>
> Looks good now, thanks.

Thanks for the review, pushed to http://mdounin.ru/hg/nginx.
Also, a fix to the only warning generated by tests is pushed to
http://mdounin.ru/hg/nginx-tests.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel