Hi,

On Thu, Feb 09, 2023 at 07:56:55PM +0400, Sergey Kandaurov wrote:
>
> > On 9 Feb 2023, at 16:33, Roman Arutyunyan <arut@nginx.com> wrote:
> >
> > Hi,
> >
> > On Thu, Feb 09, 2023 at 04:02:34PM +0400, Roman Arutyunyan wrote:
> >
> > [..]
> >
> >> 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,10 +492,34 @@ ngx_http_wait_request_handler(ngx_event_
> >> }
> >> }
> >>
> >> + ngx_reusable_connection(c, 0);
> >> +
> >> +#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)) {
> >
> > And one more fix for compilation with HTTP/2, but without SSL:
> >
> > 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
> > @@ -498,8 +498,12 @@ ngx_http_wait_request_handler(ngx_event_
> >
> > h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
> >
> > - if (!c->ssl && (h2scf->enable || hc->addr_conf->http2)) {
> > -
> > + if ((h2scf->enable || hc->addr_conf->http2)
> > +#if (NGX_HTTP_SSL)
> > + && !c->ssl
> > +#endif
> > + )
> > + {
> > size = ngx_min(sizeof(NGX_HTTP_V2_PREFACE) - 1,
> > (size_t) (b->last - b->pos));
> >
>
> I think this test needs to be replaced with !hc->ssl.
> Otherwise, it would allow to establish (and keep) h2c on ssl-enabled
> sockets, which we likely do not want to allow.

After a series of discussions we decided to go with !hc->ssl. As a result,
any non-SSL connection on an SSL port will trigger the HTTP/1 error page.
Previous attempt to trigger the HTTP/2 error in case client request is
recognized as HTTP/2, is discarded since the situation is unlikely.

[..]

--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1684240208 -14400
# Tue May 16 16:30:08 2023 +0400
# Branch quic
# Node ID 4dcd2b42c23973815a6b8a7f54bbd1460c314c93
# Parent d8272b84031bea1940ef8a5b8e2f79ec6a2dcfc1
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. The new approach
allows to share HTTP/2 and HTTP/0.9-1.1 on the same port.

For SSL connections, HTTP/2 is now selected by ALPN callback based on whether
the protocol is enabled in the virtual server chosen by SNI. This however only
works since OpenSSL 1.0.2h, where ALPN callback is invoked after SNI callback.
For older versions of OpenSSL, HTTP/2 is enabled based on the default virtual
server configuration.

For plain TCP connections, HTTP/2 is now auto-detected by HTTP/2 preface, if
HTTP/2 is enabled in the default virtual server. If preface is not matched,
HTTP/0.9-1.1 is assumed.

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
@@ -435,6 +435,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
@@ -456,12 +459,6 @@ 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
#if (NGX_HTTP_V3)
if (hc->addr_conf->quic) {

@@ -488,8 +485,19 @@ ngx_http_ssl_alpn_select(ngx_ssl_conn_t
} else
#endif
{
- srv = (unsigned char *) NGX_HTTP_ALPN_PROTOS;
- srvlen = sizeof(NGX_HTTP_ALPN_PROTOS) - 1;
+#if (NGX_HTTP_V2)
+ 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;
+
+ } else
+#endif
+ {
+ srv = (unsigned char *) NGX_HTTP_ALPN_PROTOS;
+ srvlen = sizeof(NGX_HTTP_ALPN_PROTOS) - 1;
+ }
}

if (SSL_select_next_proto((unsigned char **) out, outlen, srv, srvlen,
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
@@ -4176,6 +4176,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,10 +492,34 @@ ngx_http_wait_request_handler(ngx_event_
}
}

+ ngx_reusable_connection(c, 0);
+
+#if (NGX_HTTP_V2)
+
+ h2scf = ngx_http_get_module_srv_conf(hc->conf_ctx, ngx_http_v2_module);
+
+ if (!hc->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;
+ }
+
+ 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);
-
c->data = ngx_http_create_request(c);
if (c->data == NULL) {
ngx_http_close_connection(c);
@@ -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
@@ -63,8 +63,6 @@ static void ngx_http_v2_handle_connectio
static void ngx_http_v2_lingering_close(ngx_connection_t *c);
static void ngx_http_v2_lingering_close_handler(ngx_event_t *rev);

-static u_char *ngx_http_v2_state_proxy_protocol(ngx_http_v2_connection_t *h2c,
- u_char *pos, u_char *end);
static u_char *ngx_http_v2_state_preface(ngx_http_v2_connection_t *h2c,
u_char *pos, u_char *end);
static u_char *ngx_http_v2_state_preface_end(ngx_http_v2_connection_t *h2c,
@@ -232,6 +230,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;
@@ -314,8 +313,7 @@ ngx_http_v2_init(ngx_event_t *rev)
return;
}

- h2c->state.handler = hc->proxy_protocol ? ngx_http_v2_state_proxy_protocol
- : ngx_http_v2_state_preface;
+ h2c->state.handler = ngx_http_v2_state_preface;

ngx_queue_init(&h2c->waiting);
ngx_queue_init(&h2c->dependencies);
@@ -333,7 +331,23 @@ 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;
+ }

ngx_http_v2_read_handler(rev);
}
@@ -847,31 +861,10 @@ ngx_http_v2_lingering_close_handler(ngx_


static u_char *
-ngx_http_v2_state_proxy_protocol(ngx_http_v2_connection_t *h2c, u_char *pos,
- u_char *end)
-{
- ngx_log_t *log;
-
- log = h2c->connection->log;
- log->action = "reading PROXY protocol";
-
- pos = ngx_proxy_protocol_read(h2c->connection, pos, end);
-
- log->action = "processing HTTP/2 connection";
-
- if (pos == NULL) {
- return ngx_http_v2_connection_error(h2c, NGX_HTTP_V2_PROTOCOL_ERROR);
- }
-
- return ngx_http_v2_state_preface(h2c, pos, end);
-}
-
-
-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 +885,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,
@@ -3943,10 +3936,22 @@ static void
ngx_http_v2_run_request(ngx_http_request_t *r)
{
ngx_connection_t *fc;
+ ngx_http_v2_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 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_ */
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel