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

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Jun 2018 17:40:22 -0700

This includes several changes:

* I moved the Retry packet description.  It is more like Version Negotiation than Handshake or Initial, so it made sense to move it up.

* Retry doesn't include a packet number or payload.  Like Version Negotiation, it includes a complete definition.

* As discussed in #1451, this requires a server that might send another Retry to provide a connection ID of at least 8 octets.

* I clarified the description of fields, expanded the pictures, and made some other editorial tweaks.

Open Question: would it make sense to normalize the encoding of Retry and Initial?  The former includes a token without a length (because it's the last piece of the packet), whereas the latter includes a length-prefixed token.  It might be easier to invert the ODCID and token on Retry.  That would make the token processing more consistent.  I'm not sure if we want to consider adding a token to 0-RTT and Handshake packets, but that would fully normalize things.

Other open question (editorial): The variety of long header formats is now a little unwieldy.  Should we reduce the long header back to what is in invariants and describe the packet number encoding part as being specific to Initial, Handshake, and 0-RTT?  Something for editors to consider.

Closes #1492, #1451.
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/1498

-- Commit Summary --

  * Rework Retry packet

-- File Changes --

    M draft-ietf-quic-transport.md (221)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/1498.patch
https://github.com/quicwg/base-drafts/pull/1498.diff

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

---

From: ekr <[email-masked]>
Date: Thu, 28 Jun 2018 17:52:17 -0700

ekr requested changes on this pull request.

IIRC, there was some resistance to having a token length, though I can't recall why.

> @@ -532,6 +532,82 @@ See {{version-negotiation}} for a description of the version negotiation
 process.

+## Retry Packet {#packet-retry}
+
+A Retry packet uses long headers with a type value of 0x7E. It carries an

The comment suggests that we shouldn't say it uses long headers.

> ++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|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 uses part of the long packet
+header.  The contents of the Retry packet are not protected.  Like Version

I would say "The first X fields of a Retry packet are the same as the long header"

> +: 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.
+
+If the Original Destination Connection ID field does not match the Destination
+Connection ID from most recent the Initial packet it sent, clients MUST discard

the most recent

> +
+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 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.
+
+The client responds to a Retry packet with Initial packet that includes the
+provided Retry Token to continue connection establishment.
+
+A server that might send another Retry packet in response to a subsequent
+Initial packet MUST set the Source Connection ID to new value of at least 8
+octets in length.  This allows clients to distinguish between Retry packets when
+the server sends multiple rounds of Retry packets.  A server that will not send
+additional Retry packets can set the Source Connection ID to any value.

I think you need to add that a client MUST reject a retry that contains an ODCID of less than 8 bytes.

>  +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                            Token (*)                        ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                           Length (i)                        ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                     Packet Number (8/16/32)                   |
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                          Payload (*)                        ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

I am assuming these haven't changed.

-If the client has a suitable token available from a previous connection, it
-SHOULD populate the Token field.
+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
+value in the Token field of its Initial packet.

This seems oddly non-normative.

Also, I notice you have removed "suitable" but if you think you changed your IP then you probably shouldn;t.

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

---

From: Kazuho Oku <[email-masked]>
Date: Thu, 28 Jun 2018 18:56:51 -0700

> Open Question: would it make sense to normalize the encoding of Retry and Initial? The former includes a token without a length (because it's the last piece of the packet), whereas the latter includes a length-prefixed token. It might be easier to invert the ODCID and token on Retry. That would make the token processing more consistent.

My preference goes to keeping the current format as-is, because IMO it is better to have fields that identify the connection at the top. Having ODCID above token allows implementations to bail out before dealing with the token, if ODCID does not match the expected value.

>  I'm not sure if we want to consider adding a token to 0-RTT and Handshake packets, but that would fully normalize things.

I think that we should limit the existence of the token field to Initial packets.

Having the field in Handshake packet does not make sense at all, because it is sent by the client after the path is validated using the token. Note also that we are trying hard to cram the certificate chain into the Handshake packets that can be sent in the first flight. Having an unnecessary field (even if it is empty) goes against that.

Having the field in 0-RTT packet _might_ make some sense, but I oppose, because it allows two ways of implementing buffering of potentially reordered 0-RTT packets on the server side. One way (the only way which is possible now) is to buffer such packets without validating the paths. The other way that will be opened up by having the field is to buffer such packets after validating the paths.

This flexibility will incentivize the clients to include token for every 0-RTT packets even though it reduces the space that can be used to carry application data. And that in turn disincentivises the server to buffer potentially reordered 0-RTT packets.

To summarize, having a token field in 0-RTT packet will force us to end up in an suboptimal implementation, even though 0-RTT is about optimization. I do not think that we want to see such an outcome.

-- 
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#issuecomment-401224459

---

From: Nick Banks <[email-masked]>
Date: Thu, 28 Jun 2018 19:05:59 -0700

nibanks commented on this pull request.

Generally looks good. Just a few comments.

+Retry Token:
+

Could a server not send a Retry in response to a 0-RTT packet? Say the Initial was lost/reordered, and the 0-RTT arrived first.

Two things. One, if multiple in-path devices send a Retry, how do other devices later in the path know they are sending 'another Retry packet'? Two, why 8 bytes? Why not 4 or 6 or any other number for that matter? Why not just specify that a CID (of any length) must be sent in response?

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

---

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Jun 2018 22:14:18 -0700

FWIW, I agree with @kazuho's rationale.  Thanks for the careful analysis there.  I wanted to make sure that the question was asked and hadn't thought it through that fully.

-- 
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#issuecomment-401249606

---

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Jun 2018 22:15:47 -0700

martinthomson commented on this pull request.

+Retry Token:
+

I don't think so.  The server cannot decrypt 0-RTT, so it's only option is to 

1. buffer it in the hopes that it might complete the handshake when it is done, or

2. discard it, which seems the most likely choice if the server is predisposed to send a Retry.

-- 
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#discussion_r199054856

---

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Jun 2018 22:19:20 -0700

If things on the path aren't planning to terminate the connection (presumably they know this), then they can never send a token with <8.

8 is the number we picked for Initial.  It seemed best to align with that.  That's all.  It's probably the case that 4 would give us equivalent defense against off-path attacks to the TCP handshake, but there is value in consistency here.

-- 
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#discussion_r199055162

---

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Jun 2018 22:21:55 -0700

The point is that it can't really be normative.  You can't say MUST for sure, and frankly the whole normative thing is not valuable here: there is no interoperability end served by including a token.

"suitable" wasn't defined, but I think that it just highlighted a deficiency, so I've add text on linkability.

-- 
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#discussion_r199055423

---

From: Martin Thomson <[email-masked]>
Date: Thu, 28 Jun 2018 22:22:28 -0700

@martinthomson pushed 1 commit.

bf3f675  Review comments

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1498/files/b3755446d985d542105628c0a6509328976e89da..bf3f67597f3315018d348bfcf638d8fb9379ff0f

---

From: MikkelFJ <[email-masked]>
Date: Fri, 29 Jun 2018 02:11:21 -0700

mikkelfj commented on this pull request.

+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

grammar: Connection ID from the most recent the Initial packet it sent,

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

---

From: Nick Banks <[email-masked]>
Date: Fri, 29 Jun 2018 06:30:16 -0700

They way I read this sentence is that only for additional Retry packets (after the first one) do you need to change the Source CID. But if the server doesn't know that some other middle box (DDoS Mitigation device) has already sent a Retry packet once, then it would likely assume it is the first once to send a Retry packet. Then it might not change the Source CID.

-- 
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#discussion_r199158920

---

From: Nick Banks <[email-masked]>
Date: Fri, 29 Jun 2018 06:33:00 -0700

+Retry Token:
+

So, for a DDoS mitigation device that just sees a 0-RTT packet (assuming the Initial was lost/reordered) it should just drop it instead of sending a Retry? That works, but I just wonder why not use that extra info to do the Retry immediately. Otherwise, the client will eventually retransmit the Initial, and then have to pay an additional RTT for the retry.

-- 
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#discussion_r199159742

---

From: MikkelFJ <[email-masked]>
Date: Fri, 29 Jun 2018 06:42:28 -0700

@nibanks the way I read it, the reset is stateless so each source is semi-randomly generated, consequently a second retry will stastically differ from the first. Also across servers. This could be made clearer.

-- 
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#discussion_r199162469

---

From: ekr <[email-masked]>
Date: Fri, 29 Jun 2018 07:09:52 -0700

ekr commented on this pull request.

We have plenty of MUST requirements that don't serve interop. But SHOULD would be fine as well. Writing it as "it will" is just confusing.

-- 
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#discussion_r199171273

---

From: ianswett <[email-masked]>
Date: Fri, 29 Jun 2018 12:06:49 -0700

ianswett commented on this pull request.

This looks like an improvement overall and clarifies some points.

> +|                         Version (32)                          |
+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)                          |

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:
+

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.

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

nit: Unnecessary "that"

> @@ -624,10 +724,18 @@ server, it MUST discard any packet it receives with a different Source
 Connection ID.

-#### Tokens
+### Tokens
+

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