Welcome! Log In Create A New Profile

Advanced

Re: [PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types

Sergey Kandaurov
September 01, 2023 11:40AM
> On 1 Sep 2023, at 13:55, Roman Arutyunyan <arut@nginx.com> wrote:
>
> Hi,
>
> On Thu, Aug 31, 2023 at 06:59:46PM +0400, Sergey Kandaurov wrote:
>> # HG changeset patch
>> # User Sergey Kandaurov <pluknet@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).


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@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@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@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types

Sergey Kandaurov 269 August 31, 2023 11:00AM

Re: [PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types

Sergey Kandaurov 74 August 31, 2023 11:04AM

Re: [PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types

Roman Arutyunyan 71 September 01, 2023 05:58AM

Re: [PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types

Sergey Kandaurov 72 September 01, 2023 11:40AM

Re: [PATCH] QUIC: refined sending CONNECTION_CLOSE in various packet types

Roman Arutyunyan 90 September 01, 2023 11:56AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 75
Record Number of Users: 8 on April 13, 2023
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready