> On 13 Oct 2023, at 19:13, Sergey Kandaurov <pluk...@nginx.com> wrote:
>
>
>> On 19 Sep 2023, at 17:53, Roman Arutyunyan <a...@nginx.com> wrote:
>>
>> Hi,
>>
>> On Thu, Sep 07, 2023 at 07:13:57PM +0400, Sergey Kandaurov wrote:
>>> # HG changeset patch
>>> # User Sergey Kandaurov <pluk...@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
