xds: Add ExtAuthzClientCall - #12897
Conversation
|
|
||
| @Override | ||
| public void onNext(CheckResponse value) { | ||
| // Note on exception safety: handleResponse() internally catches |
There was a problem hiding this comment.
Uncaught exception from onNext causes the stub to cancel the stream and route directly into onError(), which closes authzContext. Its closure does not rely on GC of the stream observer, and it cannot, because it has a direct reference to it from ExtAuthzClientCall.
In a unary rpc, onNext() delivers an inbound response payload; it does not signify that the RPC is complete. A unary RPC completes only when onClose / onCompleted() / onError() is invoked.
The comment needs to be corrected on both the above fronts.
Is relying on this mechanism "Valid Code"?
Mechanically, yes, gRPC will catch the exception and route to onError(). If an unhandled RuntimeException escapes onNext(), ClientCallImpl cancels the stream and delivers onError(), which drains delayedCall and closes authzContext.
However, intentionally relying on this as a design pattern is fragile and carries real risks:
A. Security Risk: Accidentally Failing Open on a DENY
This is the biggest risk:
// in onNext():
AuthzResponse authzResponse = responseHandler.handleResponse(value);
if (authzResponse.decision() == AuthzResponse.Decision.ALLOW) { ... }
else {
// Suppose decision is DENY, but building FailingCallWithTrailerMutations throws an exception
...
}If the external authorization server returned an explicit DENY response, but an unexpected RuntimeException is thrown while preparing the rejection:
- The exception escapes
onNext(). - The stub cancels the stream and invokes
onError(t). - In
onError():if (config.failureModeAllow()) { // FAILS OPEN: creates next.newCall() and forwards request to backend! }
If failure_mode_allow: true is configured (which is only intended for network/server outages reaching the authz service), a request that the authz server explicitly denied would be forwarded to the backend!
B. The "Throwing to Trigger Another Callback on Yourself" Anti-Pattern
Relying on throwing an unhandled exception out of onNext() so that the underlying framework cancels the transport stream and calls back into your own onError() is an indirect, surprising control flow:
- It generates unnecessary transport
RST_STREAMframes andFailed to read messageerror logs fromClientCallImplexception handling. - It makes debugging difficult because the root cause in
onNextgets swallowed into a transportStatus.CANCELLEDexception passed toonError.
C. Partial Failure State
If an exception occurs after delayedCall.setCall(call) was executed (for example, if callExecutor.execute(drain) throws a RejectedExecutionException):
- When
onError()subsequently runs, its call todelayedCall.setCall(...)returnsnullbecauserealCall != nullwas already set. The failure call cannot be attached, leaving the call in an inconsistent state.
There was a problem hiding this comment.
This was interesting.
So, practically it largely doesn't matter right now, because the current code is exception safe. So, a runtime exception shouldn't happen.
But given the security considerations, maybe we should be defensive, I'll have to see if other implementations in flight are as defensive and make changes. Keeping his comment open for now.
There was a problem hiding this comment.
I agree the code is exception safe. We can just remove the comment.
|
|
||
| private void setCallAndDrain(ClientCall<ReqT, RespT> call) { | ||
| Runnable drain = delayedCall.setCall(call); | ||
| if (drain != null) { |
There was a problem hiding this comment.
Do we really need this check (and the corresponding unit test allow_whenDelayedCallNotStarted_setCallReturnsNull) considering that the only code using this class is ExtAuthzClientCall that always starts the DelayedClientCall before invoking the rpc on the ext_auth stub?
There was a problem hiding this comment.
I think it's fair enough to be defensive and handle it nonetheless. I don't want to rely on the imlementation details of when the runnable is null and when not.
There was a problem hiding this comment.
It is in fact part of the implementation detail, since it is package private and not really a utility class that has other use cases. It falls under the YAGNI principle.
Part 3 of the client-side ext_authz filter. Sits on top of the response handling PR. Introduces the ClientInterceptor that performs async ext_authz checks on outgoing RPCs. Because the authorization call is asynchronous, outgoing sendMessage() and halfClose() frames are buffered in ExtAuthzClientCall until a decision arrives. On allow, buffered operations are replayed with any header mutations applied. On deny, the call is terminated immediately. Key classes: - AuthzCallbackObserver — async listener on the authz gRPC stream that signals the buffering call on completion. - ExtAuthzClientCall — buffers outgoing frames while authz is pending; drains or cancels based on the decision. - MutatingClientCall — wraps the real call to inject header mutations from the authz response. - FailingClientCall / FailingCallWithTrailerMutations — immediate termination paths for denied or disabled-filter scenarios.
08fa030 to
ac83094
Compare
Part 3 of the client-side ext_authz filter. Sits on top of the response handling PR.
Introduces the ClientInterceptor that performs async ext_authz checks on outgoing RPCs. Because the authorization call is asynchronous, outgoing sendMessage() and halfClose() frames are buffered in ExtAuthzClientCall until a decision arrives. On allow, buffered operations are replayed with any header mutations applied. On deny, the call is terminated immediately.
Key classes: