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

fix: ignore local address when considering path migration #2458

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

camshaft
Copy link
Contributor

@camshaft camshaft commented Jan 24, 2025

Description of changes:

We currently consider the full 4-tuple (local + remote IP addresses and ports) when identifying a path. This seems to be an overly strict interpretation of how paths are considered in the RFC. I don't see any text indicating this is the correct approach. In fact, I see the following:

//= https://www.rfc-editor.org/rfc/rfc9000#section-9.3
//# Receiving a packet from a new peer address containing a non-probing frame
//# indicates that the peer has migrated to that address.

That text seems to indicate that we should really only be considering the peer address when deciding if it tried to migrate. So I've implemented that change here.

Additionally, I've ensured that the server does not update the local address it is sending on, even if received a packet on a new one, since this would also go against what the RFC recommends:

//= https://www.rfc-editor.org/rfc/rfc9000#section-9
//# If a client receives packets from an unknown server address, the client MUST discard these packets.

There's a high probability the client isn't aware that packets are being sent to different server IPs and would result in the client dropping packets, since the server's source IP would be different. As such, the server will not change its path at all after the initial packet is received.

Testing:

I've added a test for the server which forces the local IP to change before the handshake completes. Before this change, this test would fail, since we'd drop all packets with a different IP. After this change, this test passes.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@camshaft camshaft marked this pull request as ready for review January 24, 2025 21:43
Comment on lines +285 to +287
if Config::ENDPOINT_TYPE.is_client() {
path.handle.maybe_update(path_handle);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a possibility the server's local address remains on port 0 with this change? And does that matter?

Copy link
Contributor Author

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 since the server always receives a packet which contains both sides. The reason we need it on the client is:

  • The client's IP can change so we want to be using the latest value
  • The first packet the client sends, we don't really know how it will be routed so we don't know what the local IP will be at all.

@WesleyRosenblum WesleyRosenblum marked this pull request as draft January 24, 2025 23:09
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.

2 participants