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

Sergey Kandaurov pluknet at nginx.com
Fri Oct 13 15:13:00 UTC 2023


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

> 
>> +            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);
>> +}
>> +
>> +
>> 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,
>> @@ -568,8 +597,7 @@ ngx_quic_compat_create_record(ngx_quic_c
>>     ngx_memcpy(nonce, secret->iv.data, secret->iv.len);
>>     ngx_quic_compute_nonce(nonce, sizeof(nonce), rec->number);
>> 
>> -    if (ngx_quic_crypto_seal(ciphers.c, secret, &out,
>> -                             nonce, &rec->payload, &ad, rec->log)
>> +    if (ngx_quic_crypto_seal(secret, &out, nonce, &rec->payload, &ad, rec->log)
>>         != NGX_OK)
>>     {
>>         return NGX_ERROR;
>> 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
>> @@ -26,9 +26,8 @@ static ngx_int_t ngx_hkdf_extract(u_char
>> static uint64_t ngx_quic_parse_pn(u_char **pos, ngx_int_t len, u_char *mask,
>>     uint64_t *largest_pn);
>> 
>> -static ngx_int_t ngx_quic_crypto_open(const ngx_quic_cipher_t *cipher,
>> -    ngx_quic_secret_t *s, ngx_str_t *out, u_char *nonce, ngx_str_t *in,
>> -    ngx_str_t *ad, ngx_log_t *log);
>> +static ngx_int_t ngx_quic_crypto_open(ngx_quic_secret_t *s, ngx_str_t *out,
>> +    u_char *nonce, ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log);
>> static ngx_int_t ngx_quic_crypto_hp(ngx_log_t *log, const EVP_CIPHER *cipher,
>>     ngx_quic_secret_t *s, u_char *out, u_char *in);
>> 
>> @@ -108,13 +107,14 @@ ngx_int_t
>> ngx_quic_keys_set_initial_secret(ngx_quic_keys_t *keys, ngx_str_t *secret,
>>     ngx_log_t *log)
>> {
>> -    size_t              is_len;
>> -    uint8_t             is[SHA256_DIGEST_LENGTH];
>> -    ngx_str_t           iss;
>> -    ngx_uint_t          i;
>> -    const EVP_MD       *digest;
>> -    ngx_quic_hkdf_t     seq[8];
>> -    ngx_quic_secret_t  *client, *server;
>> +    size_t               is_len;
>> +    uint8_t              is[SHA256_DIGEST_LENGTH];
>> +    ngx_str_t            iss;
>> +    ngx_uint_t           i;
>> +    const EVP_MD        *digest;
>> +    ngx_quic_hkdf_t      seq[8];
>> +    ngx_quic_secret_t   *client, *server;
>> +    ngx_quic_ciphers_t   ciphers;
>> 
>>     static const uint8_t salt[20] =
>>         "\x38\x76\x2c\xf7\xf5\x59\x34\xb3\x4d\x17"
>> @@ -180,6 +180,18 @@ ngx_quic_keys_set_initial_secret(ngx_qui
>>         }
>>     }
>> 
>> +    if (ngx_quic_ciphers(0, &ciphers, ssl_encryption_initial) == NGX_ERROR) {
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    if (ngx_quic_crypto_init(ciphers.c, client, 0, log) == NGX_ERROR) {
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    if (ngx_quic_crypto_init(ciphers.c, server, 1, log) == NGX_ERROR) {
> 
> Call ngx_quic_crypto_cleanup() for client here?

Yes, it is needed for ngx_quic_send_early_cc().
Added.

> This function is called from ngx_quic_send_early_cc(), which has no keys
> cleanup handler (and I propose we remove it from regular QUIC connections as
> well).

See the above patch.

> 
>> +        return NGX_ERROR;
>> +    }
>> +
>>     return NGX_OK;
>> }
>> 
>> @@ -343,9 +355,9 @@ failed:
>> }
>> 
>> 
>> -static ngx_int_t
>> -ngx_quic_crypto_open(const ngx_quic_cipher_t *cipher, ngx_quic_secret_t *s,
>> -    ngx_str_t *out, u_char *nonce, ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log)
>> +ngx_int_t
>> +ngx_quic_crypto_init(const ngx_quic_cipher_t *cipher, ngx_quic_secret_t *s,
>> +    ngx_int_t enc, ngx_log_t *log)
>> {
>> 
>> #ifdef OPENSSL_IS_BORINGSSL
>> @@ -357,19 +369,7 @@ ngx_quic_crypto_open(const ngx_quic_ciph
>>         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_new() failed");
>>         return NGX_ERROR;
>>     }
>> -
>> -    if (EVP_AEAD_CTX_open(ctx, out->data, &out->len, out->len, nonce, s->iv.len,
>> -                          in->data, in->len, ad->data, ad->len)
>> -        != 1)
>> -    {
>> -        EVP_AEAD_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_open() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    EVP_AEAD_CTX_free(ctx);
>> #else
>> -    int              len;
>>     EVP_CIPHER_CTX  *ctx;
>> 
>>     ctx = EVP_CIPHER_CTX_new();
>> @@ -378,114 +378,9 @@ ngx_quic_crypto_open(const ngx_quic_ciph
>>         return NGX_ERROR;
>>     }
>> 
>> -    if (EVP_DecryptInit_ex(ctx, cipher, NULL, NULL, NULL) != 1) {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptInit_ex() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    in->len -= NGX_QUIC_TAG_LEN;
>> -
>> -    if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
>> -                            in->data + in->len)
>> -        == 0)
>> -    {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0,
>> -                      "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_IVLEN, s->iv.len, NULL)
>> -        == 0)
>> -    {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0,
>> -                      "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_IVLEN) failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    if (EVP_DecryptInit_ex(ctx, NULL, NULL, s->key.data, nonce) != 1) {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptInit_ex() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
>> -        && EVP_DecryptUpdate(ctx, NULL, &len, NULL, in->len) != 1)
>> -    {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    if (EVP_DecryptUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    if (EVP_DecryptUpdate(ctx, out->data, &len, in->data, in->len) != 1) {
>> +    if (EVP_CipherInit_ex(ctx, cipher, NULL, NULL, NULL, enc) != 1) {
>>         EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    out->len = len;
>> -
>> -    if (EVP_DecryptFinal_ex(ctx, out->data + out->len, &len) <= 0) {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptFinal_ex failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    out->len += len;
>> -
>> -    EVP_CIPHER_CTX_free(ctx);
>> -#endif
>> -
>> -    return NGX_OK;
>> -}
>> -
>> -
>> -ngx_int_t
>> -ngx_quic_crypto_seal(const ngx_quic_cipher_t *cipher, ngx_quic_secret_t *s,
>> -    ngx_str_t *out, u_char *nonce, ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log)
>> -{
>> -
>> -#ifdef OPENSSL_IS_BORINGSSL
>> -    EVP_AEAD_CTX  *ctx;
>> -
>> -    ctx = EVP_AEAD_CTX_new(cipher, s->key.data, s->key.len,
>> -                           EVP_AEAD_DEFAULT_TAG_LENGTH);
>> -    if (ctx == NULL) {
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_new() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    if (EVP_AEAD_CTX_seal(ctx, out->data, &out->len, out->len, nonce, s->iv.len,
>> -                          in->data, in->len, ad->data, ad->len)
>> -        != 1)
>> -    {
>> -        EVP_AEAD_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_seal() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    EVP_AEAD_CTX_free(ctx);
>> -#else
>> -    int              len;
>> -    EVP_CIPHER_CTX  *ctx;
>> -
>> -    ctx = EVP_CIPHER_CTX_new();
>> -    if (ctx == NULL) {
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CIPHER_CTX_new() failed");
>> -        return NGX_ERROR;
>> -    }
>> -
>> -    if (EVP_EncryptInit_ex(ctx, cipher, NULL, NULL, NULL) != 1) {
>> -        EVP_CIPHER_CTX_free(ctx);
>> -        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptInit_ex() failed");
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherInit_ex() failed");
>>         return NGX_ERROR;
>>     }
>> 
>> @@ -509,28 +404,121 @@ ngx_quic_crypto_seal(const ngx_quic_ciph
>>         return NGX_ERROR;
>>     }
>> 
>> -    if (EVP_EncryptInit_ex(ctx, NULL, NULL, s->key.data, nonce) != 1) {
>> +    if (EVP_CipherInit_ex(ctx, NULL, NULL, s->key.data, NULL, enc) != 1) {
>>         EVP_CIPHER_CTX_free(ctx);
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherInit_ex() failed");
>> +        return NGX_ERROR;
>> +    }
>> +#endif
>> +
>> +    s->ctx = ctx;
>> +    return NGX_OK;
>> +}
>> +
>> +
>> +static ngx_int_t
>> +ngx_quic_crypto_open(ngx_quic_secret_t *s, ngx_str_t *out, u_char *nonce,
>> +    ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log)
>> +{
>> +    ngx_quic_crypto_ctx_t  *ctx;
>> +
>> +    ctx = s->ctx;
>> +
>> +#ifdef OPENSSL_IS_BORINGSSL
>> +    if (EVP_AEAD_CTX_open(ctx, out->data, &out->len, out->len, nonce, s->iv.len,
>> +                          in->data, in->len, ad->data, ad->len)
>> +        != 1)
>> +    {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_open() failed");
>> +        return NGX_ERROR;
>> +    }
>> +#else
>> +    int  len;
>> +
>> +    if (EVP_DecryptInit_ex(ctx, NULL, NULL, NULL, nonce) != 1) {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptInit_ex() failed");
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    in->len -= NGX_QUIC_TAG_LEN;
>> +
>> +    if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
>> +                            in->data + in->len)
>> +        == 0)
>> +    {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0,
>> +                      "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    if (EVP_CIPHER_mode(EVP_CIPHER_CTX_cipher(ctx)) == EVP_CIPH_CCM_MODE
>> +        && EVP_DecryptUpdate(ctx, NULL, &len, NULL, in->len) != 1)
>> +    {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    if (EVP_DecryptUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    if (EVP_DecryptUpdate(ctx, out->data, &len, in->data, in->len) != 1) {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    out->len = len;
>> +
>> +    if (EVP_DecryptFinal_ex(ctx, out->data + out->len, &len) <= 0) {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptFinal_ex failed");
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    out->len += len;
>> +#endif
>> +
>> +    return NGX_OK;
>> +}
>> +
>> +
>> +ngx_int_t
>> +ngx_quic_crypto_seal(ngx_quic_secret_t *s, ngx_str_t *out, u_char *nonce,
>> +    ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log)
>> +{
>> +    ngx_quic_crypto_ctx_t  *ctx;
>> +
>> +    ctx = s->ctx;
>> +
>> +#ifdef OPENSSL_IS_BORINGSSL
>> +    if (EVP_AEAD_CTX_seal(ctx, out->data, &out->len, out->len, nonce, s->iv.len,
>> +                          in->data, in->len, ad->data, ad->len)
>> +        != 1)
>> +    {
>> +        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_seal() failed");
>> +        return NGX_ERROR;
>> +    }
>> +#else
>> +    int  len;
>> +
>> +    if (EVP_EncryptInit_ex(ctx, NULL, NULL, NULL, nonce) != 1) {
>>         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptInit_ex() failed");
>>         return NGX_ERROR;
>>     }
>> 
>> -    if (EVP_CIPHER_mode(cipher) == EVP_CIPH_CCM_MODE
>> +    if (EVP_CIPHER_mode(EVP_CIPHER_CTX_cipher(ctx)) == EVP_CIPH_CCM_MODE
>>         && EVP_EncryptUpdate(ctx, NULL, &len, NULL, in->len) != 1)
>>     {
>> -        EVP_CIPHER_CTX_free(ctx);
>>         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
>>         return NGX_ERROR;
>>     }
>> 
>>     if (EVP_EncryptUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
>> -        EVP_CIPHER_CTX_free(ctx);
>>         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
>>         return NGX_ERROR;
>>     }
>> 
>>     if (EVP_EncryptUpdate(ctx, out->data, &len, in->data, in->len) != 1) {
>> -        EVP_CIPHER_CTX_free(ctx);
>>         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
>>         return NGX_ERROR;
>>     }
>> @@ -538,7 +526,6 @@ ngx_quic_crypto_seal(const ngx_quic_ciph
>>     out->len = len;
>> 
>>     if (EVP_EncryptFinal_ex(ctx, out->data + out->len, &len) <= 0) {
>> -        EVP_CIPHER_CTX_free(ctx);
>>         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptFinal_ex failed");
>>         return NGX_ERROR;
>>     }
>> @@ -549,21 +536,30 @@ ngx_quic_crypto_seal(const ngx_quic_ciph
>>                             out->data + out->len)
>>         == 0)
>>     {
>> -        EVP_CIPHER_CTX_free(ctx);
>>         ngx_ssl_error(NGX_LOG_INFO, log, 0,
>>                       "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_GET_TAG) failed");
>>         return NGX_ERROR;
>>     }
>> 
>>     out->len += NGX_QUIC_TAG_LEN;
>> -
>> -    EVP_CIPHER_CTX_free(ctx);
>> #endif
>> 
>>     return NGX_OK;
>> }
> 
> Now that we have universal ngx_quic_crypto_open() which receives "enc", it's
> tempting more than ever to combine ngx_quic_crypto_seal() and
> ngx_quic_crypto_open() in a single function with "enc".  Not a part of this
> work though.

I'm a little concerned this can impair code readability and
slightly decrease efficiency due to additional branching
(though it's compensated with branching removal in other places,
the net effect should be negligible).
Other than that I don't feel why it can't be combined
at least for the OpenSSL part.

# HG changeset patch
# User Sergey Kandaurov <pluknet at nginx.com>
# Date 1697131103 -14400
#      Thu Oct 12 21:18:23 2023 +0400
# Node ID d6dc53fe48b3522954cc9ec6ac949354d0aae512
# Parent  7055f7a2992f9440a27952f1b60ed1f606aea77e
QUIC: common code for crypto open and seal operations.

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
@@ -28,6 +28,10 @@ static uint64_t ngx_quic_parse_pn(u_char
 
 static ngx_int_t ngx_quic_crypto_open(ngx_quic_secret_t *s, ngx_str_t *out,
     u_char *nonce, ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log);
+#ifndef OPENSSL_IS_BORINGSSL
+static ngx_int_t ngx_quic_crypto_common(ngx_quic_secret_t *s, ngx_str_t *out,
+    u_char *nonce, ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log);
+#endif
 static ngx_int_t ngx_quic_crypto_hp(ngx_log_t *log, const EVP_CIPHER *cipher,
     ngx_quic_secret_t *s, u_char *out, u_char *in);
 
@@ -420,65 +424,19 @@ static ngx_int_t
 ngx_quic_crypto_open(ngx_quic_secret_t *s, ngx_str_t *out, u_char *nonce,
     ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log)
 {
-    ngx_quic_crypto_ctx_t  *ctx;
-
-    ctx = s->ctx;
-
 #ifdef OPENSSL_IS_BORINGSSL
-    if (EVP_AEAD_CTX_open(ctx, out->data, &out->len, out->len, nonce, s->iv.len,
-                          in->data, in->len, ad->data, ad->len)
+    if (EVP_AEAD_CTX_open(s->ctx, out->data, &out->len, out->len, nonce,
+                          s->iv.len, in->data, in->len, ad->data, ad->len)
         != 1)
     {
         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_open() failed");
         return NGX_ERROR;
     }
-#else
-    int  len;
-
-    if (EVP_DecryptInit_ex(ctx, NULL, NULL, NULL, nonce) != 1) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptInit_ex() failed");
-        return NGX_ERROR;
-    }
-
-    in->len -= NGX_QUIC_TAG_LEN;
-
-    if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
-                            in->data + in->len)
-        == 0)
-    {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0,
-                      "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
-        return NGX_ERROR;
-    }
-
-    if (EVP_CIPHER_mode(EVP_CIPHER_CTX_cipher(ctx)) == EVP_CIPH_CCM_MODE
-        && EVP_DecryptUpdate(ctx, NULL, &len, NULL, in->len) != 1)
-    {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
-        return NGX_ERROR;
-    }
-
-    if (EVP_DecryptUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
-        return NGX_ERROR;
-    }
-
-    if (EVP_DecryptUpdate(ctx, out->data, &len, in->data, in->len) != 1) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptUpdate() failed");
-        return NGX_ERROR;
-    }
-
-    out->len = len;
-
-    if (EVP_DecryptFinal_ex(ctx, out->data + out->len, &len) <= 0) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_DecryptFinal_ex failed");
-        return NGX_ERROR;
-    }
-
-    out->len += len;
-#endif
 
     return NGX_OK;
+#else
+    return ngx_quic_crypto_common(s, out, nonce, in, ad, log);
+#endif
 }
 
 
@@ -486,67 +444,96 @@ ngx_int_t
 ngx_quic_crypto_seal(ngx_quic_secret_t *s, ngx_str_t *out, u_char *nonce,
     ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log)
 {
-    ngx_quic_crypto_ctx_t  *ctx;
-
-    ctx = s->ctx;
-
 #ifdef OPENSSL_IS_BORINGSSL
-    if (EVP_AEAD_CTX_seal(ctx, out->data, &out->len, out->len, nonce, s->iv.len,
-                          in->data, in->len, ad->data, ad->len)
+    if (EVP_AEAD_CTX_seal(s->ctx, out->data, &out->len, out->len, nonce,
+                          s->iv.len, in->data, in->len, ad->data, ad->len)
         != 1)
     {
         ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_AEAD_CTX_seal() failed");
         return NGX_ERROR;
     }
+
+    return NGX_OK;
 #else
-    int  len;
+    return ngx_quic_crypto_common(s, out, nonce, in, ad, log);
+#endif
+}
+
+
+#ifndef OPENSSL_IS_BORINGSSL
+
+static ngx_int_t
+ngx_quic_crypto_common(ngx_quic_secret_t *s, ngx_str_t *out, u_char *nonce,
+    ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log)
+{
+    int                     len, enc;
+    ngx_quic_crypto_ctx_t  *ctx;
 
-    if (EVP_EncryptInit_ex(ctx, NULL, NULL, NULL, nonce) != 1) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptInit_ex() failed");
+    ctx = s->ctx;
+    enc = EVP_CIPHER_CTX_encrypting(ctx);
+
+    if (EVP_CipherInit_ex(ctx, NULL, NULL, NULL, nonce, enc) != 1) {
+        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherInit_ex() failed");
         return NGX_ERROR;
     }
 
+    if (enc == 0) {
+        in->len -= NGX_QUIC_TAG_LEN;
+
+        if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_SET_TAG, NGX_QUIC_TAG_LEN,
+                                in->data + in->len)
+            == 0)
+        {
+            ngx_ssl_error(NGX_LOG_INFO, log, 0,
+                          "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_SET_TAG) failed");
+            return NGX_ERROR;
+        }
+    }
+
     if (EVP_CIPHER_mode(EVP_CIPHER_CTX_cipher(ctx)) == EVP_CIPH_CCM_MODE
-        && EVP_EncryptUpdate(ctx, NULL, &len, NULL, in->len) != 1)
+        && EVP_CipherUpdate(ctx, NULL, &len, NULL, in->len) != 1)
     {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
+        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherUpdate() failed");
         return NGX_ERROR;
     }
 
-    if (EVP_EncryptUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
+    if (EVP_CipherUpdate(ctx, NULL, &len, ad->data, ad->len) != 1) {
+        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherUpdate() failed");
         return NGX_ERROR;
     }
 
-    if (EVP_EncryptUpdate(ctx, out->data, &len, in->data, in->len) != 1) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptUpdate() failed");
+    if (EVP_CipherUpdate(ctx, out->data, &len, in->data, in->len) != 1) {
+        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherUpdate() failed");
         return NGX_ERROR;
     }
 
     out->len = len;
 
-    if (EVP_EncryptFinal_ex(ctx, out->data + out->len, &len) <= 0) {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_EncryptFinal_ex failed");
+    if (EVP_CipherFinal_ex(ctx, out->data + out->len, &len) <= 0) {
+        ngx_ssl_error(NGX_LOG_INFO, log, 0, "EVP_CipherFinal_ex failed");
         return NGX_ERROR;
     }
 
     out->len += len;
 
-    if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, NGX_QUIC_TAG_LEN,
-                            out->data + out->len)
-        == 0)
-    {
-        ngx_ssl_error(NGX_LOG_INFO, log, 0,
-                      "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_GET_TAG) failed");
-        return NGX_ERROR;
+    if (enc == 1) {
+        if (EVP_CIPHER_CTX_ctrl(ctx, EVP_CTRL_AEAD_GET_TAG, NGX_QUIC_TAG_LEN,
+                                out->data + out->len)
+            == 0)
+        {
+            ngx_ssl_error(NGX_LOG_INFO, log, 0,
+                          "EVP_CIPHER_CTX_ctrl(EVP_CTRL_AEAD_GET_TAG) failed");
+            return NGX_ERROR;
+        }
+
+        out->len += NGX_QUIC_TAG_LEN;
     }
 
-    out->len += NGX_QUIC_TAG_LEN;
-#endif
-
     return NGX_OK;
 }
 
+#endif
+
 
 void
 ngx_quic_crypto_cleanup(ngx_quic_secret_t *s)


> 
>> +void
>> +ngx_quic_crypto_cleanup(ngx_quic_secret_t *s)
>> +{
> 
> Although we know these functions ignore a NULL argument, I think the code
> would still look better under "if (s->ctx) {}".
> 

Indeed, it's made with the knowledge that both API have a check for NULL.
Added the check, I've no strict preference on this.

>> +#ifdef OPENSSL_IS_BORINGSSL
>> +    EVP_AEAD_CTX_free(s->ctx);
>> +#else
>> +    EVP_CIPHER_CTX_free(s->ctx);
>> +#endif
>> +    s->ctx = NULL;
>> +}
>> +
>> +
>> static ngx_int_t
>> ngx_quic_crypto_hp(ngx_log_t *log, const EVP_CIPHER *cipher,
>>     ngx_quic_secret_t *s, u_char *out, u_char *in)
>> @@ -666,6 +662,12 @@ ngx_quic_keys_set_encryption_secret(ngx_
>>         }
>>     }
>> 
>> +    if (ngx_quic_crypto_init(ciphers.c, peer_secret, is_write, log)
>> +        == NGX_ERROR)
>> +    {
>> +        return NGX_ERROR;
>> +    }
>> +
>>     return NGX_OK;
>> }
>> 
>> @@ -675,10 +677,10 @@ ngx_quic_keys_available(ngx_quic_keys_t 
>>     enum ssl_encryption_level_t level, ngx_uint_t is_write)
>> {
>>     if (is_write == 0) {
>> -        return keys->secrets[level].client.key.len != 0;
>> +        return keys->secrets[level].client.ctx != NULL;
>>     }
>> 
>> -    return keys->secrets[level].server.key.len != 0;
>> +    return keys->secrets[level].server.ctx != 0;
> 
> Maybe != NULL ?

Fixed, tnx.

> 
>> }
>> 
>> 
>> @@ -686,8 +688,13 @@ void
>> ngx_quic_keys_discard(ngx_quic_keys_t *keys,
>>     enum ssl_encryption_level_t level)
>> {
>> -    keys->secrets[level].client.key.len = 0;
>> -    keys->secrets[level].server.key.len = 0;
>> +    ngx_quic_secret_t  *client, *server;
>> +
>> +    client = &keys->secrets[level].client;
>> +    server = &keys->secrets[level].server;
>> +
>> +    ngx_quic_crypto_cleanup(client);
>> +    ngx_quic_crypto_cleanup(server);
>> }
>> 
>> 
>> @@ -699,6 +706,9 @@ ngx_quic_keys_switch(ngx_connection_t *c
>>     current = &keys->secrets[ssl_encryption_application];
>>     next = &keys->next_key;
>> 
>> +    ngx_quic_crypto_cleanup(&current->client);
>> +    ngx_quic_crypto_cleanup(&current->server);
>> +
>>     tmp = *current;
>>     *current = *next;
>>     *next = tmp;
>> @@ -762,6 +772,16 @@ ngx_quic_keys_update(ngx_event_t *ev)
>>         }
>>     }
>> 
>> +    if (ngx_quic_crypto_init(ciphers.c, &next->client, 0, c->log) == NGX_ERROR)
>> +    {
>> +        goto failed;
>> +    }
>> +
>> +    if (ngx_quic_crypto_init(ciphers.c, &next->server, 1, c->log) == NGX_ERROR)
>> +    {
>> +        goto failed;
>> +    }
>> +
>>     return;
>> 
>> failed:
>> @@ -770,6 +790,26 @@ failed:
>> }
>> 
>> 
>> +void
>> +ngx_quic_keys_cleanup(void *data)
>> +{
>> +    ngx_quic_keys_t *keys = data;
>> +
>> +    size_t               i;
>> +    ngx_quic_secrets_t  *secrets;
>> +
>> +    for (i = 0; i < NGX_QUIC_ENCRYPTION_LAST; i++) {
>> +        secrets = &keys->secrets[i];
>> +        ngx_quic_crypto_cleanup(&secrets->client);
>> +        ngx_quic_crypto_cleanup(&secrets->server);
>> +    }
>> +
>> +    secrets = &keys->next_key;
>> +    ngx_quic_crypto_cleanup(&secrets->client);
>> +    ngx_quic_crypto_cleanup(&secrets->server);
>> +}
>> +
>> +
>> static ngx_int_t
>> ngx_quic_create_packet(ngx_quic_header_t *pkt, ngx_str_t *res)
>> {
>> @@ -801,8 +841,7 @@ ngx_quic_create_packet(ngx_quic_header_t
>>     ngx_memcpy(nonce, secret->iv.data, secret->iv.len);
>>     ngx_quic_compute_nonce(nonce, sizeof(nonce), pkt->number);
>> 
>> -    if (ngx_quic_crypto_seal(ciphers.c, secret, &out,
>> -                             nonce, &pkt->payload, &ad, pkt->log)
>> +    if (ngx_quic_crypto_seal(secret, &out, nonce, &pkt->payload, &ad, pkt->log)
>>         != NGX_OK)
>>     {
>>         return NGX_ERROR;
>> @@ -862,13 +901,18 @@ ngx_quic_create_retry_packet(ngx_quic_he
>>     ngx_memcpy(secret.key.data, key, sizeof(key));
>>     secret.iv.len = NGX_QUIC_IV_LEN;
>> 
>> -    if (ngx_quic_crypto_seal(ciphers.c, &secret, &itag, nonce, &in, &ad,
>> -                             pkt->log)
>> +    if (ngx_quic_crypto_init(ciphers.c, &secret, 1, pkt->log) == NGX_ERROR) {
>> +        return NGX_ERROR;
>> +    }
>> +
>> +    if (ngx_quic_crypto_seal(&secret, &itag, nonce, &in, &ad, pkt->log)
>>         != NGX_OK)
>>     {
> 
> Need to call ngx_quic_crypto_cleanup() here.

Fixed, tnx.

I was pondering on reusing a static crypto context
to make generating Retry packets more lightweight.
Known fixed values for key and nonce make it possible to create
a single context and reuse it over all Retry packets. 

Note that the context memory is kept for reuse after the first
retry, it will be freed eventually on process exit,
the operating system will take care of it.
Not sure though this is a good solution.

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
@@ -872,7 +872,6 @@ ngx_quic_create_retry_packet(ngx_quic_he
 {
     u_char              *start;
     ngx_str_t            ad, itag;
-    ngx_quic_secret_t    secret;
     ngx_quic_ciphers_t   ciphers;
 
     /* 5.8.  Retry Packet Integrity */
@@ -882,6 +881,8 @@ ngx_quic_create_retry_packet(ngx_quic_he
         "\x46\x15\x99\xd3\x5d\x63\x2b\xf2\x23\x98\x25\xbb";
     static ngx_str_t  in = ngx_string("");
 
+    static ngx_quic_secret_t  secret;
+
     ad.data = res->data;
     ad.len = ngx_quic_create_retry_itag(pkt, ad.data, &start);
 
@@ -893,6 +894,10 @@ ngx_quic_create_retry_packet(ngx_quic_he
                    "quic retry itag len:%uz %xV", ad.len, &ad);
 #endif
 
+    if (secret.ctx) {
+        goto seal;
+    }
+
     if (ngx_quic_ciphers(0, &ciphers, pkt->level) == NGX_ERROR) {
         return NGX_ERROR;
     }
@@ -905,14 +910,14 @@ ngx_quic_create_retry_packet(ngx_quic_he
         return NGX_ERROR;
     }
 
+seal:
+
     if (ngx_quic_crypto_seal(&secret, &itag, nonce, &in, &ad, pkt->log)
         != NGX_OK)
     {
         return NGX_ERROR;
     }
 
-    ngx_quic_crypto_cleanup(&secret);
-
     res->len = itag.data + itag.len - start;
     res->data = start;
 

> 
>>         return NGX_ERROR;
>>     }
>> 
>> +    ngx_quic_crypto_cleanup(&secret);
>> +
>>     res->len = itag.data + itag.len - start;
>>     res->data = start;
>> 
>> @@ -999,7 +1043,7 @@ ngx_quic_decrypt(ngx_quic_header_t *pkt,
>>     u_char              *p, *sample;
>>     size_t               len;
>>     uint64_t             pn, lpn;
>> -    ngx_int_t            pnl, rc;
>> +    ngx_int_t            pnl;
>>     ngx_str_t            in, ad;
>>     ngx_uint_t           key_phase;
>>     ngx_quic_secret_t   *secret;
>> @@ -1088,9 +1132,9 @@ ngx_quic_decrypt(ngx_quic_header_t *pkt,
>>     pkt->payload.len = in.len - NGX_QUIC_TAG_LEN;
>>     pkt->payload.data = pkt->plaintext + ad.len;
>> 
>> -    rc = ngx_quic_crypto_open(ciphers.c, secret, &pkt->payload,
>> -                              nonce, &in, &ad, pkt->log);
>> -    if (rc != NGX_OK) {
>> +    if (ngx_quic_crypto_open(secret, &pkt->payload, nonce, &in, &ad, pkt->log)
>> +        != NGX_OK)
>> +    {
>>         return NGX_DECLINED;
>>     }
>> 
>> 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
>> @@ -26,8 +26,10 @@
>> 
>> #ifdef OPENSSL_IS_BORINGSSL
>> #define ngx_quic_cipher_t             EVP_AEAD
>> +#define ngx_quic_crypto_ctx_t         EVP_AEAD_CTX
>> #else
>> #define ngx_quic_cipher_t             EVP_CIPHER
>> +#define ngx_quic_crypto_ctx_t         EVP_CIPHER_CTX
>> #endif
>> 
>> 
>> @@ -48,6 +50,7 @@ typedef struct {
>>     ngx_quic_md_t             key;
>>     ngx_quic_iv_t             iv;
>>     ngx_quic_md_t             hp;
>> +    ngx_quic_crypto_ctx_t    *ctx;
>> } ngx_quic_secret_t;
>> 
>> 
>> @@ -100,14 +103,17 @@ 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);
>> 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);
>> ngx_int_t ngx_quic_ciphers(ngx_uint_t id, ngx_quic_ciphers_t *ciphers,
>>     enum ssl_encryption_level_t level);
>> -ngx_int_t ngx_quic_crypto_seal(const ngx_quic_cipher_t *cipher,
>> -    ngx_quic_secret_t *s, ngx_str_t *out, u_char *nonce, ngx_str_t *in,
>> -    ngx_str_t *ad, ngx_log_t *log);
>> +ngx_int_t ngx_quic_crypto_init(const ngx_quic_cipher_t *cipher,
>> +    ngx_quic_secret_t *s, ngx_int_t enc, ngx_log_t *log);
>> +ngx_int_t ngx_quic_crypto_seal(ngx_quic_secret_t *s, ngx_str_t *out,
>> +    u_char *nonce, ngx_str_t *in, ngx_str_t *ad, ngx_log_t *log);
>> +void ngx_quic_crypto_cleanup(ngx_quic_secret_t *s);
>> ngx_int_t ngx_quic_hkdf_expand(ngx_quic_hkdf_t *hkdf, const EVP_MD *digest,
>>     ngx_log_t *log);

-- 
Sergey Kandaurov


More information about the nginx-devel mailing list