From c9e026f4e9db93288d1f1aa83970000aa3ad55cb Mon Sep 17 00:00:00 2001 From: Chalard Jean Date: Thu, 6 Aug 2020 17:11:25 +0000 Subject: [PATCH] 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) --- .../com/android/server/connectivity/Vpn.java | 161 ++++++++++----- .../android/server/connectivity/VpnTest.java | 191 +++++++++++++++++- 2 files changed, 290 insertions(+), 62 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 1f85d1046523f..476e4b7c67a30 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -123,6 +123,7 @@ import java.math.BigInteger; import java.net.Inet4Address; import java.net.Inet6Address; import java.net.InetAddress; +import java.net.UnknownHostException; import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; import java.util.ArrayList; @@ -190,6 +191,7 @@ public class Vpn { // automated reconnection private final Context mContext; + @VisibleForTesting final Dependencies mDeps; private final NetworkInfo mNetworkInfo; @VisibleForTesting protected String mPackage; private int mOwnerUID; @@ -252,17 +254,106 @@ public class Vpn { // Handle of the user initiating VPN. private final int mUserHandle; + interface RetryScheduler { + void checkInterruptAndDelay(boolean sleepLonger) throws InterruptedException; + } + + static class Dependencies { + public void startService(final String serviceName) { + SystemService.start(serviceName); + } + + public void stopService(final String serviceName) { + SystemService.stop(serviceName); + } + + public boolean isServiceRunning(final String serviceName) { + return SystemService.isRunning(serviceName); + } + + public boolean isServiceStopped(final String serviceName) { + return SystemService.isStopped(serviceName); + } + + public File getStateFile() { + return new File("/data/misc/vpn/state"); + } + + public void sendArgumentsToDaemon( + final String daemon, final LocalSocket socket, final String[] arguments, + final RetryScheduler retryScheduler) throws IOException, InterruptedException { + final LocalSocketAddress address = new LocalSocketAddress( + daemon, LocalSocketAddress.Namespace.RESERVED); + + // Wait for the socket to connect. + while (true) { + try { + socket.connect(address); + break; + } catch (Exception e) { + // ignore + } + retryScheduler.checkInterruptAndDelay(true /* sleepLonger */); + } + socket.setSoTimeout(500); + + final OutputStream out = socket.getOutputStream(); + for (String argument : arguments) { + byte[] bytes = argument.getBytes(StandardCharsets.UTF_8); + if (bytes.length >= 0xFFFF) { + throw new IllegalArgumentException("Argument is too large"); + } + out.write(bytes.length >> 8); + out.write(bytes.length); + out.write(bytes); + retryScheduler.checkInterruptAndDelay(false /* sleepLonger */); + } + out.write(0xFF); + out.write(0xFF); + + // Wait for End-of-File. + final InputStream in = socket.getInputStream(); + while (true) { + try { + if (in.read() == -1) { + break; + } + } catch (Exception e) { + // ignore + } + retryScheduler.checkInterruptAndDelay(true /* sleepLonger */); + } + } + + // TODO : implement and use this. + @NonNull + public InetAddress resolve(final String endpoint) throws UnknownHostException { + try { + return InetAddress.parseNumericAddress(endpoint); + } catch (IllegalArgumentException e) { + Log.e(TAG, "Endpoint is not numeric"); + } + throw new UnknownHostException(endpoint); + } + + public boolean checkInterfacePresent(final Vpn vpn, final String iface) { + return vpn.jniCheck(iface) == 0; + } + } + public Vpn(Looper looper, Context context, INetworkManagementService netService, @UserIdInt int userHandle, @NonNull KeyStore keyStore) { - this(looper, context, netService, userHandle, keyStore, + this(looper, context, new Dependencies(), netService, userHandle, keyStore, new SystemServices(context), new Ikev2SessionCreator()); } @VisibleForTesting - protected Vpn(Looper looper, Context context, INetworkManagementService netService, + protected Vpn(Looper looper, Context context, Dependencies deps, + INetworkManagementService netService, int userHandle, @NonNull KeyStore keyStore, SystemServices systemServices, Ikev2SessionCreator ikev2SessionCreator) { mContext = context; + mDeps = deps; mNetd = netService; mUserHandle = userHandle; mLooper = looper; @@ -2129,7 +2220,8 @@ public class Vpn { } /** This class represents the common interface for all VPN runners. */ - private abstract class VpnRunner extends Thread { + @VisibleForTesting + abstract class VpnRunner extends Thread { protected VpnRunner(String name) { super(name); @@ -2638,7 +2730,7 @@ public class Vpn { } catch (InterruptedException e) { } for (String daemon : mDaemons) { - SystemService.stop(daemon); + mDeps.stopService(daemon); } } agentDisconnect(); @@ -2663,13 +2755,13 @@ public class Vpn { // Wait for the daemons to stop. for (String daemon : mDaemons) { - while (!SystemService.isStopped(daemon)) { + while (!mDeps.isServiceStopped(daemon)) { checkInterruptAndDelay(true); } } // Clear the previous state. - File state = new File("/data/misc/vpn/state"); + final File state = mDeps.getStateFile(); state.delete(); if (state.exists()) { throw new IllegalStateException("Cannot delete the state"); @@ -2696,57 +2788,19 @@ public class Vpn { // Start the daemon. String daemon = mDaemons[i]; - SystemService.start(daemon); + mDeps.startService(daemon); // Wait for the daemon to start. - while (!SystemService.isRunning(daemon)) { + while (!mDeps.isServiceRunning(daemon)) { checkInterruptAndDelay(true); } // Create the control socket. mSockets[i] = new LocalSocket(); - LocalSocketAddress address = new LocalSocketAddress( - daemon, LocalSocketAddress.Namespace.RESERVED); - // Wait for the socket to connect. - while (true) { - try { - mSockets[i].connect(address); - break; - } catch (Exception e) { - // ignore - } - checkInterruptAndDelay(true); - } - mSockets[i].setSoTimeout(500); - - // Send over the arguments. - OutputStream out = mSockets[i].getOutputStream(); - for (String argument : arguments) { - byte[] bytes = argument.getBytes(StandardCharsets.UTF_8); - if (bytes.length >= 0xFFFF) { - throw new IllegalArgumentException("Argument is too large"); - } - out.write(bytes.length >> 8); - out.write(bytes.length); - out.write(bytes); - checkInterruptAndDelay(false); - } - out.write(0xFF); - out.write(0xFF); - - // Wait for End-of-File. - InputStream in = mSockets[i].getInputStream(); - while (true) { - try { - if (in.read() == -1) { - break; - } - } catch (Exception e) { - // ignore - } - checkInterruptAndDelay(true); - } + // Wait for the socket to connect and send over the arguments. + mDeps.sendArgumentsToDaemon(daemon, mSockets[i], arguments, + this::checkInterruptAndDelay); } // Wait for the daemons to create the new state. @@ -2754,7 +2808,7 @@ public class Vpn { // Check if a running daemon is dead. for (int i = 0; i < mDaemons.length; ++i) { String daemon = mDaemons[i]; - if (mArguments[i] != null && !SystemService.isRunning(daemon)) { + if (mArguments[i] != null && !mDeps.isServiceRunning(daemon)) { throw new IllegalStateException(daemon + " is dead"); } } @@ -2764,7 +2818,8 @@ public class Vpn { // Now we are connected. Read and parse the new state. String[] parameters = FileUtils.readTextFile(state, 0, null).split("\n", -1); if (parameters.length != 7) { - throw new IllegalStateException("Cannot parse the state"); + throw new IllegalStateException("Cannot parse the state: '" + + String.join("', '", parameters) + "'"); } // Set the interface and the addresses in the config. @@ -2818,7 +2873,7 @@ public class Vpn { checkInterruptAndDelay(false); // Check if the interface is gone while we are waiting. - if (jniCheck(mConfig.interfaze) == 0) { + if (mDeps.checkInterfacePresent(Vpn.this, mConfig.interfaze)) { throw new IllegalStateException(mConfig.interfaze + " is gone"); } @@ -2849,7 +2904,7 @@ public class Vpn { while (true) { Thread.sleep(2000); for (int i = 0; i < mDaemons.length; i++) { - if (mArguments[i] != null && SystemService.isStopped(mDaemons[i])) { + if (mArguments[i] != null && mDeps.isServiceStopped(mDaemons[i])) { return; } } diff --git a/tests/net/java/com/android/server/connectivity/VpnTest.java b/tests/net/java/com/android/server/connectivity/VpnTest.java index 4ccf79a0cb372..a9313a30c1d0f 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); }