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

[Roman Arutyunyan]

Hi,

On Tue, Aug 29, 2023 at 09:56:22PM +0400, Sergey Kandaurov wrote:
> 
> > 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.

Client dcid sounds too vague.  I'd rather stick with 'original dcid' and
'last client dcid'.

> 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.

Back when retry was session-based rather than token-based, listening on the
original dcid made sense until we received retry.  This allowed to avoid
creating multiple sessions when server retry packet was lost.  Anyway it was
too long ago to make any difference now.

> > 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.

I suggest the following commit log:

QUIC: use last client dcid to receive initial packets.

Previously, original dcid was used to receive initial client packets in case
server initial response was lost.  However, last dcid should be used instead.
These two are the same unless retry is used.  In case of retry, client resends
initial packet with a new dcid, that is different from the original dcid.  If
server response is lost, the client resends this packet again with the same
dcid.  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.

[..]

--
Roman Arutyunyan