[PATCH] QUIC: posted generating TLS Key Update next keys
Sergey Kandaurov
pluknet at nginx.com
Thu Aug 24 15:37:24 UTC 2023
> 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.
>> [..]
>> -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.
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
More information about the nginx-devel
mailing list