From 896668001506e7296bd762c4ac4ba30238cd0c48 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Fri, 20 Oct 2023 13:53:12 -0700 Subject: [PATCH] binder: Allow async/slow implementations of authorization checks (#10596) Allow a security policy to returns a `ListenableFuture` 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: https://github.com/grpc/grpc-java/issues/10566 --- .../binder/internal/BinderTransportTest.java | 3 +- .../java/io/grpc/binder/BinderInternal.java | 11 ++++ .../io/grpc/binder/BinderServerBuilder.java | 4 +- .../io/grpc/binder/ServerSecurityPolicy.java | 30 ++++++++++- .../io/grpc/binder/internal/BinderServer.java | 8 +-- .../internal/BinderTransportSecurity.java | 53 ++++++++++++++++--- 6 files changed, 92 insertions(+), 17 deletions(-) diff --git a/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java b/binder/src/androidTest/java/io/grpc/binder/internal/BinderTransportTest.java index 5140057d72..c06bf88955 100644 --- 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.ServerStreamTracer; 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 @@ public final class BinderTransportTest extends AbstractTransportTest { 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/BinderInternal.java b/binder/src/main/java/io/grpc/binder/BinderInternal.java index 34f7793714..18af43ce2b 100644 --- a/binder/src/main/java/io/grpc/binder/BinderInternal.java +++ b/binder/src/main/java/io/grpc/binder/BinderInternal.java @@ -18,6 +18,7 @@ package io.grpc.binder; import android.os.IBinder; import io.grpc.Internal; +import io.grpc.binder.internal.BinderTransportSecurity; /** * Helper class to expose IBinderReceiver methods for legacy internal builders. @@ -31,4 +32,14 @@ public class BinderInternal { public static void setIBinder(IBinderReceiver receiver, IBinder binder) { receiver.set(binder); } + + /** + * Creates a {@link BinderTransportSecurity.ServerPolicyChecker} from a + * {@link ServerSecurityPolicy}. This exposes to callers an interface to check security policies + * without causing hard dependencies on a specific class. + */ + 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 index eaa94bffc4..dafb884d6b 100644 --- a/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java +++ b/binder/src/main/java/io/grpc/binder/BinderServerBuilder.java @@ -23,11 +23,11 @@ import android.app.Service; 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; import io.grpc.binder.internal.BinderTransportSecurity; -import io.grpc.ForwardingServerBuilder; import io.grpc.internal.FixedObjectPool; import io.grpc.internal.GrpcUtil; import io.grpc.internal.ServerImplBuilder; @@ -84,7 +84,7 @@ public final class 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/ServerSecurityPolicy.java b/binder/src/main/java/io/grpc/binder/ServerSecurityPolicy.java index d91a487a57..66685cfdbb 100644 --- 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; @@ -42,18 +44,42 @@ public final class ServerSecurityPolicy { } /** - * Return whether the given Android UID is authorized to access a particular service. + * Returns whether the given Android UID is authorized to access a particular service. * - * IMPORTANT: This method may block for extended periods of time. + *

IMPORTANT: 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 Application code should not need to call this method. */ @CheckReturnValue + @Deprecated public Status checkAuthorizationForService(int uid, String serviceName) { return perServicePolicies.getOrDefault(serviceName, defaultPolicy).checkAuthorization(uid); } + /** + * Returns whether the given Android UID is authorized to access a particular service. + * + *

This method never throws an exception. If the execution of the security policy check + * fails, a failed future with such exception is returned. + * + * @param uid The Android UID to authenticate. + * @param serviceName The name of the gRPC service being called. + * @return a future with the result of the authorization check. A failed future represents a + * failure to perform the authorization check, not that the access is denied. + */ + @CheckReturnValue + ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName) { + SecurityPolicy securityPolicy = perServicePolicies.getOrDefault(serviceName, defaultPolicy); + try { + Status status = securityPolicy.checkAuthorization(uid); + return Futures.immediateFuture(status); + } catch (Exception e) { + return Futures.immediateFailedFuture(e); + } + } + 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 index 74ed5cacee..e72f2851b2 100644 --- 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 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 final class BinderServer implements InternalServer, LeakSafeOneWayBinder. AndroidComponentAddress listenAddress, ObjectPool executorServicePool, List 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 final class BinderServer implements InternalServer, LeakSafeOneWayBinder. .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 index b968e74468..022ae7f6d2 100644 --- a/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java +++ b/binder/src/main/java/io/grpc/binder/internal/BinderTransportSecurity.java @@ -16,6 +16,7 @@ package io.grpc.binder.internal; +import com.google.common.util.concurrent.ListenableFuture; import io.grpc.Attributes; import io.grpc.Internal; import io.grpc.Metadata; @@ -26,9 +27,9 @@ import io.grpc.ServerCall; import io.grpc.ServerCallHandler; import io.grpc.ServerInterceptor; import io.grpc.Status; -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 +61,15 @@ public final class BinderTransportSecurity { * * @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 checker 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 +100,12 @@ public final class BinderTransportSecurity { */ private static final class TransportAuthorizationState { private final int uid; - private final ServerSecurityPolicy policy; + private final ServerPolicyChecker serverPolicyChecker; private final ConcurrentHashMap 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 +124,47 @@ public final class BinderTransportSecurity { 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. + Thread.currentThread().interrupt(); + return Status.CANCELLED.withCause(e); + } if (useCache) { serviceAuthorization.putIfAbsent(serviceName, authorization); } return authorization; } } + + /** + * Decides whether a given Android UID is authorized to access some resource. + * + *

This class provides the asynchronous version of {@link io.grpc.binder.SecurityPolicy}, + * allowing implementations of authorization logic that involves slow or asynchronous calls + * without necessarily blocking the calling thread. + * + * @see io.grpc.binder.SecurityPolicy + */ + public interface ServerPolicyChecker { + /** + * Returns whether the given Android UID is authorized to access a particular service. + * + *

This method never throws an exception. If the execution of the security policy check + * fails, a failed future with such exception is returned. + * + * @param uid The Android UID to authenticate. + * @param serviceName The name of the gRPC service being called. + * @return a future with the result of the authorization check. A failed future represents a + * failure to perform the authorization check, not that the access is denied. + */ + ListenableFuture checkAuthorizationForServiceAsync(int uid, String serviceName); + } }