feat(xds): Implement CheckRequestBuilder for external authorization #12493

sauravzg · 2025-11-11T08:33:52Z

This PR sits on top of #12492, so only the last commit + any fixups need to be reviewed.

This commit introduces the CheckRequestBuilder library, which is responsible for constructing the CheckRequest message sent to the external authorization service.

The CheckRequestBuilder gathers information from various sources, including:

ServerCall attributes (local and remote addresses, SSL session).
MethodDescriptor (full method name).
Request headers.

It uses this information to populate the AttributeContext of the CheckRequest message, which provides the authorization service with the necessary context to make an authorization decision.

This commit also introduces the ExtAuthzCertificateProvider, a helper class for extracting certificate information, such as the principal and PEM-encoded certificate.

The relevant section of the spec is: https://github.com/grpc/proposal/pull/481/files#diff-6bb76a24aa142cc33db9218509688f01b30c8885d2fd8849f164244e68cd54eaR196-R250

Unit tests for the new components are also included.

[] feat(xds): Update Envoy proto definitions and add ExtAuthz gRPC service #12491 feat(xds): Update Envoy proto definitions and add ExtAuthz gRPC service
[] feat(xds): Add configuration objects for ExtAuthz and GrpcService #12492 feat(xds): Add configuration objects for ExtAuthz and GrpcService
feat(xds): Implement CheckRequestBuilder for external authorization #12493 feat(xds): Implement request builder for external authorization
[] TBD: PR_LINK feat(xds): Add header mutations library
[] TBD: PR_LINK feat(xds): Implement response handling for external authorization
[] TBD: PR_LINK feat(xds): Add ExtAuthzClientInterceptor and related components
[] TBD: PR_LINK feat(xds): Add ExtAuthzServerInterceptor and tests
[] TBD: PR_LINK feat(xds): Add ExternalAuthorizationFilter

This commit introduces configuration objects for the external authorization (ExtAuthz) filter and the gRPC service it uses. These classes provide a structured, immutable representation of the configuration defined in the xDS protobuf messages. The main new classes are: - `ExtAuthzConfig`: Represents the configuration for the `ExtAuthz` filter, including settings for the gRPC service, header mutation rules, and other filter behaviors. - `GrpcServiceConfig`: Represents the configuration for a gRPC service, including the target URI, credentials, and other settings. - `HeaderMutationRulesConfig`: Represents the configuration for header mutation rules. This commit also includes parsers to create these configuration objects from the corresponding protobuf messages, as well as unit tests for the new classes.

This commit introduces the `CheckRequestBuilder` library, which is responsible for constructing the `CheckRequest` message sent to the external authorization service. The `CheckRequestBuilder` gathers information from various sources, including: - `ServerCall` attributes (local and remote addresses, SSL session). - `MethodDescriptor` (full method name). - Request headers. It uses this information to populate the `AttributeContext` of the `CheckRequest` message, which provides the authorization service with the necessary context to make an authorization decision. This commit also introduces the `ExtAuthzCertificateProvider`, a helper class for extracting certificate information, such as the principal and PEM-encoded certificate. Unit tests for the new components are also included.

ejona86 · 2025-12-29T21:55:44Z

xds/src/main/java/io/grpc/xds/internal/extauthz/CheckRequestBuilder.java

+/**
+ * Interface for building external authorization check requests.
+ */
+public interface CheckRequestBuilder {


What is the point of this interface existing? It doesn't seem to be for injection. The only time it is used with an implementation other than CheckRequestBuilderImpl seems to be as a mock... that has no calls to it within the tests. So that means it isn't used for verification or for injection of behavior.

The Factory is even more baffling.

ejona86 · 2025-12-29T22:04:03Z

xds/src/main/java/io/grpc/xds/internal/extauthz/CheckRequestBuilder.java

+      CheckRequestParams.Builder paramsBuilder =
+          CheckRequestParams.builder().setMethodDescriptor(serverCall.getMethodDescriptor())
+              .setHeaders(headers).setRequestTime(requestTime);
+      java.net.SocketAddress localAddress =


What's the point of this builder repacking dance? The very next call unpacks it. Have build() return the AttributeContext.Builder and then set these things afterward. Oh! You already have the method buildAttributeRequest(); just call it here. As far as I can tell, CheckRequestParams just shouldn't exist.

ejona86 · 2025-12-29T22:12:29Z

xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzCertificateProvider.java

+   * @param cert The certificate.
+   * @return The principal.
+   */
+  String getPrincipal(X509Certificate cert);


Nothing about this interface is a provider. This is just some utility that extracts a bit of information from a cert someone else provides. Just call the code directly. If you need something for a mock, have the code that needs it create the interface, unless you think it'll be heavily used.

ejona86 · 2025-12-29T22:15:36Z

xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzCertificateProvider.java

+          }
+        }
+      } catch (java.security.cert.CertificateParsingException e) {
+        logger.log(Level.WARNING, "Error parsing certificate SANs. " + "This is not expected,"


We can't be logging every RPC. Especially for something the server operator can't do much about. having this at FINE would be okay, but definitely not at WARNING. Ditto for the other similar cases in this PR.

ejona86 · 2025-12-29T22:15:47Z

xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzCertificateProvider.java

+          }
+        }
+      } catch (java.security.cert.CertificateParsingException e) {
+        logger.log(Level.WARNING, "Error parsing certificate SANs. " + "This is not expected,"


Remove the unnecessary string concatenation.

ejona86 · 2025-12-29T22:28:36Z

xds/src/main/java/io/grpc/xds/internal/extauthz/ExtAuthzCertificateProvider.java

+        }
+      } catch (java.security.cert.CertificateParsingException e) {
+        logger.log(Level.WARNING, "Error parsing certificate SANs. " + "This is not expected,"
+            + "falling back to the subject according to the spec.", e);


"according to the spec" is really, really unhelpful, as there's no way to know what "spec" it is talking about.

ejona86 · 2025-12-29T22:34:17Z

xds/src/main/java/io/grpc/xds/internal/extauthz/CheckRequestBuilder.java

+              }
+            }
+          }
+        } catch (SSLPeerUnverifiedException e) {


When possible, reduce the scope of the try, so that it is clear what is being handled. Especially in cases like this that lots of lines are throwing lots of different exceptions.

Certificate[] certs = null; try { certs = sslSession.get().getPeerCertificates(); } catch (SSLPeerUnverifiedException e) {

ejona86 · 2025-12-29T22:38:46Z

xds/src/main/java/io/grpc/xds/internal/extauthz/CheckRequestBuilder.java

+      reqBuilder.setTime(requestTime);
+      AttributeContext.HttpRequest.Builder httpReqBuilder =
+          AttributeContext.HttpRequest.newBuilder();
+      httpReqBuilder.setPath(fullMethodName);


gRPC Java does not prefix the method name with / in the MethodDescriptor API. So the path prefix needs to be added here.

ejona86 · 2025-12-29T22:40:17Z

xds/src/main/java/io/grpc/xds/internal/extauthz/CheckRequestBuilder.java

+  final class CheckRequestBuilderImpl implements CheckRequestBuilder {
+    private static final Logger logger = Logger.getLogger(CheckRequestBuilderImpl.class.getName());
+
+    private static final String METHOD = "POST";


Please inline these. They don't help anyone to be defined up here, and just make it more likely to be buggy because nobody's actually bothering to see the strings used. (I know some people think it is good practice to never use literals, but I vehemently disagree. Especially as done here where they have lost all semantics by being moved away from their usage using common words and without documentation.) If they are used multiple times, constants can be fine. If you want to give a name to a literal (like SIZE, if it weren't being passed to setSize()), just make it a local variable.

ejona86 · 2025-12-29T22:49:04Z

xds/src/main/java/io/grpc/xds/internal/extauthz/CheckRequestBuilder.java

+      Iterable<byte[]> binaryValues =
+          headers.getAll(Metadata.Key.of(key, Metadata.BINARY_BYTE_MARSHALLER));
+      if (binaryValues == null) {
+        // Unreachable code, since we iterate over the keys. Exists for defensive programming.


Delete this. It means there is a bug. You've gone out of your way to use Optional for something impossible. If Optional was needed for other reasons such that you'd be reusing the error handling, I could understand it. But there's no point here, and it makes the code more likely to have bugs. Delete the Optional.

sauravzg changed the title ~~Feat/request builder~~ feat(xds): Implement CheckRequestBuilder for external authorization Nov 11, 2025

sauravzg force-pushed the feat/request-builder branch 4 times, most recently from 55be36c to 7b5cd3c Compare November 13, 2025 18:51

sauravzg added 2 commits November 17, 2025 10:33

sauravzg force-pushed the feat/request-builder branch from 7b5cd3c to 2d08a46 Compare November 17, 2025 10:36

ejona86 reviewed Dec 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(xds): Implement CheckRequestBuilder for external authorization #12493

feat(xds): Implement CheckRequestBuilder for external authorization #12493

Uh oh!

sauravzg commented Nov 11, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

ejona86 Dec 29, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

feat(xds): Implement CheckRequestBuilder for external authorization #12493

Are you sure you want to change the base?

feat(xds): Implement CheckRequestBuilder for external authorization #12493

Uh oh!

Conversation

sauravzg commented Nov 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants