From bd079455f14c52310444f76f6f2239fc651521dc Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Fri, 2 Jul 2021 11:47:57 +0900 Subject: [PATCH] Allow non-VPNs to have underlying networks. Certain network types, like the VCN, have underlying networks for the purpose of data usage, but do not want to propagate the underlying network capabilities. Allow these networks to set underlying networks, but continue not to propagate the capabilities. Bug: 190620024 Test: new unit test Change-Id: I53d6080f48707ff3c37fbfbef534284ba77a7432 --- .../android/server/ConnectivityService.java | 19 ++--- .../server/connectivity/NetworkAgentInfo.java | 13 ++-- .../android/server/NetworkAgentWrapper.java | 10 +++ .../server/ConnectivityServiceTest.java | 73 ++++++++++++++++++- 4 files changed, 97 insertions(+), 18 deletions(-) diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index fd8397fe38..68c5ae7560 100644 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -3249,11 +3249,6 @@ public class ConnectivityService extends IConnectivityManager.Stub } case NetworkAgent.EVENT_UNDERLYING_NETWORKS_CHANGED: { // TODO: prevent loops, e.g., if a network declares itself as underlying. - if (!nai.supportsUnderlyingNetworks()) { - Log.wtf(TAG, "Non-virtual networks cannot have underlying networks"); - break; - } - final List underlying = (List) arg.second; if (isLegacyLockdownNai(nai) @@ -5246,12 +5241,12 @@ public class ConnectivityService extends IConnectivityManager.Stub * information, e.g underlying ifaces. */ private UnderlyingNetworkInfo createVpnInfo(NetworkAgentInfo nai) { - if (!nai.isVPN()) return null; - Network[] underlyingNetworks = nai.declaredUnderlyingNetworks; // see VpnService.setUnderlyingNetworks()'s javadoc about how to interpret // the underlyingNetworks list. - if (underlyingNetworks == null) { + // TODO: stop using propagateUnderlyingCapabilities here, for example, by always + // initializing NetworkAgentInfo#declaredUnderlyingNetworks to an empty array. + if (underlyingNetworks == null && nai.propagateUnderlyingCapabilities()) { final NetworkAgentInfo defaultNai = getDefaultNetworkForUid( nai.networkCapabilities.getOwnerUid()); if (defaultNai != null) { @@ -5300,7 +5295,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private boolean hasUnderlyingNetwork(NetworkAgentInfo nai, Network network) { // TODO: support more than one level of underlying networks, either via a fixed-depth search // (e.g., 2 levels of underlying networks), or via loop detection, or.... - if (!nai.supportsUnderlyingNetworks()) return false; + if (!nai.propagateUnderlyingCapabilities()) return false; final Network[] underlying = underlyingNetworksOrDefault( nai.networkCapabilities.getOwnerUid(), nai.declaredUnderlyingNetworks); return CollectionUtils.contains(underlying, network); @@ -7272,7 +7267,7 @@ public class ConnectivityService extends IConnectivityManager.Stub newNc.addCapability(NET_CAPABILITY_NOT_ROAMING); } - if (nai.supportsUnderlyingNetworks()) { + if (nai.propagateUnderlyingCapabilities()) { applyUnderlyingCapabilities(nai.declaredUnderlyingNetworks, nai.declaredCapabilities, newNc); } @@ -8407,7 +8402,7 @@ public class ConnectivityService extends IConnectivityManager.Stub networkAgent.networkCapabilities.addCapability(NET_CAPABILITY_FOREGROUND); if (!createNativeNetwork(networkAgent)) return; - if (networkAgent.supportsUnderlyingNetworks()) { + if (networkAgent.propagateUnderlyingCapabilities()) { // Initialize the network's capabilities to their starting values according to the // underlying networks. This ensures that the capabilities are correct before // anything happens to the network. @@ -9316,7 +9311,7 @@ public class ConnectivityService extends IConnectivityManager.Stub private boolean ownsVpnRunningOverNetwork(int uid, Network network) { for (NetworkAgentInfo virtual : mNetworkAgentInfos) { - if (virtual.supportsUnderlyingNetworks() + if (virtual.propagateUnderlyingCapabilities() && virtual.networkCapabilities.getOwnerUid() == uid && CollectionUtils.contains(virtual.declaredUnderlyingNetworks, network)) { return true; diff --git a/service/src/com/android/server/connectivity/NetworkAgentInfo.java b/service/src/com/android/server/connectivity/NetworkAgentInfo.java index 18becd45f6..bbf523a29d 100644 --- a/service/src/com/android/server/connectivity/NetworkAgentInfo.java +++ b/service/src/com/android/server/connectivity/NetworkAgentInfo.java @@ -157,8 +157,8 @@ public class NetworkAgentInfo implements Comparable, NetworkRa @NonNull public NetworkCapabilities networkCapabilities; @NonNull public final NetworkAgentConfig networkAgentConfig; - // Underlying networks declared by the agent. Only set if supportsUnderlyingNetworks is true. - // The networks in this list might be declared by a VPN app using setUnderlyingNetworks and are + // Underlying networks declared by the agent. + // The networks in this list might be declared by a VPN using setUnderlyingNetworks and are // not guaranteed to be current or correct, or even to exist. // // This array is read and iterated on multiple threads with no locking so its contents must @@ -168,7 +168,7 @@ public class NetworkAgentInfo implements Comparable, NetworkRa // The capabilities originally announced by the NetworkAgent, regardless of any capabilities // that were added or removed due to this network's underlying networks. - // Only set if #supportsUnderlyingNetworks is true. + // Only set if #propagateUnderlyingCapabilities is true. public @Nullable NetworkCapabilities declaredCapabilities; // Indicates if netd has been told to create this Network. From this point on the appropriate @@ -898,8 +898,11 @@ public class NetworkAgentInfo implements Comparable, NetworkRa return networkCapabilities.hasTransport(NetworkCapabilities.TRANSPORT_VPN); } - /** Whether this network might have underlying networks. Currently only true for VPNs. */ - public boolean supportsUnderlyingNetworks() { + /** + * Whether this network should propagate the capabilities from its underlying networks. + * Currently only true for VPNs. + */ + public boolean propagateUnderlyingCapabilities() { return isVPN(); } diff --git a/tests/integration/util/com/android/server/NetworkAgentWrapper.java b/tests/integration/util/com/android/server/NetworkAgentWrapper.java index 17db17923f..95ea401acd 100644 --- a/tests/integration/util/com/android/server/NetworkAgentWrapper.java +++ b/tests/integration/util/com/android/server/NetworkAgentWrapper.java @@ -55,6 +55,7 @@ import com.android.net.module.util.ArrayTrackRecord; import com.android.testutils.HandlerUtils; import com.android.testutils.TestableNetworkCallback; +import java.util.List; import java.util.Objects; import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; @@ -255,6 +256,15 @@ public class NetworkAgentWrapper implements TestableNetworkCallback.HasNetwork { } } + public void setUnderlyingNetworks(List underlyingNetworks) { + mNetworkAgent.setUnderlyingNetworks(underlyingNetworks); + } + + public void setOwnerUid(int uid) { + mNetworkCapabilities.setOwnerUid(uid); + mNetworkAgent.sendNetworkCapabilities(mNetworkCapabilities); + } + public void connect() { if (!mConnected.compareAndSet(false /* expect */, true /* update */)) { // compareAndSet returns false when the value couldn't be updated because it did not diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 3b030d65f8..106a17aa76 100644 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -5976,7 +5976,7 @@ public class ConnectivityServiceTest { verify(mStatsManager, atLeastOnce()).notifyNetworkStatus(networksCaptor.capture(), any(List.class), eq(defaultIface), vpnInfosCaptor.capture()); - assertSameElements(networksCaptor.getValue(), networks); + assertSameElements(networks, networksCaptor.getValue()); List infos = vpnInfosCaptor.getValue(); if (vpnUid != null) { @@ -6180,6 +6180,77 @@ public class ConnectivityServiceTest { reset(mStatsManager); } + @Test + public void testNonVpnUnderlyingNetworks() throws Exception { + // Ensure wifi and cellular are not torn down. + for (int transport : new int[]{TRANSPORT_CELLULAR, TRANSPORT_WIFI}) { + final NetworkRequest request = new NetworkRequest.Builder() + .addTransportType(transport) + .removeCapability(NET_CAPABILITY_NOT_VCN_MANAGED) + .build(); + mCm.requestNetwork(request, new NetworkCallback()); + } + + // Connect a VCN-managed wifi network. + final LinkProperties wifiLp = new LinkProperties(); + wifiLp.setInterfaceName(WIFI_IFNAME); + mWiFiNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_WIFI, wifiLp); + mWiFiNetworkAgent.removeCapability(NET_CAPABILITY_NOT_VCN_MANAGED); + mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); + mWiFiNetworkAgent.connect(true /* validated */); + + final List none = List.of(); + expectNotifyNetworkStatus(none, null); // Wifi is not the default network + + // Create a virtual network based on the wifi network. + final int ownerUid = 10042; + NetworkCapabilities nc = new NetworkCapabilities.Builder() + .setOwnerUid(ownerUid) + .setAdministratorUids(new int[]{ownerUid}) + .build(); + final String vcnIface = "ipsec42"; + final LinkProperties lp = new LinkProperties(); + lp.setInterfaceName(vcnIface); + final TestNetworkAgentWrapper vcn = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, lp, nc); + vcn.setUnderlyingNetworks(List.of(mWiFiNetworkAgent.getNetwork())); + vcn.connect(false /* validated */); + + final TestNetworkCallback callback = new TestNetworkCallback(); + mCm.registerDefaultNetworkCallback(callback); + callback.expectAvailableCallbacksUnvalidated(vcn); + + // The underlying wifi network's capabilities are not propagated to the virtual network, + // but NetworkStatsService is informed of the underlying interface. + nc = mCm.getNetworkCapabilities(vcn.getNetwork()); + assertFalse(nc.hasTransport(TRANSPORT_WIFI)); + assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED)); + final List onlyVcn = List.of(vcn.getNetwork()); + expectNotifyNetworkStatus(onlyVcn, vcnIface, ownerUid, vcnIface, List.of(WIFI_IFNAME)); + + // Add NOT_METERED to the underlying network, check that it is not propagated. + mWiFiNetworkAgent.addCapability(NET_CAPABILITY_NOT_METERED); + callback.assertNoCallback(); + nc = mCm.getNetworkCapabilities(vcn.getNetwork()); + assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_METERED)); + + // Switch underlying networks. + final LinkProperties cellLp = new LinkProperties(); + cellLp.setInterfaceName(MOBILE_IFNAME); + mCellNetworkAgent = new TestNetworkAgentWrapper(TRANSPORT_CELLULAR, cellLp); + mCellNetworkAgent.removeCapability(NET_CAPABILITY_NOT_VCN_MANAGED); + mCellNetworkAgent.addCapability(NET_CAPABILITY_NOT_ROAMING); + mCellNetworkAgent.connect(false /* validated */); + vcn.setUnderlyingNetworks(List.of(mCellNetworkAgent.getNetwork())); + + // The underlying capability changes do not propagate to the virtual network, but + // NetworkStatsService is informed of the new underlying interface. + callback.assertNoCallback(); + nc = mCm.getNetworkCapabilities(vcn.getNetwork()); + assertFalse(nc.hasTransport(TRANSPORT_WIFI)); + assertFalse(nc.hasCapability(NET_CAPABILITY_NOT_ROAMING)); + expectNotifyNetworkStatus(onlyVcn, vcnIface, ownerUid, vcnIface, List.of(MOBILE_IFNAME)); + } + @Test public void testBasicDnsConfigurationPushed() throws Exception { setPrivateDnsSettings(PRIVATE_DNS_MODE_OPPORTUNISTIC, "ignored.example.com");