Refactor TunConfig and TalpidVpnService and fix duplicate tun configuration

[dlon]

This PR refactors how the VPN service is set up on Android and fixes the issue of the VPN service being recreated needlessly.

Some changes:

• Move the ownership of TunConfig to TunProvider. Previously, the configuration was partially owned by TunProvider.
• Removed TalpidVpnService.defaultTunConfig.
• Split TunConfig into an internal type used by the tunnel state machine and a private type that maps to the arguments used by the VpnService.

Fix DES-1011.

---

This change is 

[Pururun]

Reviewed 17 of 20 files at r1, 1 of 1 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[Pururun]

Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[linear]

DES-1011 Remove unnecessary tunnel configurations on Android

[MarkusPettersson98]

Reviewed 17 of 20 files at r1, 2 of 3 files at r4, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dlon)

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 130 at r6 (raw file):

            dns_ips
        }
    }

Note that we can drop the #[allow(unused_variables] directive.

Also, deploying the Iterator trait and std::iter::once the code can be simplified a bit further:

    fn get_dns_servers(&self, shared_values: &SharedTunnelStateValues) -> Vec<IpAddr> {
        use std::iter::{once, Iterator};
        if let Some(ref servers) = shared_values.dns_servers {
            servers.clone()
        } else {
            once(IpAddr::from(self.metadata.ipv4_gateway))
                .chain(self.metadata.ipv6_gateway.map(IpAddr::from))
                .collect()
        }
    }

Might not be worth it tho

Code quote:

    #[allow(unused_variables)]
    fn get_dns_servers(&self, shared_values: &SharedTunnelStateValues) -> Vec<IpAddr> {
        #[cfg(not(target_os = "android"))]
        if let Some(ref servers) = shared_values.dns_servers {
            servers.clone()
        } else {
            let mut dns_ips = vec![self.metadata.ipv4_gateway.into()];
            if let Some(ipv6_gateway) = self.metadata.ipv6_gateway {
                dns_ips.push(ipv6_gateway.into());
            };
            dns_ips
        }
        #[cfg(target_os = "android")]
        {
            let mut dns_ips = vec![];
            dns_ips.push(self.metadata.ipv4_gateway.into());
            if let Some(ipv6_gateway) = self.metadata.ipv6_gateway {
                dns_ips.push(ipv6_gateway.into());
            };
            dns_ips
        }
    }

---

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

        }
        servers
    }

Would it make sense to implement this on TunnelMetadata as well?

Code quote:

    /// Return a copy of all gateway addresses
    pub fn gateway_ips(&self) -> Vec<IpAddr> {
        let mut servers = vec![self.ipv4_gateway.into()];
        if let Some(gateway) = self.ipv6_gateway {
            servers.push(gateway.into());
        }
        servers
    }

---

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

        }
    }
}

It's nice that we implement Default! Could we also explain how the values are derived? In some cases we could declare/use constants to convey the intent as well, for example using Ipv4Addr::UNSPECIFIED & Ipv6Addr::UNSPECIFIED

Code quote:

impl Default for TunConfig {
    fn default() -> Self {
        TunConfig {
            addresses: vec![IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))],
            mtu: 1380,
            ipv4_gateway: Ipv4Addr::new(10, 64, 0, 1),
            ipv6_gateway: None,
            routes: vec![
                IpNetwork::new(IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)), 0)
                    .expect("Invalid IP network prefix for IPv4 address"),
                IpNetwork::new(IpAddr::V6(Ipv6Addr::new(0, 0, 0, 0, 0, 0, 0, 0)), 0)
                    .expect("Invalid IP network prefix for IPv6 address"),
            ],
            allow_lan: false,
            dns_servers: None,
            excluded_packages: vec![],
        }
    }
}

---

talpid-tunnel/src/tun_provider/unix.rs line 45 at r6 (raw file):

    }

    pub fn get_tun(&mut self) -> Result<UnixTun, Error> {

To me, it's not that obvious that a get_ function would have the side effect of creating a new tunnel device. Please add some docs on this, and preferably change the name to something which implies a side effect. E.g. create_tun, open_tun etc.

Code quote:

    pub fn get_tun(&mut self) -> Result<UnixTun, Error> {

---

talpid-types/src/net/mod.rs line 52 at r6 (raw file):

        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xff05, 0, 0, 0, 0, 0, 0, 0), 16).unwrap()),
    ]
});

Should these constants be part of the talpid-types crate? At least some of them seem like they are plain business logic of the app. Don't really care that much, just posing the question 😊

Code quote:

/// When "allow local network" is enabled the app will allow traffic to and from these networks.
pub static ALLOWED_LAN_NETS: Lazy<[IpNetwork; 6]> = Lazy::new(|| {
    [
        IpNetwork::V4(Ipv4Network::new(Ipv4Addr::new(10, 0, 0, 0), 8).unwrap()),
        IpNetwork::V4(Ipv4Network::new(Ipv4Addr::new(172, 16, 0, 0), 12).unwrap()),
        IpNetwork::V4(Ipv4Network::new(Ipv4Addr::new(192, 168, 0, 0), 16).unwrap()),
        IpNetwork::V4(Ipv4Network::new(Ipv4Addr::new(169, 254, 0, 0), 16).unwrap()),
        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 0), 10).unwrap()),
        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xfc00, 0, 0, 0, 0, 0, 0, 0), 7).unwrap()),
    ]
});

/// When "allow local network" is enabled the app will allow traffic to these networks.
pub static ALLOWED_LAN_MULTICAST_NETS: Lazy<[IpNetwork; 8]> = Lazy::new(|| {
    [
        // Local network broadcast. Not routable
        IpNetwork::V4(Ipv4Network::new(Ipv4Addr::new(255, 255, 255, 255), 32).unwrap()),
        // Local subnetwork multicast. Not routable
        IpNetwork::V4(Ipv4Network::new(Ipv4Addr::new(224, 0, 0, 0), 24).unwrap()),
        // Admin-local IPv4 multicast.
        IpNetwork::V4(Ipv4Network::new(Ipv4Addr::new(239, 0, 0, 0), 8).unwrap()),
        // Interface-local IPv6 multicast.
        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xff01, 0, 0, 0, 0, 0, 0, 0), 16).unwrap()),
        // Link-local IPv6 multicast. IPv6 equivalent of 224.0.0.0/24
        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xff02, 0, 0, 0, 0, 0, 0, 0), 16).unwrap()),
        // Realm-local IPv6 multicast.
        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xff03, 0, 0, 0, 0, 0, 0, 0), 16).unwrap()),
        // Admin-local IPv6 multicast.
        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xff04, 0, 0, 0, 0, 0, 0, 0), 16).unwrap()),
        // Site-local IPv6 multicast.
        IpNetwork::V6(Ipv6Network::new(Ipv6Addr::new(0xff05, 0, 0, 0, 0, 0, 0, 0), 16).unwrap()),
    ]
});

---

talpid-wireguard/src/wireguard_go/mod.rs line 141 at r6 (raw file):

            mtu: config.mtu,
            ..tun_config.clone()
        };

Nit: I think this would read more intuitively if there were setter-methods on TunConfig. Allows us to potentially hide implementation details of TunConfig 🤷

impl TunConfig {
    /// Return a copy of all gateway addresses
    pub fn gateway_ips(&self) -> Vec<IpAddr> {
        let mut servers = vec![self.ipv4_gateway.into()];
        if let Some(gateway) = self.ipv6_gateway {
            servers.push(gateway.into());
        }
        servers
    }


    pub fn set_addresses(&mut self, addresses: impl Iterator<Item = IpAddr>) {
        self.addresses = addresses.into_iter().collect()
    }

    // .. 

    pub fn set_mtu(&mut self, mtu: u16) {
        self.mtu = mtu;
    }
}

    fn get_tunnel(
        tun_provider: Arc<Mutex<TunProvider>>,
        config: &Config,
        routes: impl Iterator<Item = IpNetwork>,
    ) -> Result<(Tun, RawFd)> {
        let mut last_error = None;
        let mut tun_provider = tun_provider.lock().unwrap();

        let tun_config = tun_provider.config_mut();
        tun_config.set_addresses(config.tunnel.addresses.iter().cloned());
        // ..
        tun_config.set_mtu(config.mtu);

Code quote:

        let tun_config = tun_provider.config_mut();

        *tun_config = TunConfig {
            addresses: config.tunnel.addresses.clone(),
            ipv4_gateway: config.ipv4_gateway,
            ipv6_gateway: config.ipv6_gateway,
            routes: routes.collect(),
            mtu: config.mtu,
            ..tun_config.clone()
        };

[dlon]

Reviewable status: 12 of 24 files reviewed, 6 unresolved discussions (waiting on @MarkusPettersson98 and @Pururun)

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 130 at r6 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Note that we can drop the #[allow(unused_variables] directive.

Also, deploying the Iterator trait and std::iter::once the code can be simplified a bit further:

    fn get_dns_servers(&self, shared_values: &SharedTunnelStateValues) -> Vec<IpAddr> {
        use std::iter::{once, Iterator};
        if let Some(ref servers) = shared_values.dns_servers {
            servers.clone()
        } else {
            once(IpAddr::from(self.metadata.ipv4_gateway))
                .chain(self.metadata.ipv6_gateway.map(IpAddr::from))
                .collect()
        }
    }

Might not be worth it tho

Done. (The parts that were still relevant now.)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Would it make sense to implement this on TunnelMetadata as well?

Done. Nice improvement.

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

It's nice that we implement Default! Could we also explain how the values are derived? In some cases we could declare/use constants to convey the intent as well, for example using Ipv4Addr::UNSPECIFIED & Ipv6Addr::UNSPECIFIED

Done. I'm not overly happy with the state of this, but I've tried to clarify its purpose.

---

talpid-tunnel/src/tun_provider/unix.rs line 45 at r6 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

To me, it's not that obvious that a get_ function would have the side effect of creating a new tunnel device. Please add some docs on this, and preferably change the name to something which implies a side effect. E.g. create_tun, open_tun etc.

Agree. Renamed.

[MarkusPettersson98]

Reviewed 11 of 12 files at r7, all commit messages.
Reviewable status: 23 of 24 files reviewed, 3 unresolved discussions (waiting on @dlon and @Pururun)

---

talpid-core/src/tunnel_state_machine/connected_state.rs line 130 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Done. (The parts that were still relevant now.)

Very clean! 🤩 Good job 🥳

---

talpid-tunnel/src/lib.rs line 72 at r7 (raw file):

        }
        servers
    }

Nit: servers is a bit of a misnomer, no? Anyway, it's so self-contained, so it does not matter at all

Code quote:

    pub fn gateways(&self) -> Vec<IpAddr> {
        let mut servers = vec![self.ipv4_gateway.into()];
        if let Some(gateway) = self.ipv6_gateway {
            servers.push(gateway.into());
        }
        servers
    }

---

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

Previously, dlon (David Lönnhager) wrote…

Done. I'm not overly happy with the state of this, but I've tried to clarify its purpose.

I think you improved it 😊

---

talpid-tunnel/src/tun_provider/unix.rs line 45 at r6 (raw file):

Previously, dlon (David Lönnhager) wrote…

Agree. Renamed.

🙌

---

talpid-tunnel/src/tun_provider/unix.rs line 33 at r7 (raw file):

impl UnixTunProvider {
    pub fn new(config: TunConfig) -> Self {

Nit: This could easily be made const 😊

Code quote:

    pub fn new(config: TunConfig) -> Self {

[MarkusPettersson98]

Reviewed 3 of 3 files at r8, all commit messages.
Reviewable status: 23 of 24 files reviewed, 3 unresolved discussions (waiting on @dlon and @Pururun)