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

Maintain in-flight counters separately per path #1777

Merged
merged 3 commits into from
Mar 11, 2024
Merged

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Mar 10, 2024

Fixes #1775, theoretically. @nemethf can you confirm?

RFC9000 §9.4:

The capacity available on the new path might not be the same as the old path. Packets sent on the old path MUST NOT contribute to congestion control or RTT estimation for the new path.

We accomplish this by tracking the first packet number on each path, which allows the logic that removes acked/lost packets from congestion control counters to determine which, if any, known path a packet was sent on.

@Ralith Ralith force-pushed the in-flight-per-path branch 2 times, most recently from 2d05b84 to 38d3aa0 Compare March 10, 2024 02:09
@Ralith Ralith changed the title Reset in-flight counters when migrating to a new path Maintain in-flight counters separately per path Mar 10, 2024
@nemethf
Copy link
Contributor

nemethf commented Mar 10, 2024

Fixes #1775, theoretically. @nemethf can you confirm?

Yes, it does solve my original issue and the traffic quickly migrates to the new path. Thank you.


Looking at the captured traffic, I was a bit surprised:

 No. -     Time -             Source -           Dest -             Proto -        Length -         Info -
 278       2.897193           10.0.2.1           10.0.3.1           QUIC           78               Protected Payload (KP0), DCID=7abe9d2420aaddb1, PKN: 101, PING, PADDING
 279       2.907467           10.0.3.1           10.0.1.1           QUIC           1248             Protected Payload (KP0), DCID=033dda038eeebb0b, PKN: 219, PC, PADDING
 280       2.907472           10.0.3.1           10.0.2.1           HTTP3          1248             Protected Payload (KP0), DCID=033dda038eeebb0b, PKN: 220, ACK_ECN, PC, RC, STREAM(3)

In other words the server sends payload alongside with the PATH_CHALLENGE. I did not know this was possible, but RFC9000 permits it: " An endpoint MAY send data to an unvalidated peer address, but it MUST protect against potential attacks as described in Sections 9.3.1 and 9.3.2." So it's all good. Thanks again.

@Ralith
Copy link
Collaborator Author

Ralith commented Mar 10, 2024

Yep, that's the intended behavior. Separate logic applies an anti-amplification budget to all unvalidated paths to prevent this from being too much of an amplification hazard if someone attempts the rather esoteric off-path packet forwarding attack. Thanks for confirming!

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Nice!

quinn-proto/src/connection/packet_builder.rs Outdated Show resolved Hide resolved
quinn-proto/src/connection/mod.rs Outdated Show resolved Hide resolved
@Ralith Ralith enabled auto-merge (rebase) March 11, 2024 19:14
@Ralith Ralith merged commit 62205e8 into main Mar 11, 2024
8 checks passed
@Ralith Ralith deleted the in-flight-per-path branch March 11, 2024 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PATH_CHALLENGE is never sent
3 participants