Skip to content

feat: normalize failed authorization future status#12899

Open
Zhengcy05 wants to merge 2 commits into
grpc:masterfrom
Zhengcy05:feat/binder-opaque-auth-failure-status
Open

feat: normalize failed authorization future status#12899
Zhengcy05 wants to merge 2 commits into
grpc:masterfrom
Zhengcy05:feat/binder-opaque-auth-failure-status

Conversation

@Zhengcy05

Copy link
Copy Markdown
Contributor

Background

fixed #12873

This PR makes binder server authorization failures report a uniform opaque status to peers.

Previously, BinderTransportSecurity.ServerAuthInterceptor handled failed authorization futures differently depending on whether the future was already completed. The pending path returned an opaque INTERNAL failure, but the Futures.getDone() path reused the underlying exception message as the status description. That allowed later calls to expose messages like "ouch".

Changes

  • Centralized failed-authorization-future to Status mapping in BinderTransportSecurity
  • Made both the pending callback path and the already-completed Futures.getDone() path return the same opaque INTERNAL status with description "Authorization future failed"
  • Added regression assertions for failed future descriptions in RobolectricBinderSecurityTest
  • Added coverage for the cached/already-completed failed future case to ensure later calls do not leak the underlying exception message

Validation

  • ./gradlew :grpc-binder:testDebugUnitTest --tests io.grpc.binder.RobolectricBinderSecurityTest

@AgraVator AgraVator left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work addressing #12873!

A couple of minor suggestions for completeness:

  1. Test Naming Clarification:
    In RobolectricBinderSecurityTest.java, testAsyncServerSecurityPolicy_failedFuture_cachedFailureIsOpaque implies the failure was stored in cache. Because TransportAuthorizationState evicts failed futures on onFailure to allow retries, consider renaming to testAsyncServerSecurityPolicy_failedFuture_subsequentCallHasOpaqueFailure.

  2. Defensive Sync Exception Guard:
    In BinderTransportSecurity.ServerAuthInterceptor.interceptCall, wrapping transportAuthState.checkAuthorization(...) in a try-catch (Throwable t) block ensures any synchronous exceptions thrown by custom checkers close the call safely.

  3. Cancellation Test Coverage:
    Consider adding a test for cancelled authorization futures (CancellationException).

@Zhengcy05

Copy link
Copy Markdown
Contributor Author

Nice work addressing #12873!干得漂亮 #12873

A couple of minor suggestions for completeness:为了完整性,有几个小建议:

  1. Test Naming Clarification:**测试命名澄清 **:
    In RobolectricBinderSecurityTest.java, testAsyncServerSecurityPolicy_failedFuture_cachedFailureIsOpaque implies the failure was stored in cache. Because TransportAuthorizationState evicts failed futures on onFailure to allow retries, consider renaming to testAsyncServerSecurityPolicy_failedFuture_subsequentCallHasOpaqueFailure.在 RobolectricBinderSecurityTest.java 中,testAsyncServerSecurityPolicy_failedFuture_cachedFailureIsOpaque 意味着失败被存储在缓存中。由于 TransportAuthorizationState 会在 onFailure 上淘汰失败的未来,不允许重试,考虑将重命名为 testAsyncServerSecurityPolicy_failedFuture_subsequentCallHasOpaqueFailure
  2. Defensive Sync Exception Guard:**防守同步例外护卫 **:
    In BinderTransportSecurity.ServerAuthInterceptor.interceptCall, wrapping transportAuthState.checkAuthorization(...) in a try-catch (Throwable t) block ensures any synchronous exceptions thrown by custom checkers close the call safely.在 BinderTransportSecurity.ServerAuthInterceptor.interceptCall 中,将 transportAuthState.checkAuthorization(...) 包裹在 try-catch(可投掷 t) 块中,确保自定义检查器抛出的任何同步异常都能安全关闭调用。
  3. Cancellation Test Coverage:取消测试覆盖:
    Consider adding a test for cancelled authorization futures (CancellationException).考虑添加一个针对已取消授权期货的测试(取消例外)。

Thank you very much, I will make corrections

@Zhengcy05

Copy link
Copy Markdown
Contributor Author
  1. Defensive Sync Exception Guard:
    In BinderTransportSecurity.ServerAuthInterceptor.interceptCall, wrapping transportAuthState.checkAuthorization(...) in a try-catch (Throwable t) block ensures any synchronous exceptions thrown by custom checkers close the call safely.

Regarding the defensive sync exception guard: I'd prefer to keep this as-is.

The ServerPolicyChecker.checkAuthorizationForServiceAsync contract explicitly states that it never throws — failures are returned as a failed future. This is already enforced by ServerSecurityPolicy.checkAuthorizationForServiceAsync, which wraps synchronous policy calls in a try { ... } catch (Exception e) { return Futures.immediateFailedFuture(e); } block. So by the time the result reaches TransportAuthorizationState.checkAuthorization, it's already a ListenableFuture<Status> regardless of whether the underlying policy threw.

Adding a catch (Throwable) around transportAuthState.checkAuthorization(...) in interceptCall would guard against a contract violation that's already handled one layer down, and would mask programmer errors (e.g. NullPointerException from a bug in the cache logic) by silently closing the call instead of surfacing the problem. If a synchronous exception ever does escape here, it's better to let it propagate as a server-side error than to swallow it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

binder: SecurityPolicy *failure* (not auth rejection) should be described to peers in a uniform opaque way

2 participants