[PATCH] QUIC: posted generating TLS Key Update next keys
Roman Arutyunyan
arut at nginx.com
Wed Aug 23 11:35:19 UTC 2023
Hi,
On Mon, Aug 21, 2023 at 08:17:11PM +0400, Sergey Kandaurov wrote:
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1692634530 -14400
> # Mon Aug 21 20:15:30 2023 +0400
> # Node ID 4215a1d1426c23a06df6229b986d36e838594a91
> # Parent 44536076405cf79ebdd82a6a0ab27bb3aed86b04
> QUIC: posted generating TLS Key Update next keys.
>
> Since at least f9fbeb4ee0de, which TLS Key Update support predates,
> queued data output is deferred to a posted push handler. To address
> timing signals, generating next keys is posted after handling frames,
> to run after posted push handler.
>
> 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
> @@ -283,6 +283,10 @@ ngx_quic_new_connection(ngx_connection_t
> qc->path_validation.data = c;
> qc->path_validation.handler = ngx_quic_path_handler;
>
> + qc->key_update.log = c->log;
> + qc->key_update.data = c;
> + qc->key_update.handler = ngx_quic_keys_update;
As discussed before (and addressed in the followup patch), this event
should be removed on connection close.
> qc->conf = conf;
>
> if (ngx_quic_init_transport_params(&qc->tp, conf) != NGX_OK) {
> @@ -1055,7 +1059,9 @@ ngx_quic_handle_payload(ngx_connection_t
> return rc;
> }
Unrelated to the patch, but setting the log action above this call
does not seem to make sense. Also, I'd change the way it's set in
ngx_quic_handle_frames().
> - return ngx_quic_keys_update(c, qc->keys);
> + ngx_post_event(&qc->key_update, &ngx_posted_events);
IMHO the code above can be simplified now to become something like this:
pkt->received = ngx_current_msec;
if (pkt->level == ssl_encryption_application && pkt->key_update) {
/* switch keys and generate next on Key Phase change */
qc->key_phase ^= 1;
ngx_quic_keys_switch(c, qc->keys);
ngx_post_event(&qc->key_update, &ngx_posted_events);
}
return ngx_quic_handle_frames(c, pkt);
You may argue that in case of error key update will be posted while it's not
needed, but this situtation is still possible if we receive two packets and
the second one triggers an error. Also, the situation is similar to pto
handlers. We may add connection close checks to event handlers to optimize the
behavior (not sure though).
> +
> + return NGX_OK;
> }
>
>
> diff --git a/src/event/quic/ngx_event_quic_connection.h b/src/event/quic/ngx_event_quic_connection.h
> --- a/src/event/quic/ngx_event_quic_connection.h
> +++ b/src/event/quic/ngx_event_quic_connection.h
> @@ -230,6 +230,8 @@ struct ngx_quic_connection_s {
> ngx_event_t pto;
> ngx_event_t close;
> ngx_event_t path_validation;
> + ngx_event_t key_update;
> +
> ngx_msec_t last_cc;
>
> ngx_msec_t first_rtt;
> 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
> @@ -700,13 +700,20 @@ ngx_quic_keys_switch(ngx_connection_t *c
> }
>
>
> -ngx_int_t
> -ngx_quic_keys_update(ngx_connection_t *c, ngx_quic_keys_t *keys)
> +void
> +ngx_quic_keys_update(ngx_event_t *ev)
> {
> - ngx_uint_t i;
> - ngx_quic_hkdf_t seq[6];
> - ngx_quic_ciphers_t ciphers;
> - ngx_quic_secrets_t *current, *next;
> + ngx_uint_t i;
> + ngx_quic_hkdf_t seq[6];
> + ngx_quic_keys_t *keys;
> + ngx_connection_t *c;
> + ngx_quic_ciphers_t ciphers;
> + ngx_quic_secrets_t *current, *next;
> + ngx_quic_connection_t *qc;
Now this functions needs a log action. Similarly, pto handlers need it as well.
> + c = ev->data;
> + qc = ngx_quic_get_connection(c);
> + keys = qc->keys;
>
> current = &keys->secrets[ssl_encryption_application];
> next = &keys->next_key;
> @@ -716,7 +723,7 @@ ngx_quic_keys_update(ngx_connection_t *c
> if (ngx_quic_ciphers(keys->cipher, &ciphers, ssl_encryption_application)
> == NGX_ERROR)
> {
> - return NGX_ERROR;
> + goto failed;
> }
>
> next->client.secret.len = current->client.secret.len;
> @@ -744,11 +751,15 @@ ngx_quic_keys_update(ngx_connection_t *c
>
> for (i = 0; i < (sizeof(seq) / sizeof(seq[0])); i++) {
> if (ngx_quic_hkdf_expand(&seq[i], ciphers.d, c->log) != NGX_OK) {
> - return NGX_ERROR;
> + goto failed;
> }
> }
>
> - return NGX_OK;
> + return;
> +
> +failed:
> +
> + ngx_quic_close_connection(c, NGX_ERROR);
> }
>
>
> 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
> @@ -99,7 +99,7 @@ ngx_uint_t ngx_quic_keys_available(ngx_q
> void ngx_quic_keys_discard(ngx_quic_keys_t *keys,
> enum ssl_encryption_level_t level);
> void ngx_quic_keys_switch(ngx_connection_t *c, ngx_quic_keys_t *keys);
> -ngx_int_t ngx_quic_keys_update(ngx_connection_t *c, ngx_quic_keys_t *keys);
> +void ngx_quic_keys_update(ngx_event_t *ev);
> 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);
> 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
> @@ -482,9 +482,7 @@ ngx_quic_crypto_input(ngx_connection_t *
> * Generating next keys before a key update is received.
> */
>
> - if (ngx_quic_keys_update(c, qc->keys) != NGX_OK) {
> - return NGX_ERROR;
> - }
> + ngx_post_event(&qc->key_update, &ngx_posted_events);
>
> /*
> * RFC 9001, 4.9.2. Discarding Handshake Keys
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Roman Arutyunyan
More information about the nginx-devel
mailing list