From fd43392b25de36f93027571d95c5bcb2a2a59d7b Mon Sep 17 00:00:00 2001 From: Lorenzo Colitti Date: Tue, 28 Jul 2020 09:18:46 +0000 Subject: [PATCH 1/4] Set the NetworkInfo subtype to 0. For non-telephony networks, this was always set to 0 before R. In R, it is currently set to the same value as the network type. This is incorrect because the two have different namespaces. or example, currently, any network of type WIFI (==1) will have a subtype of NETWORK_TYPE_GPRS (==1). Similarly, all ETHERNET networks will have subtype NETWORK_TYPE_1XRTT, all VPN networks will have a subtype of NETWORK_TYPE_TD_SCDMA, etd. Bug: 161653721 Test: builds, boots Change-Id: I07e111c1762e0021c931cefc27f193f78578748b --- core/java/android/net/NetworkAgent.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/core/java/android/net/NetworkAgent.java b/core/java/android/net/NetworkAgent.java index 482d2d2192..327e42bdd2 100644 --- a/core/java/android/net/NetworkAgent.java +++ b/core/java/android/net/NetworkAgent.java @@ -354,8 +354,7 @@ public abstract class NetworkAgent { private static NetworkInfo getLegacyNetworkInfo(final NetworkAgentConfig config) { // The subtype can be changed with (TODO) setLegacySubtype, but it starts // with the type and an empty description. - final NetworkInfo ni = new NetworkInfo(config.legacyType, config.legacyType, - config.legacyTypeName, ""); + final NetworkInfo ni = new NetworkInfo(config.legacyType, 0, config.legacyTypeName, ""); ni.setIsAvailable(true); ni.setExtraInfo(config.getLegacyExtraInfo()); return ni; From 2349cf350bad08e2fef85e28bd5e3bf39f1f1093 Mon Sep 17 00:00:00 2001 From: Alex Light Date: Thu, 30 Jul 2020 23:47:48 +0000 Subject: [PATCH 2/4] Update language to comply with Android's inclusive language guidance See https://source.android.com/setup/contribute/respectful-code for reference Bug: 162536543 Test: Treehugger Change-Id: I971050a2665c177870ff257bd0f41343db702892 --- core/jni/android_net_NetUtils.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/jni/android_net_NetUtils.cpp b/core/jni/android_net_NetUtils.cpp index e56809f66d..8d4c4e5311 100644 --- a/core/jni/android_net_NetUtils.cpp +++ b/core/jni/android_net_NetUtils.cpp @@ -93,13 +93,13 @@ static void android_net_utils_attachDropAllBPFFilter(JNIEnv *env, jobject clazz, static void android_net_utils_detachBPFFilter(JNIEnv *env, jobject clazz, jobject javaFd) { - int dummy = 0; + int optval_ignored = 0; int fd = jniGetFDFromFileDescriptor(env, javaFd); - if (setsockopt(fd, SOL_SOCKET, SO_DETACH_FILTER, &dummy, sizeof(dummy)) != 0) { + if (setsockopt(fd, SOL_SOCKET, SO_DETACH_FILTER, &optval_ignored, sizeof(optval_ignored)) != + 0) { jniThrowExceptionFmt(env, "java/net/SocketException", "setsockopt(SO_DETACH_FILTER): %s", strerror(errno)); } - } static jboolean android_net_utils_bindProcessToNetwork(JNIEnv *env, jobject thiz, jint netId) From 3103a6ee0381f73d188a840b542feead87e0ac33 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 6 Aug 2020 17:11:25 +0000 Subject: [PATCH 3/4] Add a test for starting the legacy VPN. The legacy VPN has, among many parameters, a host to connect to. This host can be specified as a numeric address, or as a hostname. When it's a name, resolution is required. Currently, name resolution is performed by the native VPN daemons racoon and mtpd. When a hostname is used, the framework does not know the IP address of the VPN server and does not add a throw route for the VPN server IP address. On older kernels this does not matter because the legacy PPP kernel code binds the PPP socket to the right network, but on newer devices that use the upstream PPP code, this does not work. See b/133797637. This patch instruments the legacy VPN code so that it can be run in tests, and uses this instrumentation to simulate passing a configuration that contains a host, and verifies that the arguments passed to the mptd and racoon daemons receive the expected server address, and that the expected throw route is correctly installed. It then adds two tests : one specifying the server as a numeric address, and one as a hostname. As the resolution is currently broken, the latter of these tests is added disabled, and the followup fix to the issue enables it. This test is basic and very targeted, but it's what we need right now. Also there are plans to remove this entire code path in S, so the test being ad-hoc is not much of a problem. Test: this Bug: 158974172 Change-Id: I96f4bbb9b109e3e5813d083bed1989d88fb156b8 Merged-In: I3c4a94181bd71df68121fa0f71669fa4fa588bdd (cherry picked from commit dece7f3f74cb67f2a046f3a2a9757b559abc2aac) --- .../android/server/connectivity/VpnTest.java | 191 +++++++++++++++++- 1 file changed, 182 insertions(+), 9 deletions(-) diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index 4ccf79a0cb..a9313a30c1 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -30,6 +30,7 @@ import static android.net.NetworkCapabilities.TRANSPORT_CELLULAR; import static android.net.NetworkCapabilities.TRANSPORT_VPN; import static android.net.NetworkCapabilities.TRANSPORT_WIFI; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -49,6 +50,7 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import android.annotation.NonNull; import android.annotation.UserIdInt; import android.app.AppOpsManager; import android.app.NotificationManager; @@ -65,6 +67,7 @@ import android.net.InetAddresses; import android.net.IpPrefix; import android.net.IpSecManager; import android.net.LinkProperties; +import android.net.LocalSocket; import android.net.Network; import android.net.NetworkCapabilities; import android.net.NetworkInfo.DetailedState; @@ -74,6 +77,7 @@ import android.net.VpnManager; import android.net.VpnService; import android.os.Build.VERSION_CODES; import android.os.Bundle; +import android.os.ConditionVariable; import android.os.INetworkManagementService; import android.os.Looper; import android.os.Process; @@ -94,6 +98,7 @@ import com.android.internal.net.VpnProfile; import com.android.server.IpSecService; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Answers; @@ -101,13 +106,20 @@ import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockitoAnnotations; +import java.io.BufferedWriter; +import java.io.File; +import java.io.FileWriter; +import java.io.IOException; import java.net.Inet4Address; +import java.net.InetAddress; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; import java.util.Set; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; import java.util.stream.Stream; /** @@ -133,7 +145,8 @@ public class VpnTest { managedProfileA.profileGroupId = primaryUser.id; } - static final String TEST_VPN_PKG = "com.dummy.vpn"; + static final String EGRESS_IFACE = "wlan0"; + static final String TEST_VPN_PKG = "com.testvpn.vpn"; private static final String TEST_VPN_SERVER = "1.2.3.4"; private static final String TEST_VPN_IDENTITY = "identity"; private static final byte[] TEST_VPN_PSK = "psk".getBytes(); @@ -1012,31 +1025,191 @@ public class VpnTest { // a subsequent CL. } - @Test - public void testStartLegacyVpn() throws Exception { + public Vpn startLegacyVpn(final VpnProfile vpnProfile) throws Exception { final Vpn vpn = createVpn(primaryUser.id); setMockedUsers(primaryUser); // Dummy egress interface - final String egressIface = "DUMMY0"; final LinkProperties lp = new LinkProperties(); - lp.setInterfaceName(egressIface); + lp.setInterfaceName(EGRESS_IFACE); final RouteInfo defaultRoute = new RouteInfo(new IpPrefix(Inet4Address.ANY, 0), - InetAddresses.parseNumericAddress("192.0.2.0"), egressIface); + InetAddresses.parseNumericAddress("192.0.2.0"), EGRESS_IFACE); lp.addRoute(defaultRoute); - vpn.startLegacyVpn(mVpnProfile, mKeyStore, lp); + vpn.startLegacyVpn(vpnProfile, mKeyStore, lp); + return vpn; + } + @Test + public void testStartPlatformVpn() throws Exception { + startLegacyVpn(mVpnProfile); // TODO: Test the Ikev2VpnRunner started up properly. Relies on utility methods added in - // a subsequent CL. + // a subsequent patch. + } + + @Test + public void testStartRacoonNumericAddress() throws Exception { + startRacoon("1.2.3.4", "1.2.3.4"); + } + + @Test + @Ignore("b/158974172") // remove when the bug is fixed + public void testStartRacoonHostname() throws Exception { + startRacoon("hostname", "5.6.7.8"); // address returned by deps.resolve + } + + public void startRacoon(final String serverAddr, final String expectedAddr) + throws Exception { + final ConditionVariable legacyRunnerReady = new ConditionVariable(); + final VpnProfile profile = new VpnProfile("testProfile" /* key */); + profile.type = VpnProfile.TYPE_L2TP_IPSEC_PSK; + profile.name = "testProfileName"; + profile.username = "userName"; + profile.password = "thePassword"; + profile.server = serverAddr; + profile.ipsecIdentifier = "id"; + profile.ipsecSecret = "secret"; + profile.l2tpSecret = "l2tpsecret"; + when(mConnectivityManager.getAllNetworks()) + .thenReturn(new Network[] { new Network(101) }); + when(mConnectivityManager.registerNetworkAgent(any(), any(), any(), any(), + anyInt(), any(), anyInt())).thenAnswer(invocation -> { + // The runner has registered an agent and is now ready. + legacyRunnerReady.open(); + return new Network(102); + }); + final Vpn vpn = startLegacyVpn(profile); + final TestDeps deps = (TestDeps) vpn.mDeps; + try { + // udppsk and 1701 are the values for TYPE_L2TP_IPSEC_PSK + assertArrayEquals( + new String[] { EGRESS_IFACE, expectedAddr, "udppsk", + profile.ipsecIdentifier, profile.ipsecSecret, "1701" }, + deps.racoonArgs.get(10, TimeUnit.SECONDS)); + // literal values are hardcoded in Vpn.java for mtpd args + assertArrayEquals( + new String[] { EGRESS_IFACE, "l2tp", expectedAddr, "1701", profile.l2tpSecret, + "name", profile.username, "password", profile.password, + "linkname", "vpn", "refuse-eap", "nodefaultroute", "usepeerdns", + "idle", "1800", "mtu", "1400", "mru", "1400" }, + deps.mtpdArgs.get(10, TimeUnit.SECONDS)); + // Now wait for the runner to be ready before testing for the route. + legacyRunnerReady.block(10_000); + // In this test the expected address is always v4 so /32 + final RouteInfo expectedRoute = new RouteInfo(new IpPrefix(expectedAddr + "/32"), + RouteInfo.RTN_THROW); + assertTrue("Routes lack the expected throw route (" + expectedRoute + ") : " + + vpn.mConfig.routes, + vpn.mConfig.routes.contains(expectedRoute)); + } finally { + // Now interrupt the thread, unblock the runner and clean up. + vpn.mVpnRunner.exitVpnRunner(); + deps.getStateFile().delete(); // set to delete on exit, but this deletes it earlier + vpn.mVpnRunner.join(10_000); // wait for up to 10s for the runner to die and cleanup + } + } + + private static final class TestDeps extends Vpn.Dependencies { + public final CompletableFuture racoonArgs = new CompletableFuture(); + public final CompletableFuture mtpdArgs = new CompletableFuture(); + public final File mStateFile; + + private final HashMap mRunningServices = new HashMap<>(); + + TestDeps() { + try { + mStateFile = File.createTempFile("vpnTest", ".tmp"); + mStateFile.deleteOnExit(); + } catch (final IOException e) { + throw new RuntimeException(e); + } + } + + @Override + public void startService(final String serviceName) { + mRunningServices.put(serviceName, true); + } + + @Override + public void stopService(final String serviceName) { + mRunningServices.put(serviceName, false); + } + + @Override + public boolean isServiceRunning(final String serviceName) { + return mRunningServices.getOrDefault(serviceName, false); + } + + @Override + public boolean isServiceStopped(final String serviceName) { + return !isServiceRunning(serviceName); + } + + @Override + public File getStateFile() { + return mStateFile; + } + + @Override + public void sendArgumentsToDaemon( + final String daemon, final LocalSocket socket, final String[] arguments, + final Vpn.RetryScheduler interruptChecker) throws IOException { + if ("racoon".equals(daemon)) { + racoonArgs.complete(arguments); + } else if ("mtpd".equals(daemon)) { + writeStateFile(arguments); + mtpdArgs.complete(arguments); + } else { + throw new UnsupportedOperationException("Unsupported daemon : " + daemon); + } + } + + private void writeStateFile(final String[] arguments) throws IOException { + mStateFile.delete(); + mStateFile.createNewFile(); + mStateFile.deleteOnExit(); + final BufferedWriter writer = new BufferedWriter( + new FileWriter(mStateFile, false /* append */)); + writer.write(EGRESS_IFACE); + writer.write("\n"); + // addresses + writer.write("10.0.0.1/24\n"); + // routes + writer.write("192.168.6.0/24\n"); + // dns servers + writer.write("192.168.6.1\n"); + // search domains + writer.write("vpn.searchdomains.com\n"); + // endpoint - intentionally empty + writer.write("\n"); + writer.flush(); + writer.close(); + } + + @Override + @NonNull + public InetAddress resolve(final String endpoint) { + try { + // If a numeric IP address, return it. + return InetAddress.parseNumericAddress(endpoint); + } catch (IllegalArgumentException e) { + // Otherwise, return some token IP to test for. + return InetAddress.parseNumericAddress("5.6.7.8"); + } + } + + @Override + public boolean checkInterfacePresent(final Vpn vpn, final String iface) { + return true; + } } /** * Mock some methods of vpn object. */ private Vpn createVpn(@UserIdInt int userId) { - return new Vpn(Looper.myLooper(), mContext, mNetService, + return new Vpn(Looper.myLooper(), mContext, new TestDeps(), mNetService, userId, mKeyStore, mSystemServices, mIkev2SessionCreator); } From 75d9e28a07abfcceeb1268d402c2aa0cba7e36c2 Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 6 Aug 2020 17:12:16 +0000 Subject: [PATCH 4/4] Resolve the endpoint in legacy VPN This adds code to resolve the endpoint in the legacy VPN runner if it was specified as a hostname, and enables the previously added test that was disabled because this was broken until this patch. See the linked bug for details. This patch uses the async DNS API to do the resolution. This lets the resolution be fully cancellable, though the code is more complex than with the non-interruptible getByName. Test: VpnTest and in particular VpnTest#testStartRacoon Fixes the test meant to test this Also manual testing that resolution of a real hostname works as expected, that failure to resolve returns correctly, and that cancellation/interruption will unblock the thread and terminate immediately. Bug: 158974172 Change-Id: I90bec6d85706fa9b2f9a01f81701138a54347005 Merged-In: I96691f6091c43377f23a00621242ed034fcb0444 (cherry picked from commit 8ab570d9c9eb5e52b2c038818e3e4d1d3b98fda0) --- tests/net/java/com/android/server/connectivity/VpnTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index a9313a30c1..de1c5759ee 100644 --- a/tests/net/java/com/android/server/connectivity/VpnTest.java +++ b/tests/net/java/com/android/server/connectivity/VpnTest.java @@ -98,7 +98,6 @@ import com.android.internal.net.VpnProfile; import com.android.server.IpSecService; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Answers; @@ -1054,7 +1053,6 @@ public class VpnTest { } @Test - @Ignore("b/158974172") // remove when the bug is fixed public void testStartRacoonHostname() throws Exception { startRacoon("hostname", "5.6.7.8"); // address returned by deps.resolve }