Skip to content

Commit

Permalink
Don't block an offload thread when the client's SecurityPolicy is asy…
Browse files Browse the repository at this point in the history 
…nc (grpc#11272)

grpc#10566
  • Loading branch information
@jdcormie @larry-safran
jdcormie authored and larry-safran committed Aug 13, 2024
1 parent e7e0303 commit e13212a
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 32 deletions.
54 changes: 54 additions & 0 deletions binder/src/androidTest/java/io/grpc/binder/internal/BinderClientTransportTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import android.os.RemoteException;
import androidx.test.core.app.ApplicationProvider;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import com.google.common.util.concurrent.SettableFuture;
import com.google.protobuf.Empty;
import io.grpc.CallOptions;
Expand All @@ -35,6 +37,7 @@
import io.grpc.Status;
import io.grpc.Status.Code;
import io.grpc.binder.AndroidComponentAddress;
import io.grpc.binder.AsyncSecurityPolicy;
import io.grpc.binder.BinderServerBuilder;
import io.grpc.binder.HostServices;
import io.grpc.binder.SecurityPolicy;
Expand Down Expand Up @@ -381,6 +384,34 @@ public void testBlackHoleSecurityPolicyConnectTimeout() throws Exception {
blockingSecurityPolicy.provideNextCheckAuthorizationResult(Status.OK);
}

@Test
public void testAsyncSecurityPolicyFailure() throws Exception {
SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
transport = new BinderClientTransportBuilder()
.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 testAsyncSecurityPolicySuccess() throws Exception {
SettableAsyncSecurityPolicy securityPolicy = new SettableAsyncSecurityPolicy();
transport = new BinderClientTransportBuilder()
.setSecurityPolicy(securityPolicy)
.build();
securityPolicy.setAuthorizationResult(Status.PERMISSION_DENIED);
transport.start(transportListener).run();
Status transportStatus = transportListener.awaitShutdown();
assertThat(transportStatus.getCode()).isEqualTo(Code.PERMISSION_DENIED);
transportListener.awaitTermination();
}

private static void startAndAwaitReady(
BinderTransport.BinderClientTransport transport, TestTransportListener transportListener)
throws Exception {
Expand Down Expand Up @@ -540,4 +571,27 @@ public Status checkAuthorization(int uid) {
}
}
}

/**
* An AsyncSecurityPolicy that lets a test specify the outcome of checkAuthorizationAsync().
*/
static class SettableAsyncSecurityPolicy extends AsyncSecurityPolicy {
private SettableFuture<Status> result = SettableFuture.create();

public void clearAuthorizationResult() {
result = SettableFuture.create();
}

public boolean setAuthorizationResult(Status status) {
return result.set(status);
}

public boolean setAuthorizationException(Throwable t) {
return result.setException(t);
}

public ListenableFuture<Status> checkAuthorizationAsync(int uid) {
return Futures.nonCancellationPropagating(result);
}
}
}
75 changes: 43 additions & 32 deletions binder/src/main/java/io/grpc/binder/internal/BinderTransport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Ticker;
import com.google.common.base.Verify;
import com.google.common.util.concurrent.FutureCallback;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
import io.grpc.Attributes;
import io.grpc.CallOptions;
Expand All @@ -47,6 +49,7 @@
import io.grpc.Status;
import io.grpc.StatusException;
import io.grpc.binder.AndroidComponentAddress;
import io.grpc.binder.AsyncSecurityPolicy;
import io.grpc.binder.InboundParcelablePolicy;
import io.grpc.binder.SecurityPolicy;
import io.grpc.internal.ClientStream;
Expand Down Expand Up @@ -743,8 +746,8 @@ void notifyTerminated() {
@Override
@GuardedBy("this")
protected void handleSetupTransport(Parcel parcel) {
// Add the remote uid to our attributes.
attributes = setSecurityAttrs(attributes, Binder.getCallingUid());
int remoteUid = Binder.getCallingUid();
attributes = setSecurityAttrs(attributes, remoteUid);
if (inState(TransportState.SETUP)) {
int version = parcel.readInt();
IBinder binder = parcel.readStrongBinder();
Expand All @@ -755,46 +758,54 @@ protected void handleSetupTransport(Parcel parcel) {
shutdownInternal(
Status.UNAVAILABLE.withDescription("Malformed SETUP_TRANSPORT data"), true);
} else {
offloadExecutor.execute(() -> checkSecurityPolicy(binder));
ListenableFuture<Status> authFuture = (securityPolicy instanceof AsyncSecurityPolicy) ?
((AsyncSecurityPolicy) securityPolicy).checkAuthorizationAsync(remoteUid) :
Futures.submit(() -> securityPolicy.checkAuthorization(remoteUid), offloadExecutor);
Futures.addCallback(
authFuture,
new FutureCallback<Status>() {
@Override
public void onSuccess(Status result) { handleAuthResult(binder, result); }

@Override
public void onFailure(Throwable t) { handleAuthResult(t); }
},
offloadExecutor);
}
}
}

private void checkSecurityPolicy(IBinder binder) {
Status authorization;
Integer remoteUid;
synchronized (this) {
remoteUid = attributes.get(REMOTE_UID);
}
if (remoteUid == null) {
authorization = Status.UNAUTHENTICATED.withDescription("No remote UID available");
} else {
authorization = securityPolicy.checkAuthorization(remoteUid);
}
synchronized (this) {
if (inState(TransportState.SETUP)) {
if (!authorization.isOk()) {
shutdownInternal(authorization, true);
} else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) {
shutdownInternal(
Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true);
} else {
// Check state again, since a failure inside setOutgoingBinder (or a callback it
// triggers), could have shut us down.
if (!isShutdown()) {
setState(TransportState.READY);
attributes = clientTransportListener.filterTransport(attributes);
clientTransportListener.transportReady();
if (readyTimeoutFuture != null) {
readyTimeoutFuture.cancel(false);
readyTimeoutFuture = null;
}
private synchronized void handleAuthResult(IBinder binder, Status authorization) {
if (inState(TransportState.SETUP)) {
if (!authorization.isOk()) {
shutdownInternal(authorization, true);
} else if (!setOutgoingBinder(OneWayBinderProxy.wrap(binder, offloadExecutor))) {
shutdownInternal(
Status.UNAVAILABLE.withDescription("Failed to observe outgoing binder"), true);
} else {
// Check state again, since a failure inside setOutgoingBinder (or a callback it
// triggers), could have shut us down.
if (!isShutdown()) {
setState(TransportState.READY);
attributes = clientTransportListener.filterTransport(attributes);
clientTransportListener.transportReady();
if (readyTimeoutFuture != null) {
readyTimeoutFuture.cancel(false);
readyTimeoutFuture = null;
}
}
}
}
}

private synchronized void handleAuthResult(Throwable t) {
shutdownInternal(
Status.INTERNAL
.withDescription("Could not evaluate SecurityPolicy")
.withCause(t),
true);
}

@GuardedBy("this")
@Override
protected void handlePingResponse(Parcel parcel) {
Expand Down

0 comments on commit e13212a

Please # to comment.