From 04d0085660b4ee0ed5a7ab9b1e446043d9dce913 Mon Sep 17 00:00:00 2001 From: Mateus Azis Date: Thu, 5 Oct 2023 10:50:24 -0700 Subject: [PATCH] Allow async/slow implementations of authorization checks for Binder gRPCs. Introduce `AsyncSecurityPolicy`, exposing a method 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 --- .../io/grpc/binder/BinderSecurityTest.java | 38 ++++++++- .../binder/internal/BinderTransportTest.java | 3 +- .../io/grpc/binder/AsyncSecurityPolicy.java | 66 ++++++++++++++++ .../java/io/grpc/binder/BinderInternal.java | 8 ++ .../io/grpc/binder/BinderServerBuilder.java | 4 +- .../java/io/grpc/binder/SecurityPolicy.java | 3 +- .../io/grpc/binder/ServerSecurityPolicy.java | 25 +++++- .../io/grpc/binder/internal/BinderServer.java | 8 +- .../internal/BinderTransportSecurity.java | 33 ++++++-- .../grpc/binder/ServerSecurityPolicyTest.java | 79 +++++++++++++++++++ 10 files changed, 251 insertions(+), 16 deletions(-) create mode 100644 binder/src/main/java/io/grpc/binder/AsyncSecurityPolicy.java diff --git a/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java b/binder/src/androidTest/java/io/grpc/binder/BinderSecurityTest.java index e3b9978fb36e..18d5cd99d74c 100644 --- 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 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> func) { + return new AsyncSecurityPolicy() { + @Override + public ListenableFuture 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 index 5140057d72e8..c06bf88955ee 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.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 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 new file mode 100644 index 000000000000..b6dd4df402cc --- /dev/null +++ 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. + * + *

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). + * + *

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 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 index 34f7793714f0..15839b379174 100644 --- 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 index eaa94bffc453..a565f5cd9071 100644 --- 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 index 6b0fb40310a2..3dd9fd6dfcd1 100644 --- 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). * - *

IMPORTANT: This method may block for extended periods of time. + *

IMPORTANT: This method may block for extended periods of time. For slow or + * asynchronous implementations, prefer {@link AsyncSecurityPolicy}. * *

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 index d91a487a57cf..d3022e0886cc 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; @@ -44,16 +46,37 @@ private ServerSecurityPolicy(ImmutableMap perServicePoli /** * Return 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 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 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 index 74ed5caceea6..e72f2851b29a 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 BinderServer( 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 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 index b968e7446850..1dd70e0bbec6 100644 --- 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 ServerCall.Listener interceptCall( */ 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 +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 checkAuthorizationForServiceAsync(int uid, String serviceName); + } } diff --git a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java index 95a4fe5a6e85..718829984bf2 100644 --- a/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java +++ b/binder/src/test/java/io/grpc/binder/ServerSecurityPolicyTest.java @@ -20,6 +20,10 @@ import android.os.Process; 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 io.grpc.Status; import org.junit.Test; import org.junit.runner.RunWith; @@ -81,6 +85,29 @@ public void testPerService() { .isEqualTo(Status.OK.getCode()); } + @Test + public void testPerServiceAsync() { + policy = + ServerSecurityPolicy.newBuilder() + .servicePolicy(SERVICE2, asyncPolicy(uid -> { + // Add some extra future transformation to confirm that a chain + // of futures gets properly handled. + ListenableFuture dependency = Futures.immediateVoidFuture(); + return Futures + .transform(dependency, unused -> Status.OK, MoreExecutors.directExecutor()); + })) + .build(); + + assertThat(policy.checkAuthorizationForService(MY_UID, SERVICE1).getCode()) + .isEqualTo(Status.OK.getCode()); + assertThat(policy.checkAuthorizationForService(OTHER_UID, SERVICE1).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + assertThat(policy.checkAuthorizationForService(MY_UID, SERVICE2).getCode()) + .isEqualTo(Status.OK.getCode()); + assertThat(policy.checkAuthorizationForService(OTHER_UID, SERVICE2).getCode()) + .isEqualTo(Status.OK.getCode()); + } + @Test public void testPerServiceNoDefault() { policy = @@ -109,6 +136,50 @@ SERVICE2, policy((uid) -> uid == OTHER_UID ? Status.OK : Status.PERMISSION_DENIE .isEqualTo(Status.PERMISSION_DENIED.getCode()); } + + @Test + public void testPerServiceNoDefaultAsync() { + policy = + ServerSecurityPolicy.newBuilder() + .servicePolicy( + SERVICE1, + asyncPolicy((uid) -> Futures.immediateFuture(Status.INTERNAL))) + .servicePolicy( + SERVICE2, asyncPolicy((uid) -> { + // Add some extra future transformation to confirm that a chain + // of futures gets properly handled. + ListenableFuture anotherUidFuture = + Futures.immediateFuture(uid == OTHER_UID); + return Futures + .transform( + anotherUidFuture, + anotherUid -> + anotherUid + ? Status.OK + : Status.PERMISSION_DENIED, + MoreExecutors.directExecutor()); + })) + .build(); + + // Uses the specified policy for service1. + assertThat(policy.checkAuthorizationForService(MY_UID, SERVICE1).getCode()) + .isEqualTo(Status.INTERNAL.getCode()); + assertThat(policy.checkAuthorizationForService(OTHER_UID, SERVICE1).getCode()) + .isEqualTo(Status.INTERNAL.getCode()); + + // Uses the specified policy for service2. + assertThat(policy.checkAuthorizationForService(MY_UID, SERVICE2).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + assertThat(policy.checkAuthorizationForService(OTHER_UID, SERVICE2).getCode()) + .isEqualTo(Status.OK.getCode()); + + // Falls back to the default. + assertThat(policy.checkAuthorizationForService(MY_UID, SERVICE3).getCode()) + .isEqualTo(Status.OK.getCode()); + assertThat(policy.checkAuthorizationForService(OTHER_UID, SERVICE3).getCode()) + .isEqualTo(Status.PERMISSION_DENIED.getCode()); + } + private static SecurityPolicy policy(Function func) { return new SecurityPolicy() { @Override @@ -117,4 +188,12 @@ public Status checkAuthorization(int uid) { } }; } + private static AsyncSecurityPolicy asyncPolicy(Function> func) { + return new AsyncSecurityPolicy() { + @Override + public ListenableFuture checkAuthorizationAsync(int uid) { + return func.apply(uid); + } + }; + } }