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?

[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/ConnectivityListener.kt line 49 at r3 (raw file):

Previously, Rawa (David Göransson) wrote…

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.

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

---

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

Previously, Rawa (David Göransson) wrote…

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.
}

Done

---

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

Previously, Rawa (David Göransson) wrote…

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

Moved out

---

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

Previously, Rawa (David Göransson) wrote…

nit: Unnecessary change?

Done

[MarkusPettersson98]

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