Add ability to select TCP/UDP for localhost SOCKS5 API access method

[MarkusPettersson98]

Previously, the API access method "SOCKS5 running on localhost" assumed that the transport protocol used for configuring the firewall should be TCP. This covers the majority of obfuscation protocols, but far from all. If we support both TCP and UDP we should cover ~99% of all use cases.

This PR allows outgoing API traffic to be configured to be either TCP or UDP. For now, only the "SOCKS5 running on localhost" access method uses this configuration option.

---

This change is 

[linear]

DES-409 Add ability to select TCP/UDP for localhost SOCKS5 API access method

We suddenly realized that the API access method localhost SOCKS5 hardcoded the protocol for the hole in the firewall to TCP. This covers the majority of obfuscation protocols, but far from all. We already have pretty good support for making this more flexible and possible to select between TCP and UDP.

If we support both TCP and UDP we should cover ~99% of all use cases. And that's enough and a good tradeoff in how long time it takes to fix.

There are and can exist obfuscation protocols operating at a bunch of other transport protocols, such as ICMP. But we choose to ignore these for now.

[dlon]

Reviewed 7 of 9 files at r1, all commit messages.
Reviewable status: 7 of 9 files reviewed, 5 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-api/src/proxy.rs line 75 at r1 (raw file):

                    write!(
                        f,
                        "Socks5 {}/{} via localhost:{}",

We implement Display for Endpoint, so I think you can print endpoint directly.

---

mullvad-cli/src/cmds/api_access.rs line 333 at r1 (raw file):

        /// protocols, such as ICMP. However, by supporting both TCP and UDP we
        /// should cover ~99% of all use cases.
        #[arg(default_value_t = TransportProtocol::Tcp)]

The "justification" for leaving out ICMP probably doesn't belong in user-facing documentation.

Also, if this should have a default value, it should probably not be a positional argument.

---

mullvad-daemon/src/api.rs line 260 at r1 (raw file):

                // Wait for the firewall policy to be updated.
                let _ = result_rx.await;
                log::debug!("API endpoint: {}", endpoint);

Nit: {endpoint}

---

mullvad-management-interface/proto/management_interface.proto line 346 at r1 (raw file):

    uint32 port = 2;
    uint32 local_port = 3;
    TransportProtocol peer_transport_protocol = 4;

The transport protocol should probably be grouped with ip and port, since they're referring to the same endpoint.

I suggest making the fields adjacent to each other and renamed to remote_ip, remote_port, and remote_transport, or something along those lines.

Or perhaps we should add a new message equivalent to talpid_types::net::Endpoint.

---

mullvad-types/src/access_method.rs line 207 at r1 (raw file):

    pub peer: SocketAddr,
    /// The transport protocol which should be allowed in the firewall.
    pub peer_transport_protol: TransportProtocol,

Could peer just be an Endpoint?

---

mullvad-types/src/access_method.rs line 207 at r1 (raw file):

protol
Should be "protocol", but see above.

[MarkusPettersson98]

Reviewable status: 6 of 9 files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-api/src/proxy.rs line 75 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

We implement Display for Endpoint, so I think you can print endpoint directly.

done

---

mullvad-cli/src/cmds/api_access.rs line 333 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

The "justification" for leaving out ICMP probably doesn't belong in user-facing documentation.

Also, if this should have a default value, it should probably not be a positional argument.

done
Agree, it has since been removed.

---

mullvad-daemon/src/api.rs line 260 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: {endpoint}

done

---

mullvad-management-interface/proto/management_interface.proto line 346 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

The transport protocol should probably be grouped with ip and port, since they're referring to the same endpoint.

I suggest making the fields adjacent to each other and renamed to remote_ip, remote_port, and remote_transport, or something along those lines.

Or perhaps we should add a new message equivalent to talpid_types::net::Endpoint.

Good point!

I assume that the message

message Endpoint {
  string address = 1;
  TransportProtocol protocol = 2;
}

is just that, but we could do better by not having address be a string, but instead something equivalent to std::net::SocketAddr. Let's think about that, but for now I'm moving + renaming the fields in the Socks5Local message.

---

mullvad-types/src/access_method.rs line 207 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

protol
Should be "protocol", but see above.

done
😅

---

mullvad-types/src/access_method.rs line 207 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could peer just be an Endpoint?

Yes, I think that is more appropriate

[MarkusPettersson98]

Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-types/src/access_method.rs line 207 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Yes, I think that is more appropriate

done

[MarkusPettersson98]

Reviewable status: 4 of 9 files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-types/src/access_method.rs line 207 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

done
😅

done

[MarkusPettersson98]

Reviewable status: 4 of 9 files reviewed, 2 unresolved discussions (waiting on @dlon)

---

mullvad-management-interface/proto/management_interface.proto line 346 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Good point!

I assume that the message

message Endpoint {
  string address = 1;
  TransportProtocol protocol = 2;
}

is just that, but we could do better by not having address be a string, but instead something equivalent to std::net::SocketAddr. Let's think about that, but for now I'm moving + renaming the fields in the Socks5Local message.

done

[dlon]

Reviewed 4 of 5 files at r2, 1 of 2 files at r3, all commit messages.
Reviewable status: 8 of 9 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-cli/src/cmds/api_access.rs line 128 at r3 (raw file):

                            port,
                            local_port,
                            transport_protocol,

Could we group remote_ip, remote_port, and remote_transport here as well?

---

mullvad-cli/src/cmds/api_access.rs line 332 at r3 (raw file):

        /// can optionally be set to `UDP` as well.
        #[arg(long, default_value_t = TransportProtocol::Tcp)]
        transport: TransportProtocol,

remote_transport is less ambiguous.

---

mullvad-types/src/access_method.rs line 258 at r3 (raw file):

impl Socks5Local {
    pub fn new(peer: SocketAddr, port: u16) -> Self {

Do we even need this? I didn't see any references to it. Maybe from_args should replace new as the only constructor here.

---

mullvad-types/src/access_method.rs line 277 at r3 (raw file):

        port: u16,
        localport: u16,
        transport_protocol: TransportProtocol,

Could we make this the third argument? I'd also suggest renaming those three parameters to: remote_ip, remote_port, and remote_transport.

[MarkusPettersson98]

Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

mullvad-cli/src/cmds/api_access.rs line 128 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could we group remote_ip, remote_port, and remote_transport here as well?

done

---

mullvad-cli/src/cmds/api_access.rs line 332 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

remote_transport is less ambiguous.

done

---

mullvad-types/src/access_method.rs line 277 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could we make this the third argument? I'd also suggest renaming those three parameters to: remote_ip, remote_port, and remote_transport.

done

[MarkusPettersson98]

Reviewable status: 6 of 9 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

mullvad-types/src/access_method.rs line 258 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Do we even need this? I didn't see any references to it. Maybe from_args should replace new as the only constructor here.

That was a strange side-step from the style of the other CustomAccessMethods constructors. Socks5Local now has one fallible constructor (from_args) and one infallible constructor (new).

[dlon]

Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98)

---

talpid-core/src/firewall/macos.rs line 294 at r7 (raw file):

    fn get_allowed_endpoint_rule(
        &self,
        allowed_endpoint: AllowedEndpoint,

I think you can avoid .clone() by borrowing here.

---

talpid-types/src/net/mod.rs line 290 at r7 (raw file):

        Self {
            endpoint,
            clients: Vec::new(),

Perhaps just add derive(Default) on AllowedEndpoint and get rid of the conditional compilation here.

Also, shouldn't clients always be provided as an argument here?

---

talpid-types/src/net/mod.rs line 332 at r7 (raw file):

#[cfg(windows)]
#[derive(Debug, Clone, Eq, PartialEq)]
pub struct AllowedClients(std::sync::Arc<[PathBuf]>);

Is there any reason to wrap it in an arc?

---

talpid-types/src/net/mod.rs line 383 at r7 (raw file):

    /// The most secure client(s) is our own, which runs as root.
    fn default() -> Self {
        AllowedClients::Root

You can use the #[default] attribute on the enum variant with #[derive(Default)].

[dlon]

Reviewed 5 of 5 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98)

[MarkusPettersson98]

Reviewable status: 10 of 12 files reviewed, 3 unresolved discussions (waiting on @dlon)

---

talpid-core/src/firewall/macos.rs line 294 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

I think you can avoid .clone() by borrowing here.

done

---

talpid-types/src/net/mod.rs line 290 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Perhaps just add derive(Default) on AllowedEndpoint and get rid of the conditional compilation here.

Also, shouldn't clients always be provided as an argument here?

Yes, it should indeed! Fixed

---

talpid-types/src/net/mod.rs line 332 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Is there any reason to wrap it in an arc?

AllowedClients needs to be Send, and since mutability is not needed I didn't use an owned Vec :)

---

talpid-types/src/net/mod.rs line 383 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

You can use the #[default] attribute on the enum variant with #[derive(Default)].

done

[MarkusPettersson98]

Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions (waiting on @dlon)

---

talpid-types/src/net/mod.rs line 383 at r7 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

done

Ultimately ended up scraping Default for AllowedClients 😄

[dlon]

Reviewed 1 of 2 files at r8, 7 of 11 files at r9, 4 of 4 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-api/src/proxy.rs line 163 at r10 (raw file):

            }
            ApiConnectionMode::Direct | ApiConnectionMode::Proxied(_) => {
                let daemon_exe = std::env::current_exe().expect("failed to obtain executable path");

It would be nice if we didn't have to duplicate this.

[MarkusPettersson98]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dlon)

---

mullvad-api/src/proxy.rs line 163 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

It would be nice if we didn't have to duplicate this.

Yes, it would indeed. I suspect that this will be natural to include in upcoming refactoring done in #5370, so I will postpone this change till that PR.