From a54db145e6c9718f211c61eaa46d0f80832ecbe8 Mon Sep 17 00:00:00 2001 From: Yan Yan Date: Thu, 2 Jun 2022 20:29:13 +0000 Subject: [PATCH 1/2] Make VPN more testable and update NC during network change This commit: - Updates the NetworkCapabilities with new underlying network during IKE Session re-establishment - Create wrapper classes for IkeSession - Allow injecting executor and NetworkAgent with the Dependencies class Bug: 192077544 Test: atest VpnTest(new tests) Change-Id: Ic98e3906c2b02fa8b9f4a8e1366b1dd8a363bb47 (cherry picked from commit 673162c6422fc121f8f5750c52e5a817161f819a) Merged-In: Ic98e3906c2b02fa8b9f4a8e1366b1dd8a363bb47 --- .../com/android/server/connectivity/Vpn.java | 94 ++++++++++++++----- 1 file changed, 72 insertions(+), 22 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 86b8d328aa062..7a544d977fde0 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -498,6 +498,29 @@ public class Vpn { return IKEV2_VPN_RETRY_DELAYS_SEC[retryCount]; } } + + /** Get single threaded executor for IKEv2 VPN */ + public ScheduledThreadPoolExecutor newScheduledThreadPoolExecutor() { + return new ScheduledThreadPoolExecutor(1); + } + + /** Get a NetworkAgent instance */ + public NetworkAgent newNetworkAgent( + @NonNull Context context, + @NonNull Looper looper, + @NonNull String logTag, + @NonNull NetworkCapabilities nc, + @NonNull LinkProperties lp, + @NonNull NetworkScore score, + @NonNull NetworkAgentConfig config, + @Nullable NetworkProvider provider) { + return new NetworkAgent(context, looper, logTag, nc, lp, score, config, provider) { + @Override + public void onNetworkUnwanted() { + // We are user controlled, not driven by NetworkRequest. + } + }; + } } public Vpn(Looper looper, Context context, INetworkManagementService netService, INetd netd, @@ -1474,15 +1497,10 @@ public class Vpn { ? Arrays.asList(mConfig.underlyingNetworks) : null); mNetworkCapabilities = capsBuilder.build(); - mNetworkAgent = new NetworkAgent(mContext, mLooper, NETWORKTYPE /* logtag */, + mNetworkAgent = mDeps.newNetworkAgent(mContext, mLooper, NETWORKTYPE /* logtag */, mNetworkCapabilities, lp, new NetworkScore.Builder().setLegacyInt(VPN_DEFAULT_SCORE).build(), - networkAgentConfig, mNetworkProvider) { - @Override - public void onNetworkUnwanted() { - // We are user controlled, not driven by NetworkRequest. - } - }; + networkAgentConfig, mNetworkProvider); final long token = Binder.clearCallingIdentity(); try { mNetworkAgent.register(); @@ -2692,8 +2710,7 @@ public class Vpn { * of the mutable Ikev2VpnRunner fields. The Ikev2VpnRunner is built mostly lock-free by * virtue of everything being serialized on this executor. */ - @NonNull - private final ScheduledThreadPoolExecutor mExecutor = new ScheduledThreadPoolExecutor(1); + @NonNull private final ScheduledThreadPoolExecutor mExecutor; @Nullable private ScheduledFuture mScheduledHandleNetworkLostTimeout; @Nullable private ScheduledFuture mScheduledHandleRetryIkeSessionTimeout; @@ -2714,7 +2731,7 @@ public class Vpn { @Nullable private LinkProperties mUnderlyingLinkProperties; private final String mSessionKey; - @Nullable private IkeSession mSession; + @Nullable private IkeSessionWrapper mSession; @Nullable private IkeSessionConnectionInfo mIkeConnectionInfo; // mMobikeEnabled can only be updated after IKE AUTH is finished. @@ -2728,9 +2745,11 @@ public class Vpn { */ private int mRetryCount = 0; - IkeV2VpnRunner(@NonNull Ikev2VpnProfile profile) { + IkeV2VpnRunner( + @NonNull Ikev2VpnProfile profile, @NonNull ScheduledThreadPoolExecutor executor) { super(TAG); mProfile = profile; + mExecutor = executor; mIpSecManager = (IpSecManager) mContext.getSystemService(Context.IPSEC_SERVICE); mNetworkCallback = new VpnIkev2Utils.Ikev2VpnNetworkCallback(TAG, this, mExecutor); mSessionKey = UUID.randomUUID().toString(); @@ -2743,7 +2762,7 @@ public class Vpn { // To avoid hitting RejectedExecutionException upon shutdown of the mExecutor */ mExecutor.setRejectedExecutionHandler( - (r, executor) -> { + (r, exe) -> { Log.d(TAG, "Runnable " + r + " rejected by the mExecutor"); }); } @@ -2884,7 +2903,6 @@ public class Vpn { mConfig.dnsServers.addAll(dnsAddrStrings); mConfig.underlyingNetworks = new Network[] {network}; - mConfig.disallowedApplications = getAppExclusionList(mPackage); networkAgent = mNetworkAgent; @@ -2900,6 +2918,10 @@ public class Vpn { } else { // Underlying networks also set in agentConnect() networkAgent.setUnderlyingNetworks(Collections.singletonList(network)); + mNetworkCapabilities = + new NetworkCapabilities.Builder(mNetworkCapabilities) + .setUnderlyingNetworks(Collections.singletonList(network)) + .build(); } lp = makeLinkProperties(); // Accesses VPN instance fields; must be locked @@ -4015,7 +4037,9 @@ public class Vpn { case VpnProfile.TYPE_IKEV2_IPSEC_RSA: case VpnProfile.TYPE_IKEV2_FROM_IKE_TUN_CONN_PARAMS: mVpnRunner = - new IkeV2VpnRunner(Ikev2VpnProfile.fromVpnProfile(profile)); + new IkeV2VpnRunner( + Ikev2VpnProfile.fromVpnProfile(profile), + mDeps.newScheduledThreadPoolExecutor()); mVpnRunner.start(); break; default: @@ -4185,6 +4209,31 @@ public class Vpn { return isCurrentIkev2VpnLocked(packageName) ? makeVpnProfileStateLocked() : null; } + /** + * Proxy to allow testing + * + * @hide + */ + @VisibleForTesting + public static class IkeSessionWrapper { + private final IkeSession mImpl; + + /** Create an IkeSessionWrapper */ + public IkeSessionWrapper(IkeSession session) { + mImpl = session; + } + + /** Update the underlying network of the IKE Session */ + public void setNetwork(@NonNull Network network) { + mImpl.setNetwork(network); + } + + /** Forcibly terminate the IKE Session */ + public void kill() { + mImpl.kill(); + } + } + /** * Proxy to allow testing * @@ -4193,20 +4242,21 @@ public class Vpn { @VisibleForTesting public static class Ikev2SessionCreator { /** Creates a IKE session */ - public IkeSession createIkeSession( + public IkeSessionWrapper createIkeSession( @NonNull Context context, @NonNull IkeSessionParams ikeSessionParams, @NonNull ChildSessionParams firstChildSessionParams, @NonNull Executor userCbExecutor, @NonNull IkeSessionCallback ikeSessionCallback, @NonNull ChildSessionCallback firstChildSessionCallback) { - return new IkeSession( - context, - ikeSessionParams, - firstChildSessionParams, - userCbExecutor, - ikeSessionCallback, - firstChildSessionCallback); + return new IkeSessionWrapper( + new IkeSession( + context, + ikeSessionParams, + firstChildSessionParams, + userCbExecutor, + ikeSessionCallback, + firstChildSessionCallback)); } } From 3c499c5c0af308456a85098c8433bf5eff29fa5a Mon Sep 17 00:00:00 2001 From: Yan Yan Date: Thu, 9 Jun 2022 22:16:23 +0000 Subject: [PATCH 2/2] Minor cleanups in VPN This commit includes few minor cleanups in VPN for better readability and maintainability. Specifically this commit: - explicitly sets IPsec tunnel's underlying network when Child SA is created - nulls out ScheduledFuture when the task is finished or cancelled - validates the IKE Session token before executing the scheduled handleSessionLost call Bug: 192077544 Test: atest VpnTest, IkeV2VpnTest Change-Id: Ib3cbdbfa594c55c27b78dffc00a82d371ca7a749 (cherry picked from commit 34917963368a6c67620e345c6d28a8d62fbd6b8c) Merged-In: Ib3cbdbfa594c55c27b78dffc00a82d371ca7a749 --- .../com/android/server/connectivity/Vpn.java | 55 +++++++++++-------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/Vpn.java b/services/core/java/com/android/server/connectivity/Vpn.java index 7a544d977fde0..af15735e92366 100644 --- a/services/core/java/com/android/server/connectivity/Vpn.java +++ b/services/core/java/com/android/server/connectivity/Vpn.java @@ -2712,8 +2712,8 @@ public class Vpn { */ @NonNull private final ScheduledThreadPoolExecutor mExecutor; - @Nullable private ScheduledFuture mScheduledHandleNetworkLostTimeout; - @Nullable private ScheduledFuture mScheduledHandleRetryIkeSessionTimeout; + @Nullable private ScheduledFuture mScheduledHandleNetworkLostFuture; + @Nullable private ScheduledFuture mScheduledHandleRetryIkeSessionFuture; /** Signal to ensure shutdown is honored even if a new Network is connected. */ private boolean mIsRunning = true; @@ -2955,6 +2955,8 @@ public class Vpn { } try { + mTunnelIface.setUnderlyingNetwork(mIkeConnectionInfo.getNetwork()); + // Transforms do not need to be persisted; the IkeSession will keep // them alive for us mIpSecManager.applyTunnelModeTransform(mTunnelIface, direction, transform); @@ -3136,13 +3138,13 @@ public class Vpn { // If the default network is lost during the retry delay, the mActiveNetwork will be // null, and the new IKE session won't be established until there is a new default // network bringing up. - mScheduledHandleRetryIkeSessionTimeout = + mScheduledHandleRetryIkeSessionFuture = mExecutor.schedule(() -> { startOrMigrateIkeSession(mActiveNetwork); - // Reset mScheduledHandleRetryIkeSessionTimeout since it's already run on + // Reset mScheduledHandleRetryIkeSessionFuture since it's already run on // executor thread. - mScheduledHandleRetryIkeSessionTimeout = null; + mScheduledHandleRetryIkeSessionFuture = null; }, retryDelay, TimeUnit.SECONDS); } @@ -3185,12 +3187,10 @@ public class Vpn { mActiveNetwork = null; } - if (mScheduledHandleNetworkLostTimeout != null - && !mScheduledHandleNetworkLostTimeout.isCancelled() - && !mScheduledHandleNetworkLostTimeout.isDone()) { + if (mScheduledHandleNetworkLostFuture != null) { final IllegalStateException exception = new IllegalStateException( - "Found a pending mScheduledHandleNetworkLostTimeout"); + "Found a pending mScheduledHandleNetworkLostFuture"); Log.i( TAG, "Unexpected error in onDefaultNetworkLost. Tear down session", @@ -3207,13 +3207,26 @@ public class Vpn { + " on session with token " + mCurrentToken); + final int token = mCurrentToken; // Delay the teardown in case a new network will be available soon. For example, // during handover between two WiFi networks, Android will disconnect from the // first WiFi and then connects to the second WiFi. - mScheduledHandleNetworkLostTimeout = + mScheduledHandleNetworkLostFuture = mExecutor.schedule( () -> { - handleSessionLost(null, network); + if (isActiveToken(token)) { + handleSessionLost(null, network); + } else { + Log.d( + TAG, + "Scheduled handleSessionLost fired for " + + "obsolete token " + + token); + } + + // Reset mScheduledHandleNetworkLostFuture since it's + // already run on executor thread. + mScheduledHandleNetworkLostFuture = null; }, NETWORK_LOST_TIMEOUT_MS, TimeUnit.MILLISECONDS); @@ -3224,28 +3237,26 @@ public class Vpn { } private void cancelHandleNetworkLostTimeout() { - if (mScheduledHandleNetworkLostTimeout != null - && !mScheduledHandleNetworkLostTimeout.isDone()) { + if (mScheduledHandleNetworkLostFuture != null) { // It does not matter what to put in #cancel(boolean), because it is impossible - // that the task tracked by mScheduledHandleNetworkLostTimeout is + // that the task tracked by mScheduledHandleNetworkLostFuture is // in-progress since both that task and onDefaultNetworkChanged are submitted to // mExecutor who has only one thread. Log.d(TAG, "Cancel the task for handling network lost timeout"); - mScheduledHandleNetworkLostTimeout.cancel(false /* mayInterruptIfRunning */); - mScheduledHandleNetworkLostTimeout = null; + mScheduledHandleNetworkLostFuture.cancel(false /* mayInterruptIfRunning */); + mScheduledHandleNetworkLostFuture = null; } } private void cancelRetryNewIkeSessionFuture() { - if (mScheduledHandleRetryIkeSessionTimeout != null - && !mScheduledHandleRetryIkeSessionTimeout.isDone()) { + if (mScheduledHandleRetryIkeSessionFuture != null) { // It does not matter what to put in #cancel(boolean), because it is impossible - // that the task tracked by mScheduledHandleRetryIkeSessionTimeout is + // that the task tracked by mScheduledHandleRetryIkeSessionFuture is // in-progress since both that task and onDefaultNetworkChanged are submitted to // mExecutor who has only one thread. Log.d(TAG, "Cancel the task for handling new ike session timeout"); - mScheduledHandleRetryIkeSessionTimeout.cancel(false /* mayInterruptIfRunning */); - mScheduledHandleRetryIkeSessionTimeout = null; + mScheduledHandleRetryIkeSessionFuture.cancel(false /* mayInterruptIfRunning */); + mScheduledHandleRetryIkeSessionFuture = null; } } @@ -3285,7 +3296,7 @@ public class Vpn { } private void handleSessionLost(@Nullable Exception exception, @Nullable Network network) { - // Cancel mScheduledHandleNetworkLostTimeout if the session it is going to terminate is + // Cancel mScheduledHandleNetworkLostFuture if the session it is going to terminate is // already terminated due to other failures. cancelHandleNetworkLostTimeout();