binder: Introduce server authorization strategy v2

Adds support for android:isolatedProcess Services (fixing #12293) and
moves all peer authorization checks to the handshake, allowing
subsequent transactions to go unchecked.

Author: jdcormie

binder/src/androidTest/java/io/grpc/binder/BinderChannelSmokeTest.java

@@ -97,6 +97,7 @@ public final class BinderChannelSmokeTest {
           .setType(MethodDescriptor.MethodType.BIDI_STREAMING)
           .build();
 
+  AndroidComponentAddress serverAddress;
   ManagedChannel channel;
   AtomicReference<Metadata> headersCapture = new AtomicReference<>();
   AtomicReference<PeerUid> clientUidCapture = new AtomicReference<>();
@@ -134,7 +135,7 @@ public void setUp() throws Exception {
             TestUtils.recordRequestHeadersInterceptor(headersCapture),
             PeerUids.newPeerIdentifyingServerInterceptor());
 
-    AndroidComponentAddress serverAddress = HostServices.allocateService(appContext);
+    serverAddress = HostServices.allocateService(appContext);
     HostServices.configureService(
         serverAddress,
         HostServices.serviceParamsBuilder()
@@ -149,13 +150,15 @@ public void setUp() throws Exception {
                         .build())
             .build());
 
-    channel =
-        BinderChannelBuilder.forAddress(serverAddress, appContext)
+    channel = newBinderChannelBuilder().build();
+  }
+
+  BinderChannelBuilder newBinderChannelBuilder() {
+    return BinderChannelBuilder.forAddress(serverAddress, appContext)
             .inboundParcelablePolicy(
-                InboundParcelablePolicy.newBuilder()
-                    .setAcceptParcelableMetadataValues(true)
-                    .build())
-            .build();
+                    InboundParcelablePolicy.newBuilder()
+                            .setAcceptParcelableMetadataValues(true)
+                            .build());
   }
 
   @After
@@ -185,6 +188,18 @@ public void testBasicCall() throws Exception {
     assertThat(doCall("Hello").get()).isEqualTo("Hello");
   }
 
+  @Test
+  public void testBasicCallWithLegacyAuthStrategy() throws Exception {
+    channel = newBinderChannelBuilder().useLegacyAuthStrategy().build();
+    assertThat(doCall("Hello").get()).isEqualTo("Hello");
+  }
+
+  @Test
+  public void testBasicCallWithV2AuthStrategy() throws Exception {
+    channel = newBinderChannelBuilder().useV2AuthStrategy().build();
+    assertThat(doCall("Hello").get()).isEqualTo("Hello");
+  }
+
   @Test
   public void testPeerUidIsRecorded() throws Exception {
     assertThat(doCall("Hello").get()).isEqualTo("Hello");

binder/src/main/java/io/grpc/binder/BinderChannelBuilder.java

@@ -280,6 +280,64 @@ public BinderChannelBuilder preAuthorizeServers(boolean preAuthorize) {
     return this;
   }
 
+  /**
+   * Specifies how and when to authorize a server against this Channel's {@link SecurityPolicy}.
+   *
+   * <p>This method selects the original "legacy" authorization strategy, which is no longer
+   * preferred for two reasons: First, the legacy strategy considers the UID of the server *process*
+   * we connect to. This is problematic for services using the `android:isolatedProcess` attribute,
+   * which runs them under a different "ephemeral" UID. This UID lacks all the privileges of the
+   * hosting app -- any non-trivial SecurityPolicy would fail to authorize it. Second, the legacy
+   * authorization strategy performs SecurityPolicy checks later in the connection handshake, which
+   * means the calling UID must be rechecked on every subsequent RPC. For these reasons, prefer
+   * {@link #useV2AuthStrategy} instead.
+   *
+   * <p>The server does not know which authorization strategy a client is using. Both strategies
+   * work with all versions of the grpc-binder server.
+   *
+   * <p>The default authorization strategy is unspecified. Clients that require the legacy strategy
+   * should configure it explicitly using this method. Eventually support for the legacy strategy
+   * will be removed.
+   *
+   * @return this
+   */
+  public BinderChannelBuilder useLegacyAuthStrategy() {
+    transportFactoryBuilder.setUseLegacyAuthStrategy(true);
+    return this;
+  }
+
+  /**
+   * Specifies how and when to authorize a server against this Channel's {@link SecurityPolicy}.
+   *
+   * <p>This method selects the v2 authorization strategy. It improves on the original strategy
+   * ({@link #useLegacyAuthStrategy}), by considering the UID of the server *app* we connect to,
+   * rather than the server *process*. This allows clients to connect to services configured with
+   * the `android:isolatedProcess` attribute, which run with the same authority as the hosting app,
+   * but under a different "ephemeral" UID that any non-trivial SecurityPolicy would fail to
+   * authorize.
+   *
+   * <p>Furthermore, the v2 authorization strategy performs SecurityPolicy checks earlier in the
+   * connection handshake, which allows subsequent RPCs over that connection to proceed securely
+   * without further UID checks. For these reasons, clients should prefer the v2 strategy.
+   *
+   * <p>The server does not know which authorization strategy a client is using. Both strategies
+   * work with all versions of the grpc-binder server.
+   *
+   * <p>The default authorization strategy is unspecified. Clients that require the v2 strategy
+   * should configure it explicitly using this method. Eventually support for the legacy strategy
+   * will be removed.
+   *
+   * <p>If moving to the new authorization strategy causes a robolectric test to fail, ensure your
+   * fake Service component is registered with `ShadowPackageManager` using `addOrUpdateService()`.
+   *
+   * @return this
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/12397")
+  public BinderChannelBuilder useV2AuthStrategy() {
+    transportFactoryBuilder.setUseLegacyAuthStrategy(false);
+    return this;
+  }
+
   @Override
   public BinderChannelBuilder idleTimeout(long value, TimeUnit unit) {
     checkState(

binder/src/main/java/io/grpc/binder/internal/Bindable.java

@@ -54,8 +54,11 @@ interface Observer {
    * before giving them a chance to run. However, note that the identity/existence of the resolved
    * Service can change between the time this method returns and the time you actually bind/connect
    * to it. For example, suppose the target package gets uninstalled or upgraded right after this
-   * method returns. In {@link Observer#onBound}, you should verify that the server you resolved is
-   * the same one you connected to.
+   * method returns.
+   *
+   * <p>Compare with {@link #getConnectedServiceInfo()}, which can only be called after {@link
+   * Observer#onBound(IBinder)} but can be used to learn about the service you actually connected
+   * to.
    */
   @AnyThread
   ServiceInfo resolve() throws StatusException;
@@ -68,6 +71,21 @@ interface Observer {
   @AnyThread
   void bind();
 
+  /**
+   * Asks PackageManager for details about the remote Service we *actually* connected to.
+   *
+   * <p>Can only be called after {@link Observer#onBound}.
+   *
+   * <p>Compare with {@link #resolve()}, which reports which service would be selected as of now but
+   * *without* connecting.
+   *
+   * @throws StatusException UNIMPLEMENTED if the connected service isn't found (an {@link
+   *     Observer#onUnbound} callback has likely already happened or is on its way!)
+   * @throws IllegalStateException if {@link Observer#onBound} has not "happened-before" this call
+   */
+  @AnyThread
+  ServiceInfo getConnectedServiceInfo() throws StatusException;
+
   /**
    * Unbind from the remote service if connected.
    *

binder/src/main/java/io/grpc/binder/internal/BinderClientTransport.java

@@ -119,10 +119,10 @@ public BinderClientTransport(
     Boolean preAuthServerOverride = options.getEagAttributes().get(PRE_AUTH_SERVER_OVERRIDE);
     this.preAuthorizeServer =
         preAuthServerOverride != null ? preAuthServerOverride : factory.preAuthorizeServers;
-    this.handshake = new LegacyClientHandshake();
+    this.handshake =
+        factory.useLegacyAuthStrategy ? new LegacyClientHandshake() : new V2ClientHandshake();
     numInUseStreams = new AtomicInteger();
     pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id));
-
     serviceBinding =
         new ServiceBinding(
             factory.mainThreadExecutor,
@@ -388,6 +388,56 @@ private synchronized void handleAuthResult(Status authorization) {
     handshake.onServerAuthorizationOk();
   }
 
+  private class V2ClientHandshake implements ClientHandshake {
+
+    private OneWayBinderProxy endpointBinder;
+
+    @Override
+    @GuardedBy("BinderClientTransport.this") // By way of @GuardedBy("this") `handshake` member.
+    public void onBound(OneWayBinderProxy endpointBinder) {
+      this.endpointBinder = endpointBinder;
+      Futures.addCallback(
+          Futures.submit(serviceBinding::getConnectedServiceInfo, offloadExecutor),
+          new FutureCallback<ServiceInfo>() {
+            @Override
+            public void onSuccess(ServiceInfo result) {
+              synchronized (BinderClientTransport.this) {
+                onConnectedServiceInfo(result);
+              }
+            }
+
+            @Override
+            public void onFailure(Throwable t) {
+              synchronized (BinderClientTransport.this) {
+                shutdownInternal(Status.fromThrowable(t), true);
+              }
+            }
+          },
+          offloadExecutor);
+    }
+
+    @GuardedBy("BinderClientTransport.this")
+    private void onConnectedServiceInfo(ServiceInfo serviceInfo) {
+      if (!inState(TransportState.SETUP)) {
+        return;
+      }
+      attributes = setSecurityAttrs(attributes, serviceInfo.applicationInfo.uid);
+      checkServerAuthorization(serviceInfo.applicationInfo.uid);
+    }
+
+    @Override
+    @GuardedBy("BinderClientTransport.this")
+    public void onServerAuthorizationOk() {
+      sendSetupTransaction(endpointBinder);
+    }
+
+    @Override
+    @GuardedBy("BinderClientTransport.this") // By way of @GuardedBy("this") `handshake` member.
+    public void handleSetupTransport() {
+      onHandshakeComplete();
+    }
+  }
+
   @GuardedBy("this")
   private void onHandshakeComplete() {
     setState(TransportState.READY);

binder/src/main/java/io/grpc/binder/internal/BinderClientTransportFactory.java

@@ -53,6 +53,7 @@ public final class BinderClientTransportFactory implements ClientTransportFactor
   final OneWayBinderProxy.Decorator binderDecorator;
   final long readyTimeoutMillis;
   final boolean preAuthorizeServers; // TODO(jdcormie): Default to true.
+  final boolean useLegacyAuthStrategy;
 
   ScheduledExecutorService executorService;
   Executor offloadExecutor;
@@ -73,6 +74,7 @@ private BinderClientTransportFactory(Builder builder) {
     binderDecorator = checkNotNull(builder.binderDecorator);
     readyTimeoutMillis = builder.readyTimeoutMillis;
     preAuthorizeServers = builder.preAuthorizeServers;
+    useLegacyAuthStrategy = builder.useLegacyAuthStrategy;
 
     executorService = scheduledExecutorPool.getObject();
     offloadExecutor = offloadExecutorPool.getObject();
@@ -126,6 +128,7 @@ public static final class Builder implements ClientTransportFactoryBuilder {
     OneWayBinderProxy.Decorator binderDecorator = OneWayBinderProxy.IDENTITY_DECORATOR;
     long readyTimeoutMillis = 60_000;
     boolean preAuthorizeServers;
+    boolean useLegacyAuthStrategy = true; // TODO(jdcormie): Default to false.
 
     @Override
     public BinderClientTransportFactory buildClientTransportFactory() {
@@ -219,5 +222,11 @@ public Builder setPreAuthorizeServers(boolean preAuthorizeServers) {
       this.preAuthorizeServers = preAuthorizeServers;
       return this;
     }
+
+    /** Specifies which version of the client handshake to use. */
+    public Builder setUseLegacyAuthStrategy(boolean useLegacyAuthStrategy) {
+      this.useLegacyAuthStrategy = useLegacyAuthStrategy;
+      return this;
+    }
   }
 }

binder/src/main/java/io/grpc/binder/internal/ServiceBinding.java

@@ -102,6 +102,9 @@ public String methodName() {
 
   private State reportedState; // Only used on the main thread.
 
+  @GuardedBy("this")
+  private ComponentName connectedServiceName;
+
   @AnyThread
   ServiceBinding(
       Executor mainThreadExecutor,
@@ -305,13 +308,34 @@ private void clearReferences() {
     sourceContext = null;
   }
 
+  @AnyThread
+  @Override
+  public ServiceInfo getConnectedServiceInfo() throws StatusException {
+    try {
+      return getContextForTargetUser("cross-user v2 handshake")
+          .getPackageManager()
+          .getServiceInfo(getConnectedServiceName(), /* flags= */ 0);
+    } catch (PackageManager.NameNotFoundException e) {
+      throw Status.UNIMPLEMENTED
+          .withCause(e)
+          .withDescription("connected remote service was uninstalled/disabled during handshake")
+          .asException();
+    }
+  }
+
+  private synchronized ComponentName getConnectedServiceName() {
+    checkState(connectedServiceName != null, "onBound() not yet called!");
+    return connectedServiceName;
+  }
+
   @Override
   @MainThread
   public void onServiceConnected(ComponentName className, IBinder binder) {
     boolean bound = false;
     synchronized (this) {
       if (state == State.BINDING) {
         state = State.BOUND;
+        connectedServiceName = className;
         bound = true;
       }
     }

binder/src/test/java/io/grpc/binder/internal/RobolectricBinderTransportTest.java

@@ -25,6 +25,7 @@
 import static io.grpc.binder.internal.BinderTransport.SHUTDOWN_TRANSPORT;
 import static io.grpc.binder.internal.BinderTransport.WIRE_FORMAT_VERSION;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
+import static org.junit.Assume.assumeTrue;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
@@ -111,20 +112,27 @@ public final class RobolectricBinderTransportTest extends AbstractTransportTest
 
   @Mock AsyncSecurityPolicy mockClientSecurityPolicy;
 
-  @Captor
-  ArgumentCaptor<Status> statusCaptor;
+  @Captor ArgumentCaptor<Status> statusCaptor;
 
   ApplicationInfo serverAppInfo;
   PackageInfo serverPkgInfo;
   ServiceInfo serviceInfo;
 
   private int nextServerAddress;
 
-  @Parameter public boolean preAuthServersParam;
+  @Parameter(value = 0)
+  public boolean preAuthServersParam;
 
-  @Parameters(name = "preAuthServersParam={0}")
-  public static ImmutableList<Boolean> data() {
-    return ImmutableList.of(true, false);
+  @Parameter(value = 1)
+  public boolean useLegacyAuthStrategy;
+
+  @Parameters(name = "preAuthServersParam={0};useLegacyAuthStrategy={1}")
+  public static ImmutableList<Object[]> data() {
+    return ImmutableList.of(
+        new Object[] {false, false},
+        new Object[] {false, true},
+        new Object[] {true, false},
+        new Object[] {true, true});
   }
 
   @Override
@@ -190,6 +198,7 @@ protected InternalServer newServer(
   BinderClientTransportFactory.Builder newClientTransportFactoryBuilder() {
     return new BinderClientTransportFactory.Builder()
         .setPreAuthorizeServers(preAuthServersParam)
+        .setUseLegacyAuthStrategy(useLegacyAuthStrategy)
         .setSourceContext(application)
         .setScheduledExecutorPool(executorServicePool)
         .setOffloadExecutorPool(offloadExecutorPool);
@@ -250,7 +259,11 @@ public void clientAuthorizesServerUidsInOrder() throws Exception {
     }
 
     AuthRequest authRequest = securityPolicy.takeNextAuthRequest(TIMEOUT_MS, MILLISECONDS);
-    assertThat(authRequest.uid).isEqualTo(11111);
+    if (useLegacyAuthStrategy) {
+      assertThat(authRequest.uid).isEqualTo(11111);
+    } else {
+      assertThat(authRequest.uid).isEqualTo(22222);
+    }
     verify(mockClientTransportListener, never()).transportReady();
     authRequest.setResult(Status.OK);
 
@@ -321,6 +334,10 @@ public void clientIgnoresDuplicateSetupTransaction() throws Exception {
   @Test
   public void clientIgnoresTransactionFromNonServerUids() throws Exception {
     server.start(serverListener);
+
+    // This test is not applicable to the new auth strategy which keeps the client Binder a secret.
+    assumeTrue(useLegacyAuthStrategy);
+
     client = newClientTransport(server);
     startTransport(client, mockClientTransportListener);
 
@@ -369,7 +386,10 @@ public void clientReportsAuthzErrorToServer() throws Exception {
         .transportShutdown(statusCaptor.capture());
     assertThat(statusCaptor.getValue().getCode()).isEqualTo(Status.Code.PERMISSION_DENIED);
 
+    // Client doesn't tell the server in this case by design -- we don't even want to start it!
     TruthJUnit.assume().that(preAuthServersParam).isFalse();
+    // Similar story here. The client won't send a setup transaction to an unauthorized server.
+    TruthJUnit.assume().that(useLegacyAuthStrategy).isTrue();
 
     MockServerTransportListener serverTransportListener =
         serverListener.takeListenerOrFail(TIMEOUT_MS, MILLISECONDS);