-
Notifications
You must be signed in to change notification settings - Fork 205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework Retry packet #1498
Rework Retry packet #1498
Conversation
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. Closes #1492, #1451.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, there was some resistance to having a token length, though I can't recall why.
draft-ietf-quic-transport.md
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment suggests that we shouldn't say it uses long headers.
draft-ietf-quic-transport.md
Outdated
{: #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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say "The first X fields of a Retry packet are the same as the long header"
draft-ietf-quic-transport.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the most recent
draft-ietf-quic-transport.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to add that a client MUST reject a retry that contains an ODCID of less than 8 bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's already rejecting a ODCID that doesn't match what it picked, and it has to have picked something >= 8 bytes, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: to a
new value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that I caught that already.
ekr is right, if you send the first Initial with DCIL>=8 and the server respects this rule, then you will never ever get a Retry with ODCIL>=8.
mikkel, any value is correct. The server can keep the old value if that suits it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinthomson I thought it was a typo, missing an a
not a question about semantics.
| Packet Number (8/16/32) | | ||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| Payload (*) ... | ||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This new design solves issue #1535.
draft-ietf-quic-transport.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. Just a few comments.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The server cannot decrypt 0-RTT, so it's only option is to
-
buffer it in the hopes that it might complete the handshake when it is done, or
-
discard it, which seems the most likely choice if the server is predisposed to send a Retry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not even sure it's necessary for it to be a SHOULD; there's no interoperability risk if it sends a Retry in response to a lone 0-RTT packet, is there? In which case it sounds more like servers MAY omit sending Retry in response to 0-RTT when they haven't seen the Initial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's much value in sending Retry in response to 0RTT packets. Sending it without decrypting the incoming packet makes me queasy, and sending it a few times after receiving the Initial sounds like a strange retransmission strategy. It's easy to see a server generating a ton of Retry packets in response to Initial + 0RTT packets, so it seems reasonable to recommend better behavior here, and I like the text that's there. I don't mind weakening it to a SHOULD though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump for changing this to a SHOULD for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to leave this as SHOULD for now, but will open a new issue. This wasn't the focus of this change (this is not my text), so let's have an explicit discussion about it.
draft-ietf-quic-transport.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nibanks, it's the other way around. "A server that might send another" => all but the last. If you don't know you're the last element in the chain, you MUST change the CID. (Presumably to something that will get the retransmitted packet further down the DDoS chain.)
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. |
draft-ietf-quic-transport.md
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grammar: Connection ID from the most recent the Initial packet it sent,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an improvement overall and clarifies some points.
draft-ietf-quic-transport.md
Outdated
~~~ | ||
{: #retry-format title="Retry Packet"} | ||
|
||
A Retry packet (shown in {{retry-format}}) only the invariant portion of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"A Retry packet only" uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicative of the first sentence in this section. Perhaps condense?
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| ODCIL(8) | Original Destination Connection ID (*) | | ||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| Retry Token (*) ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, you could; just that, like a short-header packet, the Retry would have to come last. But as you say, I can't imagine a use for it in this version of QUIC, and since these are version-specific packets....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit esoteric, but here's possible use cases:
Let's say a server only support QUIC v1, and it receives an Initial packet for a different version. The server would then send a Version Negotiation Packet, and could coalesce that with a Retry packet, in order to save one round trip.
I admit feels kind of clumsy though, since the Retry would have to go in front of the VNP (since the VNP doesn't have a length field).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marten: I am not sure this is a valid use case. The main use case for the retry packet is that it is sent by some kind of firewall as part of DOS defense.
The VN is sent end-to-end, and already carries a token of sorts with the Source CID. I don't see the use case of sending both retry and VN end to end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In p2p, you won't have a separate firewall, but a peer might still want to use the Retry mechanism if it's under attack. I'm aware that this is not a very strong argument, since in that case you'd probably be ok with incurring an additional RTT, on the other hand, adding a varint to the header doesn't cost us a lot either.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
draft-ietf-quic-transport.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "with a bogus new Source Connection ID." is unnecessary here, since any off-path injection is bad.
draft-ietf-quic-transport.md
Outdated
|
||
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be clearer as "Initial packets sent by the server must specify a zero token length."
draft-ietf-quic-transport.md
Outdated
### 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it SHOULD include that?
draft-ietf-quic-transport.md
Outdated
|
||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't clear enough. I think that it's SHOULD NOT reuse (blanket), and MUST NOT reuse if the client moves.
draft-ietf-quic-transport.md
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SHOULD contain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or MUST contain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly not MUST. Consider the case where the Initial packet is resent after a version negotiation, or repeated after time-out. Then the "natural" value of the sequence number will be 1. I don't see how saying "must be zero" would improve interoperability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Christian got it. MUST doesn't work, and throwing normative language at this is too heavy. I think that this is fine.
draft-ietf-quic-transport.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
MUST increase
draft-ietf-quic-transport.md
Outdated
@@ -532,6 +532,86 @@ See {{version-negotiation}} for a description of the version negotiation | |||
process. | |||
|
|||
|
|||
## Retry Packet {#packet-retry} | |||
|
|||
A Retry packet uses the invariant portion of the long packet header with a type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Invariant portion of the long packet header" feels awkward. The invariants specify what a long header looks like, and this draft specifies additional fields which are in certain types. The "Long Header" section actually adds additional fields beyond the long header which are type-specific to most (but not all) packet types used by this version. Maybe the real subdivision is "Retry Packets" and "Connection Setup Packets", along with a reminder than Version Negotiation packets are defined in invariants?
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| ODCIL(8) | Original Destination Connection ID (*) | | ||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| Retry Token (*) ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, you could; just that, like a short-header packet, the Retry would have to come last. But as you say, I can't imagine a use for it in this version of QUIC, and since these are version-specific packets....
draft-ietf-quic-transport.md
Outdated
~~~ | ||
{: #retry-format title="Retry Packet"} | ||
|
||
A Retry packet (shown in {{retry-format}}) only the invariant portion of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicative of the first sentence in this section. Perhaps condense?
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not even sure it's necessary for it to be a SHOULD; there's no interoperability risk if it sends a Retry in response to a lone 0-RTT packet, is there? In which case it sounds more like servers MAY omit sending Retry in response to 0-RTT when they haven't seen the Initial.
draft-ietf-quic-transport.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nibanks, it's the other way around. "A server that might send another" => all but the last. If you don't know you're the last element in the chain, you MUST change the CID. (Presumably to something that will get the retransmitted packet further down the DDoS chain.)
draft-ietf-quic-transport.md
Outdated
|
||
The first packet sent by a client always includes a CRYPTO frame that contains | ||
the first cryptographic handshake message. The first packet sent by a client | ||
MUST fit in a single UDP datagram (see {{handshake}}). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tautological, since we have no mechanism for a packet to span datagrams. But the first TLS message must fit in the first packet.
draft-ietf-quic-transport.md
Outdated
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. | ||
If the client has an token received in a NEW_TOKEN frame on a previous |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
an
draft-ietf-quic-transport.md
Outdated
address or network interface. Reusing a token on different network paths would | ||
allow activity to be linked between paths (see {{migration-linkability}}). A | ||
client needs to start the connection process over if it migrates prior to | ||
completing the handshake. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two separate sentences about linking, one stating "correlate", the other "linked".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, thanks.
draft-ietf-quic-transport.md
Outdated
|
||
: The length of the Original Destination Connection ID field as an unsigned | ||
8-bit integer. This field does not use the same encoding as the DCIL and SCIL | ||
fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the encoding the same as the DCIL? It's unnecessary, I agree, but it seems cleaner, since all CILs are then similarly encoded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @janaiyengar. Having the same encoding would remove error conditions such as 1<=ODCIL<=3, or ODCIL>18. But of course it would introduce the error "(ODCIL&0xF0)!=0". So maybe it is a wash.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's much value in sending Retry in response to 0RTT packets. Sending it without decrypting the incoming packet makes me queasy, and sending it a few times after receiving the Initial sounds like a strange retransmission strategy. It's easy to see a server generating a ton of Retry packets in response to Initial + 0RTT packets, so it seems reasonable to recommend better behavior here, and I like the text that's there. I don't mind weakening it to a SHOULD though.
draft-ietf-quic-transport.md
Outdated
Connection ID from the most recent Initial packet it sent, clients MUST discard | ||
the packet. This prevents an off-path attacker from injecting a Retry packet. | ||
|
||
The client responds to a Retry packet with Initial packet that includes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing "an"
draft-ietf-quic-transport.md
Outdated
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. A | ||
client MUST ignore a Retry that contains an ODCIL field with a value less than | ||
8 or greater than 18. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this last sentence to the description of ODCIL above.
draft-ietf-quic-transport.md
Outdated
It may be zero if no token is present. Initial packets sent by the server | ||
MUST include a zero-length token. | ||
This value is zero if no token is present. Initial packets sent by the server | ||
MUST specify a token of zero length. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"token length of zero"?
draft-ietf-quic-transport.md
Outdated
|
||
A client SHOULD NOT reuse a token. Reusing a token on different network paths | ||
would allow activity to be linked between paths (see {{migration-linkability}}). | ||
A client MUST NOT reuse a token if it believes that its point of network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit strong. If the client has a token that was sent under crypto cover (NEW_TOKEN as against Retry), it seems reasonable to use it on a new network path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janaiyengar , no, this is not too strong. There is a real privacy risk to reusing tokens in different contexts. This is pretty similar to the privacy issues with TCP Fast Open, or with TLS resume. Encryption of New Token provides protection against on path elements, but it does not provide protection against the server itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using the token implies willingness to be tracked by the server. I've tweaked the language accordingly. New text on breaking tracking by the server, and tighter language on reuse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like these changes. Can we adopt them as baseline for the 7th interop? The retry mechanism of draft-13 is pretty hard to implement without those.
@@ -745,6 +745,9 @@ handled separately. | |||
sample_offset = 6 + len(destination_connection_id) + | |||
len(source_connection_id) + | |||
len(payload_length) + 4 | |||
if packet_type == Initial: | |||
sample_offset += len(token_length) + | |||
len(token) | |||
~~~ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I am missing something, len(token_length) is only available after the PN has been decrypted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we have a circular dependency when decrypting. See issue #1535.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad. This is actually addressed later, as the token has now moved in front of the PN.
draft-ietf-quic-transport.md
Outdated
|
||
: The length of the Original Destination Connection ID field as an unsigned | ||
8-bit integer. This field does not use the same encoding as the DCIL and SCIL | ||
fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @janaiyengar. Having the same encoding would remove error conditions such as 1<=ODCIL<=3, or ODCIL>18. But of course it would introduce the error "(ODCIL&0xF0)!=0". So maybe it is a wash.
| Packet Number (8/16/32) | | ||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ | ||
| Payload (*) ... | ||
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This new design solves issue #1535.
draft-ietf-quic-transport.md
Outdated
|
||
A client SHOULD NOT reuse a token. Reusing a token on different network paths | ||
would allow activity to be linked between paths (see {{migration-linkability}}). | ||
A client MUST NOT reuse a token if it believes that its point of network |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janaiyengar , no, this is not too strong. There is a real privacy risk to reusing tokens in different contexts. This is pretty similar to the privacy issues with TCP Fast Open, or with TLS resume. Encryption of New Token provides protection against on path elements, but it does not provide protection against the server itself.
draft-ietf-quic-transport.md
Outdated
8-bit integer. This field does not use the same encoding as the DCIL and SCIL | ||
fields. | ||
: The length of the Original Destination Connection ID field. The length is | ||
encoded in the least significant bit of the octet using the same encoding as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs more than one bit.
draft-ietf-quic-transport.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's already rejecting a ODCID that doesn't match what it picked, and it has to have picked something >= 8 bytes, no?
Specifically, the connection where the token was issued, and any connection | ||
where it is used. Clients that want to break continuity of identity with a | ||
server MAY discard tokens provided using the NEW_TOKEN frame. Tokens obtained | ||
in Retry packets MUST NOT be discarded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The prohibition below is on using a token on two different network paths. But since the token is likely to contain address validation information, should we recommend against using the token from a different address than the one where it was received? Or is that solely a risk of server tracking and it's covered in the privacy-from-server paragraph here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the intent of this paragraph is to cover that. The paragraph below is privacy-from-network.
draft-ietf-quic-transport.md
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martinthomson, I suspect @mikkelfj's comment was directed to this line -- "to a new value"
A server that might send another Retry packet in response to a subsequent | ||
Initial packet MUST set the Source Connection ID to a 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. Consequently, a valid Retry |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of requiring the server to use a new CID, why not just make the client use a new random CID for every new token it sends in the Initial packet? That way the client has direct control to be able to differentiate between the possibly Retry responses? And no additional complexity on the server side.
Going further, I was thinking during the talks today, if we are going to recommend that initial packet routing not take the client Initial's DCID into account (because it would be an attack surface) then I don't see any reason to support the Retry packet changing the CID at all. The primary reason for adding that support initially was for routing/load balancing; but that seems to be ill advised now. It just adds unnecessary complexity in the client code, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of requiring the server to use a new CID, why not just make the client use a new random CID for every new token it sends in the Initial packet?
The clients source SCID must be new. The clients original DCID is always new and random in an initial packet. Only the source is reflected back by the server. this is why it must be new. The alternative would be to have a separate nonce in the handshake, separate from SCID, which is what I suggest.
On the going further: The server must set is own SCID - otherwise the server is forced to use a client chosen SCID which is not only a load balancer issue and it also goes against the point of a retry. A retry is a redirect to another server, not try again until you get lucky, although that can also be implemented if the server randomizes its SCID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mikkelfj if the client uses a new SCID then it would be considered an entirely new connection, and might elicit another Retry. I am talking about removing the requirement from the server in generating new random CID, and instead have the client generate a new random DCID for each try.
Also, on the going further, I am only talking about Retry. After that, in the handshake the server can/will change the CID to whatever it wants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing wrong with getting a new retry, the problem is stopping when too many happen. If the client changes SCID each time, it can stop after 1-3 retries - but it helps if it can know that all retries are from the same chain. I think a nonce is better for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clients original DCID disappears once it reaches the server. Since the server sets its own SCID and the client is not allowed to change its SCID, the context is lost.
While I agree that not changing SCID is a good thing, I wondered why that was made a requirement before solving this loss of context here.
Your proposal to modify servers SCID seems like a workaround for the fundamental problem that some nonce is required. But I dont' recall all the concerns related to this right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am considering only the Retry scenario right now. During the handshake, after the Retry, the server has the opportunity to change its SCID once again (independent of if it was changed during Retry). I am trying to make two points:
- I don't see any reason for the server to generate a new, random SCID (client's DCID) with Retry. The client itself can handle creating a new random DCID when it retries.
- Since we are going to recommend NOT load balancing off the client's initial SCID, then there is no reason to use Retry (for load balancing purposes) to change the CID at all.
So, Retry just shouldn't change the CID at all, IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nibanks, I think that the idea was to give the server better control over where to send subsequent Retry packets. That is, it can choose a value that is better for load, DoS prevention, or whatever it needs. The server can use the token to validate that the value was its own choice as well, removing concerns about the client stacking load on certain servers.
(I thought that this was one of your design constraints, but I realize now I don't know where that idea came from.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe @mcmanus originally requested the ability to change CID with the (old) Retry packet. For DoS, I only require the Token field.
Is it true or not that we recommend not using the client Initial CID for load balancing decisions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack.
I don't think that we have a concrete statement about using the Destination Connection ID from the client Initial. With a token from Retry, you could; without, it's fraught, I agree.
Generating a new cryptographic handshake message is a fine response to Retry or Version Negotiation. It's sometimes necessary, but even when it is not, it avoids having to worry about 0-RTT packet numbers. Followup for #1498.
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 Looping with multiple Retry packets #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, #1448.