Error handling for location argument in CLI DES-435

[Serock3]

Make the CLI arguments that parse LocationArgs validate the input. Specifically, make sure that the country or host exists and, if hostname is given, then validate the server type (e.g. only bridge host for the bridge command).

Some braking changes to the CLI were done, see the changelog.

You can no longer enter the blocked state through mullvad relay set location xx. Instead a hidden command mullvad debug block-connection was added. See issue https://linear.app/mullvad/issue/DES-453/update-test-instructions-to-use-new-block-command about updating test instruction.

---

This change is 

[linear]

DES-435 Better error handling when adding non-existent relay/bridge by hostname

Context

Since DES-43 and DES-287, the user should be able to just supply the hostname of a relay/bridge and have the daemon figure out if that relay exist or not. The way that resolution works in the 3 relevant mullvad commands

• mullvad relay set location <hostname | country> [city] [hostname]

• mullvad relay set tunnel wireguard entry-location <hostname | country> | [city] [hostname]

• mullvad bridge set location <hostname | country> [city] [hostname]

• mullvad custom-list edit add <list-name> <hostname | country> [city] [hostname]

  is basically the following:

1. The daemon will assume that the first argument (<hostname | country>) is the hostname of some relay/bridge, and try to backtrack to get the city and country of that relay/bridge
2. If (1) fails, the supplied arguments (<hostname | country> [city] [hostname]) are assumed to be a geographic location constraint
3. If (2) fails .. Well:

Problem

The error handling between the different CLI subcommands differ.

• mullvad relay set location will issue a warning if the user tries to set a non-existent relay:

  $ mullvad relay set location this-relay-does-not-exist
  Warning: No matching relay was found.
  Relay constraints updated
  

  It will accept the constraint, but in practice it means that the Mullvad App will block the connection. This has historically been fine, as is evident by the mullvad relay set location xx idiom

• mullvad bridge set location will not issue a warning if the user tries to set a non-existent bridge:

  $ mullvad bridge set location this-bridge-does-not-exist
  Updated bridge settings
  

• mullvad custom-list edit add will not give any feedback, nor will it issue a warning, if the user tries to set a non-existent relay but it will incorrectly serialize it

$ mullvad custom-list edit add debuggin this-relay-does-not-exist

$ cat /etc/mullvad-vpn/settings.json
...
"custom_lists": {
  "custom_lists": [
    {
      "id": "..",
      "name": "debuggin",
      "locations": [
        {
          "country": "this-relay-does-not-exist"
        },
        {
          "hostname": [
            "al",
            "tia",
            "al-tia-wg-001"
          ]
        }
      ]
    }
  ]
},
...

This shows that these 3 different subcommands which should behave the same do not.

Solution

The code/daemon/frontends can assume hostnames are unique. Two relays can never have the same hostname. So only the hostname is a unique identifier for a relay.

1. Unify the logic for relay-bridge resolution into one function resolve_location_constraint. Since all subcommands parse the same data-structure (relay_constraints::LocationArgs), it should be the input to resolve_location_constraint.
2. Deprecate specifying a location constraint as <country> <city> <hostname>. Don't remove it but print a deprecation warning. Instead, the recommended way to specify a specific server is to just give the hostname as the <hostname | country> argument.
3. Change all location constraint argument parsing (<hostname | country> [city]) to have the following way to evaluate the arguments and compute a location constraint from them:
   1. If the city argument is given, assume the first argument is a country code. Check if this is a valid city. If not, fail with an error.
   2. Check if the first argument is an existing hostname. If it is, compute the country + city and set that as the constraint.
   3. Check if the argument is an existing country code. If it is, set that as the constraint.
   4. If it's neither a hostname or country that exists print an error about it being invalid and exit with a non-zero exit code.
4. Add mullvad debug block command to enable entering the blocked state. debug should be hidden from the help output. For inspiration on how to hide it from the help-output check mullvad shell-completions.

[MarkusPettersson98]

Great job! 🎉
See my (minor) comments. After you are done squashing, I will sign the Cargo.lock-file and merge this PR for you 😊

Reviewed 10 of 11 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Serock3)

---

mullvad-cli/src/cmds/relay.rs line 345 at r2 (raw file):

    async fn list() -> Result<()> {
        let mut countries = get_active_non_bridge_relays().await?;

If I would be a little nit-picky here, I suggest changing this function's name to get_active_relays.
Bridge relays are most often talked about as just bridges, and "non-bridge" relays are refereed to as just relays.

Code quote:

get_active_non_bridge_relays()

---

mullvad-cli/src/cmds/relay.rs line 935 at r2 (raw file):

/// Parses the [`LocationArgs`] into a [`Constraint<GeographicLocationConstraint>`].
///
/// See the documentation of [SetCommands] (`mullvad relay set location`) for a description

These should be the swapped:

[`mullvad relay set location`](SetCommands)

Love the detailed doc-comments with links! 🙌

Code quote:

[SetCommands] (`mullvad relay set location`)

---

mullvad-cli/src/cmds/relay.rs line 967 at r2 (raw file):

            Constraint::from(location_constraint_args);

        // If the location constraint was not "any", the validate the country/city

then*

Code quote:

the

---

mullvad-cli/src/cmds/relay.rs line 981 at r2 (raw file):

/// Return a list of all relays that are active and not bridges
pub async fn get_active_non_bridge_relays() -> Result<Vec<RelayListCountry>> {

See previous comment regarding the naming of relays / bridges 😊

Code quote:

get_active_non_bridge_relays()

[Serock3]

Reviewable status: 10 of 11 files reviewed, 5 unresolved discussions (waiting on @MarkusPettersson98 and @MrChocolatine)

---

CHANGELOG.md line 62 at r1 (raw file):

Previously, MrChocolatine wrote…

- Validate that hostname matches correct server type for CLI commands `mullvad relay set location`, `mullvad bridge set location` and `mullvad relay set tunnel wireguard entry location`

Done.

---

mullvad-cli/src/cmds/relay.rs line 345 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

If I would be a little nit-picky here, I suggest changing this function's name to get_active_relays.
Bridge relays are most often talked about as just bridges, and "non-bridge" relays are refereed to as just relays.

Done.

---

mullvad-cli/src/cmds/relay.rs line 935 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

These should be the swapped:

[`mullvad relay set location`](SetCommands)

Love the detailed doc-comments with links! 🙌

Done.

---

mullvad-cli/src/cmds/relay.rs line 967 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

then*

Done.

---

mullvad-cli/src/cmds/relay.rs line 981 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

See previous comment regarding the naming of relays / bridges 😊

Done.

[MarkusPettersson98]

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

[MarkusPettersson98]

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