holdenk opened a new pull request, #58392: URL: https://github.com/apache/spark/pull/58392
### What changes were proposed in this pull request? `startGRPCService` registers `PreSharedKeyAuthenticationInterceptor` after the entries from `SparkConnectInterceptorRegistry` rather than before them. A `ServerBuilder` invokes interceptors in the reverse of the order they were added, so the one added last is the outermost and runs first. That is the documented contract of `ServerBuilder.intercept` -- "Interceptors run in the reverse order in which they are added, just as with consecutive calls to `ServerInterceptors.intercept()`": https://github.com/grpc/grpc-java/blob/v1.76.0/api/src/main/java/io/grpc/ServerBuilder.java#L134-L144 Adds `SparkConnectAuthInterceptorOrderSuite`, which configures an interceptor through `spark.connect.grpc.interceptor.classes` and asserts it does not run for a call that fails authentication, then that it does run for a call that passes -- so the first assertion cannot hold merely because the interceptor was never wired up. ### Why are the changes needed? Authentication was the innermost interceptor, so it ran last. Everything registered after it -- `RequestDecompressionInterceptor` and whatever an operator names in `spark.connect.grpc.interceptor.classes` -- had its `interceptCall` invoked on behalf of callers that were about to be rejected as unauthenticated. Verified with the new suite: on the previous order it fails with "1 did not equal 0 a configured interceptor ran for a call that failed authentication". Message payloads were not exposed, because gRPC stops delivering messages once the interceptor closes the call, so nothing attacker-supplied reached a listener. The exposure was the per-call setup work in the outer interceptors, and the fragility of an order in which any future interceptor doing real work in `interceptCall` or `onMessage` would run pre-authentication by default. ### Does this PR introduce _any_ user-facing change? Yes, for a server that configures both an authentication token and `spark.connect.grpc.interceptor.classes`: those interceptors no longer see calls that fail authentication. An interceptor that supplied the `Authorization` header itself, for example translating another scheme into a bearer token, relied on running ahead of authentication and no longer does. That ordering was never specified. ### How was this patch tested? `SparkConnectAuthInterceptorOrderSuite` plus `SparkConnectAuthSuite`, `InterceptorRegistrySuite` and `SparkConnectServiceKeepAliveSuite`: 16 tests, all passing, scalastyle clean. Reverting just the registration order and rerunning the new suite reproduces the failure quoted above, so the assertion is load-bearing. The reverse-invocation order and the fact that a closed call stops message delivery were both confirmed against grpc 1.76.0 with a standalone in-process harness: with the old registration order the outer interceptor's `interceptCall` ran once for an unauthenticated call while its listener saw zero `onMessage` and zero `onHalfClose` callbacks and the service handler was never invoked; with the new order it did not run at all, and on an authenticated call it ran and saw the message. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
