Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8e53c795 authored by Chalard Jean's avatar Chalard Jean
Browse files

Fix a bug where VPNs start out suspended on cellular 

As NetworkAgent is in a transition where all agents need
to include the NOT_SUSPENDED capability as part of their
migration to the system API, ConnectivityService adds it
forcefully to all agents that don't have the CELLULAR
transport. This doesn't include VPNs when VPNs have some
cellular network as their underlying network.

The best way to solve this is to make sure the VPN
capabilities reflect those of the underlying networks as
far as the NOT_SUSPENDED capability is concerned. This
is how they work for other similar capabilities.

This also happens to contain a drive-by fix for an issue
with a spurious capabilities callback is triggered when
a VPN connects and it has any underlying network (which
means almost always, because it will take the default
network if it doesn't declare any). Fixing this was
necessary to have a cogent test of this issue, but it
could be moved to another patch or it could stay unfixed
with some minor ajustment to the tests if judged too
dangerous to include in R at this point.

Test: New tests in this patch. Also manually tested with
      tcpdump as described in b/150570873.
Bug: 150570873
Original-Change: https://android-review.googlesource.com/1301317
Merged-In: I3e4ff990c0d4825b21c7679be29a482a2d1324ec
Change-Id: I3e4ff990c0d4825b21c7679be29a482a2d1324ec
parent 92d678d8

services/core/java/com/android/server/connectivity/Vpn.java

+19 −9
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ import static android.net.ConnectivityManager.NETID_UNSET; 
import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_CONGESTED;

import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_METERED;

import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_ROAMING;

import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_SUSPENDED;

import static android.net.RouteInfo.RTN_THROW;

import static android.net.RouteInfo.RTN_UNREACHABLE;
@@ -317,8 +318,7 @@ public class Vpn { 
     *

     * @param defaultNetwork underlying network for VPNs following platform's default

     */

    public synchronized NetworkCapabilities updateCapabilities(

            @Nullable Network defaultNetwork) {

    public synchronized NetworkCapabilities updateCapabilities(@Nullable Network defaultNetwork) {

        if (mConfig == null) {

            // VPN is not running.

            return null;
@@ -350,11 +350,10 @@ public class Vpn { 
        int[] transportTypes = new int[] { NetworkCapabilities.TRANSPORT_VPN };

        int downKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED;

        int upKbps = NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED;

        // VPN's meteredness is OR'd with isAlwaysMetered and meteredness of its underlying

        // networks.

        boolean metered = isAlwaysMetered;

        boolean roaming = false;

        boolean congested = false;

        boolean metered = isAlwaysMetered; // metered if any underlying is metered, or alwaysMetered

        boolean roaming = false; // roaming if any underlying is roaming

        boolean congested = false; // congested if any underlying is congested

        boolean suspended = true; // suspended if all underlying are suspended



        boolean hadUnderlyingNetworks = false;

        if (null != underlyingNetworks) {
@@ -367,15 +366,24 @@ public class Vpn { 
                    transportTypes = ArrayUtils.appendInt(transportTypes, underlyingType);

                }



                // When we have multiple networks, we have to assume the

                // worst-case link speed and restrictions.

                // Merge capabilities of this underlying network. For bandwidth, assume the

                // worst case.

                downKbps = NetworkCapabilities.minBandwidth(downKbps,

                        underlyingCaps.getLinkDownstreamBandwidthKbps());

                upKbps = NetworkCapabilities.minBandwidth(upKbps,

                        underlyingCaps.getLinkUpstreamBandwidthKbps());

                // If this underlying network is metered, the VPN is metered (it may cost money

                // to send packets on this network).

                metered |= !underlyingCaps.hasCapability(NET_CAPABILITY_NOT_METERED);

                // If this underlying network is roaming, the VPN is roaming (the billing structure

                // is different than the usual, local one).

                roaming |= !underlyingCaps.hasCapability(NET_CAPABILITY_NOT_ROAMING);

                // If this underlying network is congested, the VPN is congested (the current

                // condition of the network affects the performance of this network).

                congested |= !underlyingCaps.hasCapability(NET_CAPABILITY_NOT_CONGESTED);

                // If this network is not suspended, the VPN is not suspended (the VPN

                // is able to transfer some data).

                suspended &= !underlyingCaps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED);

            }

        }

        if (!hadUnderlyingNetworks) {
@@ -383,6 +391,7 @@ public class Vpn { 
            metered = true;

            roaming = false;

            congested = false;

            suspended = false;

        }



        caps.setTransportTypes(transportTypes);
@@ -391,6 +400,7 @@ public class Vpn { 
        caps.setCapability(NET_CAPABILITY_NOT_METERED, !metered);

        caps.setCapability(NET_CAPABILITY_NOT_ROAMING, !roaming);

        caps.setCapability(NET_CAPABILITY_NOT_CONGESTED, !congested);

        caps.setCapability(NET_CAPABILITY_NOT_SUSPENDED, !suspended);

    }



    /**

tests/net/integration/util/com/android/server/NetworkAgentWrapper.java

+3 −0
Original line number Diff line number Diff line
@@ -92,6 +92,9 @@ public class NetworkAgentWrapper implements TestableNetworkCallback.HasNetwork { 
                break;

            case TRANSPORT_VPN:

                mNetworkCapabilities.removeCapability(NET_CAPABILITY_NOT_VPN);

                // VPNs deduce the SUSPENDED capability from their underlying networks and there

                // is no public API to let VPN services set it.

                mNetworkCapabilities.removeCapability(NET_CAPABILITY_NOT_SUSPENDED);

                mScore = ConnectivityConstants.VPN_DEFAULT_SCORE;

                break;

            default:

tests/net/java/com/android/server/ConnectivityServiceTest.java

+101 −6
Original line number Diff line number Diff line
@@ -5417,6 +5417,47 @@ public class ConnectivityServiceTest { 
        callback.expectAvailableCallbacksValidated(mEthernetNetworkAgent);

    }



    @Test

    public void testVpnStartsWithUnderlyingCaps() throws Exception {

        final int uid = Process.myUid();



        final TestNetworkCallback vpnNetworkCallback = new TestNetworkCallback();

        final NetworkRequest vpnNetworkRequest = new NetworkRequest.Builder()

                .removeCapability(NET_CAPABILITY_NOT_VPN)

                .addTransportType(TRANSPORT_VPN)

                .build();

        mCm.registerNetworkCallback(vpnNetworkRequest, vpnNetworkCallback);

        vpnNetworkCallback.assertNoCallback();



        // Connect cell. It will become the default network, and in the absence of setting

        // underlying networks explicitly it will become the sole underlying network for the vpn.

        mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR);

        mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_SUSPENDED);

        mCellNetworkAgent.connect(true);



        final TestNetworkAgentWrapper vpnNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_VPN);

        final ArraySet<UidRange> ranges = new ArraySet<>();

        ranges.add(new UidRange(uid, uid));

        mMockVpn.setNetworkAgent(vpnNetworkAgent);

        mMockVpn.connect();

        mMockVpn.setUids(ranges);

        vpnNetworkAgent.connect(true /* validated */, false /* hasInternet */,

                false /* isStrictMode */);



        vpnNetworkCallback.expectAvailableCallbacks(vpnNetworkAgent.getNetwork(),

                false /* suspended */, false /* validated */, false /* blocked */, TIMEOUT_MS);

        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent.getNetwork(), TIMEOUT_MS,

                nc -> nc.hasCapability(NET_CAPABILITY_VALIDATED));



        final NetworkCapabilities nc = mCm.getNetworkCapabilities(vpnNetworkAgent.getNetwork());

        assertTrue(nc.hasTransport(TRANSPORT_VPN));

        assertTrue(nc.hasTransport(TRANSPORT_CELLULAR));

        assertFalse(nc.hasTransport(TRANSPORT_WIFI));

        assertTrue(nc.hasCapability(NET_CAPABILITY_VALIDATED));

        assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED));

        assertTrue(nc.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));

    }



    @Test

    public void testVpnSetUnderlyingNetworks() throws Exception {

        final int uid = Process.myUid();
@@ -5450,6 +5491,7 @@ public class ConnectivityServiceTest { 


        // Connect cell and use it as an underlying network.

        mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR);

        mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_SUSPENDED);

        mCellNetworkAgent.connect(true);



        mService.setUnderlyingNetworksForVpn(
@@ -5458,10 +5500,12 @@ public class ConnectivityServiceTest { 
        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED));

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI);

        mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED);

        mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_SUSPENDED);

        mWiFiNetworkAgent.connect(true);



        mService.setUnderlyingNetworksForVpn(
@@ -5470,7 +5514,8 @@ public class ConnectivityServiceTest { 
        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED));

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        // Don't disconnect, but note the VPN is not using wifi any more.

        mService.setUnderlyingNetworksForVpn(
@@ -5479,16 +5524,36 @@ public class ConnectivityServiceTest { 
        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED));

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        // Remove NOT_SUSPENDED from the only network and observe VPN is now suspended.

        mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_SUSPENDED);

        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && !caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));

        vpnNetworkCallback.expectCallback(CallbackEntry.SUSPENDED, vpnNetworkAgent);



        // Use Wifi but not cell. Note the VPN is now unmetered.

        // Add NOT_SUSPENDED again and observe VPN is no longer suspended.

        mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_SUSPENDED);

        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR) && !caps.hasTransport(TRANSPORT_WIFI)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));

        vpnNetworkCallback.expectCallback(CallbackEntry.RESUMED, vpnNetworkAgent);



        // Use Wifi but not cell. Note the VPN is now unmetered and not suspended.

        mService.setUnderlyingNetworksForVpn(

                new Network[] { mWiFiNetworkAgent.getNetwork() });



        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && !caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI)

                && caps.hasCapability(NET_CAPABILITY_NOT_METERED));

                && caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        // Use both again.

        mService.setUnderlyingNetworksForVpn(
@@ -5497,7 +5562,37 @@ public class ConnectivityServiceTest { 
        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED));

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        // Cell is suspended again. As WiFi is not, this should not cause a callback.

        mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_SUSPENDED);

        vpnNetworkCallback.assertNoCallback();



        // Stop using WiFi. The VPN is suspended again.

        mService.setUnderlyingNetworksForVpn(

                new Network[] { mCellNetworkAgent.getNetwork() });

        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && !caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));

        // While the SUSPENDED callback should in theory be sent here, it is not. This is

        // a bug in ConnectivityService, but as the SUSPENDED and RESUMED callbacks have never

        // been public and are deprecated and slated for removal, there is no sense in spending

        // resources fixing this bug now.



        // Use both again.

        mService.setUnderlyingNetworksForVpn(

                new Network[] { mCellNetworkAgent.getNetwork(), mWiFiNetworkAgent.getNetwork() });



        vpnNetworkCallback.expectCapabilitiesThat(vpnNetworkAgent,

                (caps) -> caps.hasTransport(TRANSPORT_VPN)

                && caps.hasTransport(TRANSPORT_CELLULAR) && caps.hasTransport(TRANSPORT_WIFI)

                && !caps.hasCapability(NET_CAPABILITY_NOT_METERED)

                && caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));

        // As above, the RESUMED callback not being sent here is a bug, but not a bug that's

        // worth anybody's time to fix.



        // Disconnect cell. Receive update without even removing the dead network from the

        // underlying networks – it's dead anyway. Not metered any more.

tests/net/java/com/android/server/connectivity/VpnTest.java

+7 −0
Original line number Diff line number Diff line
@@ -25,6 +25,7 @@ import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET; 
import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_CONGESTED;

import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_METERED;

import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_ROAMING;

import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_SUSPENDED;

import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR;

import static android.net.NetworkCapabilities.TRANSPORT_VPN;

import static android.net.NetworkCapabilities.TRANSPORT_WIFI;
@@ -606,6 +607,7 @@ public class VpnTest { 
                        .addCapability(NET_CAPABILITY_NOT_METERED)

                        .addCapability(NET_CAPABILITY_NOT_ROAMING)

                        .addCapability(NET_CAPABILITY_NOT_CONGESTED)

                        .addCapability(NET_CAPABILITY_NOT_SUSPENDED)

                        .setLinkUpstreamBandwidthKbps(20));

        setMockedNetworks(networks);
@@ -621,6 +623,7 @@ public class VpnTest { 
        assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        Vpn.applyUnderlyingCapabilities(

                mConnectivityManager,
@@ -635,6 +638,7 @@ public class VpnTest { 
        assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED));

        assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));

        assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        Vpn.applyUnderlyingCapabilities(

                mConnectivityManager, new Network[] {wifi}, caps, false /* isAlwaysMetered */);
@@ -646,6 +650,7 @@ public class VpnTest { 
        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_METERED));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        Vpn.applyUnderlyingCapabilities(

                mConnectivityManager, new Network[] {wifi}, caps, true /* isAlwaysMetered */);
@@ -657,6 +662,7 @@ public class VpnTest { 
        assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));



        Vpn.applyUnderlyingCapabilities(

                mConnectivityManager,
@@ -671,6 +677,7 @@ public class VpnTest { 
        assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_METERED));

        assertFalse(caps.hasCapability(NET_CAPABILITY_NOT_ROAMING));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_CONGESTED));

        assertTrue(caps.hasCapability(NET_CAPABILITY_NOT_SUSPENDED));

    }



    /**