feat(auth): Add support for Regional Access Boundaries#13499
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 support for Regional Access Boundaries (RAB) across various Google credential types by implementing the RegionalAccessBoundaryProvider interface and managing the lifecycle of boundaries via a new RegionalAccessBoundaryManager. The feedback highlights a few critical improvement opportunities: adding null checks for getAudience() in both ExternalAccountAuthorizedUserCredentials and ExternalAccountCredentials to prevent potential NullPointerExceptions, and performing a defensive copy of the locations list in the RegionalAccessBoundary constructor to guarantee the class's immutability.
| Matcher matcher = WORKFORCE_AUDIENCE_PATTERN.matcher(getAudience()); | ||
| if (!matcher.matches()) { | ||
| throw new IllegalStateException( | ||
| "The provided audience is not in the correct format for a workforce pool. " | ||
| + "Refer: https://docs.cloud.google.com/iam/docs/principal-identifiers"); | ||
| } |
There was a problem hiding this comment.
The getAudience() method is annotated with @Nullable and can return null. Calling WORKFORCE_AUDIENCE_PATTERN.matcher(getAudience()) directly without a null check will throw a NullPointerException if the audience is null. We should add a null check and throw an IllegalStateException with a clear message.
String audience = getAudience();
if (audience == null) {
throw new IllegalStateException(
"The audience is null, which is not in the correct format for a workforce pool.");
}
Matcher matcher = WORKFORCE_AUDIENCE_PATTERN.matcher(audience);
if (!matcher.matches()) {
throw new IllegalStateException(
"The provided audience is not in the correct format for a workforce pool. "
+ "Refer: https://docs.cloud.google.com/iam/docs/principal-identifiers");
}| Matcher workforceMatcher = WORKFORCE_AUDIENCE_PATTERN.matcher(getAudience()); | ||
| if (workforceMatcher.matches()) { | ||
| String poolId = workforceMatcher.group("pool"); | ||
| return String.format( | ||
| OAuth2Utils.IAM_CREDENTIALS_ALLOWED_LOCATIONS_URL_FORMAT_WORKFORCE_POOL, poolId); | ||
| } | ||
|
|
||
| Matcher workloadMatcher = WORKLOAD_AUDIENCE_PATTERN.matcher(getAudience()); |
There was a problem hiding this comment.
The getAudience() method can return null. Storing the audience in a local variable and checking it for null first prevents a potential NullPointerException and avoids redundant calls to getAudience() on the validation path.
String audience = getAudience();
if (audience == null) {
throw new IllegalStateException(
"The audience is null, which is not in a valid format for either a workload identity pool or a workforce pool.");
}
Matcher workforceMatcher = WORKFORCE_AUDIENCE_PATTERN.matcher(audience);
if (workforceMatcher.matches()) {
String poolId = workforceMatcher.group("pool");
return String.format(
OAuth2Utils.IAM_CREDENTIALS_ALLOWED_LOCATIONS_URL_FORMAT_WORKFORCE_POOL, poolId);
}
Matcher workloadMatcher = WORKLOAD_AUDIENCE_PATTERN.matcher(audience);| this.locations = | ||
| locations == null | ||
| ? Collections.<String>emptyList() | ||
| : Collections.unmodifiableList(locations); |
There was a problem hiding this comment.
To ensure the immutability of RegionalAccessBoundary, we should perform a defensive copy of the locations list passed to the constructor. Otherwise, if the caller modifies the original list, the internal state of this object will also change.
| this.locations = | |
| locations == null | |
| ? Collections.<String>emptyList() | |
| : Collections.unmodifiableList(locations); | |
| this.locations = | |
| locations == null | |
| ? Collections.<String>emptyList() | |
| : Collections.unmodifiableList(new java.util.ArrayList<>(locations)); |
| */ | ||
| package com.google.cloud.dataplex.v1; | ||
|
|
||
|
|
The Regional Access Boundaries PR to main. Contains all the changes merged to the feature branch rebased on top of main.
P.S. Opening the PR directly to main as feature branch regional-access-boundaries has drifted from main and opening a rebased-PR to the feature branch shows 5k+ lines of code diff.