On Thu, Oct 21, 2021 at 07:41:05PM +0300, Sergey Kandaurov wrote:
>
> > On 21 Oct 2021, at 15:15, Roman Arutyunyan <a...@nginx.com> wrote:
> >
> > On Tue, Oct 19, 2021 at 01:07:56PM +0300, Sergey Kandaurov wrote:
> >>
> >> [..]
> >> Below is alternative patch, it brings closer to how OCSP validation
> >> is done with SSL_read_early_data(), with its inherent design flaws.
> >> Namely, the case of regular SSL session reuse is still pessimized,
> >> but that shouldn't bring further slowdown with ssl_ocsp disabled,
> >> which is slow by itself.
> >>
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluk...@nginx.com>
> >> # Date 1634637049 -10800
> >> # Tue Oct 19 12:50:49 2021 +0300
> >> # Branch quic
> >> # Node ID 6f26d6656b4ef97a3a245354bd7fa9e5c8671237
> >> # Parent 1798acc01970ae5a03f785b7679fe34c32adcfea
> >> QUIC: speeding up processing 0-RTT.
> >>
> >> After fe919fd63b0b, processing QUIC streams was postponed until after
> >> handshake
> >> completion, which means that 0-RTT is effectively off. With ssl_ocsp
> >> enabled,
> >> it could be further delayed. This differs to how SSL_read_early_data()
> >> works.
> >
> > differs FROM ?
> >
> >> This change unlocks processing streams on successful 0-RTT packet
> >> decryption.
> >>
>
> Both forms seem to be used, but "differs to" looks less popular.
> Rewrote it this way:
>
> This differs from how OCSP validation works with SSL_read_early_data().
>
> >> diff --git a/src/event/quic/ngx_event_quic.c
> >> b/src/event/quic/ngx_event_quic.c
> >> --- a/src/event/quic/ngx_event_quic.c
> >> +++ b/src/event/quic/ngx_event_quic.c
> >> @@ -989,6 +989,21 @@ ngx_quic_process_payload(ngx_connection_
> >> }
> >> }
> >>
> >> + if (pkt->level == ssl_encryption_early_data &&
> >> !qc->streams.initialized) {
> >> + rc = ngx_ssl_ocsp_validate(c);
> >> +
> >> + if (rc == NGX_ERROR) {
> >> + return NGX_ERROR;
> >> + }
> >> +
> >> + if (rc == NGX_AGAIN) {
> >> + c->ssl->handler = ngx_quic_init_streams;
> >> +
> >> + } else {
> >> + ngx_quic_init_streams(c);
> >> + }
> >> + }
> >> +
> >> if (pkt->level == ssl_encryption_handshake) {
> >> /*
> >> * RFC 9001, 4.9.1. Discarding Initial Keys
> >> diff --git a/src/event/quic/ngx_event_quic_ssl.c
> >> b/src/event/quic/ngx_event_quic_ssl.c
> >> --- a/src/event/quic/ngx_event_quic_ssl.c
> >> +++ b/src/event/quic/ngx_event_quic_ssl.c
> >> @@ -463,6 +463,11 @@ ngx_quic_crypto_input(ngx_connection_t *
> >> return NGX_ERROR;
> >> }
> >>
> >> + if (qc->streams.initialized) {
> >> + /* done while processing 0-RTT */
> >> + return NGX_OK;
> >> + }
> >> +
> >> rc = ngx_ssl_ocsp_validate(c);
> >>
> >> if (rc == NGX_ERROR) {
> >>
> >
> > It would be nice to always call ngx_ssl_ocsp_validate() from the same source
> > file (presumably ngx_event_quic_ssl.c). But this does not seem to occur
> > naturally so let's leave it as it is.
> >
> > Looks good.
> >
> > PS: Also, this can be further refactored to move ngx_ssl_ocsp_validate()
> > inside
> > ngx_quic_init_streams(). In this case we can only call
> > ngx_quic_init_streams()
> > both times.
>
> This is feasible, if init streams closer to obtaining 0-RTT secret.
> Actually, it is even better, I believe, and it's invoked just once
> regardless of the number of 0-RTT packets.
> Requirement for successful 0-RTT decryption doesn't buy us much.
>
> N.B. I decided to leave in place "quic init streams" debug.
> This is where streams are now actually initialized,
> and it looks reasonable to see that logged only once.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluk...@nginx.com>
> # Date 1634832181 -10800
> # Thu Oct 21 19:03:01 2021 +0300
> # Branch quic
> # Node ID 11119f9fda599c890a93b348310f582e3c49ebb7
> # Parent 1798acc01970ae5a03f785b7679fe34c32adcfea
> QUIC: refactored OCSP validation in preparation for 0-RTT support.
>
> diff --git a/src/event/quic/ngx_event_quic_ssl.c
> b/src/event/quic/ngx_event_quic_ssl.c
> --- a/src/event/quic/ngx_event_quic_ssl.c
> +++ b/src/event/quic/ngx_event_quic_ssl.c
> @@ -361,7 +361,6 @@ static ngx_int_t
> ngx_quic_crypto_input(ngx_connection_t *c, ngx_chain_t *data)
> {
> int n, sslerr;
> - ngx_int_t rc;
> ngx_buf_t *b;
> ngx_chain_t *cl;
> ngx_ssl_conn_t *ssl_conn;
> @@ -463,19 +462,10 @@ ngx_quic_crypto_input(ngx_connection_t *
> return NGX_ERROR;
> }
>
> - rc = ngx_ssl_ocsp_validate(c);
> -
> - if (rc == NGX_ERROR) {
> + if (ngx_quic_init_streams(c) != NGX_OK) {
> return NGX_ERROR;
> }
>
> - if (rc == NGX_AGAIN) {
> - c->ssl->handler = ngx_quic_init_streams;
> - return NGX_OK;
> - }
> -
> - ngx_quic_init_streams(c);
> -
> return NGX_OK;
> }
>
> diff --git a/src/event/quic/ngx_event_quic_streams.c
> b/src/event/quic/ngx_event_quic_streams.c
> --- a/src/event/quic/ngx_event_quic_streams.c
> +++ b/src/event/quic/ngx_event_quic_streams.c
> @@ -16,6 +16,7 @@
> static ngx_quic_stream_t *ngx_quic_create_client_stream(ngx_connection_t *c,
> uint64_t id);
> static ngx_int_t ngx_quic_init_stream(ngx_quic_stream_t *qs);
> +static void ngx_quic_init_streams_handler(ngx_connection_t *c);
> static ngx_quic_stream_t *ngx_quic_create_stream(ngx_connection_t *c,
> uint64_t id);
> static void ngx_quic_empty_handler(ngx_event_t *ev);
> @@ -369,9 +370,37 @@ ngx_quic_init_stream(ngx_quic_stream_t *
> }
>
>
> -void
> +ngx_int_t
> ngx_quic_init_streams(ngx_connection_t *c)
> {
> + ngx_int_t rc;
> + ngx_quic_connection_t *qc;
> +
> + qc = ngx_quic_get_connection(c);
> +
> + if (qc->streams.initialized) {
> + return NGX_OK;
> + }
> +
> + rc = ngx_ssl_ocsp_validate(c);
> +
> + if (rc == NGX_ERROR) {
> + return NGX_ERROR;
> + }
> +
> + if (rc == NGX_AGAIN) {
> + c->ssl->handler = ngx_quic_init_streams_handler;
> + return NGX_OK;
> + }
> +
> + ngx_quic_init_streams_handler(c);
> +
> + return NGX_OK;
> +}
> +
Missing an empty line.
> +static void
> +ngx_quic_init_streams_handler(ngx_connection_t *c)
> +{
> ngx_queue_t *q;
> ngx_quic_stream_t *qs;
> ngx_quic_connection_t *qc;
> diff --git a/src/event/quic/ngx_event_quic_streams.h
> b/src/event/quic/ngx_event_quic_streams.h
> --- a/src/event/quic/ngx_event_quic_streams.h
> +++ b/src/event/quic/ngx_event_quic_streams.h
> @@ -31,7 +31,7 @@ ngx_int_t ngx_quic_handle_stop_sending_f
> ngx_int_t ngx_quic_handle_max_streams_frame(ngx_connection_t *c,
> ngx_quic_header_t *pkt, ngx_quic_max_streams_frame_t *f);
>
> -void ngx_quic_init_streams(ngx_connection_t *c);
> +ngx_int_t ngx_quic_init_streams(ngx_connection_t *c);
> void ngx_quic_rbtree_insert_stream(ngx_rbtree_node_t *temp,
> ngx_rbtree_node_t *node, ngx_rbtree_node_t *sentinel);
> ngx_quic_stream_t *ngx_quic_find_stream(ngx_rbtree_t *rbtree,
> # HG changeset patch
> # User Sergey Kandaurov <pluk...@nginx.com>
> # Date 1634832186 -10800
> # Thu Oct 21 19:03:06 2021 +0300
> # Branch quic
> # Node ID b53e361bee7dfbb027507a717e6648234a06ef13
> # Parent 11119f9fda599c890a93b348310f582e3c49ebb7
> QUIC: speeding up processing 0-RTT.
>
> After fe919fd63b0b, processing QUIC streams was postponed until after
> handshake
> completion, which means that 0-RTT is effectively off. With ssl_ocsp enabled,
> it could be further delayed. This differs from how OCSP validation works with
> SSL_read_early_data(). With this change, processing QUIC streams is unlocked
> when obtaining 0-RTT secret.
>
> diff --git a/src/event/quic/ngx_event_quic_ssl.c
> b/src/event/quic/ngx_event_quic_ssl.c
> --- a/src/event/quic/ngx_event_quic_ssl.c
> +++ b/src/event/quic/ngx_event_quic_ssl.c
> @@ -71,8 +71,20 @@ ngx_quic_set_read_secret(ngx_ssl_conn_t
> secret_len, rsecret);
> #endif
>
> - return ngx_quic_keys_set_encryption_secret(c->pool, 0, qc->keys, level,
> - cipher, rsecret, secret_len);
> + if (ngx_quic_keys_set_encryption_secret(c->pool, 0, qc->keys, level,
> + cipher, rsecret, secret_len)
> + != 1)
> + {
> + return 0;
> + }
> +
> + if (level == ssl_encryption_early_data) {
> + if (ngx_quic_init_streams(c) != NGX_OK) {
> + return 0;
> + }
> + }
> +
> + return 1;
> }
>
>
> @@ -131,6 +143,10 @@ ngx_quic_set_encryption_secrets(ngx_ssl_
> }
>
> if (level == ssl_encryption_early_data) {
> + if (ngx_quic_init_streams(c) != NGX_OK) {
> + return 0;
> + }
> +
> return 1;
> }
>
>
> --
> Sergey Kandaurov
>
Looks good.
--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
