Allow async/slow implementations of authorization checks for Binder g…

…RPCs. Introduce `AsyncSecurityPolicy`, exposing a method returns a `ListenableFuture<Status>` that callers can implement to perform slower auth checks (like network calls, disk I/O etc.) without necessarily blocking the gRPC calling thread. Partially addresses: grpc#10566
mateusazis · Oct 5, 2023 · 04d0085 · 04d0085
1 parent bf9ccc6
commit 04d0085
Show file tree

Hide file tree

Showing 10 changed files with 251 additions and 16 deletions.
diff --git a/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java b/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java
@@ -23,6 +23,9 @@
 import androidx.test.core.app.ApplicationProvider;
 import androidx.test.ext.junit.runners.AndroidJUnit4;
 import com.google.common.base.Function;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.MoreExecutors;
 import com.google.protobuf.Empty;
 import io.grpc.CallOptions;
 import io.grpc.ManagedChannel;
@@ -155,7 +158,7 @@ public void testAllowedCall() throws Exception {
   }
 
   @Test
-  public void testServerDisllowsCalls() throws Exception {
+  public void testServerDisallowsCalls() throws Exception {
     createChannel(
         ServerSecurityPolicy.newBuilder()
             .servicePolicy("foo", policy((uid) -> false))
@@ -197,6 +200,25 @@ public void testPerServicePolicy() throws Exception {
     }
   }
 
+  @Test
+  public void testPerServicePolicyAsync() throws Exception {
+    createChannel(
+            ServerSecurityPolicy.newBuilder()
+                    .servicePolicy("foo", asyncPolicy((uid) -> Futures.immediateFuture(true)))
+                    .servicePolicy("bar", asyncPolicy((uid) -> Futures.immediateFuture(false)))
+                    .build(),
+            SecurityPolicies.internalOnly());
+
+    assertThat(methods).isNotEmpty();
+    for (MethodDescriptor<Empty, Empty> method : methods.values()) {
+      if (method.getServiceName().equals("bar")) {
+        assertCallFailure(method, Status.PERMISSION_DENIED);
+      } else {
+        assertCallSuccess(method);
+      }
+    }
+  }
+
   @Test
   public void testSecurityInterceptorIsClosestToTransport() throws Exception {
     createChannel(
@@ -227,6 +249,20 @@ public Status checkAuthorization(int uid) {
     };
   }
 
+  private static AsyncSecurityPolicy asyncPolicy(
+      Function<Integer, ListenableFuture<Boolean>> func) {
+    return new AsyncSecurityPolicy() {
+      @Override
+      public ListenableFuture<Status> checkAuthorizationAsync(int uid) {
+        return Futures
+            .transform(
+                func.apply(uid),
+                allowed -> allowed ? Status.OK : Status.PERMISSION_DENIED,
+                MoreExecutors.directExecutor());
+      }
+    };
+  }
+
   private final class CountingServerInterceptor implements ServerInterceptor {
     int numInterceptedCalls;
 

diff --git a/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java b/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java
@@ -25,6 +25,7 @@
 import io.grpc.binder.AndroidComponentAddress;
 import io.grpc.binder.BindServiceFlags;
 import io.grpc.binder.BinderChannelCredentials;
+import io.grpc.binder.BinderInternal;
 import io.grpc.binder.HostServices;
 import io.grpc.binder.InboundParcelablePolicy;
 import io.grpc.binder.SecurityPolicies;
@@ -69,7 +70,7 @@ protected InternalServer newServer(List<ServerStreamTracer.Factory> streamTracer
     BinderServer binderServer = new BinderServer(addr,
         executorServicePool,
         streamTracerFactories,
-        SecurityPolicies.serverInternalOnly(),
+        BinderInternal.createPolicyChecker(SecurityPolicies.serverInternalOnly()),
         InboundParcelablePolicy.DEFAULT);
 
     HostServices.configureService(addr,

diff --git a/binder/src/main/java/io/grpc/binder/AsyncSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/AsyncSecurityPolicy.java
@@ -0,0 +1,66 @@
+/*
+ * Copyright 2023 The gRPC Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package io.grpc.binder;
+
+import com.google.common.util.concurrent.ListenableFuture;
+import io.grpc.ExperimentalApi;
+import io.grpc.Status;
+import java.util.concurrent.ExecutionException;
+import javax.annotation.CheckReturnValue;
+
+/**
+ * Decides whether a given Android UID is authorized to access some resource.
+ *
+ * <p>This class provides the asynchronous version of {@link SecurityPolicy}, allowing
+ * implementations of authorization logic that involves slow or asynchronous calls without
+ * necessarily blocking the calling thread.
+ *
+ * @see SecurityPolicy
+ */
+@ExperimentalApi("https://github.com/grpc/grpc-java/issues/10566")
+@CheckReturnValue
+public abstract class AsyncSecurityPolicy extends SecurityPolicy {
+
+    /**
+     * @deprecated Prefer {@link #checkAuthorizationAsync(int)} for async or slow calls or subclass
+     *     {@link SecurityPolicy} directly for quick, synchronous implementations.
+     */
+    @Override
+    @Deprecated
+    public final Status checkAuthorization(int uid) {
+        try {
+            return checkAuthorizationAsync(uid).get();
+        } catch (ExecutionException e) {
+            return Status.fromThrowable(e);
+        } catch (InterruptedException e) {
+            return Status.CANCELLED.withCause(e);
+        }
+    }
+
+    /**
+     * Decides whether the given Android UID is authorized. (Validity is implementation dependent).
+     *
+     * <p>As long as any given UID has active processes, this method should return the same value for
+     * that UID. In order words, policy changes which occur while a transport instance is active, will
+     * have no effect on that transport instance.
+     *
+     * @param uid The Android UID to authenticate.
+     * @return A {@link ListenableFuture} for a gRPC {@link Status} object, with OK indicating
+     *     authorized.
+     */
+    abstract ListenableFuture<Status> checkAuthorizationAsync(int uid);
+}
diff --git a/binder/src/main/java/io/grpc/binder/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java
@@ -17,7 +17,9 @@
 package io.grpc.binder;
 
 import android.os.IBinder;
+import io.grpc.ExperimentalApi;
 import io.grpc.Internal;
+import io.grpc.binder.internal.BinderTransportSecurity;
 
 /**
  * Helper class to expose IBinderReceiver methods for legacy internal builders.
@@ -31,4 +33,10 @@ public class BinderInternal {
   public static void setIBinder(IBinderReceiver receiver, IBinder binder) {
     receiver.set(binder);
   }
+
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10566")
+  public static BinderTransportSecurity.ServerPolicyChecker createPolicyChecker(
+          ServerSecurityPolicy securityPolicy) {
+    return securityPolicy::checkAuthorizationForServiceAsync;
+  }
 }
diff --git a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java
@@ -23,6 +23,7 @@
 import android.os.IBinder;
 import com.google.errorprone.annotations.DoNotCall;
 import io.grpc.ExperimentalApi;
+import io.grpc.ForwardingServerBuilder;
 import io.grpc.Server;
 import io.grpc.ServerBuilder;
 import io.grpc.binder.internal.BinderServer;
@@ -32,6 +33,7 @@
 import io.grpc.internal.GrpcUtil;
 import io.grpc.internal.ServerImplBuilder;
 import io.grpc.internal.ObjectPool;
+import io.grpc.internal.ServerImplBuilder;
 import io.grpc.internal.SharedResourcePool;
 import java.io.File;
 import java.util.concurrent.ScheduledExecutorService;
@@ -84,7 +86,7 @@ private BinderServerBuilder(
           listenAddress,
           schedulerPool,
           streamTracerFactories,
-          securityPolicy,
+          BinderInternal.createPolicyChecker(securityPolicy),
           inboundParcelablePolicy);
       BinderInternal.setIBinder(binderReceiver, server.getHostBinder());
       return server;

diff --git a/binder/src/main/java/io/grpc/binder/SecurityPolicy.java b/binder/src/main/java/io/grpc/binder/SecurityPolicy.java
@@ -43,7 +43,8 @@ protected SecurityPolicy() {}
   /**
    * Decides whether the given Android UID is authorized. (Validity is implementation dependent).
    *
-   * <p><b>IMPORTANT</b>: This method may block for extended periods of time.
+   * <p><b>IMPORTANT</b>: This method may block for extended periods of time. For slow or
+   * asynchronous implementations, prefer {@link AsyncSecurityPolicy}.
    *
    * <p>As long as any given UID has active processes, this method should return the same value for
    * that UID. In order words, policy changes which occur while a transport instance is active, will

diff --git a/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java
@@ -17,6 +17,8 @@
 package io.grpc.binder;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
 import io.grpc.Status;
 import java.util.HashMap;
 import java.util.Map;
@@ -44,16 +46,37 @@ private ServerSecurityPolicy(ImmutableMap<String, SecurityPolicy> perServicePoli
   /**
    * Return whether the given Android UID is authorized to access a particular service.
    *
-   * <b>IMPORTANT</b>: This method may block for extended periods of time.
+   * <p><b>IMPORTANT</b>: This method may block for extended periods of time.
    *
    * @param uid The Android UID to authenticate.
    * @param serviceName The name of the gRPC service being called.
+   * @deprecated prefer calling {@link BinderInternal#createPolicyChecker(ServerSecurityPolicy)}
+   *     then {@link
+   *     io.grpc.binder.internal.BinderTransportSecurity.ServerPolicyChecker#checkAuthorizationForServiceAsync(int,
+   *     String)}.
    */
   @CheckReturnValue
+  @Deprecated
   public Status checkAuthorizationForService(int uid, String serviceName) {
     return perServicePolicies.getOrDefault(serviceName, defaultPolicy).checkAuthorization(uid);
   }
 
+  /**
+   * Return whether the given Android UID is authorized to access a particular service.
+   *
+   * @param uid The Android UID to authenticate.
+   * @param serviceName The name of the gRPC service being called.
+   */
+  @CheckReturnValue
+  ListenableFuture<Status> checkAuthorizationForServiceAsync(int uid, String serviceName) {
+    SecurityPolicy securityPolicy = perServicePolicies.getOrDefault(serviceName, defaultPolicy);
+    if (securityPolicy instanceof AsyncSecurityPolicy) {
+      return ((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(uid);
+    }
+    return Futures.immediateFuture(securityPolicy.checkAuthorization(uid));
+  }
+
+
   public static Builder newBuilder() {
     return new Builder();
   }

diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderServer.java b/binder/src/main/java/io/grpc/binder/internal/BinderServer.java
@@ -58,7 +58,7 @@ public final class BinderServer implements InternalServer, LeakSafeOneWayBinder.
   private final ImmutableList<ServerStreamTracer.Factory> streamTracerFactories;
   private final AndroidComponentAddress listenAddress;
   private final LeakSafeOneWayBinder hostServiceBinder;
-  private final ServerSecurityPolicy serverSecurityPolicy;
+  private final BinderTransportSecurity.ServerPolicyChecker serverPolicyChecker;
   private final InboundParcelablePolicy inboundParcelablePolicy;
 
   @GuardedBy("this")
@@ -74,13 +74,13 @@ public BinderServer(
       AndroidComponentAddress listenAddress,
       ObjectPool<ScheduledExecutorService> executorServicePool,
       List<? extends ServerStreamTracer.Factory> streamTracerFactories,
-      ServerSecurityPolicy serverSecurityPolicy,
+      BinderTransportSecurity.ServerPolicyChecker serverPolicyChecker,
       InboundParcelablePolicy inboundParcelablePolicy) {
     this.listenAddress = listenAddress;
     this.executorServicePool = executorServicePool;
     this.streamTracerFactories =
         ImmutableList.copyOf(checkNotNull(streamTracerFactories, "streamTracerFactories"));
-    this.serverSecurityPolicy = checkNotNull(serverSecurityPolicy, "serverSecurityPolicy");
+    this.serverPolicyChecker = checkNotNull(serverPolicyChecker, "serverPolicyChecker");
     this.inboundParcelablePolicy = inboundParcelablePolicy;
     hostServiceBinder = new LeakSafeOneWayBinder(this);
   }
@@ -150,7 +150,7 @@ public synchronized boolean handleTransaction(int code, Parcel parcel) {
                   .set(BinderTransport.REMOTE_UID, callingUid)
                   .set(BinderTransport.SERVER_AUTHORITY, listenAddress.getAuthority())
                   .set(BinderTransport.INBOUND_PARCELABLE_POLICY, inboundParcelablePolicy);
-          BinderTransportSecurity.attachAuthAttrs(attrsBuilder, callingUid, serverSecurityPolicy);
+          BinderTransportSecurity.attachAuthAttrs(attrsBuilder, callingUid, serverPolicyChecker);
           // Create a new transport and let our listener know about it.
           BinderTransport.BinderServerTransport transport =
               new BinderTransport.BinderServerTransport(

diff --git a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java
@@ -16,7 +16,9 @@
 
 package io.grpc.binder.internal;
 
+import com.google.common.util.concurrent.ListenableFuture;
 import io.grpc.Attributes;
+import io.grpc.ExperimentalApi;
 import io.grpc.Internal;
 import io.grpc.Metadata;
 import io.grpc.MethodDescriptor;
@@ -29,6 +31,7 @@
 import io.grpc.binder.ServerSecurityPolicy;
 import io.grpc.internal.GrpcAttributes;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ExecutionException;
 import javax.annotation.CheckReturnValue;
 
 /**
@@ -60,15 +63,15 @@ public static void installAuthInterceptor(ServerBuilder<?> serverBuilder) {
    *
    * @param builder The {@link Attributes.Builder} for the transport being created.
    * @param remoteUid The remote UID of the transport.
-   * @param securityPolicy The policy to enforce on this transport.
+   * @param serverPolicyChecker The policy enforcer for this transport.
    */
   @Internal
   public static void attachAuthAttrs(
-      Attributes.Builder builder, int remoteUid, ServerSecurityPolicy securityPolicy) {
+      Attributes.Builder builder, int remoteUid, ServerPolicyChecker serverPolicyChecker) {
     builder
         .set(
             TRANSPORT_AUTHORIZATION_STATE,
-            new TransportAuthorizationState(remoteUid, securityPolicy))
+            new TransportAuthorizationState(remoteUid, serverPolicyChecker))
         .set(GrpcAttributes.ATTR_SECURITY_LEVEL, SecurityLevel.PRIVACY_AND_INTEGRITY);
   }
 
@@ -99,12 +102,12 @@ public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(
    */
   private static final class TransportAuthorizationState {
     private final int uid;
-    private final ServerSecurityPolicy policy;
+    private final ServerPolicyChecker serverPolicyChecker;
     private final ConcurrentHashMap<String, Status> serviceAuthorization;
 
-    TransportAuthorizationState(int uid, ServerSecurityPolicy policy) {
+    TransportAuthorizationState(int uid, ServerPolicyChecker serverPolicyChecker) {
       this.uid = uid;
-      this.policy = policy;
+      this.serverPolicyChecker = serverPolicyChecker;
       serviceAuthorization = new ConcurrentHashMap<>(8);
     }
 
@@ -123,11 +126,27 @@ Status checkAuthorization(MethodDescriptor<?, ?> method) {
           return authorization;
         }
       }
-      authorization = policy.checkAuthorizationForService(uid, serviceName);
+      try {
+        // TODO(10566): provide a synchronous version of "checkAuthorization" to avoid blocking the
+        // calling thread on the completion of the future.
+        authorization =
+                serverPolicyChecker.checkAuthorizationForServiceAsync(uid, serviceName).get();
+      } catch (ExecutionException e) {
+        // Do not cache this failure since it may be transient.
+        return Status.fromThrowable(e);
+      } catch (InterruptedException e) {
+        // Do not cache this failure since it may be transient.
+        return Status.CANCELLED.withCause(e);
+      }
       if (useCache) {
         serviceAuthorization.putIfAbsent(serviceName, authorization);
       }
       return authorization;
     }
   }
+
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10566")
+  public interface ServerPolicyChecker {
+    ListenableFuture<Status> checkAuthorizationForServiceAsync(int uid, String serviceName);
+  }
 }