Track IPv6 connectivity on Android

[Pururun]

Exposes the availability of IPv4 and IPv6 to the daemon.

This is done by creating a dummy socket on the uderlying network.

This aligns android with other platforms.

---

This change is 

[linear]

DROID-1700 Expose IP version availability to the daemon

[dlon]

It could be mentioned in the changelog that this prevents the app from trying to connect over IPv6 if it is not available.

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

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 32 at r1 (raw file):

        } catch (_: SocketException) {
            Logger.e("Socket could not be set up")
            false

I'm a bit worried about this failing for some unknown reason and causing the app to think it's offline. But maybe it never will?

[Pururun]

Added!

Reviewable status: all files reviewed, 1 unresolved discussion

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 32 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

I'm a bit worried about this failing for some unknown reason and causing the app to think it's offline. But maybe it never will?

Good point, since we need to have some kind of connection for this to even be attempted, maybe we should fall back to everything connected if both of IPv4 and IPv6 is not available.

---

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

        _isConnected =
            combine(connectivityManager.defaultNetworkFlow(), hasInternetCapability()) {

We will update for every default network event here, maybe that is a bit excessive?

[Pururun]

Reviewable status: 7 of 9 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 32 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Good point, since we need to have some kind of connection for this to even be attempted, maybe we should fall back to everything connected if both of IPv4 and IPv6 is not available.

I added a fallback behaviour in connectivity listener so that we will never return a disconnected state if at least one network with internet is available.

[dlon]

Reviewed 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion

[MarkusPettersson98]

Reviewed 3 of 8 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion

[hulthe]

Reviewed 2 of 8 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Pururun)

---

talpid-core/src/connectivity_listener.rs line 85 at r3 (raw file):

        Ok(ConnectivityListener {
            jvm: android_context.clone().jvm,

nit: unnecessary clone

---

talpid-core/src/connectivity_listener.rs line 140 at r3 (raw file):

            .expect("Missing ConnectionStatus.ipv6")
            .z()
            .expect("ipv6 is not a boolean");

Have you considered using derive(jnix::FromJava) instead of converting the java object manually?

---

talpid-core/src/connectivity_listener.rs line 187 at r3 (raw file):

    let isIPv4 = JNI_TRUE == is_ipv4;
    let isIPv6 = JNI_TRUE == is_ipv6;

nit: variables should be snake_case

[Rawa]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Pururun)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 49 at r3 (raw file):

        _isConnected =
            combine(connectivityManager.defaultNetworkFlow(), hasInternetCapability()) {

Is it necessary to combine hasInternetCapability and defaultNetworkFlow? Or maybe we think it is a bit slow?

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

        ip: T,
        protect: (socket: DatagramSocket) -> Unit,
    ): Boolean {

Right now we don't separate between fail to fetch if a IP version vs not available, should we? Or is it fine that they mean the same thing?

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 27 at r3 (raw file):

        val socket = DatagramSocket()
        return try {
            protect(socket)

We should verify that the protect was successful, probably also log if it isn't.

[Pururun]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 49 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Is it necessary to combine hasInternetCapability and defaultNetworkFlow? Or maybe we think it is a bit slow?

What do you mean slow? The reason is that we need to know if the have a internet connection and we want to react to changes in default network so that we can check if network has IPv4 and/or IPv6. But as I noted above we probably only need to react if the underlying default network change, when chanhing from mobile data to wifi or between wifi networks.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

Right now we don't separate between fail to fetch if a IP version vs not available, should we? Or is it fine that they mean the same thing?

To me it is fine, I have currently not seen any reason to different them.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 27 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

We should verify that the protect was successful, probably also log if it isn't.

Protect will always fail if a VPN is not set up, so if we want to verify it we need to know if the VPN is set up properly.

[dlon]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Pururun and @Rawa)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

We will update for every default network event here, maybe that is a bit excessive?

If it's not too much work, maybe "debouncing" the events would make sense. See for example BurstGuard

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 21 at r3 (raw file):

    // If the ip version is not supported on the underlying network it will trigger a socket
    // exception. If not it should return the local ip address.
    private inline fun <reified T : InetAddress> isIPAvailable(

Should we document the reason for this hack? I.e., "there's no other way to figure out which interface is the (non-VPN) default interface".

[dlon]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

To me it is fine, I have currently not seen any reason to different them.

It seems a bit risky to return "no connectivity" if the cause is something else. If possible, it seems safer to return true unless it fails for that specific reason

[Pururun]

Reviewable status: 3 of 10 files reviewed, 5 unresolved discussions (waiting on @dlon, @hulthe, @MarkusPettersson98, and @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 21 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should we document the reason for this hack? I.e., "there's no other way to figure out which interface is the (non-VPN) default interface".

Good idead, added!

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

It seems a bit risky to return "no connectivity" if the cause is something else. If possible, it seems safer to return true unless it fails for that specific reason

I guess it depends on what we expected behaviour. When I was testing it always threw a SocketException when IPv6/IPv4 was not available, but I am fine with just returning true if not exception is thrown.

---

talpid-core/src/connectivity_listener.rs line 85 at r3 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

nit: unnecessary clone

Done

---

talpid-core/src/connectivity_listener.rs line 140 at r3 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Have you considered using derive(jnix::FromJava) instead of converting the java object manually?

Done

---

talpid-core/src/connectivity_listener.rs line 187 at r3 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

nit: variables should be snake_case

Done

[Pururun]

Reviewable status: 3 of 10 files reviewed, 5 unresolved discussions (waiting on @dlon, @hulthe, @MarkusPettersson98, and @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I guess it depends on what we expected behaviour. When I was testing it always threw a SocketException when IPv6/IPv4 was not available, but I am fine with just returning true if not exception is thrown.

I changed it so that it always assumed that it is working for now if it does not throw an exception.

[hulthe]

Reviewed 5 of 7 files at r4, all commit messages.
Reviewable status: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @dlon, @Pururun, and @Rawa)

[MarkusPettersson98]

Reviewed 3 of 7 files at r4, all commit messages.
Reviewable status: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @dlon, @Pururun, and @Rawa)

[dlon]

Reviewed 2 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Pururun and @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I changed it so that it always assumed that it is working for now if it does not throw an exception.

As long as SocketException is raised only in that case, I guess it's fine

[Rawa]

Reviewed 1 of 1 files at r2, 4 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Pururun)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 49 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

What do you mean slow? The reason is that we need to know if the have a internet connection and we want to react to changes in default network so that we can check if network has IPv4 and/or IPv6. But as I noted above we probably only need to react if the underlying default network change, when chanhing from mobile data to wifi or between wifi networks.

I mean the defaultNetworkFlow could also provide if it has internet or not. But we wouldn't know for sure until the Capabilities callback is invoked.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 27 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Protect will always fail if a VPN is not set up, so if we want to verify it we need to know if the VPN is set up properly.

Yes, but we don't know if it will Succeed just because of a VPN is currently setup, it probably will but there is no guarantee in the interface. If we fail to protect we should handle it gracefully.

val protectSuccess = protect(socket)
if(!protectSuccess) {
    // return what ever makes sense if the protect has failed.
}

[Rawa]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Pururun)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 29 at r4 (raw file):

    ): Boolean {
        val socket = DatagramSocket()
        return try {

We should limit the try to be as small as possible. protect should be lifted out.

[Rawa]

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

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

Previously, dlon (David Lönnhager) wrote…

As long as SocketException is raised only in that case, I guess it's fine

I'll leave this unresolved until that has been confirmed.

[Rawa]

Reviewed 1 of 8 files at r1, 1 of 7 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dlon and @Pururun)

---

android/test/e2e/src/main/kotlin/net/mullvad/mullvadvpn/test/e2e/misc/Networking.kt line 9 at r4 (raw file):

object Networking {
    fun getDeviceIpv4Address(): String {

nit: Unnecessary change?

[MarkusPettersson98]

Reviewed 1 of 1 files at r5, 1 of 6 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: 6 of 10 files reviewed, 5 unresolved discussions (waiting on @dlon, @hulthe, @Pururun, and @Rawa)

[hulthe]

Reviewed 2 of 6 files at r6, 1 of 1 files at r7, 1 of 3 files at r8, all commit messages.
Reviewable status: 7 of 10 files reviewed, 5 unresolved discussions (waiting on @dlon, @Pururun, and @Rawa)

[Rawa]

Reviewed 1 of 1 files at r5, 2 of 6 files at r6, 1 of 1 files at r7, 2 of 3 files at r8, all commit messages.
Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @dlon and @Pururun)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 49 at r3 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Changed it to use the new defaultRawNetworkStateFlow() which I guess is as slow?

It uses the same mechanism. I was referring to the point that rawNetworkState also has the knowledge if we have internet capability as well. But it would only look at the network that is used by default right now.

[Rawa]

Reviewed 1 of 7 files at r4, 1 of 6 files at r6, 1 of 3 files at r8.
Reviewable status: 9 of 10 files reviewed, 4 unresolved discussions (waiting on @dlon and @Pururun)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/model/Connectivity.kt line 4 at r8 (raw file):

sealed class Connectivity {
    data class Status(val ipv4: Boolean, val ipv6: Boolean) : Connectivity()

Feels like we should name the variables a bit better. Given I have a Connectivity.Status.ipv4 it is not clear what it means.

[Rawa]

Reviewed 1 of 6 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dlon and @Pururun)

[Pururun]

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

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 49 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

It uses the same mechanism. I was referring to the point that rawNetworkState also has the knowledge if we have internet capability as well. But it would only look at the network that is used by default right now.

Yeah it is a bit of a conflict in that we want to look at the default network if we have IPv4 and IPv6, but we don't really want to rely on the default network to check if we have any internet connection as that is somewhat unreliable.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/model/Connectivity.kt line 4 at r8 (raw file):

Previously, Rawa (David Göransson) wrote…

Feels like we should name the variables a bit better. Given I have a Connectivity.Status.ipv4 it is not clear what it means.

Changed it to ipv4Available

[Rawa]

Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @dlon, @hulthe, and @Pururun)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/model/Connectivity.kt line 4 at r8 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Changed it to ipv4Available

Maybe hasIpv4 would be even better? ipv4Available makes it sound that it is available but not necessarily active.

[Rawa]

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

[Rawa]

I'm sort of fine with this PR given that it is well tested. :)

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

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 77 at r9 (raw file):

                    hasInternetCapability: Boolean ->
                    if (hasInternetCapability) {
                        if (rawNetworkState.isUnderlyingNetwork()) {

Maybe I'm going in circles, but would this still work when roaming? Our default network would be WiFi and suddenly the underlying network may change right?

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 167 at r9 (raw file):

        this?.hasCapability(NetworkCapabilities.NET_CAPABILITY_INTERNET) == true

    private fun RawNetworkState?.isUnderlyingNetwork(): Boolean =

Verify that this aligns with the lingo used for Vpn tunnels, I assumed the Underlying network is the network that the VPN uses, not just any network that is not vpn.

[Pururun]

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

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 77 at r9 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe I'm going in circles, but would this still work when roaming? Our default network would be WiFi and suddenly the underlying network may change right?

I assume the default network state will update with new capabilities.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 167 at r9 (raw file):

Previously, Rawa (David Göransson) wrote…

Verify that this aligns with the lingo used for Vpn tunnels, I assumed the Underlying network is the network that the VPN uses, not just any network that is not vpn.

Changed it to isNotVpn since that is what we are looking for anyway.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/model/Connectivity.kt line 4 at r8 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe hasIpv4 would be even better? ipv4Available makes it sound that it is available but not necessarily active.

Just realized the reason why it was named ipv4 and ipv6 was because it reflects a underlying type in the daemon. So changing it leads to crash. So will revert.

[Pururun]

Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @dlon and @Rawa)

---

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

Previously, dlon (David Lönnhager) wrote…

If it's not too much work, maybe "debouncing" the events would make sense. See for example BurstGuard

Added debounce (for this and other reasons)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 77 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I assume the default network state will update with new capabilities.

Code changed, new versions works fine when roaming with a debounce.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/ConnectivityListener.kt line 167 at r9 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Changed it to isNotVpn since that is what we are looking for anyway.

Changed it again to isVpn

[Pururun]

Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IPAvailabilityUtils.kt line 24 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

I'll leave this unresolved until that has been confirmed.

Tested on 4 devices with Android 11, 14 and 15 and seems to work fine.

[kl]

Reviewed 1 of 1 files at r5, 3 of 6 files at r6, 1 of 1 files at r7, 1 of 3 files at r8, 2 of 3 files at r10, 4 of 4 files at r12, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IpUtils.kt line 24 at r12 (raw file):

    // If the ip version is not supported on the underlying network it will trigger a socket
    // exception. Otherwise we assume it is available.
    private inline fun <reified T : InetAddress> hasIpVersion(

Does not need to be inline and reified IMO.

---

talpid-core/src/connectivity_listener.rs line 126 at r12 (raw file):

            .expect("Missing isConnected")
            .l()
            .expect("isConnected is not an object");

These two .expect calls will panic when the previous code returned an error.

To prevent panic we could do this instead:

        let is_connected = env.call_method(
            self.android_listener.as_obj(),
            "isConnected",
            "()Lnet/mullvad/talpid/model/Connectivity;",
            &[],
        );

        let is_connected = match is_connected {
            Ok(JValue::Object(object)) => object,
            value => {
                return Err(Error::InvalidMethodResult(
                    "ConnectivityListener",
                    "isConnected",
                    format!("{:?}", value),
                ))
            }
        };

        Ok(Connectivity::from_java(&env, is_connected))

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/ConnectivityManagerUtil.kt line 138 at r12 (raw file):

        // the underlying network
        Connectivity.Status(
            IpUtils.hasIPv4(protect = { protect(it) }),

Can be simplified to

hasIPv4(protect),
hasIPv6(protect),

[Pururun]

Reviewable status: 5 of 12 files reviewed, 6 unresolved discussions (waiting on @hulthe, @kl, @MarkusPettersson98, and @Rawa)

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/ConnectivityManagerUtil.kt line 138 at r12 (raw file):

Previously, kl (Kalle Lindström) wrote…

Can be simplified to

hasIPv4(protect),
hasIPv6(protect),

Done

---

talpid-core/src/connectivity_listener.rs line 126 at r12 (raw file):

Previously, kl (Kalle Lindström) wrote…

These two .expect calls will panic when the previous code returned an error.

To prevent panic we could do this instead:

        let is_connected = env.call_method(
            self.android_listener.as_obj(),
            "isConnected",
            "()Lnet/mullvad/talpid/model/Connectivity;",
            &[],
        );

        let is_connected = match is_connected {
            Ok(JValue::Object(object)) => object,
            value => {
                return Err(Error::InvalidMethodResult(
                    "ConnectivityListener",
                    "isConnected",
                    format!("{:?}", value),
                ))
            }
        };

        Ok(Connectivity::from_java(&env, is_connected))

Done.

---

android/lib/talpid/src/main/kotlin/net/mullvad/talpid/util/IpUtils.kt line 24 at r12 (raw file):

Previously, kl (Kalle Lindström) wrote…

Does not need to be inline and reified IMO.

Done (when @Rawa has pushed his code)

[Rawa]

Reviewed 1 of 3 files at r10, 1 of 4 files at r12, 4 of 6 files at r13, 2 of 2 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

[dlon]

Reviewed 1 of 6 files at r6, 1 of 3 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kl)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Added debounce (for this and other reasons)

👍

[kl]

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

[Rawa]

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

[Rawa]

Reviewable status: complete! all files reviewed, all discussions resolved