Do not set default password or port for custom shadowsocks access method

[MarkusPettersson98]

Do not try to set default password for custom shadowsocks access method.
The previously provided default is the password for Mullvad's own bridge severs.

---

This change is 

[linear]

DES-410 Do not set a default password for custom `shadowsocks` access method

Currently, the mullvad api-access CLI tries to be very helpful by providing a default password ("mullvad") for authenticating against a Shadowsocks proxy

❯ mullvad api-access add shadowsocks --help
Configure a custom Shadowsocks proxy to use as an API access method

Usage: mullvad api-access add shadowsocks [OPTIONS] <NAME> <REMOTE_IP> [REMOTE_PORT] [PASSWORD] [CIPHER]

Arguments:
  <NAME>         An easy to remember name for this custom proxy
  <REMOTE_IP>    The IP of the remote Shadowsocks-proxy
  [REMOTE_PORT]  Port on which the remote Shadowsocks-proxy listens for traffic [default: 443]
  [PASSWORD]     Password for authentication [default: mullvad]
  [CIPHER]       Cipher to use [default: aes-256-gcm] [possible values: aes-128-cfb, aes-128-cfb1, aes-128-cfb8, aes-128-cfb128, aes-256-cfb, aes-256-cfb1, aes-256-cfb8, aes-256-cfb128, rc4, rc4-md5, chacha20, salsa20, chacha20-ietf, aes-128-gcm, aes-256-gcm, chacha20-ietf-poly1305, xchacha20-ietf-poly1305, aes-128-pmac-siv, aes-256-pmac-siv]

Options:
  -d, --disabled  Disable the use of this custom access method. It has to be manually enabled at a later stage to be used when accessing the Mullvad API
  -h, --help      Print help
  -V, --version   Print version

This is likely a remnant from the debugging phase during implementation of mullvad api-access. We should not assume the password for the user, but instead force them to provide it.

[dlon]

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

[faern]

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

---

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

        remote_ip: IpAddr,
        /// Password for authentication
        password: String,

Sorry for drive-by review. But please don't put password between IP and port. Those two belong together logically.

Is there any reason the remote endpoint is not an SocketAddr instead?

[dlon]

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

---

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

Previously, faern (Linus Färnstrand) wrote…

Sorry for drive-by review. But please don't put password between IP and port. Those two belong together logically.

Is there any reason the remote endpoint is not an SocketAddr instead?

Consistency is one reason. If we change it to a SocketAddr here, we should do the same for the bridge command, custom relays, etc.

[MarkusPettersson98]

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

---

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

Previously, dlon (David Lönnhager) wrote…

Consistency is one reason. If we change it to a SockAddr here, we should do the same for the bridge command, custom relays, etc.

At the moment, remote_port is optional due to the provided default value of 443, and thus has to be placed after all required arguments

$ mullvad api-access add shadowsocks easy-to-remember-name 192.168.1.1 443 password
thread 'main' panicked at /home/<user>/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.4.2/src/builder/debug_asserts.rs:658:17:
Found non-required positional argument with a lower index than a required positional argument: "remote_port" index Some(3)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The simplest solution is to simply remove the default remote_port value, and then we can shuffle around the argument list so that password appear after remote_port.

David's point about concistency is a spot on explanation on why remote_ip: IpAddr & remote_port: u16 is not combined into a single SocketAddr value.
With that said, we should take the opportunity to address this before the feature is officially released 😊

[dlon]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faern and @MarkusPettersson98)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

At the moment, remote_port is optional due to the provided default value of 443, and thus has to be placed after all required arguments

$ mullvad api-access add shadowsocks easy-to-remember-name 192.168.1.1 443 password
thread 'main' panicked at /home/<user>/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.4.2/src/builder/debug_asserts.rs:658:17:
Found non-required positional argument with a lower index than a required positional argument: "remote_port" index Some(3)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The simplest solution is to simply remove the default remote_port value, and then we can shuffle around the argument list so that password appear after remote_port.

David's point about concistency is a spot on explanation on why remote_ip: IpAddr & remote_port: u16 is not combined into a single SocketAddr value.
With that said, we should take the opportunity to address this before the feature is officially released 😊

The port should probably not be optional. But if it has to be optional, it should be long rather than a positional argument, IMO.

---

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

        /// Cipher to use
        #[arg(value_parser = SHADOWSOCKS_CIPHERS, default_value = "aes-256-gcm")]
        cipher: String,

cipher should not be a positional argument, in my opinion.

[MarkusPettersson98]

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

---

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

Previously, dlon (David Lönnhager) wrote…

The port should probably not be optional. But if it has to be optional, it should be long rather than a positional argument, IMO.

Making the remote_port required by not providing a default value seems like the way to go IMO.

---

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

Previously, dlon (David Lönnhager) wrote…

cipher should not be a positional, in my opinion.

Agree, making it a longis clearer

[MarkusPettersson98]

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

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Making the remote_port required by not providing a default value seems like the way to go IMO.

IP and port are now grouped together:

Usage: mullvad api-access add shadowsocks [OPTIONS] <NAME> <REMOTE_IP> <REMOTE_PORT> <PASSWORD>

Arguments:
  <NAME>         An easy to remember name for this custom proxy
  <REMOTE_IP>    The IP of the remote Shadowsocks-proxy
  <REMOTE_PORT>  Port on which the remote Shadowsocks-proxy listens for traffic
  <PASSWORD>     Password for authentication

[faern]

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

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Agree, making it a longis clearer

Why does it have a default value? Is it likely that a random custom third party shadowsocks server will use this specific cipher? Is that more common than the port being 443?

I'd argue that all required arguments should be positional and all optional should be long flag arguments.

[faern]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dlon and @MarkusPettersson98)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

IP and port are now grouped together:

Usage: mullvad api-access add shadowsocks [OPTIONS] <NAME> <REMOTE_IP> <REMOTE_PORT> <PASSWORD>

Arguments:
  <NAME>         An easy to remember name for this custom proxy
  <REMOTE_IP>    The IP of the remote Shadowsocks-proxy
  <REMOTE_PORT>  Port on which the remote Shadowsocks-proxy listens for traffic
  <PASSWORD>     Password for authentication

🥳

[MarkusPettersson98]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dlon and @faern)

---

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

Previously, faern (Linus Färnstrand) wrote…

Why does it have a default value? Is it likely that a random custom third party shadowsocks server will use this specific cipher? Is that more common than the port being 443?

I'd argue that all required arguments should be positional and all optional should be long flag arguments.

cipher no longer has a default value, and is thus not optional.

Now, every required argument is positional and every optional argument is a long flag argument, except disabled being a short flag argument in addition to being a long flag argument. This exception is the same for all other custom access methods as well.

[dlon]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faern)

[faern]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

cipher no longer has a default value, and is thus not optional.

Now, every required argument is positional and every optional argument is a long flag argument, except disabled being a short flag argument in addition to being a long flag argument. This exception is the same for all other custom access methods as well.

Noice! 👍