Re: [quicwg/base-drafts] Rework Retry packet (#1498)

[ianswett <[e-mail]>]

ianswett commented on this pull request.

This looks like an improvement overall and clarifies some points.

> +|                         Version (32)                          |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|DCIL(4)|SCIL(4)|
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|               Destination Connection ID (0/32..144)         ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                 Source Connection ID (0/32..144)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|    ODCIL(8)   |      Original Destination Connection ID (*)   |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        Retry Token (*)                      ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #retry-format title="Retry Packet"}
+
+A Retry packet (shown in {{retry-format}}) only the invariant portion of the

"A Retry packet only" uses?

> + 0                   1                   2                   3
+ 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
++-+-+-+-+-+-+-+-+
+|1|    0x7e     |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                         Version (32)                          |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|DCIL(4)|SCIL(4)|
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|               Destination Connection ID (0/32..144)         ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                 Source Connection ID (0/32..144)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|    ODCIL(8)   |      Original Destination Connection ID (*)   |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        Retry Token (*)                      ...

I'm realizing the lack of length means one can't 'coalesce' a RETRY and another packet type in a single datagram.  I can't think why this is a practical issue, but it wouldn't surprise me if there was a use for it at some point, so I think it makes sense to add a length.

> +
+Retry Token:
+
+: An opaque token that the server can use to validate the client's address.
+
+The server populates the Destination Connection ID with the connection ID that
+the client included in the Source Connection ID of the Initial packet.
+
+The server includes a connection ID of its choice in the Source Connection ID
+field.  The client MUST use this connection ID in the Destination Connection ID
+of subsequent packets that it sends.
+
+A Retry packet does not include a packet number and cannot be explictly
+acknowledged by a client.
+
+A server MUST only send a Retry in response to a client Initial packet.

I think what Martin has is good, but I can imagine a DDoS device doing what you said, especially if it kept a small amount of state to ensure it didn't sent too many RETRY packets to a given IP/port.  So this could be a SHOULD instead of a MUST.

> +The server populates the Destination Connection ID with the connection ID that
+the client included in the Source Connection ID of the Initial packet.
+
+The server includes a connection ID of its choice in the Source Connection ID
+field.  The client MUST use this connection ID in the Destination Connection ID
+of subsequent packets that it sends.
+
+A Retry packet does not include a packet number and cannot be explictly
+acknowledged by a client.
+
+A server MUST only send a Retry in response to a client Initial packet.
+
+If the Original Destination Connection ID field does not match the Destination
+Connection ID from the most recent the Initial packet it sent, clients MUST
+discard the packet.  This prevents an off-path attacker from injecting a Retry
+packet with a bogus new Source Connection ID.

I think "with a bogus new Source Connection ID." is unnecessary here, since any off-path injection is bad.

>  
 Token Length:
 
 : A variable-length integer specifying the length of the Token field, in bytes.
-  It may be zero if no token is present. Initial packets sent by the server
+  This value is zero if no token is present.  Initial packets sent by the server
   MUST include a zero-length token.

I think this would be clearer as "Initial packets sent by the server must specify a zero token length."

>  
+Note that if the server sends a HelloRetryRequest, the client will send a second
+Initial packet with a CRYPTO frame with an offset starting at the end of the
+CRYPTO stream in the first Initial.

"end of the CRYPTO stream" makes it sound like it's after the stream has ended, ie: been FIN'd.

Possibly "with an offset starting after the CRYPTO data in the first Initial."

> @@ -624,10 +724,18 @@ server, it MUST discard any packet it receives with a different Source
 Connection ID.
 
 
-#### Tokens
+### Tokens
+
+If the client has an unused token that it received in a NEW_TOKEN frame on a

nit: Unnecessary "that"

> @@ -624,10 +724,18 @@ server, it MUST discard any packet it receives with a different Source
 Connection ID.
 
 
-#### Tokens
+### Tokens
+
+If the client has an unused token that it received in a NEW_TOKEN frame on a
+previous connection to what it believes to be the same server, it includes that

it SHOULD include that?

>  
-If the client has a suitable token available from a previous connection, it
-SHOULD populate the Token field.
+A client MUST NOT reuse a token if it believes that its point of network

By saying unused in the previous paragraph, you imply tokens should never be used more than once.  But this paragraph seems to imply it's ok if you haven't changed IP.  Which do you intend?

> @@ -649,85 +757,27 @@ Note:
   the packet is that the client might have received the token in a previous
   connection using the NEW_TOKEN frame, and if the server has lost state, it
   might be unable to validate the token at all, leading to connection failure if
-  the packet is discarded.
+  the packet is discarded.  A server might encode tokens that it provides with

might -> MAY?
And I'd suggest "that it provides" -> "provided"

>  
-The first Initial packet contains a packet number of 0. Each packet sent after
-the Initial packet is associated with a packet number space and its packet
-number increases monotonically in that space (see {{packet-numbers}}).
+The first Initial packet sent by either endpoint contains a packet number of

SHOULD contain?

>  
-The first Initial packet contains a packet number of 0. Each packet sent after
-the Initial packet is associated with a packet number space and its packet
-number increases monotonically in that space (see {{packet-numbers}}).
+The first Initial packet sent by either endpoint contains a packet number of
+0. The packet number increases monotonically thereafter.  Initial packets are in

MUST increase

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1498#pullrequestreview-133337121