binder: Introduce server authorization strategy v2

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,67 @@ 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>Callers need not specify an authorization strategy, but the default is unspecified and will
+   * eventually become {@link #useV2AuthStrategy()}. Clients that require the legacy strategy should
+   * configure it explicitly using this method. Eventually, however, legacy support will be
+   * deprecated and removed.
+   *
+   * @return this
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/12397")
+  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>Callers need not specify an authorization strategy, but the default is unspecified and can
+   * change over time. Clients that require the v2 strategy should configure it explicitly using
+   * this method. Eventually, this strategy will become the default and legacy support 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

@@ -25,6 +25,8 @@
 import android.os.IBinder;
 import android.os.Parcel;
 import android.os.Process;
+import androidx.annotation.BinderThread;
+import androidx.annotation.MainThread;
 import com.google.common.base.Ticker;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
@@ -72,6 +74,9 @@ public final class BinderClientTransport extends BinderTransport
   private final SecurityPolicy securityPolicy;
   private final Bindable serviceBinding;
 
+  @GuardedBy("this")
+  private final ClientHandshake handshake;
+
   /** Number of ongoing calls which keep this transport "in-use". */
   private final AtomicInteger numInUseStreams;
 
@@ -114,9 +119,10 @@ public BinderClientTransport(
     Boolean preAuthServerOverride = options.getEagAttributes().get(PRE_AUTH_SERVER_OVERRIDE);
     this.preAuthorizeServer =
         preAuthServerOverride != null ? preAuthServerOverride : factory.preAuthorizeServers;
+    this.handshake =
+        factory.useLegacyAuthStrategy ? new LegacyClientHandshake() : new V2ClientHandshake();
     numInUseStreams = new AtomicInteger();
     pingTracker = new PingTracker(Ticker.systemTicker(), (id) -> sendPing(id));
-
     serviceBinding =
         new ServiceBinding(
             factory.mainThreadExecutor,
@@ -136,7 +142,7 @@ void releaseExecutors() {
 
   @Override
   public synchronized void onBound(IBinder binder) {
-    sendSetupTransaction(binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor)));
+    handshake.onBound(binderDecorator.decorate(OneWayBinderProxy.wrap(binder, offloadExecutor)));
   }
 
   @Override
@@ -340,10 +346,11 @@ protected void handleSetupTransport(Parcel parcel) {
           Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true);
       return;
     }
+    handshake.handleSetupTransport();
+  }
 
-    int remoteUid = Binder.getCallingUid();
-    restrictIncomingBinderToCallsFrom(remoteUid);
-    attributes = setSecurityAttrs(attributes, remoteUid);
+  @GuardedBy("this")
+  private void checkServerAuthorization(int remoteUid) {
     ListenableFuture<Status> authResultFuture = register(checkServerAuthorizationAsync(remoteUid));
     Futures.addCallback(
         authResultFuture,
@@ -376,7 +383,61 @@ private synchronized void handleAuthResult(Status authorization) {
       shutdownInternal(authorization, true);
       return;
     }
+    handshake.onServerAuthorizationOk();
+  }
+
+  private final 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);
     attributes = clientTransportListener.filterTransport(attributes);
     clientTransportListener.transportReady();
@@ -397,6 +458,52 @@ protected void handlePingResponse(Parcel parcel) {
     pingTracker.onPingResponse(parcel.readInt());
   }
 
+  /**
+   * An abstract implementation of the client's connection handshake.
+   *
+   * <p>Supports a clean migration away from the legacy approach, one client at a time.
+   */
+  private interface ClientHandshake {
+    /**
+     * Notifies the implementation that the binding has succeeded and we are now connected to the
+     * server's "endpoint" which can be reached at 'endpointBinder'.
+     */
+    @MainThread
+    void onBound(OneWayBinderProxy endpointBinder);
+
+    /** Notifies the implementation that we've received a valid SETUP_TRANSPORT transaction. */
+    @BinderThread
+    void handleSetupTransport();
+
+    /** Notifies the implementation that the SecurityPolicy check of the server succeeded. */
+    void onServerAuthorizationOk();
+  }
+
+  private final class LegacyClientHandshake implements ClientHandshake {
+    @Override
+    @MainThread
+    @GuardedBy("BinderClientTransport.this") // By way of @GuardedBy("this") `handshake` member.
+    public void onBound(OneWayBinderProxy binder) {
+      sendSetupTransaction(binder);
+    }
+
+    @Override
+    @BinderThread
+    @GuardedBy("BinderClientTransport.this") // By way of @GuardedBy("this") `handshake` member.
+    public void handleSetupTransport() {
+      int remoteUid = Binder.getCallingUid();
+      restrictIncomingBinderToCallsFrom(remoteUid);
+      attributes = setSecurityAttrs(attributes, remoteUid);
+      checkServerAuthorization(remoteUid);
+    }
+
+    @Override
+    @GuardedBy("BinderClientTransport.this") // By way of @GuardedBy("this") `handshake` member.
+    public void onServerAuthorizationOk() {
+      onHandshakeComplete();
+    }
+  }
+
   private static ClientStream newFailingClientStream(
       Status failure, Attributes attributes, Metadata headers, ClientStreamTracer[] tracers) {
     StatsTraceContext statsTraceContext =

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;
+    }
   }
 }