[PATCH] QUIC: posted generating TLS Key Update next keys
Roman Arutyunyan
arut at nginx.com
Fri Aug 25 09:53:41 UTC 2023
Hi,
On Thu, Aug 24, 2023 at 07:37:24PM +0400, Sergey Kandaurov wrote:
>
> > On 23 Aug 2023, at 15:35, Roman Arutyunyan <arut at nginx.com> wrote:
> >
> > 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).
>
> I don't think it's a good idea.
> To prevent a possibility to create a timing signal, generating new
> keys should be made after packet processing (including sending part).
> Posting the key update event as proposed makes it possible to execute
> the event before the push event if it's posted while handing frames,
> which makes generating new keys a part of packet processing.
Yes, you're right. Let's leave it as it is.
> >> [..]
> >> -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.
>
> Added a log action.
>
> Slightly updated commit message to clarify rationale.
>
> # HG changeset patch
> # User Sergey Kandaurov <pluknet at nginx.com>
> # Date 1692891114 -14400
> # Thu Aug 24 19:31:54 2023 +0400
> # Node ID f349e28c0cf06d69b148660e7ae01a4038384a43
> # Parent 44536076405cf79ebdd82a6a0ab27bb3aed86b04
> QUIC: posted generating TLS Key Update next keys.
>
> Since at least f9fbeb4ee0de and apparently after 924882f42dea, which
> TLS Key Update support predates, queued data output is deferred to a
> posted push handler. To address timing signals after these changes,
> generating next keys is made posted to run after the push handler,
> which may be posted while handling frames.
Discussed the commit log, made a few small changes.
> 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;
> +
> qc->conf = conf;
>
> if (ngx_quic_init_transport_params(&qc->tp, conf) != NGX_OK) {
> @@ -562,6 +566,10 @@ ngx_quic_close_connection(ngx_connection
> ngx_delete_posted_event(&qc->push);
> }
>
> + if (qc->key_update.posted) {
> + ngx_delete_posted_event(&qc->key_update);
> + }
> +
> if (qc->close.timer_set) {
> return;
> }
> @@ -1055,7 +1063,9 @@ ngx_quic_handle_payload(ngx_connection_t
> return rc;
> }
>
> - return ngx_quic_keys_update(c, qc->keys);
> + ngx_post_event(&qc->key_update, &ngx_posted_events);
> +
> + 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,23 +700,32 @@ 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;
> +
> + c = ev->data;
> + qc = ngx_quic_get_connection(c);
> + keys = qc->keys;
>
> current = &keys->secrets[ssl_encryption_application];
> next = &keys->next_key;
>
> ngx_log_debug0(NGX_LOG_DEBUG_EVENT, c->log, 0, "quic key update");
>
> + c->log->action = "updating keys";
> +
> 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 +753,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
>
> --
> Sergey Kandaurov
Looks ok
More information about the nginx-devel
mailing list