Make it possible to disable in-tunnel IPv6 on Android

[Pururun]

This adds the in-tunnel IPv6 setting to VPN settings on android.

If IPv6 is not routed through the tunnel, according to documentation, the android system should just drop the IPv6 traffic.
This is only true though if none of routes, addresses and dns servers contains a IPv6 address.

This PR makes it so by changing the following, all which is required to avoid leaks:

1. Do not add :: to the configured route if IPv6 is not available in the tunnel.
2. Do not add any IPv6 routes if local network sharing is turned on.
3. Filter out any IPv6 dns addresses if IPv6 is not routed.

It also informs the user if that the dns will not work if they have set a IPv6 dns address and IPv6 is not enabled in the tunnel.

Fixes: DROID-1579

---

This change is 

[linear]

DROID-1669 Workaround IPv6 leak when disabling in-tunnel IPv6

[linear]

DROID-1579 Implement in-tunnel IP version setting

[MarkusPettersson98]

Reviewed 3 of 22 files at r1, all commit messages.
Reviewable status: 3 of 22 files reviewed, 1 unresolved discussion

---

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

            .unwrap_or_else(|| config.gateways())
            // If IPv6 not available we need to disable all IPv6 DNS servers as that will cause leaks
            .iter().filter(|ip| !ip.is_ipv6() || config.ipv6_gateway.is_some()).copied().collect()

⛏️ If you use into_iter instead of iter, the iterator will own the values and we can get rid of the call to copied!

// If IPv6 not available we need to disable all IPv6 DNS servers as that will cause leaks
let want_ipv6 = |ip: &IpAddr| config.ipv6_gateway.is_some() || !ip.is_ipv6();
config
    .dns_servers
    .clone()
    .unwrap_or_else(|| config.gateways())
    .into_iter()
    .filter(want_ipv6)
    .collect()

Code quote:

        config
            .dns_servers
            .clone()
            .unwrap_or_else(|| config.gateways())
            // If IPv6 not available we need to disable all IPv6 DNS servers as that will cause leaks
            .iter().filter(|ip| !ip.is_ipv6() || config.ipv6_gateway.is_some()).copied().collect()

[Pururun]

Reviewable status: 2 of 22 files reviewed, all discussions resolved (waiting on @MarkusPettersson98)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ If you use into_iter instead of iter, the iterator will own the values and we can get rid of the call to copied!

// If IPv6 not available we need to disable all IPv6 DNS servers as that will cause leaks
let want_ipv6 = |ip: &IpAddr| config.ipv6_gateway.is_some() || !ip.is_ipv6();
config
    .dns_servers
    .clone()
    .unwrap_or_else(|| config.gateways())
    .into_iter()
    .filter(want_ipv6)
    .collect()

Done

[Rawa]

Reviewed 2 of 22 files at r1.
Reviewable status: 3 of 22 files reviewed, 2 unresolved discussions (waiting on @MarkusPettersson98)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 313 at r1 (raw file):

    onToggleAutoStartAndConnectOnBoot: (Boolean) -> Unit,
    onSelectDeviceIpVersion: (ipVersion: Constraint<IpVersion>) -> Unit,
    onToggleIPv6Toggle: (Boolean) -> Unit,

We have a mix of Ipv6, IPv6 and IpV6, we should align them so they look the same. Not sure what looks the best, IPv6 is what wikipedia has as short for it in written text, but think we should use Ipv6 for variables, then it would be similar as Dns, we never write it as DNS in variables.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 111 at r1 (raw file):

            validationError = input.validateDnsEntry(currentIndex, customDnsList).leftOrNull(),
            isLocal = input.isLocalAddress(),
            isIpv6 = input.isIpv6(),

Maybe we can bake these (isIpv6 & isLocal) into the ViewState right away? Or may there is a reason why we didn't do it for isLocal?

[MarkusPettersson98]

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

[Rawa]

Reviewed 20 of 22 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DnsCell.kt line 53 at r1 (raw file):

                )
            }
            if (isUnreachableIpv6DnsWarningVisible) {

Should we show both warnings if both are applicable? Now it would be 2 warning signs, right? E.g using a FE80:x address with IPv6 disabled and local network sharing inactive.

[Rawa]

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

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DnsCell.kt line 53 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Should we show both warnings if both are applicable? Now it would be 2 warning signs, right? E.g using a FE80:x address with IPv6 disabled and local network sharing inactive.

I can check, currently the warnings are placed on top of each other so that is wrong regardless.

Just made one icon visible.

Should check with design to see how to solve multiple errors.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 313 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

We have a mix of Ipv6, IPv6 and IpV6, we should align them so they look the same. Not sure what looks the best, IPv6 is what wikipedia has as short for it in written text, but think we should use Ipv6 for variables, then it would be similar as Dns, we never write it as DNS in variables.

Changed what I could find to Ipv6

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 111 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

Maybe we can bake these (isIpv6 & isLocal) into the ViewState right away? Or may there is a reason why we didn't do it for isLocal?

Done

[dlon]

Reviewed 3 of 22 files at r1, 1 of 1 files at r2, 2 of 2 files at r5, all commit messages.
Reviewable status: 17 of 44 files reviewed, 4 unresolved discussions (waiting on @Rawa)

---

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

            .unwrap_or_else(|| config.gateways())
            .into_iter()
            .filter(want_ipv6)

config.gateways() already takes care of this

[Pururun]

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

---

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

Previously, dlon (David Lönnhager) wrote…

config.gateways() already takes care of this

Maybe I should calrify the comment, but the purpose of this code is to filter out dns servers set by user in "Custom DNS". These are sent to openTun unless they are removed somewhere.

[dlon]

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

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Maybe I should calrify the comment, but the purpose of this code is to filter out dns servers set by user in "Custom DNS". These are sent to openTun unless they are removed somewhere.

Ah! I'm blind. That makes sense.

[Rawa]

Reviewed 27 of 28 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Pururun)

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DnsCell.kt line 53 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

I can check, currently the warnings are placed on top of each other so that is wrong regardless.

Just made one icon visible.

Should check with design to see how to solve multiple errors.

👍 We should make sure if it makes sense that we prioritize the isUnreachableLocalDnsWarningVisible content description as well. Also this behavior needs to align with the error message in the dialog, where we explain the reason for the problem.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 313 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Changed what I could find to Ipv6

👍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 111 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Done

I believe you misunderstood me. I was thinking we possibly can remove isLocal and isIpv6 from the constructor, because they are strictly derived from the input parameter that is already in the constructor itself. Then it wouldn't be possible to create a ViewState that has a IPv6 address, but then has isIpv6 = false.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 275 at r5 (raw file):

        onToggleAutoStartAndConnectOnBoot = vm::onToggleAutoStartAndConnectOnBoot,
        onSelectDeviceIpVersion = vm::onDeviceIpVersionSelected,
        onToggleIpv6Toggle = vm::setIpV6Enabled,

setIpv6Enabled

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreenTest.kt line 77 at r5 (raw file):

        onToggleAutoStartAndConnectOnBoot: (Boolean) -> Unit = {},
        onSelectDeviceIpVersion: (Constraint<IpVersion>) -> Unit = {},
        onToggleIPv6Toggle: (Boolean) -> Unit = {},

Ipv6

---

android/CHANGELOG.md line 39 at r5 (raw file):

### Fixed
- Will no longer try to connect over IPv6 if IPv6 is not available.
- Will no longer try to connect over IPv4 if IPv4 is not available.

Can/should we can combine these? E.g "Relay selector will no longer try a IP version that is no longer present" or something along those lines?

[Pururun]

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

---

android/CHANGELOG.md line 39 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

Can/should we can combine these? E.g "Relay selector will no longer try a IP version that is no longer present" or something along those lines?

That changelog change was pushed to the wrong branch, will add that to the other PR.

[Pururun]

Reviewable status: 13 of 44 files reviewed, 5 unresolved discussions (waiting on @dlon, @MarkusPettersson98, and @Rawa)

---

android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreenTest.kt line 77 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

Ipv6

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DnsCell.kt line 53 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

👍 We should make sure if it makes sense that we prioritize the isUnreachableLocalDnsWarningVisible content description as well. Also this behavior needs to align with the error message in the dialog, where we explain the reason for the problem.

Yes agree

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/VpnSettingsScreen.kt line 275 at r5 (raw file):

Previously, Rawa (David Göransson) wrote…

setIpv6Enabled

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 111 at r1 (raw file):

Previously, Rawa (David Göransson) wrote…

I believe you misunderstood me. I was thinking we possibly can remove isLocal and isIpv6 from the constructor, because they are strictly derived from the input parameter that is already in the constructor itself. Then it wouldn't be possible to create a ViewState that has a IPv6 address, but then has isIpv6 = false.

Right, fixed

[dlon]

Reviewed 3 of 31 files at r6.
Reviewable status: 15 of 45 files reviewed, 5 unresolved discussions (waiting on @Rawa)

[Rawa]

Reviewed 13 of 31 files at r6, 3 of 4 files at r7.
Reviewable status: 31 of 45 files reviewed, 4 unresolved discussions (waiting on @dlon and @Pururun)

---

android/CHANGELOG.md line 39 at r5 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

That changelog change was pushed to the wrong branch, will add that to the other PR.

👍

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 111 at r1 (raw file):

Previously, Pururun (Jonatan Rhodin) wrote…

Right, fixed

Nice ⭐

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DnsCell.kt line 62 at r7 (raw file):

                imageVector = Icons.Rounded.Error,
                contentDescription =
                    if (isUnreachableLocalDnsWarningVisible) {

A when statement would be more clear I believe:

when {
    isUnreachableLocalDnsWarningVisible -> stringResource(id = R.string.confirm_local_dns)
    isUnreachableIpv6DnsWarningVisible -> stringResource(id = R.string.confirm_ipv6_dns)
    else -> null
}

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DnsDialog.kt line 97 at r7 (raw file):

                errorText =
                    when {
                        state.validationError is ValidationError.DuplicateAddress -> {

nit: Remove all the {, shouldn't be necessary since it is all one liners.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 48 at r7 (raw file):

    fun isValid() = validationError == null

    private fun String.isLocalAddress(): Boolean {

=instead of { and return

[Rawa]

Reviewed 15 of 31 files at r6, 1 of 4 files at r7, all commit messages.
Reviewable status: 44 of 45 files reviewed, 3 unresolved discussions (waiting on @dlon and @Pururun)

[Rawa]

Reviewed 1 of 31 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Pururun)

[Pururun]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/DnsCell.kt line 62 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

A when statement would be more clear I believe:

when {
    isUnreachableLocalDnsWarningVisible -> stringResource(id = R.string.confirm_local_dns)
    isUnreachableIpv6DnsWarningVisible -> stringResource(id = R.string.confirm_ipv6_dns)
    else -> null
}

Done.

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DnsDialog.kt line 97 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

nit: Remove all the {, shouldn't be necessary since it is all one liners.

Done

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/viewmodel/DnsDialogViewModel.kt line 48 at r7 (raw file):

Previously, Rawa (David Göransson) wrote…

=instead of { and return

Done.

[Rawa]

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

---

android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/DnsDialog.kt line 99 at r8 (raw file):

                        state.validationError is ValidationError.DuplicateAddress ->
                            stringResource(R.string.duplicate_address_warning)
                        // Ordering is important, as we consider the lan error to have higher

Nice comment 👍

[Rawa]

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