Merge branch 'keep-alive' of https://github.com/jdcormie/grpc-java into keep-alive

Author: jdcormie

binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java

@@ -58,10 +58,8 @@
 import java.io.InputStream;
 import java.util.ArrayDeque;
 import java.util.Deque;
-import java.util.concurrent.BlockingQueue;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
-import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import javax.annotation.Nullable;
@@ -102,7 +100,7 @@ public final class BinderClientTransportTest {
 
   AndroidComponentAddress serverAddress;
   BinderTransport.BinderClientTransport transport;
-  BlockingSecurityPolicy blockingSecurityPolicy = new BlockingSecurityPolicy();
+  SettableAsyncSecurityPolicy blockingSecurityPolicy = new SettableAsyncSecurityPolicy();
 
   private final ObjectPool<ScheduledExecutorService> executorServicePool =
       new FixedObjectPool<>(Executors.newScheduledThreadPool(1));
@@ -170,6 +168,11 @@ public BinderClientTransportBuilder setReadyTimeoutMillis(int timeoutMillis) {
       return this;
     }
 
+    public BinderClientTransportBuilder setPreAuthorizeServer(boolean preAuthorizeServer) {
+      factoryBuilder.setPreAuthorizeServers(preAuthorizeServer);
+      return this;
+    }
+
     public BinderTransport.BinderClientTransport build() {
       return factoryBuilder
           .buildClientTransportFactory()
@@ -179,7 +182,7 @@ public BinderTransport.BinderClientTransport build() {
 
   @After
   public void tearDown() throws Exception {
-    blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.ABORTED);
+    blockingSecurityPolicy.setAuthorizationResult(Status.ABORTED);
     transport.shutdownNow(Status.OK);
     HostServices.awaitServiceShutdown();
     shutdownAndTerminate(executorServicePool.getObject());
@@ -285,16 +288,16 @@ public void testMessageProducerClosedAfterStream_b169313545() throws Exception {
   @Test
   public void testNewStreamBeforeTransportReadyFails() throws Exception {
     // Use a special SecurityPolicy that lets us act before the transport is setup/ready.
-    transport =
-        new BinderClientTransportBuilder().setSecurityPolicy(blockingSecurityPolicy).build();
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
     transport.start(transportListener).run();
     ClientStream stream =
         transport.newStream(streamingMethodDesc, new Metadata(), CallOptions.DEFAULT, tracers);
     stream.start(streamListener);
     assertThat(streamListener.awaitClose().getCode()).isEqualTo(Code.INTERNAL);
 
     // Unblock the SETUP_TRANSPORT handshake and make sure it becomes ready in the usual way.
-    blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.OK);
+    securityPolicy.setAuthorizationResult(Status.OK);
     transportListener.awaitReady();
   }
 
@@ -370,24 +373,60 @@ public void testBlackHoleEndpointConnectTimeout() throws Exception {
   }
 
   @Test
-  public void testBlackHoleSecurityPolicyConnectTimeout() throws Exception {
+  public void testBlackHoleSecurityPolicyAuthTimeout() throws Exception {
+    transport =
+        new BinderClientTransportBuilder()
+            .setSecurityPolicy(blockingSecurityPolicy)
+            .setPreAuthorizeServer(false)
+            .setReadyTimeoutMillis(1_234)
+            .build();
+    transport.start(transportListener).run();
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
+    assertThat(transportStatus.getDescription()).contains("1234");
+    transportListener.awaitTermination();
+  }
+
+  @Test
+  public void testBlackHoleSecurityPolicyPreAuthTimeout() throws Exception {
     transport =
         new BinderClientTransportBuilder()
             .setSecurityPolicy(blockingSecurityPolicy)
+            .setPreAuthorizeServer(true)
             .setReadyTimeoutMillis(1_234)
             .build();
     transport.start(transportListener).run();
     Status transportStatus = transportListener.awaitShutdown();
     assertThat(transportStatus.getCode()).isEqualTo(Code.DEADLINE_EXCEEDED);
     assertThat(transportStatus.getDescription()).contains("1234");
     transportListener.awaitTermination();
-    blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.OK);
   }
 
   @Test
-  public void testAsyncSecurityPolicyFailure() throws Exception {
+  public void testAsyncSecurityPolicyAuthFailure() throws Exception {
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
-    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(false)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    RuntimeException exception = new NullPointerException();
+    securityPolicy.setAuthorizationException(exception);
+    transport.start(transportListener).run();
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.INTERNAL);
+    assertThat(transportStatus.getCause()).isEqualTo(exception);
+    transportListener.awaitTermination();
+  }
+
+  @Test
+  public void testAsyncSecurityPolicyPreAuthFailure() throws Exception {
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(true)
+            .setSecurityPolicy(securityPolicy)
+            .build();
     RuntimeException exception = new NullPointerException();
     securityPolicy.setAuthorizationException(exception);
     transport.start(transportListener).run();
@@ -400,11 +439,32 @@ public void testAsyncSecurityPolicyFailure() throws Exception {
   @Test
   public void testAsyncSecurityPolicySuccess() throws Exception {
     SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
-    transport = new BinderClientTransportBuilder().setSecurityPolicy(securityPolicy).build();
-    securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED);
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(false)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED.withDescription("xyzzy"));
+    transport.start(transportListener).run();
+    Status transportStatus = transportListener.awaitShutdown();
+    assertThat(transportStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED);
+    assertThat(transportStatus.getDescription()).contains("xyzzy");
+    transportListener.awaitTermination();
+  }
+
+  @Test
+  public void testAsyncSecurityPolicyPreAuthSuccess() throws Exception {
+    SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
+    transport =
+        new BinderClientTransportBuilder()
+            .setPreAuthorizeServer(true)
+            .setSecurityPolicy(securityPolicy)
+            .build();
+    securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED.withDescription("xyzzy"));
     transport.start(transportListener).run();
     Status transportStatus = transportListener.awaitShutdown();
     assertThat(transportStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED);
+    assertThat(transportStatus.getDescription()).contains("xyzzy");
     transportListener.awaitTermination();
   }
 
@@ -548,26 +608,6 @@ public synchronized void closed(Status status, RpcProgress rpcProgress, Metadata
     }
   }
 
-  /**
-   * A SecurityPolicy that blocks the transport authorization check until a test sets the outcome.
-   */
-  static class BlockingSecurityPolicy extends SecurityPolicy {
-    private final BlockingQueue<Status> results = new LinkedBlockingQueue<>();
-
-    public void provideNextCheckAuthorizationResult(Status status) {
-      results.add(status);
-    }
-
-    @Override
-    public Status checkAuthorization(int uid) {
-      try {
-        return results.take();
-      } catch (InterruptedException e) {
-        return Status.fromThrowable(e);
-      }
-    }
-  }
-
   /** An AsyncSecurityPolicy that lets a test specify the outcome of checkAuthorizationAsync(). */
   static class SettableAsyncSecurityPolicy extends AsyncSecurityPolicy {
     private SettableFuture<Status> result = SettableFuture.create();

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

@@ -17,7 +17,9 @@
 package io.grpc.binder;
 
 import android.content.Intent;
+import android.content.pm.ServiceInfo;
 import android.os.UserHandle;
+import io.grpc.Attributes;
 import io.grpc.ExperimentalApi;
 import io.grpc.NameResolver;
 

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

@@ -176,9 +176,13 @@ private BinderChannelBuilder(
       managedChannelImplBuilder =
           new ManagedChannelImplBuilder(
               directAddress, directAddress.getAuthority(), transportFactoryBuilder, null);
+      // TODO(jdcormie): Rollout step 2: Pre-auth *all* addresses by default.
+      preAuthorizeServers(false);
     } else {
       managedChannelImplBuilder =
           new ManagedChannelImplBuilder(target, transportFactoryBuilder, null);
+      // TODO(jdcormie): Rollout step 1: Pre-auth NameResolver results by default.
+      preAuthorizeServers(false);
     }
     idleTimeout(60, TimeUnit.SECONDS);
   }
@@ -279,6 +283,33 @@ public BinderChannelBuilder strictLifecycleManagement() {
     return this;
   }
 
+  /**
+   * Checks servers against this channel's {@link SecurityPolicy} *before* binding.
+   *
+   * <p>Android users can be tricked into installing a malicious app with the same package name as a
+   * legitimate server. That's why we don't send calls to a server until it has been authorized by
+   * an appropriate {@link SecurityPolicy}. But merely binding to a malicious server can enable
+   * "keep-alive" and "background activity launch" attacks, even if security policy ultimately
+   * causes the grpc connection to fail. Pre-authorization is especially important for security when
+   * the server's address isn't known in advance but rather resolved via target URI or discovered by
+   * other means.
+   *
+   * <p>Note that, unlike ordinary authorization, pre-authorization is performed against the server
+   * app's UID, not the UID of the server process. These can be different, most commonly due to
+   * services that set `android:isolatedProcess=true`.
+   *
+   * <p>Pre-authorization is strongly recommended but it remains optional for now because of this
+   * behavior change and the small performance cost.
+   *
+   * <p>The default value of this property is false but it will become true in a future release.
+   * Clients that require a particular behavior should configure it explicitly using this method
+   * rather than relying on the default.
+   */
+  public BinderChannelBuilder preAuthorizeServers(boolean preAuthorize) {
+    transportFactoryBuilder.setPreAuthorizeServers(preAuthorize);
+    return this;
+  }
+
   @Override
   public BinderChannelBuilder idleTimeout(long value, TimeUnit unit) {
     checkState(

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

@@ -16,10 +16,13 @@
 
 package io.grpc.binder.internal;
 
+import android.content.pm.ServiceInfo;
 import android.os.IBinder;
 import androidx.annotation.AnyThread;
 import androidx.annotation.MainThread;
+import com.google.common.util.concurrent.ListenableFuture;
 import io.grpc.Status;
+import io.grpc.StatusException;
 
 /** An interface for managing a {@code Binder} connection. */
 interface Bindable {
@@ -45,6 +48,10 @@ interface Observer {
     void onUnbound(Status reason);
   }
 
+  /** Fetches details about the remote service from PackageManager *before* binding to it. */
+  @AnyThread
+  ServiceInfo resolve() throws StatusException;
+
   /**
    * Attempt to bind with the remote service.
    *

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

@@ -55,6 +55,7 @@ public final class BinderClientTransportFactory implements ClientTransportFactor
   final InboundParcelablePolicy inboundParcelablePolicy;
   final OneWayBinderProxy.Decorator binderDecorator;
   final long readyTimeoutMillis;
+  final boolean preAuthorizeServers;
 
   ScheduledExecutorService executorService;
   Executor offloadExecutor;
@@ -75,6 +76,7 @@ private BinderClientTransportFactory(Builder builder) {
     inboundParcelablePolicy = checkNotNull(builder.inboundParcelablePolicy);
     binderDecorator = checkNotNull(builder.binderDecorator);
     readyTimeoutMillis = builder.readyTimeoutMillis;
+    preAuthorizeServers = builder.preAuthorizeServers;
 
     executorService = scheduledExecutorPool.getObject();
     offloadExecutor = offloadExecutorPool.getObject();
@@ -128,6 +130,7 @@ public static final class Builder implements ClientTransportFactoryBuilder {
     InboundParcelablePolicy inboundParcelablePolicy = InboundParcelablePolicy.DEFAULT;
     OneWayBinderProxy.Decorator binderDecorator = OneWayBinderProxy.IDENTITY_DECORATOR;
     long readyTimeoutMillis = 60_000;
+    boolean preAuthorizeServers;
 
     @Override
     public BinderClientTransportFactory buildClientTransportFactory() {
@@ -216,5 +219,11 @@ public Builder setReadyTimeoutMillis(long readyTimeoutMillis) {
       this.readyTimeoutMillis = readyTimeoutMillis;
       return this;
     }
+
+    /** Whether to check server addresses against the SecurityPolicy before binding to them. */
+    public Builder setPreAuthorizeServers(boolean preAuthorizeServers) {
+      this.preAuthorizeServers = preAuthorizeServers;
+      return this;
+    }
   }
 }