fix: Ensure authentication is required for all endpoints that require it and add an AuthInterceptor that can obtain credentials from a CredentialService #292

fjuma · 2025-09-22T21:03:29Z

Description

Thank you for opening a Pull Request!
Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

Follow the CONTRIBUTING Guide.
Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
- Important Prefixes for release-please:
  - fix: which represents bug fixes, and correlates to a SemVer patch.
  - feat: represents a new feature, and correlates to a SemVer minor.
  - feat!:, or fix!:, refactor!:, etc., which represent a breaking change (indicated by the !) and will result in a SemVer major.
Ensure the tests pass
Appropriate READMEs were updated (if necessary)

Fixes #275 and #274 🦕

gemini-code-assist · 2025-09-22T21:03:48Z

Summary of Changes

Hello @fjuma, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the security posture of the application by introducing a robust client-side authentication interception mechanism and enforcing authentication on critical server-side endpoints. It provides a flexible way to handle various security schemes (API Key, OAuth2, OIDC, Bearer) by integrating with a CredentialService, while also ensuring that public metadata endpoints remain accessible.

Highlights

AuthInterceptor Implementation: A new AuthInterceptor has been introduced to automatically add authentication details to outgoing client requests based on the AgentCard's security schemes and available credentials.
Credential Service: A CredentialService interface and its InMemoryContextCredentialService implementation are added to manage and retrieve credentials, typically tied to a session ID.
Server-Side Authentication: Server-side gRPC, JSON-RPC, and REST endpoints are now secured using Quarkus's @authenticated annotation, ensuring that requests to these services require authentication.
Public Endpoint Exemption: The /well-known/agent-card.json REST endpoint is explicitly marked with @permitAll to allow unauthenticated access, as it typically provides public metadata.
Testing Infrastructure: JUnit Jupiter dependencies were added, and comprehensive unit tests for the new AuthInterceptor and CredentialService were implemented.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces an authentication interceptor for client calls and enforces authentication on server endpoints. The client-side implementation has a critical logic flaw in how it processes security requirements, which could lead to incorrect authentication headers being sent. Additionally, the in-memory credential service is not thread-safe, which could cause issues in a concurrent environment. The tests for the new interceptor are also not comprehensive enough. The server-side changes to enforce authentication appear correct.

gemini-code-assist · 2025-09-22T21:05:48Z

...ansport/spi/src/main/java/io/a2a/client/transport/spi/interceptors/auth/AuthInterceptor.java

+    public PayloadAndHeaders intercept(String methodName, Object payload, Map<String, String> headers,
+                                       AgentCard agentCard, ClientCallContext clientCallContext) {
+        Map<String, String> updatedHeaders = new HashMap<>(headers == null ? new HashMap<>() : headers);
+        if (agentCard == null || agentCard.security() == null || agentCard.securitySchemes() == null) {
+            return new PayloadAndHeaders(payload, updatedHeaders);
+        }
+        for (Map<String, List<String>> requirement : agentCard.security()) {
+            for (String securitySchemeName : requirement.keySet()) {
+                String credential = credentialService.getCredential(securitySchemeName, clientCallContext);
+                if (credential != null && agentCard.securitySchemes().containsKey(securitySchemeName)) {
+                    SecurityScheme securityScheme = agentCard.securitySchemes().get(securitySchemeName);
+                    if (securityScheme == null) {
+                        continue;
+                    }
+                    if (securityScheme instanceof HTTPAuthSecurityScheme httpAuthSecurityScheme) {
+                        if (httpAuthSecurityScheme.getScheme().toLowerCase(Locale.ROOT).equals(BEARER_SCHEME)) {
+                            updatedHeaders.put(AUTHORIZATION, getBearerValue(credential));
+                            return new PayloadAndHeaders(payload, updatedHeaders);
+                        }
+                    } else if (securityScheme instanceof OAuth2SecurityScheme
+                            || securityScheme instanceof OpenIdConnectSecurityScheme) {
+                        updatedHeaders.put(AUTHORIZATION, getBearerValue(credential));
+                        return new PayloadAndHeaders(payload, updatedHeaders);
+                    } else if (securityScheme instanceof APIKeySecurityScheme apiKeySecurityScheme) {
+                        updatedHeaders.put(apiKeySecurityScheme.getName(), credential);
+                    }
+                }
+            }
+        }
+        return new PayloadAndHeaders(payload, updatedHeaders);
+    }


The logic for handling security requirements is incorrect. According to OpenAPI specifications, a list of security requirements acts as an OR, while schemes within a single requirement object act as an AND. The current implementation incorrectly handles this, especially with the early return for bearer-like schemes. This can lead to authentication failures when multiple schemes are required (AND condition) or when alternative requirements should be considered (OR condition). The method should iterate through each requirement, attempt to satisfy all schemes within it, and only if the entire requirement is satisfied, apply the headers and return.

public PayloadAndHeaders intercept(String methodName, Object payload, Map<String, String> headers, AgentCard agentCard, ClientCallContext clientCallContext) { Map<String, String> originalHeaders = headers == null ? new HashMap<>() : headers; if (agentCard == null || agentCard.security() == null || agentCard.security().isEmpty() || agentCard.securitySchemes() == null) { return new PayloadAndHeaders(payload, new HashMap<>(originalHeaders)); } for (Map<String, List<String>> requirement : agentCard.security()) { Map<String, String> headersForRequirement = new HashMap<>(); boolean requirementSatisfied = true; if (requirement.isEmpty()) { // Empty requirement means no security. This is a valid case for public endpoints. return new PayloadAndHeaders(payload, new HashMap<>(originalHeaders)); } for (String securitySchemeName : requirement.keySet()) { String credential = credentialService.getCredential(securitySchemeName, clientCallContext); SecurityScheme securityScheme = agentCard.securitySchemes().get(securitySchemeName); if (credential != null && securityScheme != null) { if (securityScheme instanceof HTTPAuthSecurityScheme httpAuthSecurityScheme) { if (BEARER_SCHEME.equalsIgnoreCase(httpAuthSecurityScheme.getScheme())) { headersForRequirement.put(AUTHORIZATION, getBearerValue(credential)); } else { // This interceptor only supports bearer auth for HTTPAuthSecurityScheme requirementSatisfied = false; break; } } else if (securityScheme instanceof OAuth2SecurityScheme || securityScheme instanceof OpenIdConnectSecurityScheme) { headersForRequirement.put(AUTHORIZATION, getBearerValue(credential)); } else if (securityScheme instanceof APIKeySecurityScheme apiKeySecurityScheme) { headersForRequirement.put(apiKeySecurityScheme.getName(), credential); } else { // Unknown security scheme type requirementSatisfied = false; break; } } else { // Credential not found or scheme not defined, so this requirement cannot be met. requirementSatisfied = false; break; } } if (requirementSatisfied) { Map<String, String> updatedHeaders = new HashMap<>(originalHeaders); updatedHeaders.putAll(headersForRequirement); return new PayloadAndHeaders(payload, updatedHeaders); } } // No security requirement could be satisfied. Send without auth. return new PayloadAndHeaders(payload, new HashMap<>(originalHeaders)); }

Going to ignore this since we're following the same logic from https://github.com/a2aproject/a2a-python/blob/main/src/a2a/client/auth/interceptor.py

...ain/java/io/a2a/client/transport/spi/interceptors/auth/InMemoryContextCredentialService.java

gemini-code-assist · 2025-09-22T21:05:48Z

...ort/spi/src/test/java/io/a2a/client/transport/spi/interceptors/auth/AuthInterceptorTest.java

+
+import static org.junit.jupiter.api.Assertions.*;
+
+public class AuthInterceptorTest {


The tests for AuthInterceptor only cover simple cases with a single security scheme. They do not cover more complex scenarios specified by OpenAPI, such as:

Multiple security schemes within a single requirement (AND condition).

Multiple alternative security requirements (OR condition).

It would be beneficial to add tests for these cases to ensure the interceptor logic is robust and correct. This is especially important given the logic flaw in the current AuthInterceptor implementation.

Additionally, there are several unused imports that can be removed (ParameterizedTest, Arguments, MethodSource, Stream).

Covered the same test cases as the Python implementation: https://github.com/a2aproject/a2a-python/blob/main/tests/client/test_auth_middleware.py

Can create issues for more advanced tests.

fjuma · 2025-09-23T20:26:19Z

Just FYI, am working on updating this as discussed here: #285

kabir · 2025-09-25T09:23:35Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a significant and valuable feature by adding an authentication layer to the client and enforcing it on the server-side endpoints. The implementation of AuthInterceptor to handle various security schemes is well-structured. I have identified a critical bug in the interceptor's logic that could lead to incorrect authentication behavior, a high-severity issue regarding an unsafe type cast, and a medium-severity suggestion for a minor performance improvement. Addressing these points will enhance the robustness and correctness of the new authentication mechanism.

gemini-code-assist · 2025-09-25T09:25:38Z

...ansport/spi/src/main/java/io/a2a/client/transport/spi/interceptors/auth/AuthInterceptor.java

+                    } else if (securityScheme instanceof APIKeySecurityScheme apiKeySecurityScheme) {
+                        updatedHeaders.put(apiKeySecurityScheme.getName(), credential);
+                    }


There's a potential bug in the security scheme handling. The interceptor returns immediately after processing HTTPAuthSecurityScheme, OAuth2SecurityScheme, or OpenIdConnectSecurityScheme. However, for APIKeySecurityScheme, it only adds the header but does not return, causing the loop to continue. This inconsistent behavior can lead to unexpected outcomes, such as sending multiple authentication headers or applying the wrong one if multiple schemes are satisfied.

Assuming the intent is to use the first credential found, you should also return after handling the APIKeySecurityScheme to ensure consistent behavior across all security scheme types.

} else if (securityScheme instanceof APIKeySecurityScheme apiKeySecurityScheme) { updatedHeaders.put(apiKeySecurityScheme.getName(), credential); return new PayloadAndHeaders(payload, updatedHeaders); }

@fjuma Not totally sure what Gemini is on about here :-) You're more familiar with this than me though

gemini-code-assist · 2025-09-25T09:25:38Z

...ain/java/io/a2a/client/transport/spi/interceptors/auth/InMemoryContextCredentialService.java

+            return null;
+        }
+
+        String sessionId = (String) clientCallContext.getState().get(SESSION_ID);


This is an unsafe cast from Object to String. If the value associated with the SESSION_ID key in the context state is not a String, this will throw a ClassCastException at runtime. It's safer to validate the type before casting to improve the robustness of the credential service.

Suggested change

String sessionId = (String) clientCallContext.getState().get(SESSION_ID);

Object sessionIdObj = clientCallContext.getState().get(SESSION_ID);

if (!(sessionIdObj instanceof String)) {

// Or log a warning that the session ID is not a String

return null;

}

String sessionId = (String) sessionIdObj;

gemini-code-assist · 2025-09-25T09:25:38Z

client/transport/grpc/src/main/java/io/a2a/client/transport/grpc/GrpcTransport.java

+                Metadata.Key<String> authorizationKey = Metadata.Key.of(
+                        AuthInterceptor.AUTHORIZATION,
+                        Metadata.ASCII_STRING_MARSHALLER);
+                metadata.put(authorizationKey, authorizationHeader);


The Metadata.Key for the Authorization header is created on every call within createGrpcMetadata. This is slightly inefficient. You can improve performance and readability by defining this as a static final constant in the class and reusing it here.

For example, you could add a field to GrpcTransport:

private static final Metadata.Key<String> AUTHORIZATION_METADATA_KEY = Metadata.Key.of( AuthInterceptor.AUTHORIZATION, Metadata.ASCII_STRING_MARSHALLER);

And then use it in createGrpcMetadata:

if (authorizationHeader != null) { metadata.put(AUTHORIZATION_METADATA_KEY, authorizationHeader); }

Updated this and also added the ability to handle additional headers for API keys (custom header names can be used to specify these).

kabir

To me it looks fine, but there are a few Gemini comments which may or may not be important

kabir · 2025-09-25T09:30:33Z

client/transport/grpc/src/main/java/io/a2a/client/transport/grpc/GrpcTransport.java

+                Metadata.Key<String> authorizationKey = Metadata.Key.of(
+                        AuthInterceptor.AUTHORIZATION,
+                        Metadata.ASCII_STRING_MARSHALLER);
+                metadata.put(authorizationKey, authorizationHeader);


kabir · 2025-09-25T09:36:53Z

...ansport/spi/src/main/java/io/a2a/client/transport/spi/interceptors/auth/AuthInterceptor.java

+                    } else if (securityScheme instanceof APIKeySecurityScheme apiKeySecurityScheme) {
+                        updatedHeaders.put(apiKeySecurityScheme.getName(), credential);
+                    }


@fjuma Not totally sure what Gemini is on about here :-) You're more familiar with this than me though

… .well-known URI

…ntialService

…d update the gRPC metadata with any auth headers that are set

… it and add an AuthInterceptor that can obtain credentials from a CredentialService (a2aproject#292) # Description Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly: - [x] Follow the [`CONTRIBUTING` Guide](../CONTRIBUTING.md). - [x] Make your Pull Request title in the <https://www.conventionalcommits.org/> specification. - Important Prefixes for [release-please](https://github.com/googleapis/release-please): - `fix:` which represents bug fixes, and correlates to a [SemVer](https://semver.org/) patch. - `feat:` represents a new feature, and correlates to a SemVer minor. - `feat!:`, or `fix!:`, `refactor!:`, etc., which represent a breaking change (indicated by the `!`) and will result in a SemVer major. - [x] Ensure the tests pass - [x] Appropriate READMEs were updated (if necessary) Fixes a2aproject#275 and a2aproject#274 🦕

fjuma requested a review from kabir September 22, 2025 21:03

gemini-code-assist bot reviewed Sep 22, 2025

View reviewed changes

fjuma force-pushed the security branch from 899c202 to 857389a Compare September 22, 2025 21:15

fjuma mentioned this pull request Sep 23, 2025

feat: ServerCallContext ports from Python implementation #285

Merged

fjuma force-pushed the security branch from 857389a to 367fdca Compare September 24, 2025 21:07

gemini-code-assist bot reviewed Sep 25, 2025

View reviewed changes

kabir reviewed Sep 25, 2025

View reviewed changes

fjuma added 3 commits September 25, 2025 14:32

fix: Ensure authentication is required for all A2A methods except the…

e1e543f

… .well-known URI

feat: Add an AuthInterceptor that can obtain credentials from a Crede…

e633090

…ntialService

feat: Add the ability to apply interceptors for the gRPC transport an…

36e1942

…d update the gRPC metadata with any auth headers that are set

fjuma force-pushed the security branch from 367fdca to 36e1942 Compare September 25, 2025 18:33

kabir approved these changes Sep 26, 2025

View reviewed changes

kabir merged commit 19119ab into a2aproject:main Sep 26, 2025
4 checks passed

fjuma mentioned this pull request Sep 26, 2025

Add an AuthInterceptor that can be used by an A2A client #274

Closed


		import static org.junit.jupiter.api.Assertions.*;

		public class AuthInterceptorTest {

-        String sessionId = (String) clientCallContext.getState().get(SESSION_ID);
+        Object sessionIdObj = clientCallContext.getState().get(SESSION_ID);
+        if (!(sessionIdObj instanceof String)) {
+            // Or log a warning that the session ID is not a String
+            return null;
+        }
+        String sessionId = (String) sessionIdObj;

fix: Ensure authentication is required for all endpoints that require it and add an AuthInterceptor that can obtain credentials from a CredentialService #292

fix: Ensure authentication is required for all endpoints that require it and add an AuthInterceptor that can obtain credentials from a CredentialService #292

Uh oh!

Conversation

fjuma commented Sep 22, 2025

Description

Uh oh!

gemini-code-assist bot commented Sep 22, 2025

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gemini-code-assist bot Sep 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjuma commented Sep 23, 2025

Uh oh!

kabir commented Sep 25, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 25, 2025

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!

gemini-code-assist bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 25, 2025

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!

kabir left a comment

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants