From 3745c39f98f913373e00102d125686d2e1966488 Mon Sep 17 00:00:00 2001 From: Rambo Wang Date: Wed, 22 Apr 2020 15:24:51 -0700 Subject: [PATCH 1/6] Restrict match conditions of TelephonyNetworkSpecifier#canBeSatisfied TelephonyNetworkSpecifier will now treat null as matching nothing. When the request specifies a TelephonyNetworkSpecifier while the network does not, this should not be treated as a match. Bug: 154703135 Test: atest android.net.TelephonyNetworkSpecifierTest Change-Id: I329110e929995c9eae6c6ce33b5414777acea1e1 --- .../net/TelephonyNetworkSpecifierTest.java | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/tests/net/java/android/net/TelephonyNetworkSpecifierTest.java b/tests/net/java/android/net/TelephonyNetworkSpecifierTest.java index 47afed441a..efb92033df 100644 --- a/tests/net/java/android/net/TelephonyNetworkSpecifierTest.java +++ b/tests/net/java/android/net/TelephonyNetworkSpecifierTest.java @@ -19,7 +19,10 @@ package android.net; import static com.android.testutils.ParcelUtilsKt.assertParcelSane; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import android.net.wifi.WifiNetworkSpecifier; import android.telephony.SubscriptionManager; import androidx.test.filters.SmallTest; @@ -32,6 +35,7 @@ import org.junit.Test; @SmallTest public class TelephonyNetworkSpecifierTest { private static final int TEST_SUBID = 5; + private static final String TEST_SSID = "Test123"; /** * Validate that IllegalArgumentException will be thrown if build TelephonyNetworkSpecifier @@ -79,4 +83,31 @@ public class TelephonyNetworkSpecifierTest { .build(); assertParcelSane(specifier, 1 /* fieldCount */); } + + /** + * Validate the behavior of method canBeSatisfiedBy(). + */ + @Test + public void testCanBeSatisfiedBy() { + final TelephonyNetworkSpecifier tns1 = new TelephonyNetworkSpecifier.Builder() + .setSubscriptionId(TEST_SUBID) + .build(); + final TelephonyNetworkSpecifier tns2 = new TelephonyNetworkSpecifier.Builder() + .setSubscriptionId(TEST_SUBID) + .build(); + final WifiNetworkSpecifier wns = new WifiNetworkSpecifier.Builder() + .setSsid(TEST_SSID) + .build(); + final MatchAllNetworkSpecifier mans = new MatchAllNetworkSpecifier(); + + // Test equality + assertEquals(tns1, tns2); + assertTrue(tns1.canBeSatisfiedBy(tns1)); + assertTrue(tns1.canBeSatisfiedBy(tns2)); + + // Test other edge cases. + assertFalse(tns1.canBeSatisfiedBy(null)); + assertFalse(tns1.canBeSatisfiedBy(wns)); + assertTrue(tns1.canBeSatisfiedBy(mans)); + } } From f577197908e135532371c1638655bd6294fb3931 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Fri, 24 Apr 2020 12:19:37 +0000 Subject: [PATCH 2/6] Address comments on NetworkStack AIDL v6 Address issues found during AIDL review: - Rename clientAddr to singleClientAddr - Do not use a ParcelableBundle for notifyNetworkTested or notifyDataStallSuspected; instead use AIDL parcelables for stronger backwards compatibility guarantees. Test: atest NetworkMonitorTest ConnectivityServiceTest ConnectivityServiceIntegrationTest, manual Bug: 153500847 Merged-In: Id9b71784e5f6294d203230e57737979e063ff0f8 Change-Id: Id9b71784e5f6294d203230e57737979e063ff0f8 --- .../android/server/ConnectivityService.java | 79 ++++++++++++------- .../server/ConnectivityServiceTest.java | 55 ++++++------- 2 files changed, 80 insertions(+), 54 deletions(-) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 552331ede9..e4eb585e98 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -18,6 +18,14 @@ package com.android.server; import static android.Manifest.permission.RECEIVE_DATA_ACTIVITY_CHANGE; import static android.content.pm.PackageManager.PERMISSION_GRANTED; +import static android.net.ConnectivityDiagnosticsManager.ConnectivityReport.KEY_NETWORK_PROBES_ATTEMPTED_BITMASK; +import static android.net.ConnectivityDiagnosticsManager.ConnectivityReport.KEY_NETWORK_PROBES_SUCCEEDED_BITMASK; +import static android.net.ConnectivityDiagnosticsManager.ConnectivityReport.KEY_NETWORK_VALIDATION_RESULT; +import static android.net.ConnectivityDiagnosticsManager.DataStallReport.DETECTION_METHOD_DNS_EVENTS; +import static android.net.ConnectivityDiagnosticsManager.DataStallReport.DETECTION_METHOD_TCP_METRICS; +import static android.net.ConnectivityDiagnosticsManager.DataStallReport.KEY_DNS_CONSECUTIVE_TIMEOUTS; +import static android.net.ConnectivityDiagnosticsManager.DataStallReport.KEY_TCP_METRICS_COLLECTION_PERIOD_MILLIS; +import static android.net.ConnectivityDiagnosticsManager.DataStallReport.KEY_TCP_PACKET_FAIL_RATE; import static android.net.ConnectivityManager.CONNECTIVITY_ACTION; import static android.net.ConnectivityManager.NETID_UNSET; import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OPPORTUNISTIC; @@ -72,6 +80,7 @@ import android.net.ConnectionInfo; import android.net.ConnectivityDiagnosticsManager.ConnectivityReport; import android.net.ConnectivityDiagnosticsManager.DataStallReport; import android.net.ConnectivityManager; +import android.net.DataStallReportParcelable; import android.net.ICaptivePortal; import android.net.IConnectivityDiagnosticsCallback; import android.net.IConnectivityManager; @@ -108,6 +117,7 @@ import android.net.NetworkSpecifier; import android.net.NetworkStack; import android.net.NetworkStackClient; import android.net.NetworkState; +import android.net.NetworkTestResultParcelable; import android.net.NetworkUtils; import android.net.NetworkWatchlistManager; import android.net.PrivateDnsConfigParcel; @@ -2820,14 +2830,6 @@ public class ConnectivityService extends IConnectivityManager.Stub handleNetworkTested(nai, results.mTestResult, (results.mRedirectUrl == null) ? "" : results.mRedirectUrl); - - // Invoke ConnectivityReport generation for this Network test event. - final Message m = - mConnectivityDiagnosticsHandler.obtainMessage( - ConnectivityDiagnosticsHandler.EVENT_NETWORK_TESTED, - new ConnectivityReportEvent(results.mTimestampMillis, nai)); - m.setData(msg.getData()); - mConnectivityDiagnosticsHandler.sendMessage(m); break; } case EVENT_PROVISIONING_NOTIFICATION: { @@ -3006,23 +3008,33 @@ public class ConnectivityService extends IConnectivityManager.Stub @Override public void notifyNetworkTested(int testResult, @Nullable String redirectUrl) { - notifyNetworkTestedWithExtras(testResult, redirectUrl, SystemClock.elapsedRealtime(), - PersistableBundle.EMPTY); + // Legacy version of notifyNetworkTestedWithExtras. + // Would only be called if the system has a NetworkStack module older than the + // framework, which does not happen in practice. } @Override - public void notifyNetworkTestedWithExtras( - int testResult, - @Nullable String redirectUrl, - long timestampMillis, - @NonNull PersistableBundle extras) { - final Message msg = - mTrackerHandler.obtainMessage( - EVENT_NETWORK_TESTED, - new NetworkTestedResults( - mNetId, testResult, timestampMillis, redirectUrl)); - msg.setData(new Bundle(extras)); + public void notifyNetworkTestedWithExtras(NetworkTestResultParcelable p) { + final Message msg = mTrackerHandler.obtainMessage( + EVENT_NETWORK_TESTED, + new NetworkTestedResults( + mNetId, p.result, p.timestampMillis, p.redirectUrl)); mTrackerHandler.sendMessage(msg); + + // Invoke ConnectivityReport generation for this Network test event. + final NetworkAgentInfo nai = getNetworkAgentInfoForNetId(mNetId); + if (nai == null) return; + final Message m = mConnectivityDiagnosticsHandler.obtainMessage( + ConnectivityDiagnosticsHandler.EVENT_NETWORK_TESTED, + new ConnectivityReportEvent(p.timestampMillis, nai)); + + final PersistableBundle extras = new PersistableBundle(); + extras.putInt(KEY_NETWORK_VALIDATION_RESULT, p.result); + extras.putInt(KEY_NETWORK_PROBES_SUCCEEDED_BITMASK, p.probesSucceeded); + extras.putInt(KEY_NETWORK_PROBES_ATTEMPTED_BITMASK, p.probesAttempted); + + m.setData(new Bundle(extras)); + mConnectivityDiagnosticsHandler.sendMessage(m); } @Override @@ -3071,12 +3083,25 @@ public class ConnectivityService extends IConnectivityManager.Stub } @Override - public void notifyDataStallSuspected( - long timestampMillis, int detectionMethod, PersistableBundle extras) { - final Message msg = - mConnectivityDiagnosticsHandler.obtainMessage( - ConnectivityDiagnosticsHandler.EVENT_DATA_STALL_SUSPECTED, - detectionMethod, mNetId, timestampMillis); + public void notifyDataStallSuspected(DataStallReportParcelable p) { + final Message msg = mConnectivityDiagnosticsHandler.obtainMessage( + ConnectivityDiagnosticsHandler.EVENT_DATA_STALL_SUSPECTED, + p.detectionMethod, mNetId, p.timestampMillis); + + final PersistableBundle extras = new PersistableBundle(); + switch (p.detectionMethod) { + case DETECTION_METHOD_DNS_EVENTS: + extras.putInt(KEY_DNS_CONSECUTIVE_TIMEOUTS, p.dnsConsecutiveTimeouts); + break; + case DETECTION_METHOD_TCP_METRICS: + extras.putInt(KEY_TCP_PACKET_FAIL_RATE, p.tcpPacketFailRate); + extras.putInt(KEY_TCP_METRICS_COLLECTION_PERIOD_MILLIS, + p.tcpMetricsCollectionPeriodMillis); + break; + default: + log("Unknown data stall detection method, ignoring: " + p.detectionMethod); + return; + } msg.setData(new Bundle(extras)); // NetworkStateTrackerHandler currently doesn't take any actions based on data diff --git a/tests/net/java/com/android/server/ConnectivityServiceTest.java b/tests/net/java/com/android/server/ConnectivityServiceTest.java index a478e68c7d..a992778fd4 100644 --- a/tests/net/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/net/java/com/android/server/ConnectivityServiceTest.java @@ -144,6 +144,7 @@ import android.net.ConnectivityManager.PacketKeepalive; import android.net.ConnectivityManager.PacketKeepaliveCallback; import android.net.ConnectivityManager.TooManyRequestsException; import android.net.ConnectivityThread; +import android.net.DataStallReportParcelable; import android.net.IConnectivityDiagnosticsCallback; import android.net.IDnsResolver; import android.net.IIpConnectivityMetrics; @@ -170,6 +171,7 @@ import android.net.NetworkSpecifier; import android.net.NetworkStack; import android.net.NetworkStackClient; import android.net.NetworkState; +import android.net.NetworkTestResultParcelable; import android.net.NetworkUtils; import android.net.ProxyInfo; import android.net.ResolverParamsParcel; @@ -196,7 +198,6 @@ import android.os.Looper; import android.os.Parcel; import android.os.ParcelFileDescriptor; import android.os.Parcelable; -import android.os.PersistableBundle; import android.os.Process; import android.os.RemoteException; import android.os.SystemClock; @@ -580,14 +581,6 @@ public class ConnectivityServiceTest { } private class TestNetworkAgentWrapper extends NetworkAgentWrapper { - private static final int VALIDATION_RESULT_BASE = NETWORK_VALIDATION_PROBE_DNS - | NETWORK_VALIDATION_PROBE_HTTP - | NETWORK_VALIDATION_PROBE_HTTPS; - private static final int VALIDATION_RESULT_VALID = VALIDATION_RESULT_BASE - | NETWORK_VALIDATION_RESULT_VALID; - private static final int VALIDATION_RESULT_PARTIAL = VALIDATION_RESULT_BASE - | NETWORK_VALIDATION_PROBE_FALLBACK - | NETWORK_VALIDATION_RESULT_PARTIAL; private static final int VALIDATION_RESULT_INVALID = 0; private static final long DATA_STALL_TIMESTAMP = 10L; @@ -595,12 +588,10 @@ public class ConnectivityServiceTest { private INetworkMonitor mNetworkMonitor; private INetworkMonitorCallbacks mNmCallbacks; - private int mNmValidationResult = VALIDATION_RESULT_BASE; + private int mNmValidationResult = VALIDATION_RESULT_INVALID; private int mProbesCompleted; private int mProbesSucceeded; private String mNmValidationRedirectUrl = null; - private PersistableBundle mValidationExtras = PersistableBundle.EMPTY; - private PersistableBundle mDataStallExtras = PersistableBundle.EMPTY; private boolean mNmProvNotificationRequested = false; private final ConditionVariable mNetworkStatusReceived = new ConditionVariable(); @@ -668,8 +659,13 @@ public class ConnectivityServiceTest { } mNmCallbacks.notifyProbeStatusChanged(mProbesCompleted, mProbesSucceeded); - mNmCallbacks.notifyNetworkTestedWithExtras( - mNmValidationResult, mNmValidationRedirectUrl, TIMESTAMP, mValidationExtras); + final NetworkTestResultParcelable p = new NetworkTestResultParcelable(); + p.result = mNmValidationResult; + p.probesAttempted = mProbesCompleted; + p.probesSucceeded = mProbesSucceeded; + p.redirectUrl = mNmValidationRedirectUrl; + p.timestampMillis = TIMESTAMP; + mNmCallbacks.notifyNetworkTestedWithExtras(p); if (mNmValidationRedirectUrl != null) { mNmCallbacks.showProvisioningNotification( @@ -751,9 +747,9 @@ public class ConnectivityServiceTest { } void setNetworkValid(boolean isStrictMode) { - mNmValidationResult = VALIDATION_RESULT_VALID; + mNmValidationResult = NETWORK_VALIDATION_RESULT_VALID; mNmValidationRedirectUrl = null; - int probesSucceeded = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTP; + int probesSucceeded = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS; if (isStrictMode) { probesSucceeded |= NETWORK_VALIDATION_PROBE_PRIVDNS; } @@ -765,8 +761,9 @@ public class ConnectivityServiceTest { void setNetworkInvalid(boolean isStrictMode) { mNmValidationResult = VALIDATION_RESULT_INVALID; mNmValidationRedirectUrl = null; - int probesCompleted = VALIDATION_RESULT_BASE; - int probesSucceeded = VALIDATION_RESULT_INVALID; + int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS + | NETWORK_VALIDATION_PROBE_HTTP; + int probesSucceeded = 0; // If the isStrictMode is true, it means the network is invalid when NetworkMonitor // tried to validate the private DNS but failed. if (isStrictMode) { @@ -782,7 +779,7 @@ public class ConnectivityServiceTest { mNmValidationRedirectUrl = redirectUrl; // Suppose the portal is found when NetworkMonitor probes NETWORK_VALIDATION_PROBE_HTTP // in the beginning, so the NETWORK_VALIDATION_PROBE_HTTPS hasn't probed yet. - int probesCompleted = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTPS; + int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTP; int probesSucceeded = VALIDATION_RESULT_INVALID; if (isStrictMode) { probesCompleted |= NETWORK_VALIDATION_PROBE_PRIVDNS; @@ -791,18 +788,20 @@ public class ConnectivityServiceTest { } void setNetworkPartial() { - mNmValidationResult = VALIDATION_RESULT_PARTIAL; + mNmValidationResult = NETWORK_VALIDATION_RESULT_PARTIAL; mNmValidationRedirectUrl = null; - int probesCompleted = VALIDATION_RESULT_BASE; - int probesSucceeded = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTPS; + int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS + | NETWORK_VALIDATION_PROBE_FALLBACK; + int probesSucceeded = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_FALLBACK; setProbesStatus(probesCompleted, probesSucceeded); } void setNetworkPartialValid(boolean isStrictMode) { setNetworkPartial(); - mNmValidationResult |= VALIDATION_RESULT_VALID; - int probesCompleted = VALIDATION_RESULT_BASE; - int probesSucceeded = VALIDATION_RESULT_BASE & ~NETWORK_VALIDATION_PROBE_HTTPS; + mNmValidationResult |= NETWORK_VALIDATION_RESULT_VALID; + int probesCompleted = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTPS + | NETWORK_VALIDATION_PROBE_HTTP; + int probesSucceeded = NETWORK_VALIDATION_PROBE_DNS | NETWORK_VALIDATION_PROBE_HTTP; // Suppose the partial network cannot pass the private DNS validation as well, so only // add NETWORK_VALIDATION_PROBE_DNS in probesCompleted but not probesSucceeded. if (isStrictMode) { @@ -838,8 +837,10 @@ public class ConnectivityServiceTest { } void notifyDataStallSuspected() throws Exception { - mNmCallbacks.notifyDataStallSuspected( - DATA_STALL_TIMESTAMP, DATA_STALL_DETECTION_METHOD, mDataStallExtras); + final DataStallReportParcelable p = new DataStallReportParcelable(); + p.detectionMethod = DATA_STALL_DETECTION_METHOD; + p.timestampMillis = DATA_STALL_TIMESTAMP; + mNmCallbacks.notifyDataStallSuspected(p); } } From 3cb1e030829a8327d34ef89e5d6d515c3a6fe94e Mon Sep 17 00:00:00 2001 From: Chiachang Wang Date: Wed, 29 Apr 2020 13:44:50 +0800 Subject: [PATCH 3/6] Add test prefix into test cases Bug: 152678151 Test: atest com.android.server.net.NetworkStatsFactoryTest Change-Id: I6e18915e383ac20072cb238d0136d7a8e4ceb811 Merged-In: I6e18915e383ac20072cb238d0136d7a8e4ceb811 --- .../server/net/NetworkStatsFactoryTest.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java b/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java index a21f5095c7..5ab2e323b6 100644 --- a/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java +++ b/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java @@ -104,7 +104,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnRewriteTrafficThroughItself() throws Exception { + public void testVpnRewriteTrafficThroughItself() throws Exception { VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; mFactory.updateVpnInfos(vpnInfos); @@ -133,7 +133,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithClat() throws Exception { + public void testVpnWithClat() throws Exception { VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {CLAT_PREFIX + TEST_IFACE})}; mFactory.updateVpnInfos(vpnInfos); mFactory.noteStackedIface(CLAT_PREFIX + TEST_IFACE, TEST_IFACE); @@ -166,7 +166,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithOneUnderlyingIface() throws Exception { + public void testVpnWithOneUnderlyingIface() throws Exception { VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; mFactory.updateVpnInfos(vpnInfos); @@ -189,7 +189,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithOneUnderlyingIfaceAndOwnTraffic() throws Exception { + public void testVpnWithOneUnderlyingIfaceAndOwnTraffic() throws Exception { // WiFi network is connected and VPN is using WiFi (which has TEST_IFACE). VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; mFactory.updateVpnInfos(vpnInfos); @@ -217,7 +217,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithOneUnderlyingIface_withCompression() throws Exception { + public void testVpnWithOneUnderlyingIface_withCompression() throws Exception { // WiFi network is connected and VPN is using WiFi (which has TEST_IFACE). VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; mFactory.updateVpnInfos(vpnInfos); @@ -238,7 +238,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithTwoUnderlyingIfaces_packetDuplication() throws Exception { + public void testVpnWithTwoUnderlyingIfaces_packetDuplication() throws Exception { // WiFi and Cell networks are connected and VPN is using WiFi (which has TEST_IFACE) and // Cell (which has TEST_IFACE2) and has declared both of them in its underlying network set. // Additionally, VPN is duplicating traffic across both WiFi and Cell. @@ -264,7 +264,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithTwoUnderlyingIfaces_splitTraffic() throws Exception { + public void testVpnWithTwoUnderlyingIfaces_splitTraffic() throws Exception { // WiFi and Cell networks are connected and VPN is using WiFi (which has TEST_IFACE) and // Cell (which has TEST_IFACE2) and has declared both of them in its underlying network set. // Additionally, VPN is arbitrarily splitting traffic across WiFi and Cell. @@ -291,7 +291,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithTwoUnderlyingIfaces_splitTrafficWithCompression() throws Exception { + public void testVpnWithTwoUnderlyingIfaces_splitTrafficWithCompression() throws Exception { // WiFi and Cell networks are connected and VPN is using WiFi (which has TEST_IFACE) and // Cell (which has TEST_IFACE2) and has declared both of them in its underlying network set. // Additionally, VPN is arbitrarily splitting compressed traffic across WiFi and Cell. @@ -314,7 +314,7 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { } @Test - public void vpnWithIncorrectUnderlyingIface() throws Exception { + public void testVpnWithIncorrectUnderlyingIface() throws Exception { // WiFi and Cell networks are connected and VPN is using Cell (which has TEST_IFACE2), // but has declared only WiFi (TEST_IFACE) in its underlying network set. VpnInfo[] vpnInfos = new VpnInfo[] {createVpnInfo(new String[] {TEST_IFACE})}; From bd162416785c0b157e600da633e27127927fffed Mon Sep 17 00:00:00 2001 From: Chiachang Wang Date: Tue, 28 Apr 2020 22:15:56 +0800 Subject: [PATCH 4/6] Filter debug entries for each vpn NetworkStats calculation needs to filter out debug entries to prevent over counting. While NetworkStatsFactory migrates data usage over a VPN to the TUN network, NetworkStatsFactory does not filter out debug entries per vpn which will cause debug entries left and cause exception. Bug: 152678151 Test: atest com.android.server.net.NetworkStatsFactoryTest and verify no exception Change-Id: I3525edc385b07858b48c7add2d331c4b5a2e84ad Merged-In: I3525edc385b07858b48c7add2d331c4b5a2e84ad --- .../server/net/NetworkStatsBaseTest.java | 9 ++++- .../server/net/NetworkStatsFactoryTest.java | 40 +++++++++++++++++++ .../raw/xt_qtaguid_vpn_one_underlying_two_vpn | 9 +++++ 3 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 tests/net/res/raw/xt_qtaguid_vpn_one_underlying_two_vpn diff --git a/tests/net/java/com/android/server/net/NetworkStatsBaseTest.java b/tests/net/java/com/android/server/net/NetworkStatsBaseTest.java index 28785f7c97..3aafe0b075 100644 --- a/tests/net/java/com/android/server/net/NetworkStatsBaseTest.java +++ b/tests/net/java/com/android/server/net/NetworkStatsBaseTest.java @@ -41,6 +41,7 @@ abstract class NetworkStatsBaseTest { static final String TEST_IFACE = "test0"; static final String TEST_IFACE2 = "test1"; static final String TUN_IFACE = "test_nss_tun0"; + static final String TUN_IFACE2 = "test_nss_tun1"; static final int UID_RED = 1001; static final int UID_BLUE = 1002; @@ -107,10 +108,14 @@ abstract class NetworkStatsBaseTest { assertEquals("unexpected operations", operations, entry.operations); } - VpnInfo createVpnInfo(String[] underlyingIfaces) { + static VpnInfo createVpnInfo(String[] underlyingIfaces) { + return createVpnInfo(TUN_IFACE, underlyingIfaces); + } + + static VpnInfo createVpnInfo(String vpnIface, String[] underlyingIfaces) { VpnInfo info = new VpnInfo(); info.ownerUid = UID_VPN; - info.vpnIface = TUN_IFACE; + info.vpnIface = vpnIface; info.underlyingIfaces = underlyingIfaces; return info; } diff --git a/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java b/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java index 5ab2e323b6..4473492d79 100644 --- a/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java +++ b/tests/net/java/com/android/server/net/NetworkStatsFactoryTest.java @@ -263,6 +263,46 @@ public class NetworkStatsFactoryTest extends NetworkStatsBaseTest { assertValues(tunStats, TEST_IFACE2, UID_VPN, 1200L, 100L, 1200L, 100L); } + @Test + public void testConcurrentVpns() throws Exception { + // Assume two VPNs are connected on two different network interfaces. VPN1 is using + // TEST_IFACE and VPN2 is using TEST_IFACE2. + final VpnInfo[] vpnInfos = new VpnInfo[] { + createVpnInfo(TUN_IFACE, new String[] {TEST_IFACE}), + createVpnInfo(TUN_IFACE2, new String[] {TEST_IFACE2})}; + mFactory.updateVpnInfos(vpnInfos); + + // create some traffic (assume 10 bytes of MTU for VPN interface and 1 byte encryption + // overhead per packet): + // 1000 bytes (100 packets) were sent, and 2000 bytes (200 packets) were received by UID_RED + // over VPN1. + // 700 bytes (70 packets) were sent, and 3000 bytes (300 packets) were received by UID_RED + // over VPN2. + // 500 bytes (50 packets) were sent, and 1000 bytes (100 packets) were received by UID_BLUE + // over VPN1. + // 250 bytes (25 packets) were sent, and 500 bytes (50 packets) were received by UID_BLUE + // over VPN2. + // VPN1 sent 1650 bytes (150 packets), and received 3300 (300 packets) over TEST_IFACE. + // Of 1650 bytes sent over WiFi, expect 1000 bytes attributed to UID_RED, 500 bytes + // attributed to UID_BLUE, and 150 bytes attributed to UID_VPN. + // Of 3300 bytes received over WiFi, expect 2000 bytes attributed to UID_RED, 1000 bytes + // attributed to UID_BLUE, and 300 bytes attributed to UID_VPN. + // VPN2 sent 1045 bytes (95 packets), and received 3850 (350 packets) over TEST_IFACE2. + // Of 1045 bytes sent over Cell, expect 700 bytes attributed to UID_RED, 250 bytes + // attributed to UID_BLUE, and 95 bytes attributed to UID_VPN. + // Of 3850 bytes received over Cell, expect 3000 bytes attributed to UID_RED, 500 bytes + // attributed to UID_BLUE, and 350 bytes attributed to UID_VPN. + final NetworkStats tunStats = + parseDetailedStats(R.raw.xt_qtaguid_vpn_one_underlying_two_vpn); + + assertValues(tunStats, TEST_IFACE, UID_RED, 2000L, 200L, 1000L, 100L); + assertValues(tunStats, TEST_IFACE, UID_BLUE, 1000L, 100L, 500L, 50L); + assertValues(tunStats, TEST_IFACE2, UID_RED, 3000L, 300L, 700L, 70L); + assertValues(tunStats, TEST_IFACE2, UID_BLUE, 500L, 50L, 250L, 25L); + assertValues(tunStats, TEST_IFACE, UID_VPN, 300L, 0L, 150L, 0L); + assertValues(tunStats, TEST_IFACE2, UID_VPN, 350L, 0L, 95L, 0L); + } + @Test public void testVpnWithTwoUnderlyingIfaces_splitTraffic() throws Exception { // WiFi and Cell networks are connected and VPN is using WiFi (which has TEST_IFACE) and diff --git a/tests/net/res/raw/xt_qtaguid_vpn_one_underlying_two_vpn b/tests/net/res/raw/xt_qtaguid_vpn_one_underlying_two_vpn new file mode 100644 index 0000000000..eb0513b100 --- /dev/null +++ b/tests/net/res/raw/xt_qtaguid_vpn_one_underlying_two_vpn @@ -0,0 +1,9 @@ +idx iface acct_tag_hex uid_tag_int cnt_set rx_bytes rx_packets tx_bytes tx_packets rx_tcp_bytes rx_tcp_packets rx_udp_bytes rx_udp_packets rx_other_bytes rx_other_packets tx_tcp_bytes tx_tcp_packets tx_udp_bytes tx_udp_packets tx_other_bytes tx_other_packets +2 test_nss_tun0 0x0 1001 0 2000 200 1000 100 0 0 0 0 0 0 0 0 0 0 0 0 +3 test_nss_tun0 0x0 1002 0 1000 100 500 50 0 0 0 0 0 0 0 0 0 0 0 0 +4 test_nss_tun1 0x0 1001 0 3000 300 700 70 0 0 0 0 0 0 0 0 0 0 0 0 +5 test_nss_tun1 0x0 1002 0 500 50 250 25 0 0 0 0 0 0 0 0 0 0 0 0 +6 test0 0x0 1004 0 3300 300 0 0 0 0 0 0 0 0 0 0 0 0 0 0 +7 test0 0x0 1004 1 0 0 1650 150 0 0 0 0 0 0 0 0 0 0 0 0 +8 test1 0x0 1004 0 3850 350 0 0 0 0 0 0 0 0 0 0 0 0 0 0 +9 test1 0x0 1004 1 0 0 1045 95 0 0 0 0 0 0 0 0 0 0 0 0 \ No newline at end of file From 0fff1ed7b909b448f5241c87c4d3804877761088 Mon Sep 17 00:00:00 2001 From: Benedict Wong Date: Tue, 28 Apr 2020 19:26:15 +0000 Subject: [PATCH 5/6] Add filtering for IPsec algorithms in IKEv2 VPNs This commit adds support for validating and filtering IPsec algorithms. Without a public API exposing IKEv2 algorithms (and their respective public APIs), the allowedAlgorithms can only filter the proposals for IPsec (Child) SA algorithms. Additionally, this removes the HMAC_SHA1 from the IKE SA's integrity algorithm proposals due to insecurity Bug: 153701879 Test: FrameworksNetTests passing, new tests added Change-Id: I7e61a1612692db275b751330af5bacbf86836a8c Merged-In: I7e61a1612692db275b751330af5bacbf86836a8c (cherry picked from commit 94e1c08a9ad4b0ff17e0f3a77fff0d3364040ba5) --- .../java/android/net/Ikev2VpnProfileTest.java | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/tests/net/java/android/net/Ikev2VpnProfileTest.java b/tests/net/java/android/net/Ikev2VpnProfileTest.java index 2273bc6122..ada5494efd 100644 --- a/tests/net/java/android/net/Ikev2VpnProfileTest.java +++ b/tests/net/java/android/net/Ikev2VpnProfileTest.java @@ -40,7 +40,10 @@ import java.security.KeyPair; import java.security.KeyPairGenerator; import java.security.PrivateKey; import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.Arrays; import java.util.Date; +import java.util.List; import java.util.concurrent.TimeUnit; import javax.security.auth.x500.X500Principal; @@ -106,6 +109,7 @@ public class Ikev2VpnProfileTest { assertTrue(profile.isBypassable()); assertTrue(profile.isMetered()); assertEquals(TEST_MTU, profile.getMaxMtu()); + assertEquals(Ikev2VpnProfile.DEFAULT_ALGORITHMS, profile.getAllowedAlgorithms()); } @Test @@ -159,6 +163,78 @@ public class Ikev2VpnProfileTest { assertNull(profile.getUserCert()); } + @Test + public void testBuildWithAllowedAlgorithmsAead() throws Exception { + final Ikev2VpnProfile.Builder builder = getBuilderWithDefaultOptions(); + builder.setAuthPsk(PSK_BYTES); + + List allowedAlgorithms = Arrays.asList(IpSecAlgorithm.AUTH_CRYPT_AES_GCM); + builder.setAllowedAlgorithms(allowedAlgorithms); + + final Ikev2VpnProfile profile = builder.build(); + assertEquals(allowedAlgorithms, profile.getAllowedAlgorithms()); + } + + @Test + public void testBuildWithAllowedAlgorithmsNormal() throws Exception { + final Ikev2VpnProfile.Builder builder = getBuilderWithDefaultOptions(); + builder.setAuthPsk(PSK_BYTES); + + List allowedAlgorithms = + Arrays.asList(IpSecAlgorithm.AUTH_HMAC_SHA512, IpSecAlgorithm.CRYPT_AES_CBC); + builder.setAllowedAlgorithms(allowedAlgorithms); + + final Ikev2VpnProfile profile = builder.build(); + assertEquals(allowedAlgorithms, profile.getAllowedAlgorithms()); + } + + @Test + public void testSetAllowedAlgorithmsEmptyList() throws Exception { + final Ikev2VpnProfile.Builder builder = getBuilderWithDefaultOptions(); + + try { + builder.setAllowedAlgorithms(new ArrayList<>()); + fail("Expected exception due to no valid algorithm set"); + } catch (IllegalArgumentException expected) { + } + } + + @Test + public void testSetAllowedAlgorithmsInvalidList() throws Exception { + final Ikev2VpnProfile.Builder builder = getBuilderWithDefaultOptions(); + List allowedAlgorithms = new ArrayList<>(); + + try { + builder.setAllowedAlgorithms(Arrays.asList(IpSecAlgorithm.AUTH_HMAC_SHA256)); + fail("Expected exception due to missing encryption"); + } catch (IllegalArgumentException expected) { + } + + try { + builder.setAllowedAlgorithms(Arrays.asList(IpSecAlgorithm.CRYPT_AES_CBC)); + fail("Expected exception due to missing authentication"); + } catch (IllegalArgumentException expected) { + } + } + + @Test + public void testSetAllowedAlgorithmsInsecureAlgorithm() throws Exception { + final Ikev2VpnProfile.Builder builder = getBuilderWithDefaultOptions(); + List allowedAlgorithms = new ArrayList<>(); + + try { + builder.setAllowedAlgorithms(Arrays.asList(IpSecAlgorithm.AUTH_HMAC_MD5)); + fail("Expected exception due to insecure algorithm"); + } catch (IllegalArgumentException expected) { + } + + try { + builder.setAllowedAlgorithms(Arrays.asList(IpSecAlgorithm.AUTH_HMAC_SHA1)); + fail("Expected exception due to insecure algorithm"); + } catch (IllegalArgumentException expected) { + } + } + @Test public void testBuildNoAuthMethodSet() throws Exception { final Ikev2VpnProfile.Builder builder = getBuilderWithDefaultOptions(); From 3d3c9f7b76751dda461c9252c52c005241efaa69 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Mon, 27 Apr 2020 08:08:57 +0000 Subject: [PATCH 6/6] Add comment / logging in NetworkMonitor callbacks Add a comment explaining the ordering of messages sent to the tracker and connectivity diagnostics handlers. Add a Slog.wtf call in case the deprecated notifyNetworkTested callback is called. Bug: 153500847 Test: atest ConnectivityServiceTest Merged-In: I2dbfc9bf7b2f785ea4594851bd354e9fd0fc0bd1 Change-Id: I2dbfc9bf7b2f785ea4594851bd354e9fd0fc0bd1 --- services/core/java/com/android/server/ConnectivityService.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index e4eb585e98..2c63c6f8a6 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -3011,10 +3011,13 @@ public class ConnectivityService extends IConnectivityManager.Stub // Legacy version of notifyNetworkTestedWithExtras. // Would only be called if the system has a NetworkStack module older than the // framework, which does not happen in practice. + Slog.wtf(TAG, "Deprecated notifyNetworkTested called: no action taken"); } @Override public void notifyNetworkTestedWithExtras(NetworkTestResultParcelable p) { + // Notify mTrackerHandler and mConnectivityDiagnosticsHandler of the event. Both use + // the same looper so messages will be processed in sequence. final Message msg = mTrackerHandler.obtainMessage( EVENT_NETWORK_TESTED, new NetworkTestedResults(