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
diff --git a/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java b/tests/net/integration/util/com/android/server/NetworkAgentWrapper.java
index a35fb40..0ffafd4 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 @@
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 bef682b..4ba71a4 100644
--- a/tests/net/java/com/android/server/ConnectivityServiceTest.java
+++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java
@@ -5423,6 +5423,47 @@
}
@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();
@@ -5455,6 +5496,7 @@
// 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 @@
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 @@
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 @@
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 @@
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 1994d1f..f8d8a56 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_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 @@
.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 @@
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 @@
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 @@
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 @@
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 @@
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));
}
/**