From fbd6a2c012b12b39f84b56ca43749274df5092ac Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Mon, 27 Apr 2020 14:34:48 +0900 Subject: [PATCH] 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 Change-Id: I3e4ff990c0d4825b21c7679be29a482a2d1324ec --- .../android/server/NetworkAgentWrapper.java | 3 + .../server/ConnectivityServiceTest.java | 107 +++++++++++++++++- .../android/server/connectivity/VpnTest.java | 7 ++ 3 files changed, 111 insertions(+), 6 deletions(-) diff --git a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java index a35fb407bc..0ffafd4561 100644 --- a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java +++ b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java @@ -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: diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index bef682b282..4ba71a416f 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -5422,6 +5422,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 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(); @@ -5455,6 +5496,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( @@ -5463,10 +5505,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( @@ -5475,7 +5519,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( @@ -5484,16 +5529,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)); - // Use Wifi but not cell. Note the VPN is now unmetered. + // 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); + + // 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( @@ -5502,7 +5567,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. diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index 1994d1f2ed..f8d8a56b57 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -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)); } /**