Hi,

On Tue, Feb 07, 2023 at 06:12:50AM +0300, Maxim Dounin wrote:
> Hello!
>
> On Wed, Feb 01, 2023 at 06:01:09PM +0400, Roman Arutyunyan wrote:
>
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1675255790 -14400
> > # Wed Feb 01 16:49:50 2023 +0400
> > # Branch quic
> > # Node ID 139b348815b3f19753176988f06b590a3e0af4c0
> > # Parent 2fcc1c60be1c89aad5464bcc06f1189d1adc885a
> > HTTP/2: "http2" directive.
> >
> > The directive enables HTTP/2 in the current server. The previous way to
> > enable HTTP/2 via "listen ... http2" is now deprecated.
>
> It might be a good idea to describe here how it works, notably
> ALPN negotiation (and OpenSSL version where it works for
> non-default virtual servers) and preface-based protocol detection
> for non-SSL listening sockets. As well as benefits of this
> approach.

OK, added more details.

> >
> > diff --git a/src/http/modules/ngx_http_ssl_module.c b/src/http/modules/ngx_http_ssl_module.c
> > --- a/src/http/modules/ngx_http_ssl_module.c
> > +++ b/src/http/modules/ngx_http_ssl_module.c
> > @@ -427,6 +427,9 @@ ngx_http_ssl_alpn_select(ngx_ssl_conn_t
> > #if (NGX_HTTP_V2 || NGX_HTTP_V3)
> > ngx_http_connection_t *hc;
> > #endif
> > +#if (NGX_HTTP_V2)
> > + ngx_http_v2_srv_conf_t *h2scf;
> > +#endif
> > #if (NGX_HTTP_V3)
> > ngx_http_v3_srv_conf_t *h3scf;
> > #endif
> > @@ -448,12 +451,9 @@ ngx_http_ssl_alpn_select(ngx_ssl_conn_t
> > hc = c->data;
> > #endif
> >
> > -#if (NGX_HTTP_V2)
> > - if (hc->addr_conf->http2) {
> > - srv = (unsigned char *) NGX_HTTP_V2_ALPN_PROTO NGX_HTTP_ALPN_PROTOS;
> > - srvlen = sizeof(NGX_HTTP_V2_ALPN_PROTO NGX_HTTP_ALPN_PROTOS) - 1;
> > - } else
> > -#endif
> > + srv = (unsigned char *) NGX_HTTP_ALPN_PROTOS;
> > + srvlen = sizeof(NGX_HTTP_ALPN_PROTOS) - 1;
> > +
> > #if (NGX_HTTP_V3)
> > if (hc->addr_conf->quic) {
> >
> > @@ -479,10 +479,16 @@ ngx_http_ssl_alpn_select(ngx_ssl_conn_t
> >
> > } else
> > #endif
> > +#if (NGX_HTTP_V2)
> > {
> > - srv = (unsigned char *) NGX_HTTP_ALPN_PROTOS;
> > - srvlen = sizeof(NGX_HTTP_ALPN_PROTOS) - 1;
> > + h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
> > +
> > + if (h2scf->enable || hc->addr_conf->http2) {
> > + srv = (unsigned char *) NGX_HTTP_V2_ALPN_PROTO NGX_HTTP_ALPN_PROTOS;
> > + srvlen = sizeof(NGX_HTTP_V2_ALPN_PROTO NGX_HTTP_ALPN_PROTOS) - 1;
> > + }
> > }
> > +#endif
> >
> > if (SSL_select_next_proto((unsigned char **) out, outlen, srv, srvlen,
> > in, inlen)
> > diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
> > --- a/src/http/ngx_http_core_module.c
> > +++ b/src/http/ngx_http_core_module.c
> > @@ -4179,6 +4179,11 @@ ngx_http_core_listen(ngx_conf_t *cf, ngx
> >
> > if (ngx_strcmp(value[n].data, "http2") == 0) {
> > #if (NGX_HTTP_V2)
> > + ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
> > + "the \"listen ... http2\" directive "
> > + "is deprecated, use "
> > + "the \"http2\" directive instead");
> > +
> > lsopt.http2 = 1;
> > continue;
> > #else
> > diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> > --- a/src/http/ngx_http_request.c
> > +++ b/src/http/ngx_http_request.c
> > @@ -318,12 +318,6 @@ ngx_http_init_connection(ngx_connection_
> > rev->handler = ngx_http_wait_request_handler;
> > c->write->handler = ngx_http_empty_handler;
> >
> > -#if (NGX_HTTP_V2)
> > - if (hc->addr_conf->http2) {
> > - rev->handler = ngx_http_v2_init;
> > - }
> > -#endif
> > -
> > #if (NGX_HTTP_V3)
> > if (hc->addr_conf->quic) {
> > ngx_http_v3_init_stream(c);
> > @@ -383,6 +377,9 @@ ngx_http_wait_request_handler(ngx_event_
> > ngx_buf_t *b;
> > ngx_connection_t *c;
> > ngx_http_connection_t *hc;
> > +#if (NGX_HTTP_V2)
> > + ngx_http_v2_srv_conf_t *h2scf;
> > +#endif
> > ngx_http_core_srv_conf_t *cscf;
> >
> > c = rev->data;
> > @@ -429,6 +426,8 @@ ngx_http_wait_request_handler(ngx_event_
> > b->end = b->last + size;
> > }
> >
> > + size = b->end - b->last;
> > +
> > n = c->recv(c, b->last, size);
> >
> > if (n == NGX_AGAIN) {
> > @@ -443,12 +442,16 @@ ngx_http_wait_request_handler(ngx_event_
> > return;
> > }
> >
> > - /*
> > - * We are trying to not hold c->buffer's memory for an idle connection.
> > - */
> > -
> > - if (ngx_pfree(c->pool, b->start) == NGX_OK) {
> > - b->start = NULL;
> > + if (b->pos == b->last) {
> > +
> > + /*
> > + * We are trying to not hold c->buffer's memory for an
> > + * idle connection.
> > + */
> > +
> > + if (ngx_pfree(c->pool, b->start) == NGX_OK) {
> > + b->start = NULL;
> > + }
> > }
> >
> > return;
> > @@ -489,6 +492,30 @@ ngx_http_wait_request_handler(ngx_event_
> > }
> > }
> >
> > +#if (NGX_HTTP_V2)
> > +
> > + h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
> > +
> > + if (!c->ssl && (h2scf->enable || hc->addr_conf->http2)) {
> > +
> > + size = ngx_min(sizeof(NGX_HTTP_V2_PREFACE) - 1,
> > + (size_t) (b->last - b->pos));
> > +
> > + if (ngx_memcmp(b->pos, NGX_HTTP_V2_PREFACE, size) == 0) {
> > +
> > + if (size == sizeof(NGX_HTTP_V2_PREFACE) - 1) {
> > + ngx_http_v2_init(rev);
> > + return;
> > + }
>
> Note that ngx_http_wait_request_handler() is used for both
> new and keepalive connections. It does not look like a good idea
> to start an HTTP/2 connection after processing some HTTP/1.x
> requests.

Keepalive has its own handler ngx_http_keepalive_handler(), which is similar
to ngx_http_wait_request_handler(), but does not try to recognize HTTP/2.

> > +
> > + c->log->action = "waiting for request";
> > + ngx_post_event(rev, &ngx_posted_events);
> > + return;
> > + }
> > + }
> > +
> > +#endif
> > +
> > c->log->action = "reading client request line";
> >
> > ngx_reusable_connection(c, 0);
> > @@ -808,13 +835,16 @@ ngx_http_ssl_handshake_handler(ngx_conne
> > #if (NGX_HTTP_V2 \
> > && defined TLSEXT_TYPE_application_layer_protocol_negotiation)
> > {
> > - unsigned int len;
> > - const unsigned char *data;
> > - ngx_http_connection_t *hc;
> > + unsigned int len;
> > + const unsigned char *data;
> > + ngx_http_connection_t *hc;
> > + ngx_http_v2_srv_conf_t *h2scf;
> >
> > hc = c->data;
> >
> > - if (hc->addr_conf->http2) {
> > + h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
> > +
> > + if (h2scf->enable || hc->addr_conf->http2) {
> >
> > SSL_get0_alpn_selected(c->ssl->connection, &data, &len);
> >
> > diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
> > --- a/src/http/v2/ngx_http_v2.c
> > +++ b/src/http/v2/ngx_http_v2.c
> > @@ -232,6 +232,7 @@ static ngx_http_v2_parse_header_t ngx_h
> > void
> > ngx_http_v2_init(ngx_event_t *rev)
> > {
> > + u_char *p, *end;
> > ngx_connection_t *c;
> > ngx_pool_cleanup_t *cln;
> > ngx_http_connection_t *hc;
> > @@ -335,6 +336,29 @@ ngx_http_v2_init(ngx_event_t *rev)
> > c->idle = 1;
> > ngx_reusable_connection(c, 0);
> >
> > + if (c->buffer) {
> > + p = c->buffer->pos;
> > + end = c->buffer->last;
> > +
> > + do {
> > + p = h2c->state.handler(h2c, p, end);
> > +
> > + if (p == NULL) {
> > + return;
> > + }
> > +
> > + } while (p != end);
> > +
> > + h2c->total_bytes += p - c->buffer->pos;
> > + c->buffer->pos = p;
> > +
> > + if (h2c->total_bytes / 8 > h2c->payload_bytes + 1048576) {
> > + ngx_log_error(NGX_LOG_INFO, c->log, 0, "http2 flood detected");
> > + ngx_http_v2_finalize_connection(h2c, NGX_HTTP_V2_NO_ERROR);
> > + return;
> > + }
>
> I think flood protection can be safely omitted here, given that
> client_header_buffer_size is expected to be small (1k by default),
> and flood protection is not going to kick in till at least 8M of
> total bytes from the client.

Yes, let's remove this block.

> > + }
> > +
> > ngx_http_v2_read_handler(rev);
> > }
> >
> > @@ -871,7 +895,7 @@ static u_char *
> > ngx_http_v2_state_preface(ngx_http_v2_connection_t *h2c, u_char *pos,
> > u_char *end)
> > {
> > - static const u_char preface[] = "PRI * HTTP/2.0\r\n";
> > + static const u_char preface[] = NGX_HTTP_V2_PREFACE_START;
> >
> > if ((size_t) (end - pos) < sizeof(preface) - 1) {
> > return ngx_http_v2_state_save(h2c, pos, end, ngx_http_v2_state_preface);
> > @@ -892,7 +916,7 @@ static u_char *
> > ngx_http_v2_state_preface_end(ngx_http_v2_connection_t *h2c, u_char *pos,
> > u_char *end)
> > {
> > - static const u_char preface[] = "\r\nSM\r\n\r\n";
> > + static const u_char preface[] = NGX_HTTP_V2_PREFACE_END;
> >
> > if ((size_t) (end - pos) < sizeof(preface) - 1) {
> > return ngx_http_v2_state_save(h2c, pos, end,
> > @@ -3946,10 +3970,22 @@ static void
> > ngx_http_v2_run_request(ngx_http_request_t *r)
> > {
> > ngx_connection_t *fc;
> > + ngx_http_v3_srv_conf_t *h2scf;
> > ngx_http_v2_connection_t *h2c;
> >
> > fc = r->connection;
> >
> > + h2scf = ngx_http_get_module_srv_conf(r, ngx_http_v2_module);
> > +
> > + if (!h2scf->enable && !r->http_connection->addr_conf->http2) {
> > + ngx_log_error(NGX_LOG_INFO, fc->log, 0,
> > + "client attempted to request the server name "
> > + "for which the negotitated protocol is unavailable");
>
> Typo, should be "negotiated".

Thanks, changed to:

"for which the negotiated protocol is disabled"

> > +
> > + ngx_http_finalize_request(r, NGX_HTTP_MISDIRECTED_REQUEST);
> > + goto failed;
> > + }
> > +
> > if (ngx_http_v2_construct_request_line(r) != NGX_OK) {
> > goto failed;
> > }
> > diff --git a/src/http/v2/ngx_http_v2.h b/src/http/v2/ngx_http_v2.h
> > --- a/src/http/v2/ngx_http_v2.h
> > +++ b/src/http/v2/ngx_http_v2.h
> > @@ -64,6 +64,16 @@ typedef u_char *(*ngx_http_v2_handler_pt
> >
> >
> > typedef struct {
> > + ngx_flag_t enable;
> > + size_t pool_size;
> > + ngx_uint_t concurrent_streams;
> > + ngx_uint_t concurrent_pushes;
> > + size_t preread_size;
> > + ngx_uint_t streams_index_mask;
> > +} ngx_http_v2_srv_conf_t;
> > +
> > +
> > +typedef struct {
> > ngx_str_t name;
> > ngx_str_t value;
> > } ngx_http_v2_header_t;
> > @@ -408,9 +418,17 @@ ngx_int_t ngx_http_v2_table_size(ngx_htt
> > #define NGX_HTTP_V2_USER_AGENT_INDEX 58
> > #define NGX_HTTP_V2_VARY_INDEX 59
> >
> > +#define NGX_HTTP_V2_PREFACE_START "PRI * HTTP/2.0\r\n"
> > +#define NGX_HTTP_V2_PREFACE_END "\r\nSM\r\n\r\n"
> > +#define NGX_HTTP_V2_PREFACE NGX_HTTP_V2_PREFACE_START \
> > + NGX_HTTP_V2_PREFACE_END
> > +
> >
> > u_char *ngx_http_v2_string_encode(u_char *dst, u_char *src, size_t len,
> > u_char *tmp, ngx_uint_t lower);
> >
> >
> > +extern ngx_module_t ngx_http_v2_module;
> > +
> > +
> > #endif /* _NGX_HTTP_V2_H_INCLUDED_ */
> > diff --git a/src/http/v2/ngx_http_v2_module.c b/src/http/v2/ngx_http_v2_module.c
> > --- a/src/http/v2/ngx_http_v2_module.c
> > +++ b/src/http/v2/ngx_http_v2_module.c
> > @@ -75,6 +75,13 @@ static ngx_conf_post_t ngx_http_v2_chun
> >
> > static ngx_command_t ngx_http_v2_commands[] = {
> >
> > + { ngx_string("http2"),
> > + NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_CONF_FLAG,
> > + ngx_conf_set_flag_slot,
> > + NGX_HTTP_SRV_CONF_OFFSET,
> > + offsetof(ngx_http_v2_srv_conf_t, enable),
> > + NULL },
> > +
> > { ngx_string("http2_recv_buffer_size"),
> > NGX_HTTP_MAIN_CONF|NGX_CONF_TAKE1,
> > ngx_conf_set_size_slot,
> > @@ -314,6 +321,8 @@ ngx_http_v2_create_srv_conf(ngx_conf_t *
> > return NULL;
> > }
> >
> > + h2scf->enable = NGX_CONF_UNSET;
> > +
> > h2scf->pool_size = NGX_CONF_UNSET_SIZE;
> >
> > h2scf->concurrent_streams = NGX_CONF_UNSET_UINT;
> > @@ -333,6 +342,8 @@ ngx_http_v2_merge_srv_conf(ngx_conf_t *c
> > ngx_http_v2_srv_conf_t *prev = parent;
> > ngx_http_v2_srv_conf_t *conf = child;
> >
> > + ngx_conf_merge_value(conf->enable, prev->enable, 0);
> > +
> > ngx_conf_merge_size_value(conf->pool_size, prev->pool_size, 4096);
> >
> > ngx_conf_merge_uint_value(conf->concurrent_streams,
> > diff --git a/src/http/v2/ngx_http_v2_module.h b/src/http/v2/ngx_http_v2_module.h
> > --- a/src/http/v2/ngx_http_v2_module.h
> > +++ b/src/http/v2/ngx_http_v2_module.h
> > @@ -21,15 +21,6 @@ typedef struct {
> >
> >
> > typedef struct {
> > - size_t pool_size;
> > - ngx_uint_t concurrent_streams;
> > - ngx_uint_t concurrent_pushes;
> > - size_t preread_size;
> > - ngx_uint_t streams_index_mask;
> > -} ngx_http_v2_srv_conf_t;
> > -
> > -
> > -typedef struct {
> > size_t chunk_size;
> >
> > ngx_flag_t push_preload;
> > @@ -39,7 +30,4 @@ typedef struct {
> > } ngx_http_v2_loc_conf_t;
> >
> >
> > -extern ngx_module_t ngx_http_v2_module;
> > -
> > -
> > #endif /* _NGX_HTTP_V2_MODULE_H_INCLUDED_ */
>
> Otherwise looks good.

Also, ngx_http_wait_request_handler() disables reusable mode for the connection
prior to creating the request, and ngx_http_v2_init() does the same.
It seems a good idea to move ngx_reusable_connection(c, 0) up before detecting
HTTP/2 preface and remove this call from ngx_http_v2_init(). This will
simplify HTTP/2 code and also keep the connection from being closed while
reading the preface, which is what we do while reading HTTP/1 request line.

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

--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel