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

The only_v6 flag of the with_bind_address method is not honoured #50

Open
bugnano opened this issue Sep 15, 2023 · 7 comments
Open

The only_v6 flag of the with_bind_address method is not honoured #50

bugnano opened this issue Sep 15, 2023 · 7 comments

Comments

@bugnano
Copy link

bugnano commented Sep 15, 2023

I have a ServerConfig builder that looks like this:

    let config = ServerConfig::builder()
        .with_bind_address(bind_address, false)
        .with_certificate(certificate)
        .keep_alive_interval(Some(Duration::from_secs(3)))
        .build();

So the only_v6 flag is false (side note: it's not good API design to pass boolean parameters to a function, because if I read that function without reading its documentation, what does false mean? It's better to use an enum in this case, but I digress).

The output of the command ss -ulpen is as follows:

UNCONN                    0                         0                                                     [::1]:4433                                                  [::]:*                        users
(("beam.smp",pid=7804,fd=20)) uid:1000 ino:54876 sk:9 cgroup:/user.slice/user-1000.slice/[email protected]/app.slice/app-org.gnome.Terminal.slice/vte-spawn-394db383-bbc8-4b17-adb2-a74b89adb521.scope v6only:1 <->

Note the v6only:1 part in the output.

@BiagioFesta
Copy link
Owner

side note: it's not good API design to pass boolean parameters to a function

Completely agree. Thanks for the feedback and I am going to change that.


I cannot find any good reference/documentation for now (starting from https://man7.org/linux/man-pages/man7/ipv6.7.html), but not all configurations and platforms support IPV6_V6ONLY.

In particular, I have found loopback binding (LOCALHOST) does not support dual-stack. I see [::1]:4433 in your example.

Could you please confirm me that it instead works with INADDR_ANY?

@bugnano
Copy link
Author

bugnano commented Sep 16, 2023

I confirm that binding to :: binds dualstack:

UNCONN                    0                         0                                                         *:4433                                                     *:*                        users:(("beam.smp",pid=5151,fd=20)) uid:1000 ino:48796 sk:3001 cgroup:/user.slice/user-1000.slice/[email protected]/app.slice/app-org.gnome.Terminal.slice/vte-spawn-f3042582-c0bb-4356-8771-3b1f28ecadd8.scope v6only:0 <->

Notice the v6only:0 flag.

While I know that there exists the with_bind_config function, I was wondering whether it would be possible to call the server builder with with_bind_address multiple times, 1 for 127.0.0.1 and the other for ::1 in the case for localhost.
Specifically, I'm using:

let mut addrs_iter = (host, port).to_socket_addrs()?;

the to_socket_addrs returns an iterator of socket addresses, so when I set host to localhost, it returns both the address for 127.0.0.1, and the one for ::1.

So in reality my request would be to be able to bind to multiple addresses, as many as returned by to_socket_addrs.
This could be useful, for example, also if a host has both an A and an AAAA DNS record, and the server would bind to both addresses.
Is something like that possible?

@BiagioFesta
Copy link
Owner

BiagioFesta commented Sep 16, 2023

This crate does not allow that directly, but yeah it is possible.

Like its underlying transport, an Endpoint represent a single point of binding (thus, a single socket).

However, nothing stops you from having multiple endpoints for your server.

Something like:

    let socket_addresses = vec![
        SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 4433),
        SocketAddr::new(Ipv6Addr::LOCALHOST.into(), 4433),
        SocketAddr::new(Ipv4Addr::UNSPECIFIED.into(), 4433),
        SocketAddr::new(Ipv6Addr::UNSPECIFIED.into(), 4433),
    ];

    // Vec<Endpoint<Server>>
    let endpoints = socket_addresses
        .into_iter()
        .map(|address| {
            let config = ServerConfig::builder()
                .with_bind_address(address, false)  // No dual stack, as we have multiple bindings
                .with_certificate(Certificate::load("cert.pem", "key.pem")?)
                .build();
            Ok(Endpoint::server(config)?)
        })
        .collect::<Result<Vec<_>>>()?;

     loop {
        let (accepting, ..) = select_all(endpoints.iter().map(|ep| Box::pin(ep.accept()))).await;
        accepting.await...
     }

Each Endpoint::accept() produces a Future so you can select one of them in different ways.

@bugnano
Copy link
Author

bugnano commented Sep 16, 2023

However, nothing stops you from having multiple endpoints for your server.

True, but isn't building multiple sockets less efficient than having only one?
I mean I'd like to have something like IpBindConfig::LocalDual, but more flexible.
Is IpBindConfig::LocalDual binding 2 endpoints internally and hiding the complexity, or does it work differently?

@BiagioFesta
Copy link
Owner

I am not aware about any performance penalties about binding multiple sockets.

And in any case, binding multiple addresses necessarily require binding multiple sockets (generally).

IpBindConfig::LocalDual does not actually bind two sockets (nor internally creates multiple endpoint), it just leverages OS Dual-stack IP implementations (https://datatracker.ietf.org/doc/html/rfc4213). The OS binds 1 socket anyway and let that same port to also redirect IPv4 connections (merely because of transition v6->v4).

@bugnano
Copy link
Author

bugnano commented Sep 16, 2023

On further testing, binding with:

    let config = ServerConfig::builder()
        .with_bind_config(wtransport::config::IpBindConfig::LocalDual, port)
        .with_certificate(certificate)
        .keep_alive_interval(Some(Duration::from_secs(3)))
        .build();

the output of ss -ulpen is as follows:

UNCONN       0             0                            [::1]:4433                        [::]:*           users:(("beam.smp",pid=3550,fd=20)) uid:1000 ino:36858 sk:b cgroup:/user.slice/user-1000.slice/[email protected]/app.slice/app-org.gnome.Terminal.slice/vte-spawn-401c8b2f-e4d3-4ecc-8f97-b1f9626dd142.scope v6only:1 <->

So the v6only:1 flag is still set, and I don't see any 127.0.0.1 binding for IPv4.
This might be a separate bug, but it confirms your point that for binding to localhost both on IPv4 and IPv6 we need 2 sockets indeed.

EDIT: To calrify, I tested connecting from the browser using [::1]:4433 and it worked, and using 127.0.0.1:4433 and it didn't work.

Thank you for the example code btw.

@BiagioFesta
Copy link
Owner

This might be a separate bug, but it confirms your point that for binding to localhost both on IPv4 and IPv6 we need 2 sockets indeed.

I am not sure that could be classified as bug. As said, dual-stack is OS implementation details and might not be supported for localhost.

In fact, I would change the API in this patch: #54

The dual-stack configuration will be more explicit:

/// Configuration for IPv6 dual stack.
#[derive(Debug, Copy, Clone)]
pub enum Ipv6DualStackConfig {
    /// Do not configure dual stack. Use OS's default.
    OsDefault,

    /// Deny dual stack. This is equivalent to `IPV6_V6ONLY`.
    ///
    /// Socket will only bind for IPv6 (IPv4 port will still be available).
    Deny,

    /// Allow dual stack.
    ///
    /// Please note that not all configurations/platforms support dual stack.
    Allow,
}

Where Ipv6DualStackConfig::Allow suggests to allow (not enable) the dual stack. Not guaranteed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants