Welcome! Log In Create A New Profile

Advanced

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov
October 17, 2023 06:40AM
> On 13 Oct 2023, at 19:13, Sergey Kandaurov <pluknet@nginx.com> wrote:
>
>
>> On 19 Sep 2023, at 17:53, Roman Arutyunyan <arut@nginx.com> wrote:
>>
>> Hi,
>>
>> On Thu, Sep 07, 2023 at 07:13:57PM +0400, Sergey Kandaurov wrote:
>>> # HG changeset patch
>>> # User Sergey Kandaurov <pluknet@nginx.com>
>>> # Date 1694099424 -14400
>>> # Thu Sep 07 19:10:24 2023 +0400
>>> # Node ID 28f7491bc79771f9cfa882b1b5584fa48ea42e6b
>>> # Parent 24e5d652ecc861f0c68607d20941abbf3726fdf1
>>> QUIC: reusing crypto contexts for packet protection.
>>>
>>> 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
>>> @@ -225,6 +225,7 @@ ngx_quic_new_connection(ngx_connection_t
>>> {
>>> ngx_uint_t i;
>>> ngx_quic_tp_t *ctp;
>>> + ngx_pool_cleanup_t *cln;
>>> ngx_quic_connection_t *qc;
>>>
>>> qc = ngx_pcalloc(c->pool, sizeof(ngx_quic_connection_t));
>>> @@ -237,6 +238,14 @@ ngx_quic_new_connection(ngx_connection_t
>>> return NULL;
>>> }
>>>
>>> + cln = ngx_pool_cleanup_add(c->pool, 0);
>>> + if (cln == NULL) {
>>> + return NULL;
>>> + }
>>> +
>>> + cln->handler = ngx_quic_keys_cleanup;
>>> + cln->data = qc->keys;
>>
>> I think it's better to cleanup keys in ngx_quic_close_connection().
>> We do the same with sockets by calling ngx_quic_close_sockets().
>> We just have to carefully handle the errors later in this function and cleanup
>> keys when ngx_quic_open_sockets() fails.
>
> While this may look error prone compared with the cleanup handler,
> you convinced me to remove it because of ngx_quic_send_early_cc().
> To be merged:
>
> 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
> @@ -227,7 +227,6 @@ ngx_quic_new_connection(ngx_connection_t
> {
> ngx_uint_t i;
> ngx_quic_tp_t *ctp;
> - ngx_pool_cleanup_t *cln;
> ngx_quic_connection_t *qc;
>
> qc = ngx_pcalloc(c->pool, sizeof(ngx_quic_connection_t));
> @@ -240,14 +239,6 @@ ngx_quic_new_connection(ngx_connection_t
> return NULL;
> }
>
> - cln = ngx_pool_cleanup_add(c->pool, 0);
> - if (cln == NULL) {
> - return NULL;
> - }
> -
> - cln->handler = ngx_quic_keys_cleanup;
> - cln->data = qc->keys;
> -
> qc->version = pkt->version;
>
> ngx_rbtree_init(&qc->streams.tree, &qc->streams.sentinel,
> @@ -344,6 +335,7 @@ ngx_quic_new_connection(ngx_connection_t
> qc->validated = pkt->validated;
>
> if (ngx_quic_open_sockets(c, qc, pkt) != NGX_OK) {
> + ngx_quic_keys_cleanup(qc->keys);
> return NULL;
> }
>
> @@ -594,6 +586,8 @@ ngx_quic_close_connection(ngx_connection
>
> ngx_quic_close_sockets(c);
>
> + ngx_quic_keys_cleanup(qc->keys);
> +
> ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0, "quic close completed");
>
> /* may be tested from SSL callback during SSL shutdown */
> diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
> --- a/src/event/quic/ngx_event_quic_output.c
> +++ b/src/event/quic/ngx_event_quic_output.c
> @@ -941,13 +941,17 @@ ngx_quic_send_early_cc(ngx_connection_t
> res.data = dst;
>
> if (ngx_quic_encrypt(&pkt, &res) != NGX_OK) {
> + ngx_quic_keys_cleanup(pkt.keys);
> return NGX_ERROR;
> }
>
> if (ngx_quic_send(c, res.data, res.len, c->sockaddr, c->socklen) < 0) {
> + ngx_quic_keys_cleanup(pkt.keys);
> return NGX_ERROR;
> }
>
> + ngx_quic_keys_cleanup(pkt.keys);
> +
> return NGX_DONE;
> }
>
> diff --git a/src/event/quic/ngx_event_quic_protection.c b/src/event/quic/ngx_event_quic_protection.c
> --- a/src/event/quic/ngx_event_quic_protection.c
> +++ b/src/event/quic/ngx_event_quic_protection.c
> @@ -189,10 +189,16 @@ ngx_quic_keys_set_initial_secret(ngx_qui
> }
>
> if (ngx_quic_crypto_init(ciphers.c, server, 1, log) == NGX_ERROR) {
> - return NGX_ERROR;
> + goto failed;
> }
>
> return NGX_OK;
> +
> +failed:
> +
> + ngx_quic_keys_cleanup(keys);
> +
> + return NGX_ERROR;
> }
>
>
> @@ -793,10 +799,8 @@ failed:
>
>
> void
> -ngx_quic_keys_cleanup(void *data)
> +ngx_quic_keys_cleanup(ngx_quic_keys_t *keys)
> {
> - ngx_quic_keys_t *keys = data;
> -
> size_t i;
> ngx_quic_secrets_t *secrets;
>
> diff --git a/src/event/quic/ngx_event_quic_protection.h b/src/event/quic/ngx_event_quic_protection.h
> --- a/src/event/quic/ngx_event_quic_protection.h
> +++ b/src/event/quic/ngx_event_quic_protection.h
> @@ -103,7 +103,7 @@ void ngx_quic_keys_discard(ngx_quic_keys
> enum ssl_encryption_level_t level);
> void ngx_quic_keys_switch(ngx_connection_t *c, ngx_quic_keys_t *keys);
> void ngx_quic_keys_update(ngx_event_t *ev);
> -void ngx_quic_keys_cleanup(void *data);
> +void ngx_quic_keys_cleanup(ngx_quic_keys_t *keys);
> ngx_int_t ngx_quic_encrypt(ngx_quic_header_t *pkt, ngx_str_t *res);
> ngx_int_t ngx_quic_decrypt(ngx_quic_header_t *pkt, uint64_t *largest_pn);
> void ngx_quic_compute_nonce(u_char *nonce, size_t len, uint64_t pn);
>
>
>>
>>> qc->version = pkt->version;
>>>
>>> ngx_rbtree_init(&qc->streams.tree, &qc->streams.sentinel,
>>> 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
>>> @@ -54,9 +54,10 @@ struct ngx_quic_compat_s {
>>>
>>>
>>> static void ngx_quic_compat_keylog_callback(const SSL *ssl, const char *line);
>>> -static ngx_int_t ngx_quic_compat_set_encryption_secret(ngx_log_t *log,
>>> +static ngx_int_t ngx_quic_compat_set_encryption_secret(ngx_connection_t *c,
>>> ngx_quic_compat_keys_t *keys, enum ssl_encryption_level_t level,
>>> const SSL_CIPHER *cipher, const uint8_t *secret, size_t secret_len);
>>> +static void ngx_quic_compat_cleanup_encryption_secret(void *data);
>>> static int ngx_quic_compat_add_transport_params_callback(SSL *ssl,
>>> unsigned int ext_type, unsigned int context, const unsigned char **out,
>>> size_t *outlen, X509 *x, size_t chainidx, int *al, void *add_arg);
>>> @@ -214,14 +215,14 @@ ngx_quic_compat_keylog_callback(const SS
>>> com->method->set_read_secret((SSL *) ssl, level, cipher, secret, n);
>>> com->read_record = 0;
>>>
>>> - (void) ngx_quic_compat_set_encryption_secret(c->log, &com->keys, level,
>>> + (void) ngx_quic_compat_set_encryption_secret(c, &com->keys, level,
>>> cipher, secret, n);
>>> }
>>> }
>>>
>>>
>>> static ngx_int_t
>>> -ngx_quic_compat_set_encryption_secret(ngx_log_t *log,
>>> +ngx_quic_compat_set_encryption_secret(ngx_connection_t *c,
>>> ngx_quic_compat_keys_t *keys, enum ssl_encryption_level_t level,
>>> const SSL_CIPHER *cipher, const uint8_t *secret, size_t secret_len)
>>> {
>>> @@ -231,6 +232,7 @@ ngx_quic_compat_set_encryption_secret(ng
>>> ngx_quic_hkdf_t seq[2];
>>> ngx_quic_secret_t *peer_secret;
>>> ngx_quic_ciphers_t ciphers;
>>> + ngx_pool_cleanup_t *cln;
>>>
>>> peer_secret = &keys->secret;
>>>
>>> @@ -239,12 +241,12 @@ ngx_quic_compat_set_encryption_secret(ng
>>> key_len = ngx_quic_ciphers(keys->cipher, &ciphers, level);
>>>
>>> if (key_len == NGX_ERROR) {
>>> - ngx_ssl_error(NGX_LOG_INFO, log, 0, "unexpected cipher");
>>> + ngx_ssl_error(NGX_LOG_INFO, c->log, 0, "unexpected cipher");
>>> return NGX_ERROR;
>>> }
>>>
>>> if (sizeof(peer_secret->secret.data) < secret_len) {
>>> - ngx_log_error(NGX_LOG_ALERT, log, 0,
>>> + ngx_log_error(NGX_LOG_ALERT, c->log, 0,
>>> "unexpected secret len: %uz", secret_len);
>>> return NGX_ERROR;
>>> }
>>> @@ -262,15 +264,42 @@ ngx_quic_compat_set_encryption_secret(ng
>>> ngx_quic_hkdf_set(&seq[1], "tls13 iv", &peer_secret->iv, &secret_str);
>>>
>>> for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
>>> - if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, log) != NGX_OK) {
>>> + if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, c->log) != NGX_OK) {
>>> return NGX_ERROR;
>>> }
>>> }
>>>
>>> + ngx_quic_crypto_cleanup(peer_secret);
>>> +
>>> + if (ngx_quic_crypto_init(ciphers.c, peer_secret, 1, c->log) == NGX_ERROR) {
>>> + return NGX_ERROR;
>>> + }
>>> +
>>> + /* register cleanup handler once */
>>> +
>>> + if (level == ssl_encryption_handshake) {
>>
>> Does not look perfect, but I don't see a simpler and better solution.
>
> I don't see either, without introducing some state (see below).
>
>>
>>> + cln = ngx_pool_cleanup_add(c->pool, 0);
>>> + if (cln == NULL) {
>>
>> Cleanup peer_secret here?
>>
>> Alternatively, move this block up.
>
> Fixed, thanks.
>
> With introducing keys->cleanup:
>
> 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
> @@ -25,6 +25,7 @@
> typedef struct {
> ngx_quic_secret_t secret;
> ngx_uint_t cipher;
> + ngx_uint_t cleanup; /* unsigned cleanup:1 */
> } ngx_quic_compat_keys_t;
>
>
> @@ -269,15 +270,11 @@ ngx_quic_compat_set_encryption_secret(ng
> }
> }
>
> - ngx_quic_crypto_cleanup(peer_secret);
> -
> - if (ngx_quic_crypto_init(ciphers.c, peer_secret, 1, c->log) == NGX_ERROR) {
> - return NGX_ERROR;
> - }
> -
> /* register cleanup handler once */
>
> - if (level == ssl_encryption_handshake) {
> + if (!keys->cleanup) {
> + keys->cleanup = 1;
> +
> cln = ngx_pool_cleanup_add(c->pool, 0);
> if (cln == NULL) {
> return NGX_ERROR;
> @@ -287,6 +284,12 @@ ngx_quic_compat_set_encryption_secret(ng
> cln->data = peer_secret;
> }
>
> + ngx_quic_crypto_cleanup(peer_secret);
> +
> + if (ngx_quic_crypto_init(ciphers.c, peer_secret, 1, c->log) == NGX_ERROR) {
> + return NGX_ERROR;
> + }
> +
> return NGX_OK;
> }
>

Version without a cleanup field (to be merged).
While it may look less readable (and I like it less),
it allows to save ngx_uint_t per connection.

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
@@ -269,15 +269,12 @@ ngx_quic_compat_set_encryption_secret(ng
}
}

- ngx_quic_crypto_cleanup(peer_secret);
-
- if (ngx_quic_crypto_init(ciphers.c, peer_secret, 1, c->log) == NGX_ERROR) {
- return NGX_ERROR;
- }
-
/* register cleanup handler once */

- if (level == ssl_encryption_handshake) {
+ if (peer_secret->ctx) {
+ ngx_quic_crypto_cleanup(peer_secret);
+
+ } else {
cln = ngx_pool_cleanup_add(c->pool, 0);
if (cln == NULL) {
return NGX_ERROR;
@@ -287,6 +284,10 @@ ngx_quic_compat_set_encryption_secret(ng
cln->data = peer_secret;
}

+ if (ngx_quic_crypto_init(ciphers.c, peer_secret, 1, c->log) == NGX_ERROR) {
+ return NGX_ERROR;
+ }
+
return NGX_OK;
}


>
>>
>>> + return NGX_ERROR;
>>> + }
>>> +
>>> + cln->handler = ngx_quic_compat_cleanup_encryption_secret;
>>> + cln->data = peer_secret;
>>> + }
>>> +
>>> return NGX_OK;
>>> }
>>>
>>>
>>> +static void
>>> +ngx_quic_compat_cleanup_encryption_secret(void *data)
>>> +{
>>> + ngx_quic_secret_t *secret = data;
>>> +
>>> + ngx_quic_crypto_cleanup(secret);
>>> +}
>>> +
>>> +

[..]

--
Sergey Kandaurov
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH 0 of 8] [quic] reusing crypto contexts, and more

Sergey Kandaurov 485 September 07, 2023 11:18AM

[PATCH 1 of 8] QUIC: split keys availability checks to read and write sides

Sergey Kandaurov 86 September 07, 2023 11:18AM

Re: [PATCH 1 of 8] QUIC: split keys availability checks to read and write sides

Roman Arutyunyan 85 September 21, 2023 09:30AM

[PATCH 2 of 8] QUIC: added check to prevent packet output with discarded keys

Sergey Kandaurov 82 September 07, 2023 11:18AM

Re: [PATCH 2 of 8] QUIC: added check to prevent packet output with discarded keys

Roman Arutyunyan 89 September 18, 2023 03:10AM

Re: [PATCH 2 of 8] QUIC: added check to prevent packet output with discarded keys

Sergey Kandaurov 99 October 13, 2023 11:10AM

[PATCH 3 of 8] QUIC: prevented output of ACK frame when discarding handshake keys

Sergey Kandaurov 80 September 07, 2023 11:18AM

[PATCH 4 of 8] QUIC: renamed protection functions

Sergey Kandaurov 80 September 07, 2023 11:18AM

Re: [PATCH 4 of 8] QUIC: renamed protection functions

Roman Arutyunyan 88 September 21, 2023 09:32AM

[PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 85 September 07, 2023 11:18AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Roman Arutyunyan 101 September 19, 2023 09:54AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 85 October 13, 2023 11:14AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 78 October 17, 2023 06:40AM

Re: [PATCH 5 of 8] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 97 October 23, 2023 06:38PM

[PATCH 6 of 8] QUIC: reusing crypto contexts for header protection

Sergey Kandaurov 81 September 07, 2023 11:18AM

Re: [PATCH 6 of 8] QUIC: reusing crypto contexts for header protection

Roman Arutyunyan 80 September 20, 2023 08:14AM

Re: [PATCH 6 of 8] QUIC: reusing crypto contexts for header protection

Sergey Kandaurov 75 October 13, 2023 11:14AM

[PATCH 7 of 8] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Sergey Kandaurov 86 September 07, 2023 11:18AM

Re: [PATCH 7 of 8] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Roman Arutyunyan 95 September 20, 2023 08:28AM

Re: [PATCH 7 of 8] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Sergey Kandaurov 87 October 13, 2023 11:16AM

[PATCH 8 of 8] QUIC: explicitly zero out unused keying material

Sergey Kandaurov 79 September 07, 2023 11:18AM

Re: [PATCH 8 of 8] QUIC: explicitly zero out unused keying material

Roman Arutyunyan 83 September 21, 2023 09:30AM

Re: [PATCH 8 of 8] QUIC: explicitly zero out unused keying material

Sergey Kandaurov 73 October 13, 2023 11:16AM

[PATCH 00 of 11] [quic] reusing crypto contexts, and more #2

Sergey Kandaurov 78 October 18, 2023 11:28AM

[PATCH 01 of 11] QUIC: split keys availability checks to read and write sides

Sergey Kandaurov 76 October 18, 2023 11:28AM

[PATCH 02 of 11] QUIC: added safety belt to prevent using discarded keys

Sergey Kandaurov 77 October 18, 2023 11:28AM

[PATCH 03 of 11] QUIC: prevented generating ACK frames with discarded keys

Sergey Kandaurov 80 October 18, 2023 11:28AM

[PATCH 04 of 11] QUIC: renamed protection functions

Sergey Kandaurov 73 October 18, 2023 11:28AM

[PATCH 05 of 11] QUIC: reusing crypto contexts for packet protection

Sergey Kandaurov 76 October 18, 2023 11:28AM

[PATCH 06 of 11] QUIC: common code for crypto open and seal operations

Sergey Kandaurov 76 October 18, 2023 11:28AM

[PATCH 07 of 11] QUIC: reusing crypto contexts for header protection

Sergey Kandaurov 74 October 18, 2023 11:30AM

[PATCH 08 of 11] QUIC: cleaned up now unused ngx_quic_ciphers() calls

Sergey Kandaurov 73 October 18, 2023 11:30AM

[PATCH 09 of 11] QUIC: simplified ngx_quic_ciphers() API

Sergey Kandaurov 72 October 18, 2023 11:30AM

[PATCH 10 of 11] QUIC: removed key field from ngx_quic_secret_t

Sergey Kandaurov 75 October 18, 2023 11:30AM

[PATCH 11 of 11] QUIC: explicitly zero out unused keying material

Sergey Kandaurov 74 October 18, 2023 11:38AM

Re: [PATCH 00 of 11] [quic] reusing crypto contexts, and more #2

Roman Arutyunyan 81 October 20, 2023 03:28AM



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

Online Users

Guests: 337
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