Revamp API access methods

[MarkusPettersson98]

This PR implements the daemon logic + CLI for using custom SOCKS5/Shadowsocks proxies for accessing the Mullvad API.
A user will now be able to configure custom SOCKS5/Shadowsocks proxies which may be used together with or instead of Mullvad's own bridges, which may enable circumventing censorship.

CLI

A new CLI command has been added: mullvad api-access!

$ mullvad api-access
Manage use of proxies for reaching the Mullvad API.

Can make the daemon connect to the the Mullvad API via one of the 
Mullvad bridge servers or a custom proxy (SOCKS5 & Shadowsocks)

Usage: mullvad api-access <COMMAND>

Commands:
  list     List the configured API proxies
  add      Add a custom API proxy
  edit     Edit an API proxy
  remove   Remove an API proxy
  enable   Enable an API proxy
  disable  Disable an API proxy
  test     Test an API proxy
  use      Force the use of a specific API proxy
  help     Print this message or the help of the given subcommand(s)

Options:
  -h, --help     Print help
  -V, --version  Print version

---

This change is 

[dlon]

Reviewed 2 of 19 files at r1.
Reviewable status: 2 of 19 files reviewed, 5 unresolved discussions (waiting on @MarkusPettersson98)

---

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

  // API Access methods
  rpc GetApiAccessMethods(google.protobuf.Empty) returns (ApiAccessMethods) {}

Do we need this many RPCs? For example, Get... is unnecessary since the methods are already returned in Settings. For access methods, I would argue that we only need two functions: one for creating or updating an access method, and one for removing access methods. The former can be used to toggle a method on or off, etc.

// Add or update access method. Returns an id, and creates a new method if id isn't set on input
rpc AddApiAccessMethod(ApiAccessMethod) returns (google.protobuf.StringValue) {}
// remove an access method by id
rpc RemoveApiAccessMethod(google.protobuf.StringValue) returns (google.protobuf.Empty) {}

---

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

message ApiAccessMethodReplace {
  uint32 index = 1;

I would argue that we shouldn't rely on indices if the methods have unique identifiers. Could we not just find the access method based on the on the id that is passed in, and replace all other information with the new info? If so, we also don't need this message.

---

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

message ApiAccessMethodToggle {
  ApiAccessMethod access_method = 1;
  bool enable = 2;

Why do we need this message if the ApiAccessMethod also contains the same state?

---

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

    uint32 port = 5;
  }
  message Socks5 {

I would remove Socks5 and use Socks5Local and Socks5Remote directly. They are two different access methods. Is there any benefit to grouping them here?

---

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

  }
  message Shadowsocks {
    bool enabled = 1;

Should we not move enabled (and id and name, even though they're not relevant for the built-in methods) out of the individual messages? Put them next to the oneof below. Maybe I'm misremembering how protobuf works, but I believe that this is possible.

[MarkusPettersson98]

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

---

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

Previously, dlon (David Lönnhager) wrote…

Do we need this many RPCs? For example, Get... is unnecessary since the methods are already returned in Settings. For access methods, I would argue that we only need two functions: one for creating or updating an access method, and one for removing access methods. The former can be used to toggle a method on or off, etc.

// Add or update access method. Returns an id, and creates a new method if id isn't set on input
rpc AddApiAccessMethod(ApiAccessMethod) returns (google.protobuf.StringValue) {}
// remove an access method by id
rpc RemoveApiAccessMethod(google.protobuf.StringValue) returns (google.protobuf.Empty) {}

Hmm, having add & update seems a bit code golfy to me tbh, but I can see how I can do without GetApiAccessMethods, ReplaceApiAccessMethod and ToggleApiAccessMethod.

---

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

Previously, dlon (David Lönnhager) wrote…

I would argue that we shouldn't rely on indices if the methods have unique identifiers. Could we not just find the access method based on the on the id that is passed in, and replace all other information with the new info? If so, we also don't need this message.

We can use the id of the ApiAccessMethod struct instead

---

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

Previously, dlon (David Lönnhager) wrote…

Why do we need this message if the ApiAccessMethod also contains the same state?

If an update message is added, we can get rid of this

---

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

Previously, dlon (David Lönnhager) wrote…

I would remove Socks5 and use Socks5Local and Socks5Remote directly. They are two different access methods. Is there any benefit to grouping them here?

There is no benefit, but Socks5 is the protocol while Socks5Local and Socks5Remote are instances of the same protocol. But I'll separate them

---

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

Previously, dlon (David Lönnhager) wrote…

Should we not move enabled (and id and name, even though they're not relevant for the built-in methods) out of the individual messages? Put them next to the oneof below. Maybe I'm misremembering how protobuf works, but I believe that this is possible.

Hmm, that would indeed be way nicer. If that works I'll do that immediately 🔥

[dlon]

Reviewed 2 of 19 files at r1.
Reviewable status: 4 of 19 files reviewed, 7 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-daemon/src/access_method.rs line 87 at r3 (raw file):

    /// If settings were changed due to an update, notify all listeners.
    fn notify_on_change(&mut self, settings_changed: MadeChanges) {

It seems like we're missing a lot of expected rotations. For example, if the current access method is removed or replaced, this should force a rotation, but doesn't. And calling set_api_access_method without making any changes should do nothing.

Writing down some ideas we've discussed elsewhere:

It might simplify things to have the access method "generator" output access methods to a channel/stream/closure/subscribers, and use that as input into the API client (and perhaps also to another thing that updates the firewall's allowed endpoint), replacing the current closure that just provides a socket address.

---

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

use crate::{
    access_method,

api_access might be clearer.

---

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

    /// API access methods.
    #[cfg_attr(target_os = "android", jnix(skip))]
    pub api_access_methods: access_method::Settings,

Nit: How about simply api_access?

[faern]

Reviewable status: 4 of 19 files reviewed, 8 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

---

mullvad-cli/src/main.rs line 74 at r3 (raw file):

    Relay(relay::Relay),

    /// Manage use of proxies for reaching the Mullvad API.

As I said yesterday, I don't think we should use the word "proxy" in this context. All of this is about "API access methods/settings". At this layer of abstraction it's not a proxy, it's an access method IMO.

mullvad_types::access_method::Socks5 is a proxy. But currently our built direct method is also a method, and here even that is denoted as "proxy", but it's not.

[faern]

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

---

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

pub mod daemon {
    use super::*;
    /// Argument to protobuf rpc `ApiAccessMethodReplace`.

gRPC should probably not leak into mullvad-types. Can you instead contain such bridging in mullvad-management-interface?

[dlon]

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

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Hmm, having add & update seems a bit code golfy to me tbh, but I can see how I can do without GetApiAccessMethods, ReplaceApiAccessMethod and ToggleApiAccessMethod.

I think you're right. My suggestion from earlier today was to add a separate Add type once id etc. has been lifted out of the messages used by different access methods:

rpc AddApiAccessMethod(AddApiAccessMethod) returns (Uuid) {}
rpc ReplaceApiAccessMethod(ApiAccessMethod) returns (google.protobuf.Empty) {}
rpc RemoveApiAccessMethod(Uuid) returns (google.protobuf.Empty) {}

message AddApiAccessMethod {
   // identical to ApiAccessMethod but without an id
  string name = 1;
  oneof method {
    Direct direct = 1;
    // ...
  }
}

If id isn't part of those messages, then there's no duplication.

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

There is no benefit, but Socks5 is the protocol while Socks5Local and Socks5Remote are instances of the same protocol. But I'll separate them

That's true, but as access methods, they're completely distinct. I think this will just cost more conversion code, etc., for no gain.

[MarkusPettersson98]

Reviewable status: 2 of 23 files reviewed, 8 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-cli/src/main.rs line 74 at r3 (raw file):

Previously, faern (Linus Färnstrand) wrote…

As I said yesterday, I don't think we should use the word "proxy" in this context. All of this is about "API access methods/settings". At this layer of abstraction it's not a proxy, it's an access method IMO.

mullvad_types::access_method::Socks5 is a proxy. But currently our built direct method is also a method, and here even that is denoted as "proxy", but it's not.

Done.

---

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

Previously, dlon (David Lönnhager) wrote…

I think you're right. My suggestion from earlier was to add a separate Add type once id etc. has been lifted out of the messages used by different access methods:

rpc AddApiAccessMethod(AddApiAccessMethod) returns (Uuid) {}
rpc ReplaceApiAccessMethod(ApiAccessMethod) returns (google.protobuf.Empty) {}
rpc RemoveApiAccessMethod(Uuid) returns (google.protobuf.Empty) {}

message AddApiAccessMethod {
   // identical to ApiAccessMethod but without an id
  string name = 1;
  oneof method {
    Direct direct = 1;
    // ...
  }
}

If id isn't part of those messages, then there's no duplication.

Cut down on amount of RPCs, now down to:

  // API Access methods
  rpc AddApiAccessMethod(ApiAccessMethod) returns (google.protobuf.Empty) {}
  rpc RemoveApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc SetApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc UpdateApiAccessMethod(ApiAccessMethodUpdate) returns (google.protobuf.Empty) {}

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

We can use the id of the ApiAccessMethod struct instead

done

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

If an update message is added, we can get rid of this

done

---

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

Previously, dlon (David Lönnhager) wrote…

That's true, but as access methods, they're completely distinct. I think this will just cost more conversion code, etc., for no gain.

done

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Hmm, that would indeed be way nicer. If that works I'll do that immediately 🔥

done

---

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

Previously, faern (Linus Färnstrand) wrote…

gRPC should probably not leak into mullvad-types. Can you instead contain such bridging in mullvad-management-interface?

I think this has been done already in the case of CustomList:

mullvad-types: https://github.com/mullvad/mullvadvpn-app/blob/bdac804720b5ad9de3db6a149042b1719c6c0631/mullvad-types/src/custom_list.rs#L23C1-L33C2
management_interface.proto: https://github.com/mullvad/mullvadvpn-app/blob/bdac804720b5ad9de3db6a149042b1719c6c0631/mullvad-management-interface/proto/management_interface.proto#L327C1-L335C2

It does not justify the leak, just putting it out there. I have now moved the ApiAccessMethodUpdate struct into mullvad-management-interface 😊
In particular, check out the changes in 3f0c591

---

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

Previously, dlon (David Lönnhager) wrote…

api_access might be clearer.

done

Yeah, think so too 😊

---

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

Previously, dlon (David Lönnhager) wrote…

Nit: How about simply api_access?

Hmm, it would be hard to distinguish an api_access from multiple api_access by just looking at the name, so I'm a bit hestitant on this one. Do you still think a renaming to api_access is needed in this case? 😊

[MarkusPettersson98]

Reviewable status: 1 of 23 files reviewed, 8 unresolved discussions (waiting on @dlon and @faern)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I think this has been done already in the case of CustomList:

mullvad-types: https://github.com/mullvad/mullvadvpn-app/blob/bdac804720b5ad9de3db6a149042b1719c6c0631/mullvad-types/src/custom_list.rs#L23C1-L33C2
management_interface.proto: https://github.com/mullvad/mullvadvpn-app/blob/bdac804720b5ad9de3db6a149042b1719c6c0631/mullvad-management-interface/proto/management_interface.proto#L327C1-L335C2

It does not justify the leak, just putting it out there. I have now moved the ApiAccessMethodUpdate struct into mullvad-management-interface 😊
In particular, check out the changes in f8158a1

Hmm, this seems not as straight forward. Currently blocked from building on android: https://github.com/mullvad/mullvadvpn-app/actions/runs/6300618174/job/17103869545?pr=5178

I see 3 solutions:

1. Revert 3f0c591
2. Move the ApIAccessMethodUpdate type somewhere else
3. List mullvad-management-interface as a dependency on android (not really a solution)

What do you think? 😊

[MarkusPettersson98]

Reviewable status: 1 of 23 files reviewed, 8 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-daemon/src/access_method.rs line 87 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

It seems like we're missing a lot of expected rotations. For example, if the current access method is removed or replaced, this should force a rotation, but doesn't. And calling set_api_access_method without making any changes should do nothing.

Writing down some ideas we've discussed elsewhere:

It might simplify things to have the access method "generator" output access methods to a channel/stream/closure/subscribers, and use that as input into the API client (and perhaps also to another thing that updates the firewall's allowed endpoint), replacing the current closure that just provides a socket address.

Yes, re-working how the infrastructure for how ConnectionModes are generated would definitely be nice and could potentially make the code easier to grasp. Getting the rotations right is doable today (I am fairly certain), so I will do that first to produce the correct behavior before any major refactor.

What is missing as of 2ae66c2 is:

• Rotate if the current access mode is removed
• Re-set the current access mode if it is edited

What is implemented as of the same commit:

• Rotate if API call fails
• Set on set_api_access_method
  • set_api_access_method is idempotent, so repeating it the action with the same access method does not change the observable behavior
• Set on mullvad api-access test X success
• Rotate on mullvad api-access test X failure

Any case that is missing? 😊

[dlon]

Reviewable status: 1 of 23 files reviewed, 5 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Cut down on amount of RPCs, now down to:

  // API Access methods
  rpc AddApiAccessMethod(ApiAccessMethod) returns (google.protobuf.Empty) {}
  rpc RemoveApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc SetApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc UpdateApiAccessMethod(ApiAccessMethodUpdate) returns (google.protobuf.Empty) {}

Nice! UpdateApiAccessMethod() still doesn't make much sense to me, though. ApiAccessMethod already contains the id, and the id is immutable. We could use the id in ApiAccessMethod to identify the access method, and use the other fields to edit the method. I.e., we only need to pass an ApiAccessMethod to UpdateApiAccessMethod().

The only weird thing besides that is that AddApiAccessMethod() takes an ApiAccessMethod with an ID that the client isn't supposed to specify. We could either (1) assume that the ID is optional (and should be unspecified for AddApiAccessMethod()), or (2) make an ApiAccessMethod message for the add method that's completely identical to ApiAccessMethod but without an id. I'm guessing (1) is the current solution, which is probably fine.

[dlon]

Reviewable status: 1 of 23 files reviewed, 6 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

mullvad-management-interface/src/client.rs line 583 at r7 (raw file):

    pub async fn update_access_method(
        &mut self,
        access_method_update: rpc::api_access_method_update::ApiAccessMethodUpdate,

This is moot due to my other point (about removing ApiAccessMethodUpdate), but this would have fit better in mullvad-types. Android does not even depend on this crate.

[dlon]

Reviewable status: 1 of 23 files reviewed, 12 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

mullvad-types/src/api_access.rs line 10 at r7 (raw file):

/// Daemon settings for API access methods.
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
pub struct Settings {

How about AccessMethods instead?

---

mullvad-types/src/api_access.rs line 16 at r7 (raw file):

impl Settings {
    /// Append an [`AccessMethod`] to the end of `api_access_methods`.
    #[inline(always)]

In my opinion, we should not set inline(always) without justification. It's fine to leave it out.

---

mullvad-types/src/api_access.rs line 87 at r7 (raw file):

pub struct ApiAccessMethod {
    /// Some unique id (distinct for each `AccessMethod`).
    id: ApiAccessMethodId,

In this case, I think it would be fine, and probably cleaner, to make all fields public and remove the accessor methods (the ones that are just duplicates of the fields). As this is just a collection of data.

---

mullvad-types/src/api_access.rs line 98 at r7 (raw file):

impl ApiAccessMethodId {
    /// It is up to the caller to make sure that the supplied String is actually
    /// a valid UUID in the context of [`ApiAccessMethod`]s.

Why not simply use a Uuid?

---

mullvad-types/src/api_access.rs line 118 at r7 (raw file):

}

impl From<&AccessMethod> for ApiAccessMethodId {

This is a strange way to go about generating an id, IMO. The id should not have to depend on the hash of the access method, as it shouldn't change if any of its settings change. And yet it does, according to this From impl. What would you think of just generating a random uuid (Uuid::new_v4()) in ApiAccessMethod's constructor?

---

mullvad-types/src/api_access.rs line 129 at r7 (raw file):

/// Access Method datastructure.
#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Hash)]
pub enum AccessMethod {

Having both an AccessMethod and an ApiAccessMethod is pretty confusing.

[dlon]

Reviewable status: 1 of 23 files reviewed, 11 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Hmm, it would be hard to distinguish an api_access from multiple api_access by just looking at the name, so I'm a bit hestitant on this one. Do you still think a renaming to api_access is needed in this case? 😊

This is fine!

[MarkusPettersson98]

Reviewable status: 1 of 23 files reviewed, 10 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-types/src/api_access.rs line 10 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

How about AccessMethods instead?

Visually distinguishing AccessMethods from AccessMethod is kind of hard, and I would rather gravitate to the convention used in mullvad-types/src/custom_list.rs: CustomListSettings

Applied to this case, that would work out to ApiAccessMethodSettings. Thoughts? 😊

---

mullvad-types/src/api_access.rs line 16 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

In my opinion, we should not set inline(always) without justification. It's fine to leave it out.

done

---

mullvad-types/src/api_access.rs line 87 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

In this case, I think it would be fine, and probably cleaner, to make all fields public and remove the accessor methods (the ones that are just duplicates of the fields). As this is just a collection of data.

I don't agree that id should be public, as it is not supposed to be mutated anywhere. Removing getters/setters for the other fields is fine 😊

---

mullvad-types/src/api_access.rs line 129 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Having both an AccessMethod and an ApiAccessMethod is pretty confusing.

Yes, I'm not to happy how this ended up. Currently AccessMethod represent all the technical details needed to access the api in a certain way, while ApiAccessMethod adds app-specific (settings) details on top of that: id, enabled & name.

My obvious candidate for renaming would be AccessMethod, but we could also rename ApiAccessMethod to something like AccessMethodSetting. Thoughts?

[MarkusPettersson98]

Reviewable status: 1 of 23 files reviewed, 10 unresolved discussions (waiting on @dlon and @faern)

---

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

Previously, dlon (David Lönnhager) wrote…

Nice! UpdateApiAccessMethod() still doesn't make much sense to me, though. ApiAccessMethod already contains the id, and the id is immutable. We could use the id in ApiAccessMethod to identify the access method, and use the other fields to edit the method. I.e., we only need to pass an ApiAccessMethod to UpdateApiAccessMethod().

The only weird thing besides that is that AddApiAccessMethod() takes an ApiAccessMethod with an ID that the client isn't supposed to specify. We could either (1) assume that the ID is optional (and should be unspecified for AddApiAccessMethod()), or (2) make an ApiAccessMethod message for the add method that's completely identical to ApiAccessMethod but without an id. I'm guessing (1) is the current solution, which is probably fine.

This is the current RPC-interface:

  // API Access methods
  rpc AddApiAccessMethod(ApiAccessMethodAdd) returns (UUID) {}
  rpc RemoveApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc SetApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc UpdateApiAccessMethod(ApiAccessMethod) returns (google.protobuf.Empty) {}

...

message ApiAccessMethodAdd {
  string name = 1;
  bool enabled = 2;
  AccessMethod access_method = 3;
}

• ApiAccessMethodUpdate has been removed
• AddApiAccessMethod returns a UUID
• ApiAccessMethodAdd was added to be used in AddApiAccessMethod

---

mullvad-management-interface/src/client.rs line 583 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

This is moot due to my other point (about removing ApiAccessMethodUpdate), but this would have fit better in mullvad-types. Android does not even depend on this crate.

The move of RPC-only types from mullvad-types -> mullvad-management-interface was revert, and RPC-only types (ApiAccessMethodUpdate, ApiAccessMethodAdd) was removed entirely from mullvad-types

---

mullvad-types/src/api_access.rs line 98 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Why not simply use a Uuid?

A UUID is now used instead

---

mullvad-types/src/api_access.rs line 118 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

This is a strange way to go about generating an id, IMO. The id should not have to depend on the hash of the access method, as it shouldn't change if any of its settings change. And yet it does, according to this From impl. What would you think of just generating a random uuid (Uuid::new_v4()) in ApiAccessMethod's constructor?

The id does not change anymore. It is instead generated once per AccessMethodSetting using Uuid::new_v4

[dlon]

Reviewed 1 of 19 files at r1, 2 of 13 files at r5, 3 of 11 files at r9, 12 of 15 files at r10, all commit messages.
Reviewable status: 19 of 23 files reviewed, 10 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

mullvad-api/src/https_client_with_sni.rs line 174 at r10 (raw file):

    /// Set up a SOCKS5-socket connection.
    async fn handle_socks_connection(

These "handle" functions are all nearly identical except for the part where they "wrap" the TCP socket. I assume this is because TlsStream's exact type depends on the type parameters? You can generalize it by just passing in a closure for that part, though:

  async fn connect_proxied<ProxyFactory: FnOnce(TcpStream) -> P, P: Future<Output = io::Result<Proxy>>, Proxy: AsyncRead + AsyncWrite + Unpin + Send + 'static>(
      first_hop: SocketAddr,
      hostname: &str,
      make_proxy_stream: ProxyFactory,
      #[cfg(target_os = "android")] socket_bypass_tx: Option<mpsc::Sender<SocketBypassRequest>>,
  ) -> Result<ApiConnection, io::Error> {
      let socket = HttpsConnectorWithSni::open_socket(
          first_hop,
          #[cfg(target_os = "android")]
          socket_bypass_tx,
      )
      .await?;
      let proxy = make_proxy_stream(socket).await?;
  
      #[cfg(feature = "api-override")]
      if API.disable_tls {
          return Ok(ApiConnection::new(Box::new(ConnectionDecorator(proxy))));
      }
  
      let tls_stream = TlsStream::connect_https(proxy, hostname).await?;
      Ok(ApiConnection::new(Box::new(tls_stream)))
  }

---

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

        let mut rpc = MullvadProxyClient::new().await?;
        let access_method = Self::get_access_method(&mut rpc, &item).await?;
        rpc.set_access_method(access_method.get_id()).await?;

Are you not permanently pinning the access method here? Perhaps if we only need to test the current method, we could just use the current method here, and no argument.

---

mullvad-daemon/src/access_method.rs line 87 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Yes, re-working how the infrastructure for how ConnectionModes are generated would definitely be nice and could potentially make the code easier to grasp. Getting the rotations right is doable today (I am fairly certain), so I will do that first to produce the correct behavior before any major refactor.

What is missing as of 2ae66c2 is:

• Rotate if the current access mode is removed
• Re-set the current access mode if it is edited

What is implemented as of the same commit:

• Rotate if API call fails
• Set on set_api_access_method
  • set_api_access_method is idempotent, so repeating it the action with the same access method does not change the observable behavior
• Set on mullvad api-access test X success
• Rotate on mullvad api-access test X failure

Any case that is missing? 😊

How about simply rotating (calling next_api_method) if the current method is removed or edited in any way? It's not perfect, but it might be the easiest to understand, at least until that code has been refactored.

You could hide this logic inside update_access_methods(..) -> bool, perhaps. But it would require ApiConnectionModeProvider to have a notion of current method. You could then return whether the new access methods contain an identical copy of the current access method, and rotate if that's not the case. Also, ApiConnectionModeProvider needs to remember what the current position is, and update it even if the access method has moved in the new list.

Or, dumber still, just rotate when the settings change. Not the best UX, but it's perhaps not horrible.

---

mullvad-daemon/src/api.rs line 66 at r10 (raw file):

        let connection_mode = self.new_connection_mode();
        self.retry_attempt = self.retry_attempt.wrapping_add(1);

This (retry_attempt) appears to be dead code now.

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

This is the current RPC-interface:

  // API Access methods
  rpc AddApiAccessMethod(ApiAccessMethodAdd) returns (UUID) {}
  rpc RemoveApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc SetApiAccessMethod(UUID) returns (google.protobuf.Empty) {}
  rpc UpdateApiAccessMethod(ApiAccessMethod) returns (google.protobuf.Empty) {}

...

message ApiAccessMethodAdd {
  string name = 1;
  bool enabled = 2;
  AccessMethod access_method = 3;
}

• ApiAccessMethodUpdate has been removed
• AddApiAccessMethod returns a UUID
• ApiAccessMethodAdd was added to be used in AddApiAccessMethod

Looks a lot better now!

---

mullvad-management-interface/proto/management_interface.proto line 25 at r10 (raw file):

  rpc GetCurrentVersion(google.protobuf.Empty) returns (google.protobuf.StringValue) {}
  rpc GetVersionInfo(google.protobuf.Empty) returns (AppVersionInfo) {}
  rpc GetApiAddressess(google.protobuf.Empty) returns (google.protobuf.Empty) {}

Should say GetApiAddresses.

---

mullvad-management-interface/proto/management_interface.proto line 390 at r10 (raw file):

}

message AccessMethodSettingAdd {

I'm being very nitpicky, but NewAccessMethodSetting probably is a bit nicer.

---

mullvad-management-interface/proto/management_interface.proto line 396 at r10 (raw file):

}

message AccessMethodSettingUpdate {

This seems to stick around.

---

mullvad-management-interface/src/client.rs line 543 at r10 (raw file):

    }

    pub async fn enable_access_method(

Can we remove these enable/disable methods (and, in my opinion, get_access_method as well)? It's easy enough to get the access method and to update it. Adding lots of convenience methods to this struct is clutter, IMO.

---

mullvad-types/src/api_access.rs line 10 at r7 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Visually distinguishing AccessMethods from AccessMethod is kind of hard, and I would rather gravitate to the convention used in mullvad-types/src/custom_list.rs: CustomListSettings

Applied to this case, that would work out to ApiAccessMethodSettings. Thoughts? 😊

None of these are too bad, so I will stop bikeshedding.

---

mullvad-types/src/api_access.rs line 87 at r7 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I don't agree that id should be public, as it is not supposed to be mutated anywhere. Removing getters/setters for the other fields is fine 😊

That's a good point. I agree.

---

mullvad-types/src/api_access.rs line 129 at r7 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Yes, I'm not to happy how this ended up. Currently AccessMethod represent all the technical details needed to access the api in a certain way, while ApiAccessMethod adds app-specific (settings) details on top of that: id, enabled & name.

My obvious candidate for renaming would be AccessMethod, but we could also rename ApiAccessMethod to something like AccessMethodSetting. Thoughts?

Much better!

[MarkusPettersson98]

Reviewable status: 16 of 23 files reviewed, 7 unresolved discussions (waiting on @dlon and @faern)

---

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

Previously, dlon (David Lönnhager) wrote…

Are you not permanently pinning the access method here? Perhaps if we only need to test the current method, we could just use the current method here, and no argument.

Well, if the API is reachable, we will pin the method-under-test. This is intended behavior

---

mullvad-daemon/src/api.rs line 66 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

This (retry_attempt) appears to be dead code now.

done

---

mullvad-management-interface/proto/management_interface.proto line 25 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should say GetApiAddresses.

Whoops, done.

---

mullvad-management-interface/proto/management_interface.proto line 390 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

I'm being very nitpicky, but NewAccessMethodSetting probably is a bit nicer.

Done.

---

mullvad-management-interface/proto/management_interface.proto line 396 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

This seems to stick around.

Not intended, well spotted! Done.

---

mullvad-management-interface/src/client.rs line 543 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

Can we remove these enable/disable methods (and, in my opinion, get_access_method as well)? It's easy enough to get the access method and to update it. Adding lots of convenience methods to this struct is clutter, IMO.

Done.
e64ba92

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Hmm, this seems not as straight forward. Currently blocked from building on android: https://github.com/mullvad/mullvadvpn-app/actions/runs/6300618174/job/17103869545?pr=5178

I see 3 solutions:

1. Revert f8158a1
2. Move the ApIAccessMethodUpdate type somewhere else
3. List mullvad-management-interface as a dependency on android (not really a solution)

What do you think? 😊

Fixed by simply removing the ApIAccessMethodUpdate struct altogether.

---

mullvad-types/src/api_access.rs line 129 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Much better!

😄

[MarkusPettersson98]

Reviewable status: 16 of 23 files reviewed, 7 unresolved discussions (waiting on @dlon and @faern)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Well, if the API is reachable, we will pin the method-under-test. This is intended behavior

From a UX-standpoint, it would be nice to keep track of which access-method that was previously active. If the test of some second access method fails, the expected behavior is probably to have the daemon roll back to the previous access method. This is not the case today. But this touches upon another comment that you gave, so I will resume the work/discussion there instead 😊

[faern]

Reviewable status: 15 of 23 files reviewed, 6 unresolved discussions (waiting on @dlon and @MarkusPettersson98)

---

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

    Disable(SelectItem),
    /// Test an API access method
    Test(SelectItem),

Can have more clear docs. Currently it more or less just repeat what the code says. What does "test" mean? Can say that we try to reach the API via this method to check if it works etc.

---

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

    ///
    /// Selecting "Mullvad Bridges" respects your current bridge settings.
    Use(SelectItem),

Is this really "forced"? How is it implemented? I think we need to discuss this. And together with Matilda as well. I think what we said is that it will 1) enable the method if it's disabled. 2) Test if the method works. 3) If the method did reach the API, then it will be set as the current access method.

[MarkusPettersson98]

Reviewable status: 8 of 25 files reviewed, 6 unresolved discussions (waiting on @dlon and @faern)

---

mullvad-api/src/https_client_with_sni.rs line 174 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

These "handle" functions are all nearly identical except for the part where they "wrap" the TCP socket. I assume this is because TcpStream's exact type depends on the type parameters? You can generalize it by just passing in a closure for that part, though:

  async fn connect_proxied<ProxyFactory: FnOnce(TcpStream) -> P, P: Future<Output = io::Result<Proxy>>, Proxy: AsyncRead + AsyncWrite + Unpin + Send + 'static>(
      first_hop: SocketAddr,
      hostname: &str,
      make_proxy_stream: ProxyFactory,
      #[cfg(target_os = "android")] socket_bypass_tx: Option<mpsc::Sender<SocketBypassRequest>>,
  ) -> Result<ApiConnection, io::Error> {
      let socket = HttpsConnectorWithSni::open_socket(
          first_hop,
          #[cfg(target_os = "android")]
          socket_bypass_tx,
      )
      .await?;
      let proxy = make_proxy_stream(socket).await?;
  
      #[cfg(feature = "api-override")]
      if API.disable_tls {
          return Ok(ApiConnection::new(Box::new(ConnectionDecorator(proxy))));
      }
  
      let tls_stream = TlsStream::connect_https(proxy, hostname).await?;
      Ok(ApiConnection::new(Box::new(tls_stream)))
  }

Very good suggestion! This has now been implemented

---

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

Previously, faern (Linus Färnstrand) wrote…

Can have more clear docs. Currently it more or less just repeat what the code says. What does "test" mean? Can say that we try to reach the API via this method to check if it works etc.

Of course, these docs have to be overhauled before merging this feature. I'll add the gist of what you mentioned in this comment, and then let's bring this to Matilda 😊

---

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

Previously, faern (Linus Färnstrand) wrote…

Is this really "forced"? How is it implemented? I think we need to discuss this. And together with Matilda as well. I think what we said is that it will 1) enable the method if it's disabled. 2) Test if the method works. 3) If the method did reach the API, then it will be set as the current access method.

Currently, it naively sets the selected access method and tries to use it in subsequent API calls. Changing the semantics according to the steps above (1-2-3) is easily done, but before I do that we should probably anchor it with Matilda.

[dlon]

Reviewed 17 of 17 files at r13, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

mullvad-api/src/https_client_with_sni.rs line 85 at r13 (raw file):

impl InnerConnectionMode {
    async fn connect(
        &self,

Another more nitpicky suggestion here: There's no need to clone any configs below. All of InnerConnectionMode is already cloned, so you could just consume self instead.

---

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

#[derive(Subcommand, Debug, Clone)]
pub enum AddSocks5Commands {

Are we intentionally not including user/password here?

---

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

    fn name(&self) -> String {
        match self {
            AddCustomCommands::Shadowsocks { name, .. } => name,

Nit: You could match with a single arm:

AddCustomCommands::Shadowsocks { name, .. } | AddCustomCommands::Socks5(AddSocks5Commands::Remote { name, .. })

etc.

Might look horrible, to be fair.

---

mullvad-daemon/src/api.rs line 153 at r13 (raw file):

                    access_method::CustomAccessMethod::Shadowsocks(shadowsocks_config) => {
                        ApiConnectionMode::Proxied(ProxyConfig::Shadowsocks(
                            shadowsocks_config.clone(),

Nit: If you consume Option<AccessMethodSettings>, you don't need to clone this.

[raksooo]

Reviewed 1 of 17 files at r13.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @dlon, @faern, and @MarkusPettersson98)

---

mullvad-management-interface/proto/management_interface.proto line 25 at r13 (raw file):

  rpc GetCurrentVersion(google.protobuf.Empty) returns (google.protobuf.StringValue) {}
  rpc GetVersionInfo(google.protobuf.Empty) returns (AppVersionInfo) {}
  rpc GetApiAddresses(google.protobuf.Empty) returns (google.protobuf.Empty) {}

Should this return something? It currently returns Empty