Fix interactions between relay and bridge settings.

[pinkisemils]

A couple of improvements w.r.t. bridge mode:

• when new relay constraints are applied, they will change the bridge state if they enforce constraints that don't support bridges
• when bridge mode is set to On, then relay constraints will be cleared to auto
• take bridge mode into account when determining preferred relay constraints

---

This change is 

[faern]

Reviewed 1 of 6 files at r1.
Reviewable status: 1 of 6 files reviewed, 5 unresolved discussions (waiting on @pinkisemils)

---

CHANGELOG.md, line 36 at r1 (raw file):

- Upgrade OpenSSL from 1.1.0h to 1.1.1c.
- Upgrade wireguard-go library to v0.0.20190805.
- Improve bridge and relay setting interaction.

I just merged the 2019.7-beta1 stuff and the changelog header. Please rebase and move this to [Unreleased]

---

mullvad-types/src/relay_constraints.rs, line 362 at r1 (raw file):

        match &self {
            RelaySettingsUpdate::CustomTunnelEndpoint(endpoint) => {
                return endpoint.endpoint().protocol == TransportProtocol::Tcp;

No need for return keyword. Clippy will likely complain.

---

mullvad-types/src/relay_constraints.rs, line 373 at r1 (raw file):

                    }
                }
                return true;

No need for return keyword. Clippy will likely complain.

---

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

&BridgeState::On == &self.bridge_state

Can't you remove the borrowing since it occurs on both sides? Also, if comparing a variable with a constant is it not more standard to put the variable first? This is not C so we don't need to worry about yoda conditions and such :)

---

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

        if self.relay_settings != new_settings {
            if !update_supports_bridge && &BridgeState::On == &self.bridge_state {
                self.bridge_state = BridgeState::Auto;

This check-and-change is very closely related to the opposite one at https://github.com/mullvad/mullvadvpn-app/blob/master/mullvad-daemon/src/lib.rs#L1139

It would be very nice if all of the similar settings sanity checks were performed on the same abstraction level/place. Is that possible or are they different in some way that does not allow this? 🤔

I would agree that it's probably more sane to do this settings compatibility check here, like you do now, rather than in mullvad-daemon, since talpid should be aware of what is incompatible or not and should make sure no settings become incompatible.

So yeah, I think this code is good. But we should at some point consider if the other check should be moved here as well.

[pinkisemils]

Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @faern)

---

CHANGELOG.md, line 36 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I just merged the 2019.7-beta1 stuff and the changelog header. Please rebase and move this to [Unreleased]

Done.

---

mullvad-types/src/relay_constraints.rs, line 362 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

No need for return keyword. Clippy will likely complain.

Done.

---

mullvad-types/src/relay_constraints.rs, line 373 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

No need for return keyword. Clippy will likely complain.

Done.

---

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

Previously, faern (Linus Färnstrand) wrote…

&BridgeState::On == &self.bridge_state

Can't you remove the borrowing since it occurs on both sides? Also, if comparing a variable with a constant is it not more standard to put the variable first? This is not C so we don't need to worry about yoda conditions and such :)

Done.

---

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

Previously, faern (Linus Färnstrand) wrote…

This check-and-change is very closely related to the opposite one at https://github.com/mullvad/mullvadvpn-app/blob/master/mullvad-daemon/src/lib.rs#L1139

It would be very nice if all of the similar settings sanity checks were performed on the same abstraction level/place. Is that possible or are they different in some way that does not allow this? 🤔

I would agree that it's probably more sane to do this settings compatibility check here, like you do now, rather than in mullvad-daemon, since talpid should be aware of what is incompatible or not and should make sure no settings become incompatible.

So yeah, I think this code is good. But we should at some point consider if the other check should be moved here as well.

I moved both of the checks into the setters of the Settings struct.

[faern]

Reviewed 1 of 6 files at r1, 2 of 4 files at r2.
Reviewable status: 3 of 6 files reviewed, 4 unresolved discussions (waiting on @faern and @pinkisemils)

---

CHANGELOG.md, line 27 at r2 (raw file):

## [Unreleased]
### Changed
- Improve bridge and relay setting interaction.

I should have said it in the earlier thread, but did not think about it on that level then :/

Should this not be under ### Fixed. Since before this it was possible to set a combination that simply never yielded any working connections: Bridges on and OpenVPN over UDP.

---

mullvad-types/src/settings/mod.rs, line 310 at r2 (raw file):

    // Set the OpenVPN tunnel to use TCP.
    fn apply_proxy_constraints(&mut self) {

We are mixing the words "proxy" and "bridge" in this file. I also don't feel I get exactly what this method does from the name. Would it be more suitable with something like make_relay_constraints_bridge_compatible?

---

mullvad-types/src/settings/mod.rs, line 312 at r2 (raw file):

    fn apply_proxy_constraints(&mut self) {
        let constraints_update = RelayConstraintsUpdate {
            tunnel_protocol: Some(Constraint::Only(TunnelProtocol::OpenVpn)),

I think we agreed on Slack to set this to Any as well?

---

mullvad-types/src/settings/mod.rs, line 322 at r2 (raw file):

        let settings_update = RelaySettingsUpdate::Normal(constraints_update);

        self.relay_settings.merge(settings_update);

I know this code is just moved. But would it be possible to write without using a RelaySettingsUpdate? This feels very complicated compared to what it would have to be. The following suggestion looks more complicated than the above. But it also incorporates more features, i.e. only resetting settings if they are actually incompatible:

if self.relay_settings.tunnel_protocol == Constraint::Only(TunnelProtocol::Wireguard) {
    self.relay_settings.tunnel_protocol = Constraint::Any;
}
if self.relay_settings.openvpn_constraints.protocol == Constraint::Udp {
    self.relay_settings.openvpn_constraints = OpenVpnConstraints {
        protocol: Constraint::Any,
        port: Constraint::Any,
    };
}

[pinkisemils]

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

---

CHANGELOG.md, line 27 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I should have said it in the earlier thread, but did not think about it on that level then :/

Should this not be under ### Fixed. Since before this it was possible to set a combination that simply never yielded any working connections: Bridges on and OpenVPN over UDP.

Done.

---

mullvad-types/src/settings/mod.rs, line 310 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

We are mixing the words "proxy" and "bridge" in this file. I also don't feel I get exactly what this method does from the name. Would it be more suitable with something like make_relay_constraints_bridge_compatible?

Done.

---

mullvad-types/src/settings/mod.rs, line 312 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think we agreed on Slack to set this to Any as well?

Done.

---

mullvad-types/src/settings/mod.rs, line 322 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I know this code is just moved. But would it be possible to write without using a RelaySettingsUpdate? This feels very complicated compared to what it would have to be. The following suggestion looks more complicated than the above. But it also incorporates more features, i.e. only resetting settings if they are actually incompatible:

if self.relay_settings.tunnel_protocol == Constraint::Only(TunnelProtocol::Wireguard) {
    self.relay_settings.tunnel_protocol = Constraint::Any;
}
if self.relay_settings.openvpn_constraints.protocol == Constraint::Udp {
    self.relay_settings.openvpn_constraints = OpenVpnConstraints {
        protocol: Constraint::Any,
        port: Constraint::Any,
    };
}

self.relay_settings is an enumeration rather than a plain struct since custom relay settings can be set as well. I'll play around with this to make it more feature-full.

[faern]

Reviewed 1 of 3 files at r3.
Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @faern and @pinkisemils)

---

CHANGELOG.md, line 27 at r2 (raw file):
I think the text itself says very little about what actually changed. In one weeks time neither of us will have any idea what this actually changes.

Compare with other ### Fixed entries such as this random one:

• Adjust network interface checks in offline detection logic. Prevents the app from being stuck
  in the offline state when the computer is in fact online.

I think we should expand this to something like

• Check and adjust relay and bridge constraints when they are updated, so no incompatible combinations are used.

[pinkisemils]

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

---

CHANGELOG.md, line 27 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

I think the text itself says very little about what actually changed. In one weeks time neither of us will have any idea what this actually changes.

Compare with other ### Fixed entries such as this random one:

• Adjust network interface checks in offline detection logic. Prevents the app from being stuck
  in the offline state when the computer is in fact online.

I think we should expand this to something like

• Check and adjust relay and bridge constraints when they are updated, so no incompatible combinations are used.

That's fair. I've updated it with your suggestion now!

[faern]

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

---

mullvad-types/src/relay_constraints.rs, line 95 at r4 (raw file):

    }

    pub(crate) fn ensure_bridge_compatibility(&mut self) {

Awesome! 🎉

---

mullvad-types/src/relay_constraints.rs, line 110 at r4 (raw file):

                }
            }
            RelaySettings::CustomTunnelEndpoint(_) => {}

Would it make sense to log something here? Especially if the custom tunnel endpoint happens to use UDP? Like log::warn!("Custom relay uses UDP. Will likely not work with bridges") or something? It's far from our main use case with custom tunnels, but having some logging when we are unsure about stuff might save a few pulled hairs when things don't go as expected.

[pinkisemils]

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @faern)

---

mullvad-types/src/relay_constraints.rs, line 110 at r4 (raw file):

Previously, faern (Linus Färnstrand) wrote…

Would it make sense to log something here? Especially if the custom tunnel endpoint happens to use UDP? Like log::warn!("Custom relay uses UDP. Will likely not work with bridges") or something? It's far from our main use case with custom tunnels, but having some logging when we are unsure about stuff might save a few pulled hairs when things don't go as expected.

Fair point. I have added some logging now.

[faern]

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

---

mullvad-types/src/relay_constraints.rs, line 110 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Fair point. I have added some logging now.

Ok, but I would personally have put it under warn. Since we at this point is fairly certain that the user has an invalid config, we just can't do anything about it for them automatically.