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

Specify src_ip correctly in Transmits in iroh-net #2632

Open
matheus23 opened this issue Aug 15, 2024 · 3 comments
Open

Specify src_ip correctly in Transmits in iroh-net #2632

matheus23 opened this issue Aug 15, 2024 · 3 comments
Labels
c-iroh-net feat New feature or request

Comments

@matheus23
Copy link
Contributor

Currently, the src_ip values for quinn::Transmits are indirectly set by us via our rewriting of dst_ip (and choice of dst_ip in the relay case) in the RecvMeta of poll_recv (in MagicSock).

This can cause issues, e.g. when unspecified IP addrs (0.0.0.0 or ::) make it to src_ip. Windows seems more pedantic about this and errors out.

We return unspecified IP addrs, because we set dst_ip to MagicSock::normalized_local_addr.

As an interim solution, we've set it to None on windows.
I suspect this can cause issues when there's multiple IP interfaces connected to the device and the OS ends up choosing the wrong one.

Instead, we should probably figure out the interface we want to send from via netcheck.

More information in this PR comment.

@Ralith
Copy link

Ralith commented Aug 23, 2024

I suspect this can cause issues when there's multiple IP interfaces connected to the device and the OS ends up choosing the wrong one.

The problem also arises when you have multiple addresses on the same subnet on the same interface. IIRC this is the default on Windows, and a supported configuration on Linux, due to RFC 4941 privacy extensions: a host may have both a stable EUI-64 address, a primary temporary address, and one or more old temporary addresses being turned down, all on the same WAN subnet. An IPv6-based protocol is unlikely to be able to establish a connection correctly in this environment without correctly specifying source addresses.

As a general rule of thumb, you should send from whatever address you received on, for a given path.

@flub
Copy link
Contributor

flub commented Aug 23, 2024

(I previously wrote this on discord somewhere, repeating here)

We need to store the dst_ip of the RecvMeta in the PathState and when we return the "addr_for_send" it should be returned with it so that the send path can put it in src_ip.

I'm not sure if this should be blocking for the transition to 0.11, what do you think @matheus23 ?

I do think we should do the same on all platforms regardless.

@matheus23
Copy link
Contributor Author

matheus23 commented Aug 23, 2024

We need to store the dst_ip of the RecvMeta in the PathState and when we return the "addr_for_send" it should be returned with it so that the send path can put it in src_ip.

Yeah agreed, we should do this & do it like this.

I'm not sure if this should be blocking for the transition to 0.11, what do you think @matheus23 ?

I don't think this should be blocking the transition.

  • With the current setup, iroh works on windows, both on CI and when I was testing it manually in my home network (it works on my machine (tm)).
  • There was a change in quinn 0.10 to quinn 0.11 behavior: Previously, quinn-udp didn't use transmit.src_ip information, in quinn 0.11 it does. The current state in the dig/quinn11 branch simply restores the quinn 0.10 behavior.
  • It's not ideal, but I want to minimize changes from the dig/quinn11 branch. Let's get that merged with the fewest bugs possible & the least changes. Then we can start "using newer features" of quinn 11, such as being able to supply src_ip in transmits on windows (and in general use the correct ones).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-iroh-net feat New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants