[PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types
Roman Arutyunyan
arut at nginx.com
Fri Sep 1 15:54:52 UTC 2023
Hi,
On Fri, Sep 01, 2023 at 07:39:46PM +0400, Sergey Kandaurov wrote:
>
> > On 1 Sep 2023, at 13:55, Roman Arutyunyan <arut at nginx.com> wrote:
> >
> > Hi,
> >
> > On Thu, Aug 31, 2023 at 06:59:46PM +0400, Sergey Kandaurov wrote:
> >> # HG changeset patch
> >> # User Sergey Kandaurov <pluknet at nginx.com>
> >> # Date 1693493736 -14400
> >> # Thu Aug 31 18:55:36 2023 +0400
> >> # Node ID 358c657a4a7afef502a00b9a41bddbe08f6859ae
> >> # Parent 015353ca1be7acc176f6369ed92ec6c49975ee6a
> >> QUIC: refined sending CONNECTION_CLOSE in various packet types.
> >>
> >> As per RFC 9000, section 10.2.3, to ensure that peer successfully removed
> >> packet protection, CONNECTION_CLOSE can be sent in multiple packets using
> >> different packet protection levels.
> >>
> >> Specifically, new logic is added to more strictly follow these rules:
> >>
> >> - by default, the highest available level of packet protection is used;
> >> - unless handshake is confirmed, but we have got application keys available,
> >> that means the client may have or may have not application keys to remove
> >
> > may *not* have ?
>
> Thanks, replaced with "the client may or may not have".
>
> >
> >> 1-RTT packet protection; in such case, send both 1-RTT and HS packets;
> >> - additionally, if we still have initial protection keys not yet discarded,
> >> which happens if the path was not yet validated by successfully removing
> >> Handshake packet protection, that means the client may not have handshake
> >> keys; in such case, send an Initial packet too.
> >>
> >> This roughly resembles the following paragraph:
> >>
> >> * Prior to confirming the handshake, a peer might be unable to process 1-RTT
> >> packets, so an endpoint SHOULD send a CONNECTION_CLOSE frame in both Handshake
> >> and 1-RTT packets. A server SHOULD also send a CONNECTION_CLOSE frame in an
> >> Initial packet.
> >>
> >> In practice, this change allows to avoid sending Initial packet when we know
> >> the client has handshake keys, and fixes sending CONNECTION_CLOSE when using
> >> QuicTLS with old QUIC API, where TLS stack releases application read keys
> >> before handshake confirmation, sending it additionally in a Handshake packet.
> >>
> >> 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
> >> @@ -540,8 +540,20 @@ ngx_quic_close_connection(ngx_connection
> >>
> >> (void) ngx_quic_send_cc(c);
> >>
> >> - if (qc->error_level == ssl_encryption_handshake) {
> >> - /* for clients that might not have handshake keys */
> >> + if (qc->error_level == ssl_encryption_application
> >> + && ngx_quic_keys_available(qc->keys,
> >> + ssl_encryption_handshake))
> >> + {
> >> + /* handshake not confirmed, client may not have app keys */
> >> + qc->error_level = ssl_encryption_handshake;
> >> + (void) ngx_quic_send_cc(c);
> >> + }
> >> +
> >> + if (qc->error_level == ssl_encryption_handshake
> >> + && ngx_quic_keys_available(qc->keys,
> >> + ssl_encryption_initial))
> >> + {
> >> + /* path not validated, client may not have hs keys */
> >> qc->error_level = ssl_encryption_initial;
> >> (void) ngx_quic_send_cc(c);
> >> }
> >
> > I have a feeling that we can just send CC for all levels for which keys are
> > available:
> >
> > for (i = 0; i < NGX_QUIC_SEND_CTX_LAST; i++) {
> > ctx = &qc->send_ctx[i];
> >
> > if (!ngx_quic_keys_available(qc->keys, ctx->level)) {
> > continue;
> > }
> >
> > qc->error_level = ctx->level;
> > (void) ngx_quic_send_cc(c);
> >
> > if (rc == NGX_OK && !qc->close.timer_set) {
> > ngx_add_timer(&qc->close, 3 * ngx_quic_pto(c, ctx));
> > }
> > }
>
> Agree, this should be equivalent code, and probably it is simpler.
> It also removes the need to set something to qc->error_level above
> in the patch that eliminates the use of SSL_quic_read/write_level.
> So these lines can be also removed:
>
> qc->error_level = c->ssl ? SSL_quic_read_level(c->ssl->connection)
> : ssl_encryption_initial;
>
> I had a concern to keep setting PTO out of the cycle, to use the highest
> protection level available, because ngx_quic_pto() computation depends on
> a passed level, it takes into account max_ack_delay on the app level.
> But looking at ngx_quic_pto() I see this depends on whether we have
> handshaked (c->ssl->handshaked). If yes, then lower levels are already
> discarded and will be skipped in the cycle, so this should be safe:
> either the timer is set for application level only (with max_ack_delay),
> or for any first level available (and max_ack_delay isn't applied).
Yes, exactly.
> So it looks good in the end, applied.
> Also, refined commit logs to reflect the change.
> Since this and the "levels" patch now depend, below are both:
>
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1693581796 -14400
> # Fri Sep 01 19:23:16 2023 +0400
> # Node ID 139d7219cecbc99ebf74475cd6446a5a81f9745f
> # Parent 8f7e6d8c061e1f4b7656141d2fac85ce6846ac23
> QUIC: refined sending CONNECTION_CLOSE in various packet types.
>
> As per RFC 9000, section 10.2.3, to ensure that peer successfully removed
> packet protection, CONNECTION_CLOSE can be sent in multiple packets using
> different packet protection levels.
>
> Now it is sent in all protection levels available.
> This roughly corresponds to the following paragraph:
>
> * Prior to confirming the handshake, a peer might be unable to process 1-RTT
> packets, so an endpoint SHOULD send a CONNECTION_CLOSE frame in both Handshake
> and 1-RTT packets. A server SHOULD also send a CONNECTION_CLOSE frame in an
> Initial packet.
>
> In practice, this change allows to avoid sending an Initial packet when we know
> the client has handshake keys, by checking if we have discarded initial keys.
> Also, this fixes sending CONNECTION_CLOSE when using QuicTLS with old QUIC API,
> where TLS stack releases application read keys before handshake confirmation;
> it is fixed by sending CONNECTION_CLOSE additionally in a Handshake packet.
>
> 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
> @@ -509,9 +509,6 @@ ngx_quic_close_connection(ngx_connection
> * to terminate the connection immediately.
> */
>
> - qc->error_level = c->ssl ? SSL_quic_read_level(c->ssl->connection)
> - : ssl_encryption_initial;
> -
> if (qc->error == (ngx_uint_t) -1) {
> qc->error = NGX_QUIC_ERR_INTERNAL_ERROR;
> qc->error_app = 0;
> @@ -524,17 +521,19 @@ ngx_quic_close_connection(ngx_connection
> qc->error_app ? "app " : "", qc->error,
> qc->error_reason ? qc->error_reason : "");
>
> - if (rc == NGX_OK) {
> - ctx = ngx_quic_get_send_ctx(qc, qc->error_level);
> - ngx_add_timer(&qc->close, 3 * ngx_quic_pto(c, ctx));
> - }
> + for (i = 0; i < NGX_QUIC_SEND_CTX_LAST; i++) {
> + ctx = &qc->send_ctx[i];
> +
> + if (!ngx_quic_keys_available(qc->keys, ctx->level)) {
> + continue;
> + }
>
> - (void) ngx_quic_send_cc(c);
> + qc->error_level = ctx->level;
> + (void) ngx_quic_send_cc(c);
>
> - if (qc->error_level == ssl_encryption_handshake) {
> - /* for clients that might not have handshake keys */
> - qc->error_level = ssl_encryption_initial;
> - (void) ngx_quic_send_cc(c);
> + if (rc == NGX_OK && !qc->close.timer_set) {
> + ngx_add_timer(&qc->close, 3 * ngx_quic_pto(c, ctx));
> + }
> }
> }
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1693582604 -14400
> # Fri Sep 01 19:36:44 2023 +0400
> # Node ID 207a930a2c1d7467e7a8e3c98e1d3851fcdd4d2f
> # Parent 139d7219cecbc99ebf74475cd6446a5a81f9745f
> QUIC: removed use of SSL_quic_read_level and SSL_quic_write_level.
>
> As explained in BoringSSL change[1], levels were introduced in the original
> QUIC API to draw a line between when keys are released and when are active.
> In the new QUIC API they are released in separate calls when it's needed.
> BoringSSL has then a consideration to remove levels API, hence the change.
>
> If not available e.g. from a QUIC packet header, levels can be taken based on
> keys availability. The only real use of levels is to prevent using app keys
> before they are active in QuicTLS that provides the old BoringSSL QUIC API,
> it is replaced with an equivalent check of c->ssl->handshaked.
>
> This change also removes OpenSSL compat shims since they are no longer used.
> The only exception left is caching write level from the keylog callback in
> the internal field which is a handy equivalent of checking keys availability.
>
> [1] https://boringssl.googlesource.com/boringssl/+/1e859054
>
> 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
> @@ -963,10 +963,7 @@ ngx_quic_handle_payload(ngx_connection_t
> #if !defined (OPENSSL_IS_BORINGSSL)
> /* OpenSSL provides read keys for an application level before it's ready */
>
> - if (pkt->level == ssl_encryption_application
> - && SSL_quic_read_level(c->ssl->connection)
> - < ssl_encryption_application)
> - {
> + if (pkt->level == ssl_encryption_application && !c->ssl->handshaked) {
> ngx_log_error(NGX_LOG_INFO, c->log, 0,
> "quic no %s keys ready, ignoring packet",
> ngx_quic_level_name(pkt->level));
> diff --git a/src/event/quic/ngx_event_quic_openssl_compat.c b/src/event/quic/ngx_event_quic_openssl_compat.c
> --- a/src/event/quic/ngx_event_quic_openssl_compat.c
> +++ b/src/event/quic/ngx_event_quic_openssl_compat.c
> @@ -44,7 +44,6 @@ struct ngx_quic_compat_s {
> const SSL_QUIC_METHOD *method;
>
> enum ssl_encryption_level_t write_level;
> - enum ssl_encryption_level_t read_level;
>
> uint64_t read_record;
> ngx_quic_compat_keys_t keys;
> @@ -213,7 +212,6 @@ ngx_quic_compat_keylog_callback(const SS
>
> } else {
> com->method->set_read_secret((SSL *) ssl, level, cipher, secret, n);
> - com->read_level = level;
> com->read_record = 0;
>
> (void) ngx_quic_compat_set_encryption_secret(c->log, &com->keys, level,
> @@ -583,32 +581,6 @@ ngx_quic_compat_create_record(ngx_quic_c
> }
>
>
> -enum ssl_encryption_level_t
> -SSL_quic_read_level(const SSL *ssl)
> -{
> - ngx_connection_t *c;
> - ngx_quic_connection_t *qc;
> -
> - c = ngx_ssl_get_connection(ssl);
> - qc = ngx_quic_get_connection(c);
> -
> - return qc->compat->read_level;
> -}
> -
> -
> -enum ssl_encryption_level_t
> -SSL_quic_write_level(const SSL *ssl)
> -{
> - ngx_connection_t *c;
> - ngx_quic_connection_t *qc;
> -
> - c = ngx_ssl_get_connection(ssl);
> - qc = ngx_quic_get_connection(c);
> -
> - return qc->compat->write_level;
> -}
> -
> -
> int
> SSL_set_quic_transport_params(SSL *ssl, const uint8_t *params,
> size_t params_len)
> diff --git a/src/event/quic/ngx_event_quic_openssl_compat.h b/src/event/quic/ngx_event_quic_openssl_compat.h
> --- a/src/event/quic/ngx_event_quic_openssl_compat.h
> +++ b/src/event/quic/ngx_event_quic_openssl_compat.h
> @@ -48,8 +48,6 @@ ngx_int_t ngx_quic_compat_init(ngx_conf_
> int SSL_set_quic_method(SSL *ssl, const SSL_QUIC_METHOD *quic_method);
> int SSL_provide_quic_data(SSL *ssl, enum ssl_encryption_level_t level,
> const uint8_t *data, size_t len);
> -enum ssl_encryption_level_t SSL_quic_read_level(const SSL *ssl);
> -enum ssl_encryption_level_t SSL_quic_write_level(const SSL *ssl);
> int SSL_set_quic_transport_params(SSL *ssl, const uint8_t *params,
> size_t params_len);
> void SSL_get_peer_quic_transport_params(const SSL *ssl,
> 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
> @@ -43,7 +43,8 @@ static int ngx_quic_add_handshake_data(n
> static int ngx_quic_flush_flight(ngx_ssl_conn_t *ssl_conn);
> static int ngx_quic_send_alert(ngx_ssl_conn_t *ssl_conn,
> enum ssl_encryption_level_t level, uint8_t alert);
> -static ngx_int_t ngx_quic_crypto_input(ngx_connection_t *c, ngx_chain_t *data);
> +static ngx_int_t ngx_quic_crypto_input(ngx_connection_t *c, ngx_chain_t *data,
> + enum ssl_encryption_level_t level);
>
>
> #if (NGX_QUIC_BORINGSSL_API)
> @@ -354,7 +355,7 @@ ngx_quic_handle_crypto_frame(ngx_connect
> }
>
> if (f->offset == ctx->crypto.offset) {
> - if (ngx_quic_crypto_input(c, frame->data) != NGX_OK) {
> + if (ngx_quic_crypto_input(c, frame->data, pkt->level) != NGX_OK) {
> return NGX_ERROR;
> }
>
> @@ -372,7 +373,7 @@ ngx_quic_handle_crypto_frame(ngx_connect
> cl = ngx_quic_read_buffer(c, &ctx->crypto, (uint64_t) -1);
>
> if (cl) {
> - if (ngx_quic_crypto_input(c, cl) != NGX_OK) {
> + if (ngx_quic_crypto_input(c, cl, pkt->level) != NGX_OK) {
> return NGX_ERROR;
> }
>
> @@ -384,7 +385,8 @@ ngx_quic_handle_crypto_frame(ngx_connect
>
>
> static ngx_int_t
> -ngx_quic_crypto_input(ngx_connection_t *c, ngx_chain_t *data)
> +ngx_quic_crypto_input(ngx_connection_t *c, ngx_chain_t *data,
> + enum ssl_encryption_level_t level)
> {
> int n, sslerr;
> ngx_buf_t *b;
> @@ -397,17 +399,10 @@ ngx_quic_crypto_input(ngx_connection_t *
>
> ssl_conn = c->ssl->connection;
>
> - ngx_log_debug2(NGX_LOG_DEBUG_EVENT, c->log, 0,
> - "quic SSL_quic_read_level:%d SSL_quic_write_level:%d",
> - (int) SSL_quic_read_level(ssl_conn),
> - (int) SSL_quic_write_level(ssl_conn));
> -
> for (cl = data; cl; cl = cl->next) {
> b = cl->buf;
>
> - if (!SSL_provide_quic_data(ssl_conn, SSL_quic_read_level(ssl_conn),
> - b->pos, b->last - b->pos))
> - {
> + if (!SSL_provide_quic_data(ssl_conn, level, b->pos, b->last - b->pos)) {
> ngx_ssl_error(NGX_LOG_INFO, c->log, 0,
> "SSL_provide_quic_data() failed");
> return NGX_ERROR;
> @@ -416,11 +411,6 @@ ngx_quic_crypto_input(ngx_connection_t *
>
> n = SSL_do_handshake(ssl_conn);
>
> - ngx_log_debug2(NGX_LOG_DEBUG_EVENT, c->log, 0,
> - "quic SSL_quic_read_level:%d SSL_quic_write_level:%d",
> - (int) SSL_quic_read_level(ssl_conn),
> - (int) SSL_quic_write_level(ssl_conn));
> -
> ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0, "SSL_do_handshake: %d", n);
>
> if (n <= 0) {
>
> --
> Sergey Kandaurov
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
Both patches look ok.
--
Roman Arutyunyan
More information about the nginx-devel
mailing list