Hi, On Thu, Aug 24, 2023 at 07:37:24PM +0400, Sergey Kandaurov wrote: > > > On 23 Aug 2023, at 15:35, Roman Arutyunyan <a...@nginx.com> wrote: > > > > Hi, > > > > On Mon, Aug 21, 2023 at 08:17:11PM +0400, Sergey Kandaurov wrote: > >> # HG changeset patch > >> # User Sergey Kandaurov <pluk...@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 <pluk...@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 _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org https://mailman.nginx.org/mailman/listinfo/nginx-devel