Merge "Provide more feedback to Settings when sessions fail" am: 9df1022c7a am: da78e17b0a am: 6d6a965712

Original change: https://android-review.googlesource.com/c/platform/frameworks/base/+/1399645

Change-Id: Ibe4fcec14650e38f40bf916f46960585a5af548c

services/core/java/com/android/server/connectivity/Vpn.java

@@ -77,6 +77,7 @@ import android.net.ipsec.ike.ChildSessionParams;
 import android.net.ipsec.ike.IkeSession;
 import android.net.ipsec.ike.IkeSessionCallback;
 import android.net.ipsec.ike.IkeSessionParams;
+import android.net.ipsec.ike.exceptions.IkeProtocolException;
 import android.os.Binder;
 import android.os.Build.VERSION_CODES;
 import android.os.Bundle;
@@ -142,6 +143,7 @@ import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.RejectedExecutionException;
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
@@ -2301,7 +2303,7 @@ public class Vpn {
         void onChildTransformCreated(
                 @NonNull Network network, @NonNull IpSecTransform transform, int direction);
 
-        void onSessionLost(@NonNull Network network);
+        void onSessionLost(@NonNull Network network, @Nullable Exception exception);
     }
 
     /**
@@ -2458,7 +2460,7 @@ public class Vpn {
                 networkAgent.sendLinkProperties(lp);
             } catch (Exception e) {
                 Log.d(TAG, "Error in ChildOpened for network " + network, e);
-                onSessionLost(network);
+                onSessionLost(network, e);
             }
         }
 
@@ -2488,7 +2490,7 @@ public class Vpn {
                 mIpSecManager.applyTunnelModeTransform(mTunnelIface, direction, transform);
             } catch (IOException e) {
                 Log.d(TAG, "Transform application failed for network " + network, e);
-                onSessionLost(network);
+                onSessionLost(network, e);
             }
         }
 
@@ -2546,11 +2548,20 @@ public class Vpn {
                     Log.d(TAG, "Ike Session started for network " + network);
                 } catch (Exception e) {
                     Log.i(TAG, "Setup failed for network " + network + ". Aborting", e);
-                    onSessionLost(network);
+                    onSessionLost(network, e);
                 }
             });
         }
 
+        /** Marks the state as FAILED, and disconnects. */
+        private void markFailedAndDisconnect(Exception exception) {
+            synchronized (Vpn.this) {
+                updateState(DetailedState.FAILED, exception.getMessage());
+            }
+
+            disconnectVpnRunner();
+        }
+
         /**
          * Handles loss of a session
          *
@@ -2560,7 +2571,7 @@ public class Vpn {
          * <p>This method MUST always be called on the mExecutor thread in order to ensure
          * consistency of the Ikev2VpnRunner fields.
          */
-        public void onSessionLost(@NonNull Network network) {
+        public void onSessionLost(@NonNull Network network, @Nullable Exception exception) {
             if (!isActiveNetwork(network)) {
                 Log.d(TAG, "onSessionLost() called for obsolete network " + network);
 
@@ -2572,6 +2583,27 @@ public class Vpn {
                 return;
             }
 
+            if (exception instanceof IkeProtocolException) {
+                final IkeProtocolException ikeException = (IkeProtocolException) exception;
+
+                switch (ikeException.getErrorType()) {
+                    case IkeProtocolException.ERROR_TYPE_NO_PROPOSAL_CHOSEN: // Fallthrough
+                    case IkeProtocolException.ERROR_TYPE_INVALID_KE_PAYLOAD: // Fallthrough
+                    case IkeProtocolException.ERROR_TYPE_AUTHENTICATION_FAILED: // Fallthrough
+                    case IkeProtocolException.ERROR_TYPE_SINGLE_PAIR_REQUIRED: // Fallthrough
+                    case IkeProtocolException.ERROR_TYPE_FAILED_CP_REQUIRED: // Fallthrough
+                    case IkeProtocolException.ERROR_TYPE_TS_UNACCEPTABLE:
+                        // All the above failures are configuration errors, and are terminal
+                        markFailedAndDisconnect(exception);
+                        return;
+                    // All other cases possibly recoverable.
+                }
+            } else if (exception instanceof IllegalArgumentException) {
+                // Failed to build IKE/ChildSessionParams; fatal profile configuration error
+                markFailedAndDisconnect(exception);
+                return;
+            }
+
             mActiveNetwork = null;
 
             // Close all obsolete state, but keep VPN alive incase a usable network comes up.
@@ -2621,12 +2653,18 @@ public class Vpn {
         }
 
         /**
-         * Cleans up all Ikev2VpnRunner internal state
+         * Disconnects and shuts down this VPN.
+         *
+         * <p>This method resets all internal Ikev2VpnRunner state, but unless called via
+         * VpnRunner#exit(), this Ikev2VpnRunner will still be listed as the active VPN of record
+         * until the next VPN is started, or the Ikev2VpnRunner is explicitly exited. This is
+         * necessary to ensure that the detailed state is shown in the Settings VPN menus; if the
+         * active VPN is cleared, Settings VPNs will not show the resultant state or errors.
          *
          * <p>This method MUST always be called on the mExecutor thread in order to ensure
          * consistency of the Ikev2VpnRunner fields.
          */
-        private void shutdownVpnRunner() {
+        private void disconnectVpnRunner() {
             mActiveNetwork = null;
             mIsRunning = false;
 
@@ -2640,9 +2678,13 @@ public class Vpn {
 
         @Override
         public void exitVpnRunner() {
-            mExecutor.execute(() -> {
+            try {
-                shutdownVpnRunner();
+                mExecutor.execute(() -> {
-            });
+                    disconnectVpnRunner();
+                });
+            } catch (RejectedExecutionException ignored) {
+                // The Ikev2VpnRunner has already shut down.
+            }
         }
     }
 

services/core/java/com/android/server/connectivity/VpnIkev2Utils.java

@@ -270,13 +270,13 @@ public class VpnIkev2Utils {
         @Override
         public void onClosed() {
             Log.d(mTag, "IkeClosed for network " + mNetwork);
-            mCallback.onSessionLost(mNetwork); // Server requested session closure. Retry?
+            mCallback.onSessionLost(mNetwork, null); // Server requested session closure. Retry?
         }
 
         @Override
         public void onClosedExceptionally(@NonNull IkeException exception) {
             Log.d(mTag, "IkeClosedExceptionally for network " + mNetwork, exception);
-            mCallback.onSessionLost(mNetwork);
+            mCallback.onSessionLost(mNetwork, exception);
         }
 
         @Override
@@ -306,13 +306,13 @@ public class VpnIkev2Utils {
         @Override
         public void onClosed() {
             Log.d(mTag, "ChildClosed for network " + mNetwork);
-            mCallback.onSessionLost(mNetwork);
+            mCallback.onSessionLost(mNetwork, null);
         }
 
         @Override
         public void onClosedExceptionally(@NonNull IkeException exception) {
             Log.d(mTag, "ChildClosedExceptionally for network " + mNetwork, exception);
-            mCallback.onSessionLost(mNetwork);
+            mCallback.onSessionLost(mNetwork, exception);
         }
 
         @Override
@@ -349,7 +349,7 @@ public class VpnIkev2Utils {
         @Override
         public void onLost(@NonNull Network network) {
             Log.d(mTag, "Tearing down; lost network: " + network);
-            mCallback.onSessionLost(network);
+            mCallback.onSessionLost(network, null);
         }
     }
 

tests/net/Android.bp

@@ -63,6 +63,7 @@ android_test {
         "services.net",
     ],
     libs: [
+        "android.net.ipsec.ike.stubs.module_lib",
         "android.test.runner",
         "android.test.base",
         "android.test.mock",

tests/net/java/com/android/server/connectivity/VpnTest.java

@@ -20,6 +20,7 @@ import static android.content.pm.UserInfo.FLAG_ADMIN;
 import static android.content.pm.UserInfo.FLAG_MANAGED_PROFILE;
 import static android.content.pm.UserInfo.FLAG_PRIMARY;
 import static android.content.pm.UserInfo.FLAG_RESTRICTED;
+import static android.net.ConnectivityManager.NetworkCallback;
 import static android.net.NetworkCapabilities.LINK_BANDWIDTH_UNSPECIFIED;
 import static android.net.NetworkCapabilities.NET_CAPABILITY_INTERNET;
 import static android.net.NetworkCapabilities.NET_CAPABILITY_NOT_CONGESTED;
@@ -45,7 +46,9 @@ import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.timeout;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -66,6 +69,7 @@ import android.net.Ikev2VpnProfile;
 import android.net.InetAddresses;
 import android.net.IpPrefix;
 import android.net.IpSecManager;
+import android.net.IpSecTunnelInterfaceResponse;
 import android.net.LinkProperties;
 import android.net.LocalSocket;
 import android.net.Network;
@@ -75,6 +79,8 @@ import android.net.RouteInfo;
 import android.net.UidRange;
 import android.net.VpnManager;
 import android.net.VpnService;
+import android.net.ipsec.ike.IkeSessionCallback;
+import android.net.ipsec.ike.exceptions.IkeProtocolException;
 import android.os.Build.VERSION_CODES;
 import android.os.Bundle;
 import android.os.ConditionVariable;
@@ -101,6 +107,7 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Answers;
+import org.mockito.ArgumentCaptor;
 import org.mockito.InOrder;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
@@ -150,6 +157,11 @@ public class VpnTest {
     private static final String TEST_VPN_IDENTITY = "identity";
     private static final byte[] TEST_VPN_PSK = "psk".getBytes();
 
+    private static final Network TEST_NETWORK = new Network(Integer.MAX_VALUE);
+    private static final String TEST_IFACE_NAME = "TEST_IFACE";
+    private static final int TEST_TUNNEL_RESOURCE_ID = 0x2345;
+    private static final long TEST_TIMEOUT_MS = 500L;
+
     /**
      * Names and UIDs for some fake packages. Important points:
      *  - UID is ordered increasing.
@@ -227,6 +239,13 @@ public class VpnTest {
         // Deny all appops by default.
         when(mAppOps.noteOpNoThrow(anyInt(), anyInt(), anyString()))
                 .thenReturn(AppOpsManager.MODE_IGNORED);
+
+        // Setup IpSecService
+        final IpSecTunnelInterfaceResponse tunnelResp =
+                new IpSecTunnelInterfaceResponse(
+                        IpSecManager.Status.OK, TEST_TUNNEL_RESOURCE_ID, TEST_IFACE_NAME);
+        when(mIpSecService.createTunnelInterface(any(), any(), any(), any(), any()))
+                .thenReturn(tunnelResp);
     }
 
     @Test
@@ -988,6 +1007,52 @@ public class VpnTest {
                         eq(AppOpsManager.MODE_IGNORED));
     }
 
+    private NetworkCallback triggerOnAvailableAndGetCallback() {
+        final ArgumentCaptor<NetworkCallback> networkCallbackCaptor =
+                ArgumentCaptor.forClass(NetworkCallback.class);
+        verify(mConnectivityManager, timeout(TEST_TIMEOUT_MS))
+                .requestNetwork(any(), networkCallbackCaptor.capture());
+
+        final NetworkCallback cb = networkCallbackCaptor.getValue();
+        cb.onAvailable(TEST_NETWORK);
+        return cb;
+    }
+
+    @Test
+    public void testStartPlatformVpnAuthenticationFailed() throws Exception {
+        final ArgumentCaptor<IkeSessionCallback> captor =
+                ArgumentCaptor.forClass(IkeSessionCallback.class);
+        final IkeProtocolException exception = mock(IkeProtocolException.class);
+        when(exception.getErrorType())
+                .thenReturn(IkeProtocolException.ERROR_TYPE_AUTHENTICATION_FAILED);
+
+        final Vpn vpn = startLegacyVpn(mVpnProfile);
+        final NetworkCallback cb = triggerOnAvailableAndGetCallback();
+
+        // Wait for createIkeSession() to be called before proceeding in order to ensure consistent
+        // state
+        verify(mIkev2SessionCreator, timeout(TEST_TIMEOUT_MS))
+                .createIkeSession(any(), any(), any(), any(), captor.capture(), any());
+        final IkeSessionCallback ikeCb = captor.getValue();
+        ikeCb.onClosedExceptionally(exception);
+
+        verify(mConnectivityManager, timeout(TEST_TIMEOUT_MS)).unregisterNetworkCallback(eq(cb));
+        assertEquals(DetailedState.FAILED, vpn.getNetworkInfo().getDetailedState());
+    }
+
+    @Test
+    public void testStartPlatformVpnIllegalArgumentExceptionInSetup() throws Exception {
+        when(mIkev2SessionCreator.createIkeSession(any(), any(), any(), any(), any(), any()))
+                .thenThrow(new IllegalArgumentException());
+        final Vpn vpn = startLegacyVpn(mVpnProfile);
+        final NetworkCallback cb = triggerOnAvailableAndGetCallback();
+
+        // Wait for createIkeSession() to be called before proceeding in order to ensure consistent
+        // state
+        verify(mConnectivityManager, timeout(TEST_TIMEOUT_MS)).unregisterNetworkCallback(eq(cb));
+        assertEquals(DetailedState.FAILED, vpn.getNetworkInfo().getDetailedState());
+    }
+
     private void setAndVerifyAlwaysOnPackage(Vpn vpn, int uid, boolean lockdownEnabled) {
         assertTrue(vpn.setAlwaysOnPackage(TEST_VPN_PKG, lockdownEnabled, null, mKeyStore));