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

Martin Duke Transport Comment 2 #4453

Closed
LPardue opened this issue Dec 22, 2020 · 4 comments · Fixed by #4696
Closed

Martin Duke Transport Comment 2 #4453

LPardue opened this issue Dec 22, 2020 · 4 comments · Fixed by #4696
Assignees
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. iesg An issue raised during IESG review.

Comments

@LPardue
Copy link
Member

LPardue commented Dec 22, 2020

@martinduke said:

21.13 "This means that client-controlled fields, such as the initial
Destination Connection ID used on Initial and 0-RTT packets SHOULD NOT be used
by themselves to make routing decisions." There was a lot of discussion in the
QUIC-LB design team about whether this was an attack to be worried about or
not, and we came down in favor of "not".

More importantly, I don't see how this is practical advice. If we're to use
Retry SCIDs to route subsequent packets, then load balancers have to use the
DCID of Initials. Without validating the token, which most LBs will not do,
they have no way of distinguishing between attacker-generated DCIDs with a
bogus token and those that originally came from the server. One option is to
simply remove this recommendation.

Alternatively, you could leave this section unaltered and delete the bit in
8.1.2 about using Retry to reroute packets. In practice, keeping 21.13 would
require a revision of QUIC-LB to just use 4-tuple routing for long header
packets or make it less robust for new versions.

@LPardue LPardue added -transport iesg An issue raised during IESG review. labels Dec 22, 2020
@LPardue LPardue added this to the transport-iesg milestone Dec 22, 2020
@martinthomson
Copy link
Member

I think that the point of this section is to highlight the possibility that routing based on client-selected values exposes servers to load that is balanced under client - and therefore attacker - control. I don't think that it needs to include such a strong recommendation.

If people deploying load balancers are not concerned about this, then that is good. That makes the recommendation less good.

As this is very old text, it needs a bit of a cleanup anyway. I'm going to suggest something along the lines of my statement above in a pull request and we can polish the wording there.

I don't think that we should remove the Retry SCID language. That remains an option for servers, even if they don't want to exercise the option (whether load balancers look at Retry tokens is very much down to how the various functions are distributed). What could happen is that the Retry SCID is chosen so that the next initial can be routed as though it were chosen by a client, but in such a way as it results in the connection being sent to a lightly-loaded server instance. That is, if the load balancer routes inchoate Initials based on connection ID, knowledge of that routing algorithm is used to direct traffic. That said, I understand that what is more likely here is that Initials with inauthentic Destination Connection ID fields will be routed using other information: round-robin, random, five-tuple, or whatever.

@martinduke
Copy link
Contributor

I think you're proposing that the text I quoted be deleted. That is satisfactory, and what I would recommend as well.

martinthomson added a commit that referenced this issue Jan 7, 2021
This was just old and needed a little bit of a refresh.

Removing the recommendation, which was counter to established views and
unnecessary.

I will let others determine whether this is editorial/design.

Closes #4453.
@janaiyengar janaiyengar added the design An issue that affects the design of the protocol; resolution requires consensus. label Jan 11, 2021
@larseggert larseggert added this to Triage in Late Stage Processing via automation Jan 11, 2021
@larseggert larseggert moved this from Triage to Design Issues in Late Stage Processing Jan 12, 2021
Late Stage Processing automation moved this from Design Issues to Issue Handled Jan 14, 2021
@martinthomson
Copy link
Member

Returning to an open state as the chairs directed in their plan.

@martinthomson martinthomson reopened this Jan 14, 2021
Late Stage Processing automation moved this from Issue Handled to Triage Jan 14, 2021
@larseggert larseggert moved this from Triage to Design Issues in Late Stage Processing Jan 15, 2021
@LPardue LPardue added the call-issued An issue that the Chairs have issued a Consensus call for. label Jan 18, 2021
@project-bot project-bot bot moved this from Design Issues to Consensus Call issued in Late Stage Processing Jan 18, 2021
@LPardue
Copy link
Member Author

LPardue commented Feb 3, 2021

Closing this now that the IESG have approved the document(s).

@LPardue LPardue closed this as completed Feb 3, 2021
Late Stage Processing automation moved this from Consensus Call issued to Issue Handled Feb 3, 2021
@LPardue LPardue added has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. and removed call-issued An issue that the Chairs have issued a Consensus call for. labels Feb 21, 2021
@project-bot project-bot bot moved this from Issue Handled to Consensus Declared in Late Stage Processing Feb 21, 2021
@LPardue LPardue moved this from Consensus Declared to Issue Handled in Late Stage Processing Feb 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-transport design An issue that affects the design of the protocol; resolution requires consensus. has-consensus An issue that the Chairs have determined has consensus, by canvassing the mailing list. iesg An issue raised during IESG review.
Projects
Late Stage Processing
  
Issue Handled
Development

Successfully merging a pull request may close this issue.

4 participants