Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SecurityPolicy should support async/slow implementations #10566

Open
mateusazis opened this issue Sep 18, 2023 · 1 comment
Open

SecurityPolicy should support async/slow implementations #10566

mateusazis opened this issue Sep 18, 2023 · 1 comment

Comments

@mateusazis
Copy link
Contributor

mateusazis commented Sep 18, 2023

Is your feature request related to a problem?

The SecurityPolicy class defines an abstract method that returns a Status. If computing such status requires running some slow piece of code (that, for instance, reads flags from storage or from the network), then the only option is to block the calling thread until such operation is completed.

public class FlagBasedSecurityPolicy extends SecurityPolicy {
  @Override
  public Status checkAuthorization(int uid) {
    try {
      // must block the calling thread to satisfy SecurityPolicy's API
      return fetchAllowlistedUids().get().contains(uid) ? Status.OK : Status.PERMISSION_DENIED;
    } catch (InterruptedException e) {
      return Status.fromThrowable(e);
    }
  }

  // does not block
  private ListenableFuture<List<Integer>> fetchAllowlistedUids() {
    // ...
  }
}

Blocking is generally undesirable, because it monopolizes a thread from the system for mostly idle work, which leads to sub-optimal performance by requiring new threads to be spawned or reducing the amount of threads available, thus the responsiveness of the system.

Describe the solution you'd like

SecurityPolicy (or an equivalent replacement) should provide an API for returning a ListenableFuture<Status> instead. Clients are free to implement it however they see fit. The case commonly supported today (simple non-blocking computations) would be equivalent to returning a Futures.immediateFuture(status) (from Guava).

public interface AsyncSecurityPolicy {
  ListenableFuture<Status> checkAuthorization(int uid);
}

Then the code above can be written as non-blocking:

public class FlagBasedSecurityPolicy implements AsyncSecurityPolicy {
  @Override
  public ListenableFuture<Status> checkAuthorization(int uid) {
    return Futures.transform(
      fetchAllowlistedUids(),
      allowlist -> allowlist.contains(uid) ? Status.OK : Status.PERMISSION_DENIED,
      someExecutor);
  }

  // does not block
  private ListenableFuture<List<Integer>> fetchAllowlistedUids() {
    // ...
  }
}

Describe alternatives you've considered

N/A

Additional context

Some apps use StrictPolicy.VmPolicy to instruct which kinds of threads are appropriate for different kinds of tasks (e.g. lightweight computations vs blocking network access). It can be used to configure different "penalties", like just logging in production or crashing in development. The issue above has triggered such violations during the development of an internal Google app.

@ejona86
Copy link
Member

ejona86 commented Sep 19, 2023

@ejona86 ejona86 added this to the Unscheduled milestone Sep 19, 2023
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Oct 5, 2023
…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 added a commit to mateusazis/grpc-java that referenced this issue Oct 5, 2023
…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 added a commit to mateusazis/grpc-java that referenced this issue Oct 5, 2023
…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
ejona86 pushed a commit that referenced this issue Oct 20, 2023
)

Allow a security policy to 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: #10566
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Oct 20, 2023
This is the async variant of SecurityPolicy, allowing callers to implement security checks based on slow calls that aren't meant to block the gRPC thread.

BinderTransportSecurity.checkAuthorization **STILL** blocks while attempting to resolve the ListenableFuture<Status> it gets from the policy object. That will still be adressed in a follow-up.

Relate issue: grpc#10566
markb74 pushed a commit that referenced this issue Oct 26, 2023
This is the async variant of SecurityPolicy, allowing callers to implement security checks based on slow calls that aren't meant to block the gRPC thread.

BinderTransportSecurity.checkAuthorization **STILL** blocks while attempting to resolve the ListenableFuture<Status> it gets from the policy object. That will still be adressed in a follow-up.

Relate issue: #10566
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Oct 26, 2023
- Introduce PendingAuthListener to handle a ListenableFuture<Status>, progressing the gRPC through each stage in sequence once the future completes and is OK.
- Move unit tests away from `checkAuthorizationForService` and into `checkAuthorizationForServiceAsync` since that should be the only method called in production now.

This should be the last PR to address grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Oct 26, 2023
- Introduce PendingAuthListener to handle a ListenableFuture<Status>, progressing the gRPC through each stage in sequence once the future completes and is OK.
- Move unit tests away from `checkAuthorizationForService` and into `checkAuthorizationForServiceAsync` since that should be the only method called in production now.

This should be the last PR to address grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Oct 26, 2023
- Introduce PendingAuthListener to handle a ListenableFuture<Status>, progressing the gRPC through each stage in sequence once the future completes and is OK.
- Move unit tests away from `checkAuthorizationForService` and into `checkAuthorizationForServiceAsync` since that should be the only method called in production now.

This should be the last PR to address grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Oct 26, 2023
- Introduce PendingAuthListener to handle a ListenableFuture<Status>, progressing the gRPC through each stage in sequence once the future completes and is OK.
- Move unit tests away from `checkAuthorizationForService` and into `checkAuthorizationForServiceAsync` since that should be the only method called in production now.
- Some tests in `ServerSecurityPolicyTest` had their expectations updated; they previously called synchornous APIs that transformed failed `ListenableFuture<Status>` into one or another status. Now, we call the sync API, so those transformations do not happen anymore, thus the test needs to deal with failed futures directly.
- I couldn't figure out if this PR needs extra tests. AFAICT `BinderSecurityTest` should already cover the new codepaths, but please let me know otherwise.

This should be the last PR to address grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Dec 12, 2023
Currently, if caching is enabled (as is often the case) and AsyncSecurityPolicy returns a failed future, then this future is cached forever, without giving the SecurityPolicy implementation a chance to be retried. Going forward, new invocations will trigger new security checks if the last one could not be completed successfuly.

Part of grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Dec 12, 2023
Currently, if caching is enabled (as is often the case) and AsyncSecurityPolicy returns a failed future, then this future is cached forever, without giving the SecurityPolicy implementation a chance to be retried. Going forward, new invocations will trigger new security checks if the last one could not be completed successfuly.

Part of grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Jan 8, 2024
This class was supposedly already tested via `ServerSecurityPolicyTest.java`, thought that class is actually triggering the short-circuit codepaths (thus, test coverage is reported at 0%).

After this PR, I shall send another one to adapt those tests to force the non-short-circuit async path.

Part of grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Jan 18, 2024
The usage of Futures.immediateFuture makes the implementation take several shortcuts to process server authorization. This leads to PendingAuthListener being under-tested.

This commit adds delays to the futures in the hope that they will be triggered by the new codepaths.

Before: https://fusion2.corp.google.com/invocations/fafaa334-2ac1-4d12-814c-33755fa08ad6/coverage
After: https://fusion2.corp.google.com/invocations/6e3b4b41-461b-418d-8e9f-d9043dc3e5c6/coverage

Part of grpc#10566.
mateusazis added a commit to mateusazis/grpc-java that referenced this issue Jan 18, 2024
I initially omitted the visibility modifier because this class began as an interface. Since it moved to an abstract class, we must make it public so it can be overriden by subclasses in the integrator's packages.

Part of grpc#10566.
jdcormie pushed a commit that referenced this issue Jan 22, 2024
I initially omitted the visibility modifier because this class began as an interface. Since it moved to an abstract class, we must make it public so it can be overriden by subclasses in the integrator's packages.

Part of #10566.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants