Implement support for custom OpenVPN socks5 bridge client in daemon

[Jontified]

This PR is meant to make it easier for us to use a socks5 server as an obfuscation layer for OpenVPN connections.
It aims to make local socks5 servers work without having to use split tunneling, create a CLI UI to allow easily setting this up.
To set up for easy integration into the UI.

---

This change is 

[linear]

DES-430 Implement custom OpenVPN socks5 bridge client in daemon

[dlon]

Reviewed 8 of 15 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Jontified)

---

dist-assets/binaries line 1 at r2 (raw file):

Subproject commit d9db5c930cb3eb1d3ee048e5546be6377dedf8ab

Accidentally committed?

---

talpid-core/src/firewall/mod.rs line 130 at r2 (raw file):

        /// TODO
        #[cfg(any(target_os = "linux", target_os = "macos"))]
        allow_all_traffic_to_peer: bool,

What do you think of replacing peer_endpoint with an AllowedEndpoint? It has the exact same purpose.

I'm not sure if the type's name would still make sense, but you'd be able to remove both relay_client and allow_all_traffic_to_peer

---

talpid-core/src/firewall/mod.rs line 146 at r2 (raw file):

        /// TODO
        #[cfg(any(target_os = "linux", target_os = "macos"))]
        allow_all_traffic_to_peer: bool,

This should probably be moved down so it's grouped with relay_client.

---

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

    pub fn get_openvpn_local_proxy_settings(&self) -> Option<&LocalProxySettings> {
        match &self {

Nit: It's already borrowed

[Jontified]

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

---

talpid-core/src/firewall/mod.rs line 130 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

What do you think of replacing peer_endpoint with an AllowedEndpoint? It has the exact same purpose.

I'm not sure if the type's name would still make sense, but you'd be able to remove both relay_client and allow_all_traffic_to_peer

Done.

---

dist-assets/binaries line 1 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Accidentally committed?

Done. Why is the old submodule not set to main though?

[dlon]

Reviewed 1 of 30 files at r3.
Reviewable status: 7 of 36 files reviewed, 1 unresolved discussion

---

dist-assets/binaries line 1 at r2 (raw file):

Previously, Jontified wrote…

Done. Why is the old submodule not set to main though?

The most recent commit doesn't modify any artifacts, so there's no real reason to point to it.

[dlon]

Reviewed 1 of 30 files at r3, 3 of 4 files at r4, 8 of 8 files at r5, all commit messages.
Reviewable status: 14 of 37 files reviewed, 1 unresolved discussion (waiting on @Jontified)

---

talpid-core/src/firewall/windows.rs line 162 at r5 (raw file):

        let relay_client_wstr_ptrs: Vec<*const u16> = relay_client_wstrs.iter().map(|wstr| wstr.as_ptr()).collect();
        let relay_client_wstr_ptrs_len = relay_client_wstr_ptrs.len();
        //let relay_client_wstr_ptr: *const u16 = if let Some(ref wstr) = relay_client_wstr {

Comments

---

windows/winfw/src/winfw/winfw.cpp line 408 at r5 (raw file):

		}

		auto relayClientWstrings = std::vector<std::wstring>();

Nit: std::vector<std::wstring> relayClientWstrings;

[dlon]

Reviewed 3 of 30 files at r3, 15 of 23 files at r6.
Reviewable status: 26 of 38 files reviewed, 6 unresolved discussions (waiting on @Jontified)

---

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

    item: SelectItem,
    /// Name of the API access method in the Mullvad client [All]
    #[arg(long)]

Is it possible to specify a group here and for item? That would make the arguments mutually exclusive.

---

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

    edit_params: ProxyEditParams,
    ///// Username for authentication [Socks5 (Remote proxy)]
    //#[arg(long)]

The commented code should probably be removed.

---

mullvad-daemon/src/management_interface.rs line 709 at r6 (raw file):

    }

    async fn get_custom_bridge(&self, _: Request<()>) -> ServiceResult<CustomBridgeSettings> {

This probably does not require a special command. This is present in the settings already.

---

mullvad-daemon/src/management_interface.rs line 726 at r6 (raw file):

        log::debug!("update_custom_bridge");
        let (tx, rx) = oneshot::channel();
        self.send_command_to_daemon(DaemonCommand::UpdateCustomBridge(

For consistency, maybe the settings should be updated using GetSettings + SetCustomBridge if a bridge is already set.

However, I'm not sure why we can't use the existing function SetBridgeSettings for this.

---

mullvad-management-interface/proto/management_interface.proto line 86 at r6 (raw file):

  // Custom proxy
  rpc SetCustomBridge(google.protobuf.Empty) returns (google.protobuf.Empty) {}

Set confused me a lot at first. I think "enable" or something might be clearer.

But this also seems redundant given that we already have a bridge_state.

---

mullvad-types/src/settings/mod.rs line 85 at r6 (raw file):

    /// A potential custom bridge.
    #[cfg_attr(target_os = "android", jnix(skip))]
    pub custom_bridge: CustomBridgeSettings,

I found it pretty confusing to understand the purpose of having both BridgeSettings and CustomBridgeSettings. It's probably not ideal to have a duplicated "runtime-only" state in the settings.

Have you considered updating BridgeSettings to simply store all constraints/settings independently of the current "bridge type"?

Instead of

  pub enum BridgeSettings {
      Normal(BridgeConstraints),
      Custom(ProxySettings),
  }

use something like

  enum BridgeType {
      Builtin,
      Custom,
  }

  pub struct BridgeSettings {
      bridge_type: BridgeSettingsType,
      normal: BridgeConstraints,
      custom: ProxySettings,       
  }

Then you won't lose the custom bridge settings when you stop using the custom bridge.

[Jontified]

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

---

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

Previously, dlon (David Lönnhager) wrote…

Is it possible to specify a group here and for item? That would make the arguments mutually exclusive.

I think this is far enough away from the purpose of this PR that it should be fixed somewhere else.

---

mullvad-management-interface/proto/management_interface.proto line 86 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Set confused me a lot at first. I think "enable" or something might be clearer.

But this also seems redundant given that we already have a bridge_state.

This is improved in the new other PR

---

mullvad-types/src/settings/mod.rs line 85 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

I found it pretty confusing to understand the purpose of having both BridgeSettings and CustomBridgeSettings. It's probably not ideal to have a duplicated "runtime-only" state in the settings.

Have you considered updating BridgeSettings to simply store all constraints independently of the current "bridge type"?

Instead of

  pub enum BridgeSettings {
      Normal(BridgeConstraints),
      Custom(ProxySettings),
  }

use something like

  enum BridgeType {
      Builtin,
      Custom,
  }

  pub struct BridgeSettings {
      bridge_type: BridgeSettingsType,
      normal: BridgeConstraints,
      custom: ProxySettings,       
  }

Then you won't lose the custom bridge settings when you stop using the custom bridge.

Done. This is fixed, among other things in the other PR.

[MarkusPettersson98]

Reviewed 2 of 23 files at r6, 3 of 4 files at r7, 1 of 3 files at r9, 33 of 40 files at r10, 8 of 8 files at r11, 3 of 3 files at r12, 2 of 2 files at r13, 3 of 3 files at r14, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon and @Jontified)

---

talpid-openvpn/src/lib.rs line 763 at r11 (raw file):

    use talpid_types::ErrorExt;
    #[cfg(target_os = "macos")]
    use talpid_types::ErrorExt;

Nit: No conditional compilation is needed here. use can be merged as well:
use talpid_types::{net::proxy::CustomProxy, ErrorExt};

Code quote:

    use talpid_types::net::proxy::CustomProxy;
    #[cfg(any(target_os = "linux", windows))]
    use talpid_types::ErrorExt;
    #[cfg(target_os = "macos")]
    use talpid_types::ErrorExt;

---

talpid-openvpn/src/lib.rs line 846 at r11 (raw file):

            let mut routes = HashSet::new();
            #[cfg(not(target_os = "linux"))]
            if let Some(talpid_types::net::proxy::CustomProxy::Socks5Local(proxy_settings)) =

Nit: CustomProxy is already imported in this scope, might as well use it unqualified.

Code quote:

talpid_types::net::proxy::CustomProxy::Socks5Local(proxy_settings)

[MarkusPettersson98]

Reviewed 30 of 40 files at r16, 6 of 8 files at r17, 3 of 3 files at r18, 7 of 7 files at r19, 1 of 1 files at r20, 6 of 6 files at r21, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dlon)

[dlon]

Reviewed 1 of 40 files at r16, 1 of 8 files at r17, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

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