chore(auth): Porting the changes from the old RAB PR #13559#13713
chore(auth): Porting the changes from the old RAB PR #13559#13713vverman wants to merge 6 commits into
Conversation
googleapis#12787) Migrates RAB changes from the older repo -> https://github.com/googleapis/google-auth-library-java/tree/feat-tb-sa
…gleapis#12867) 1. The RAB refresh uses a direct executor with a fixed thread pool as opposed to instantiating a new thread each time. 2. The RAB env gate -> GOOGLE_AUTH_TRUST_BOUNDARY_ENABLE_EXPERIMENT has been removed. This means RAB refresh triggers by default. 3. Added other fixes/suggestions made in the previous Java [PR](googleapis/google-auth-library-java#1880).
…oogleapis#13331) In ComputeEngineCredentials when running on GKE platform, the getAccount() call may return a value which isn't an email. In this case the right behaviour is to skip RAB lookup which is what this PR does. Added tests.
There was a problem hiding this comment.
Code Review
This pull request introduces a Regional Access Boundary (RAB) mechanism to the Google Auth Library, allowing credentials to fetch and include 'x-allowed-locations' headers in API requests for regional security enforcement. The changes include the addition of a 'RegionalAccessBoundaryManager' for caching and asynchronous refreshing, and updates to various credential classes to integrate this functionality. The reviewer identified a critical issue regarding audience mismatch in the self-signed JWT flow and suggested skipping the RAB refresh in that scenario. Additionally, the reviewer recommended performance and correctness improvements, such as using case-insensitive hostname comparisons, removing redundant null checks, and optimizing map operations to reduce unnecessary object allocations.
| requestMetadata = addRegionalAccessBoundaryToRequestMetadata(uri, requestMetadata); | ||
| refreshRegionalAccessBoundaryWithSelfSignedJwtIfExpired(uri, requestMetadata); |
There was a problem hiding this comment.
In the self-signed JWT flow, the JWT is generated with an audience specific to the target API (e.g., https://pubsub.googleapis.com/). Sending this JWT to the IAM Credentials API (https://iamcredentials.googleapis.com/) will result in a 401/403 audience mismatch error in production, causing the Regional Access Boundary (RAB) refresh to consistently fail and enter cooldown. Since self-signed JWTs cannot be used to authenticate against the IAM Credentials API, we should skip the RAB refresh in this flow.
requestMetadata = addRegionalAccessBoundaryToRequestMetadata(uri, requestMetadata);
// Note: We skip calling refreshRegionalAccessBoundaryWithSelfSignedJwtIfExpired here
// because self-signed JWTs have a mismatched audience and will fail authentication
// at the IAM Credentials API, causing unnecessary cooldowns.| if (uri != null && uri.getHost() != null) { | ||
| String host = uri.getHost(); | ||
| if (host.endsWith(".rep.googleapis.com") || host.endsWith(".rep.sandbox.googleapis.com")) { | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
Hostname comparisons should be case-insensitive per RFC 3986. Converting the host to lowercase before the check ensures correctness. Note that a null check on uri.getHost() is unnecessary as the URI is already validated and guaranteed to have a host if it is not null.
| if (uri != null && uri.getHost() != null) { | |
| String host = uri.getHost(); | |
| if (host.endsWith(".rep.googleapis.com") || host.endsWith(".rep.sandbox.googleapis.com")) { | |
| return; | |
| } | |
| } | |
| if (uri != null) { | |
| String host = uri.getHost().toLowerCase(java.util.Locale.US); | |
| if (host.endsWith(".rep.googleapis.com") || host.endsWith(".rep.sandbox.googleapis.com")) { | |
| return; | |
| } | |
| } |
References
- A null check on a parsed URL component (like the host) is unnecessary if other parts of the constructor would have already failed with a malformed URL (e.g., one missing a scheme).
| if (uri != null && uri.getHost() != null) { | ||
| String host = uri.getHost(); | ||
| if (host.endsWith(".rep.googleapis.com") || host.endsWith(".rep.sandbox.googleapis.com")) { | ||
| return requestMetadata; | ||
| } | ||
| } |
There was a problem hiding this comment.
Hostname comparisons should be case-insensitive per RFC 3986. Converting the host to lowercase before checking the suffix ensures that mixed-case or uppercase hostnames are correctly matched. Note that a null check on uri.getHost() is unnecessary as the URI is already validated and guaranteed to have a host if it is not null.
| if (uri != null && uri.getHost() != null) { | |
| String host = uri.getHost(); | |
| if (host.endsWith(".rep.googleapis.com") || host.endsWith(".rep.sandbox.googleapis.com")) { | |
| return requestMetadata; | |
| } | |
| } | |
| if (uri != null) { | |
| String host = uri.getHost().toLowerCase(java.util.Locale.US); | |
| if (host.endsWith(".rep.googleapis.com") || host.endsWith(".rep.sandbox.googleapis.com")) { | |
| return requestMetadata; | |
| } | |
| } |
References
- A null check on a parsed URL component (like the host) is unnecessary if other parts of the constructor would have already failed with a malformed URL (e.g., one missing a scheme).
| Map<String, List<String>> newMetadata = new HashMap<>(requestMetadata); | ||
| newMetadata.put( | ||
| RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY, | ||
| Collections.singletonList(rab.getEncodedLocations())); | ||
| return ImmutableMap.copyOf(newMetadata); |
There was a problem hiding this comment.
Creating a HashMap, copying requestMetadata into it, and then copying it again into an ImmutableMap using ImmutableMap.copyOf on every single request introduces unnecessary object allocations and overhead on the critical path. Returning an unmodifiable map wrapper using Collections.unmodifiableMap avoids the second copy while maintaining immutability.
| Map<String, List<String>> newMetadata = new HashMap<>(requestMetadata); | |
| newMetadata.put( | |
| RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY, | |
| Collections.singletonList(rab.getEncodedLocations())); | |
| return ImmutableMap.copyOf(newMetadata); | |
| Map<String, List<String>> newMetadata = new HashMap<>(requestMetadata); | |
| newMetadata.put( | |
| RegionalAccessBoundary.X_ALLOWED_LOCATIONS_HEADER_KEY, | |
| Collections.singletonList(rab.getEncodedLocations())); | |
| return Collections.unmodifiableMap(newMetadata); |
| } | ||
| return requestMetadata; | ||
| } |
There was a problem hiding this comment.
If quotaProjectId is null, addQuotaProjectIdToRequestMetadata returns the mutable HashMap directly, exposing a mutable map to callers of getAdditionalHeaders(). Returning Collections.unmodifiableMap(requestMetadata) ensures consistency and immutability.
| } | |
| return requestMetadata; | |
| } | |
| } | |
| return Collections.unmodifiableMap(requestMetadata); | |
| } |
No description provided.