Skip to content
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

Merged
merged 13 commits into from Jul 31, 2018
11 changes: 7 additions & 4 deletions draft-ietf-quic-transport.md
Expand Up @@ -569,8 +569,8 @@ Packet Number, and Payload fields. These are replaced with:
ODCIL:

: 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
the DCIL and SCIL fields. The most significant 4 bits of this octet are
encoded in the least significant 4 bits of the octet, using the same encoding
as the DCIL and SCIL fields. The most significant 4 bits of this octet are
reserved. Unless a use for these bits has been negotiated, endpoints SHOULD
send randomized values and MUST ignore any value that it receives.

Expand Down Expand Up @@ -606,8 +606,11 @@ 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
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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"

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.
the server sends multiple rounds of Retry packets. Consequently, a valid Retry
Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nibanks

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The client should not change its SCID each time. See #1474 and the associated PR #1491.

Copy link
Contributor

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.

Copy link
Member

@nibanks nibanks Jul 19, 2018

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:

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

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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.

packet will always have an Original Destinagion Connection ID that is at least 8
octets long; clients MUST discard Retry packets that include a shorter value. A
server that will not send additional Retry packets can set the Source Connection
ID to any value.


## Cryptographic Handshake Packets {#handshake-packets}
Expand Down