Set the app in a blocking state if invalid ipv6 config

[Pururun]

NOTE! Due to how the android implementation of tunnel state machine is set up if the blocking config also is invalid it will throw a critical error and not be in a blocking state.

We could change this, but that would add behavior related specifically to this error.

Easiest way to test:
git revert fde04b51be6b541f890abb1bbc0dabc3e995607a
This will revoke the fix for IPv6.
After that the error will trigger if you turn off "In-tunnel IPv6"

---

This change is 

[linear]

DROID-2384 Set the app in a blocking state instead of showing a critical error if IPv6 config is wrong

[hulthe]

@hulthe reviewed 8 files and all commit messages, and made 8 comments.
Reviewable status: 8 of 16 files reviewed, 8 unresolved discussions (waiting on @Pururun).

---

talpid-types/src/tunnel.rs line 92 at r1 (raw file):

    /// Android has rejected due to invalid IPV6 config.
    #[cfg(target_os = "android")]
    InvalidIPv6Config(Vec<IpAddr>, Vec<IpAddr>, Vec<IpAddr>),

nit: Ditto named fields

---

talpid-types/src/tunnel.rs line 210 at r1 (raw file):

                return write!(
                    f,
                    "Invalid ipv6 tunnel configuration addresses: {} routes: {} dns_servers: {}",

Should there be a period or colon or something in between "configuration addresses"?

---

talpid-tunnel/src/tun_provider/android/mod.rs line 462 at r1 (raw file):

    },
    InvalidIpv6Config {
        addresses: Vec<IpAddr>,

What does addresses mean? Is it relay addresses/endpoints?

---

talpid-tunnel/src/tun_provider/android/mod.rs line 53 at r1 (raw file):

        "Attempt to configure the tunnel with invalid config addresses: {0:?} routes: {1:?} dnsServers: {2:?}"
    )]
    InvalidIpv6Config(Vec<IpAddr>, Vec<IpAddr>, Vec<IpAddr>),

nit: I think this guy should use named fields, just like CreateTunResult::InvalidIpv6Config, since it's not obvious what the fields mean when just looking at the types. Attaching doc comments to the fields is also valid if you feel its warranted.

InvalidIpv6Config {
    addresses: Vec<IpAddr>,
    routes: Vec<IpAddr>,
    dns_servers: Vec<IpAddr>,
}

---

talpid-tunnel/src/tun_provider/android/mod.rs line 463 at r1 (raw file):

    InvalidIpv6Config {
        addresses: Vec<IpAddr>,
        routes: Vec<IpAddr>,

I'm guessing routes refers to the ip-subnets that should be routed into the tun device, but then it should include the subnet mask, no? e.g. the /16 at the end of an ip address.

---

talpid-tunnel/src/tun_provider/mod.rs line 103 at r1 (raw file):

    #[cfg(target_os = "android")]
    pub fn set_blocking_config(&mut self) {

This function could use a doc comment

---

talpid-tunnel/src/tun_provider/mod.rs line 109 at r1 (raw file):

        self.ipv4_gateway = blocking_config.ipv4_gateway;
        self.ipv6_gateway = blocking_config.ipv6_gateway;
        self.routes = blocking_config.routes;

Would it make sense to remove DNS servers also?

---

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

    INVALID_IPV6_CONFIG = 12;
    SPLIT_TUNNEL_ERROR = 13;
    NEED_FULL_DISK_PERMISSIONS = 14;

IMO, it's best to avoid making breaking changes to the proto-file if possible (i.e. changing ids). It makes daemon development annoying when you have to juggle different versions of the GUI between branches.

P.S. the IDs don't need to be sorted, so this is valid:

    // Android only
    INVALID_DNS_SERVERS = 11;
    // Android only
    INVALID_IPV6_CONFIG = 14;
    SPLIT_TUNNEL_ERROR = 12;
    NEED_FULL_DISK_PERMISSIONS = 13;

[Rawa]

In general really good improvement 👍, great work 👏, just had a few remarks.

@Rawa reviewed 16 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @hulthe and @Pururun).

---

talpid-tunnel/src/tun_provider/mod.rs line 109 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Would it make sense to remove DNS servers also?

DNS servers needs to be set, otherwise we might leak DNS (Thanks android). But a dummy DNS could be an alternative, like we do on the android side of things.

---

talpid-tunnel/src/tun_provider/android/mod.rs line 53 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

nit: I think this guy should use named fields, just like CreateTunResult::InvalidIpv6Config, since it's not obvious what the fields mean when just looking at the types. Attaching doc comments to the fields is also valid if you feel its warranted.

InvalidIpv6Config {
    addresses: Vec<IpAddr>,
    routes: Vec<IpAddr>,
    dns_servers: Vec<IpAddr>,
}

Agree 👍

---

talpid-tunnel/src/tun_provider/android/mod.rs line 462 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

What does addresses mean? Is it relay addresses/endpoints?

It is the addresses that are assigned to the VPN interface. It is the same language that is used on the TunConfig. This aligns with what android is to consume, but yeah I think it could deserve a comment explaining it.

---

talpid-tunnel/src/tun_provider/android/mod.rs line 463 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

I'm guessing routes refers to the ip-subnets that should be routed into the tun device, but then it should include the subnet mask, no? e.g. the /16 at the end of an ip address.

Yes, this should be a Vec<InetNetwork>.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/TalpidVpnService.kt line 87 at r1 (raw file):

        config.excludedPackages.forEach { app -> builder.addDisallowedApplication(app) }

        val configureAddressesAndRoutes: Either<TunConfig, Unit> = either {

I'm fully fond of how we handle the errors here. Nothing new, but it becomes more obvious as we introduce more errors. I don't have a great way to fix it on top of my head though. Since this is such a critical piece of code I think it could be worth exploring some alternatives together to see if we can make something even more clear.

It is also a bit scary with all the combinations that can happen, in both InvalidDnsAddresses & InvalidIpv6Config case or end goal is a blocking config. So maybe we should introduce that, a more "fixed" blocking config (we'd still have to consider the local network sharing settings etc which might make it hard though).

[hulthe]

@hulthe reviewed 1 file and made 2 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Pururun and @Rawa).

---

talpid-tunnel/src/tun_provider/android/mod.rs line 462 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

It is the addresses that are assigned to the VPN interface. It is the same language that is used on the TunConfig. This aligns with what android is to consume, but yeah I think it could deserve a comment explaining it.

Ah, of course.

---

talpid-core/src/tunnel_state_machine/mod.rs line 731 at r1 (raw file):

        if blocking {
            config.set_blocking_config();
            config.dns_servers = Some(vec![]);

@Rawa this seems to be the only place we call set_blocking_config. Here we also set dns_servers to an empty list. Should we move the dns server thing into set_blocking_config?

[Rawa]

@Rawa made 2 comments.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @hulthe and @Pururun).

---

talpid-core/src/tunnel_state_machine/mod.rs line 731 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

@Rawa this seems to be the only place we call set_blocking_config. Here we also set dns_servers to an empty list. Should we move the dns server thing into set_blocking_config?

Yeah a blocking config should never have empty dns servers if we want it to be "correct". E.g what we fallback to in TalpidVpnService is a dummy dns 192.0.2.1

---

talpid-tunnel/src/tun_provider/mod.rs line 103 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

This function could use a doc comment

Word!