Wireguard: Bind server to specified address only

[errorxyz]

Refs mitmproxy/mitmproxy#6671
Fixes bug where wireguard server binding over IPv6 and IPv4 is handled incorrectly when no host is specified.

[mhils]

Thanks for tackling this! Would it make sense to merge both implementations into a common function with an explanatory docstring?

[decathorpe]

FWIW, reading the PR title / commit message first and the actual code changes second I was very confused what this PR was actually doing 😅

As far as I can tell, you're

1. dropping the unintended fallback to an IPv6 socket if binding to an IPv4 socket fails
2. requiring the "host" argument not to be empty, and
3. setting IPv6 only mode if the address is an IPv6 address.

The changes 1. and 3. look good to me, but I'm not sure I like losing the default behaviour of "if no host is specified, fall back to 0.0.0.0, which is a good default IMO.

Maybe falling back to 0.0.0.0 would be ok if the "host" argument is empty? That should match the current behaviour in all cases except the pathological one where binding to 0.0.0.0 failed but binding to ::1 works for some strange reason.

[mhils]

I think there's an argument to be made that we should just mirror what std::net::UdpListener does, which also requires "host" to not be empty. What do you think? We could also just take std::net::ToSocketAddrs and be done with it. We don't need special empty string logic. :)

[decathorpe]

Ok, if the UDP mode already does it this way, it makes sense to have consistent behaviour.

[errorxyz]

FWIW, reading the PR title / commit message first and the actual code changes second I was very confused what this PR was actually doing 😅

Totally agreed 😅 I was drafting this in the middle of the night and I guess I just got too tired by the time I submitted the PR. Will make it clearer in the coming days :)

[mhils]

Looks great already, thank you!

[mhils]

Suggested change

	pub fn create_udp_socket(host: &str, port: u16) -> Result<tokio::net::UdpSocket> {
	pub fn create_and_bind_udp_socket<A: ToSocketAddrs>(addr: A) -> Result<tokio::net::UdpSocket> {

[decathorpe]

Would it be better to make this pub(crate) instead of pub? Probably doesn't make sense to have a helper function like that in the public API. But given that everything and the kitchen sink in the mitmproxy crate is already pub, not sure if that makes a difference 😆

[mhils]

The crate is not public, so we can't break any downstream users. But I agree, we can make it pub(crate). 😛

[errorxyz]

I've assumed addr.to_socket_addrs() returns an iterator containing a single address only.

[mhils]

Looking at the diff I actually didn't like my own suggestion of using ToSocketAddrs. Here's an alternative proposal to go with just SocketAddr. What do you think?

[errorxyz]

Looking at the diff I actually didn't like my own suggestion of using ToSocketAddrs. Here's an alternative proposal to go with just SocketAddr. What do you think?

I prefer passing the host as a string and converting it to IpAddr on the Rust side, as it feels more natural. But I don't have a strong preference.