Support override-endpoint with the unspecified address #315
Conversation
There was a problem hiding this comment.
Somehow it was pretty challenging for my mind to review this. I think this is a good direction in general, but I suggest some slight changes to the server logic.
In addition to inline comments, I think we should also consider backwards and forwards compatibility:
-
new server, old client scenario: should work just fine; even one new client reporting unspecified addr while other clients being old works well, the server "resolves" the endpoint for them.
-
new client, old server: then I think the server ends up sending address like 0.0.0.0:1234 to clients. I'd say this is still acceptable if it doesn't break connections - i.e. if Report wireguard endpoint as a candidate when an endpoint override is in place #305 still saves the day in that case. But we could check the server version on the client and refuse to use unspecified address if it is old, if that's easy.
We could also filter our the unspecified addresses from peer endpoints/candidates on client side for this scenario when trying to connect to other peers.
| pub fn inject_endpoints(session: &Session, peers: &mut Vec<Peer>) { | ||
| for peer in peers { | ||
| let endpoints = session.context.endpoints.read(); | ||
| if let Some(wg_endpoint) = endpoints.get(&peer.public_key) { | ||
| if peer.contents.endpoint.is_none() { | ||
| peer.contents.endpoint = Some(wg_endpoint.to_owned().into()); | ||
| let wg_endpoint_ip = wg_endpoint.ip(); | ||
| let wg_endpoint: Endpoint = wg_endpoint.to_owned().into(); | ||
| if let Some(endpoint) = &mut peer.contents.endpoint { | ||
| if endpoint.is_host_unspecified() { | ||
| // (2) Unspecified endpoint host | ||
| *endpoint = SocketAddr::new(wg_endpoint_ip, endpoint.port()).into(); | ||
| } else if *endpoint != wg_endpoint { | ||
| // (1) NAT holepunching | ||
| // The peer already has an endpoint specified, but it might be stale. | ||
| // If there is an endpoint reported from wireguard, we should add it | ||
| // to the list of candidates so others can try to connect using it. | ||
| peer.contents.candidates.push(wg_endpoint); | ||
| } | ||
| } else { | ||
| // The peer already has an endpoint specified, but it might be stale. | ||
| // If there is an endpoint reported from wireguard, we should add it | ||
| // to the list of candidates so others can try to connect using it. | ||
| peer.contents.candidates.push(wg_endpoint.to_owned().into()); | ||
| // (1) NAT holepunching | ||
| peer.contents.endpoint = Some(wg_endpoint); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think the logic should be changed subtly. As-is, this undoes #305 if a just port endpoint override is set.
Imagine that a peer has just their port overridden, but that port got stale (not working). The code as-is would never report the endpoint as seen by wireguard: it would replace its port (with a bogus value) instead.
So I think the logic should be:
- If
peer.contents.endpointhas unspecified address, replace its address with the one from our endpoint. - Independently of that, always (unless duplicate) add the untouched wireguard-reported endpoint (either as the main one if None or add to candidates).
There's also a little corner case that we should handle: if there's no wireguard-reported endpoint and their override is unspecified address with a port, then we should filter that one out. Announcing 0.0.0.0:1234 as a peer's endpoind is never correct.
| .interact()? | ||
| { | ||
| publicip::get_any(Preference::Ipv4) | ||
| } else if Confirm::with_theme(&*THEME) | ||
| .wait_for_newline(true) | ||
| .with_prompt( | ||
| "Use an unspecified IP address? (this can occur if you do not have a fixed global IP)", | ||
| ) | ||
| .interact()? | ||
| { | ||
| Some(IpAddr::V6(Ipv6Addr::UNSPECIFIED)) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
GitHub won't let me comment on the whole span, but should we first ask whether to use unspecified address, and only if not ask where to auto-detect the address? Otherwise the "use unspecified" prompt may be easily missed.
| } else if Confirm::with_theme(&*THEME) | ||
| .wait_for_newline(true) | ||
| .with_prompt( | ||
| "Use an unspecified IP address? (this can occur if you do not have a fixed global IP)", |
There was a problem hiding this comment.
| "Use an unspecified IP address? (this can occur if you do not have a fixed global IP)", | |
| "Use an unspecified IP address and override just the port? (use if you do not have \ | |
| a fixed global IP)", |
| /// The external endpoint that you'd like the innernet server to broadcast | ||
| /// to other peers. The IP address may be unspecified, in which case the | ||
| /// server will try to resolve it based on its most recent connection. | ||
| /// The port will still be used even if you decide to use the unspecified | ||
| /// IP address. | ||
| #[clap(short, long)] | ||
| pub endpoint: Option<Endpoint>, |
There was a problem hiding this comment.
Before this I didn't even know what "unspecified IP address" exactly means. Shall we give an example like 0.0.0.0:1234?
| ) | ||
| .interact()? | ||
| { | ||
| Some(IpAddr::V6(Ipv6Addr::UNSPECIFIED)) |
There was a problem hiding this comment.
I guess it does not really make a difference whether the unspecified IP address is IPv4 or IPv6? Still, we generally default to IPv4, so we may as well follow suit here.
|
@skywhale pingy ping in case you still have cycles to iterate this PR. |
An alternative way to implement #308
What do you think @bschwind?