[PATCH] QUIC: use client dcid rather than odcid to receive packets

[Sergey Kandaurov]

> On 29 Aug 2023, at 11:19, Roman Arutyunyan <arut at nginx.com> wrote:
> 
> # HG changeset patch
> # User Roman Arutyunyan <arut at nginx.com>
> # Date 1693292146 -14400
> #      Tue Aug 29 10:55:46 2023 +0400
> # Node ID 7f451ca6f449958011e29aee5231e70be4992374
> # Parent  58afcd72446ff33811e773f1cabb7866a92a09a0
> QUIC: use client dcid rather than odcid to receive packets.

QUIC: using client dcid to receive initial packets.

?

> 
> Previously, odcid was used to receive initial client packets in case server
> initial response was lost.  However, dcid should be used instead.  These two

It takes time to recall meaning of these acronyms, I would expand
them in the commit log as initial dcid and client dcid for clarity.

Previously, initial dcid was used ..

> are the same unless retry is used.  In case of retry, client resends initial
> packets with the retry dcid.  If server response is lost, the client resends
> this packet again with the same retry dcid, but not odcid.  This is shown in
> RFC 9000, 7.3. Authenticating Connection IDs, Figure 8.
> 
> The issue manifested itself with creating multiple server sessions in response
> to each post-retry client initial packet, if server response is lost.

My archeologist analysis refers to dffb66fb783b, where it seemingly
was broken.  Before stateless retry support, retry id was inserted
as part of sending Retry packet, this part was lost in transit.

> 
> 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
> @@ -1100,7 +1100,7 @@ ngx_quic_discard_ctx(ngx_connection_t *c
>     }
> 
>     if (level == ssl_encryption_initial) {
> -        /* close temporary listener with odcid */
> +        /* close temporary listener with initial dcid */
>         qsock = ngx_quic_find_socket(c, NGX_QUIC_UNSET_PN);
>         if (qsock) {
>             ngx_quic_close_socket(c, qsock);
> diff --git a/src/event/quic/ngx_event_quic_socket.c b/src/event/quic/ngx_event_quic_socket.c
> --- a/src/event/quic/ngx_event_quic_socket.c
> +++ b/src/event/quic/ngx_event_quic_socket.c
> @@ -93,8 +93,8 @@ ngx_quic_open_sockets(ngx_connection_t *
> 
>     tmp->sid.seqnum = NGX_QUIC_UNSET_PN; /* temporary socket */
> 
> -    ngx_memcpy(tmp->sid.id, pkt->odcid.data, pkt->odcid.len);
> -    tmp->sid.len = pkt->odcid.len;
> +    ngx_memcpy(tmp->sid.id, pkt->dcid.data, pkt->dcid.len);
> +    tmp->sid.len = pkt->dcid.len;
> 
>     if (ngx_quic_listen(c, qc, tmp) != NGX_OK) {
>         goto failed;

The change looks good.

For convenience, it's caught with the following test.
(Also couple of test bugs fixed:
- stop regenerating private key on retry, so secrets match
- ClientHello is built once to avoid breaking digest).

diff --git a/lib/Test/Nginx/HTTP3.pm b/lib/Test/Nginx/HTTP3.pm
--- a/lib/Test/Nginx/HTTP3.pm
+++ b/lib/Test/Nginx/HTTP3.pm
@@ -59,6 +59,7 @@ sub new {
 	$self->{buf} = '';
 
 	$self->init();
+	$self->init_key_schedule();
 	$self->retry(%extra) or return;
 
 	return $self;
@@ -100,7 +101,6 @@ sub retry {
 	$self->set_traffic_keys('tls13 client in', 'SHA256', 32, 0, 'w', $prk);
 	$self->set_traffic_keys('tls13 server in', 'SHA256', 32, 0, 'r', $prk);
 
-	$self->init_key_schedule();
 	$self->initial();
 	return $self if $extra{probe};
 	$self->handshake() or return;
@@ -134,7 +134,8 @@ sub init_key_schedule {
 
 sub initial {
 	my ($self) = @_;
-	$self->{tlsm}{ch} = $self->build_tls_client_hello();
+	$self->{tlsm}{ch} = $self->build_tls_client_hello()
+		if length($self->{tlsm}{ch}) == 0;
 	my $ch = $self->{tlsm}{ch};
 	my $crypto = build_crypto($ch);
 	my $padding = 1200 - length($crypto);
diff --git a/quic_retry.t b/quic_retry.t
--- a/quic_retry.t
+++ b/quic_retry.t
@@ -24,7 +24,7 @@ select STDERR; $| = 1;
 select STDOUT; $| = 1;
 
 my $t = Test::Nginx->new()->has(qw/http http_v3 cryptx/)
-	->has_daemon('openssl')->plan(7)
+	->has_daemon('openssl')->plan(8)
 	->write_file_expand('nginx.conf', <<'EOF');
 
 %%TEST_GLOBALS%%
@@ -41,6 +41,8 @@ http {
     ssl_certificate localhost.crt;
     quic_retry on;
 
+    keepalive_timeout 3s;
+
     server {
         listen       127.0.0.1:%%PORT_8980_UDP%% quic;
         server_name  localhost;
@@ -120,4 +122,41 @@ is($frame->{error}, 11, 'retry token dec
 
 }
 
+# dup'ed Initial to simulate packet loss, was used to create extra nginx
+# connections, caught due to CRYPTO mismatch in server's Initial packets
+
+TODO: {
+local $TODO = 'not yet';
+
+$s = new_dup_initial();
+$sid = $s->new_stream();
+
+eval {
+	# would die with "bad inner" CRYPTO insert error
+is($frame->{headers}->{':status'}, 403, 'duplicate initial');
+
+}
+
 ###############################################################################
+
+# expanded handshake version to send repetitive Initial packets
+
+sub new_dup_initial {
+	$s = Test::Nginx::HTTP3->new(8980, probe => 1);
+	$s->{socket}->sysread($s->{buf}, 65527);
+	# read token and updated connection IDs
+	(undef, undef, $s->{token}) = $s->decrypt_retry($s->{buf});
+	# apply connection IDs for new Initial secrets
+	$s->retry(probe => 1);
+	# send the second Initial packet
+	$s->initial();
+	# the rest of handshake, advancing key schedule
+	$s->handshake();
+	return $s;
+}
+
+###############################################################################

-- 
Sergey Kandaurov