feat(auth): make RAB feature production ready#17390
Conversation
…lookup Initialize impersonated credentials inside ExternalAccountCredentials.__init__() when an impersonation URL is set. This ensures that RAB lookup targets the Service Account endpoint.
There was a problem hiding this comment.
Code Review
This pull request enables the Regional Access Boundary (RAB) feature by default by removing the GOOGLE_AUTH_TRUST_BOUNDARY_ENABLED environment variable gate. It also expands regional endpoint detection to support MTLS domains and updates external account credentials to initialize impersonated credentials immediately, delegating token and expiry management. Unit tests have been adjusted to remove obsolete environment variable mocks and to mock the RAB allowed locations endpoint. There are no review comments, so I have no feedback to provide.
lsirac
left a comment
There was a problem hiding this comment.
While the new unit tests cover the basic happy paths, there are a few critical testing gaps around backward compatibility, error handling, and subclassing that should be addressed before merging:
Backward Compatibility with Pickled States:
- The Gap: No test verifies unpickling a credential object serialized in a previous release.
- The Risk: Because
tokenandexpiryare now properties (data descriptors), they override instance__dict__lookups. Older unpickled credentials (which store'token'and'expiry'as plain keys) will have their cached tokens silently ignored, forcing redundant STS network calls.- Recommendation: Add a test that unpickles a legacy state and asserts the token is preserved.
Extensibility & Custom Subclassing:
- The Gap: No test verifies that external developers can safely subclass these credentials.
- The Risk: Triggering immediate impersonation in the base constructor calls
_constructor_args()before the subclass has finished setting up. This causes a fatalAttributeErroron subclass instantiation.- Recommendation: Add a test verifying instantiation of a mock custom subclass.
Error-Handling on Misconfiguration:
- The Gap: No test verifies that invalid/misconfigured setups fail gracefully.
- The Risk: Misconfiguring the credential source and supplier triggers the immediate copying flow, crashing with a confusing, internal
AttributeErrorinstead of raising a cleanexceptions.InvalidValuevalidation error.- Recommendation: Add a test asserting that invalid configurations raise
exceptions.InvalidValue.Programmatic Suppliers in
from_info:
- The Gap: No test verifies passing programmatic suppliers to the static
from_infomethod.- The Risk:
from_infoin bothaws.pyandidentity_pool.pyunconditionally updateskwargswithNonefrom the JSON dictionary, silently discarding the user's programmatic supplier.- Recommendation: Add a test calling
Credentials.from_info(info, supplier=my_callback)and assert the callback retention.callback)` and assert the check that the callback is preserved supplier works.
| "credentials" | ||
| ) | ||
|
|
||
| # Initialize impersonated credentials immediately upon creation. |
There was a problem hiding this comment.
We shouldn't initialize impersonated credentials inside the base constructor because it introduces a constructor initialization race condition.
When a subclass is instantiated, it calls super().init() before setting up its own local attributes. Because the parent class init immediately calls _initialize_impersonated_credentials(), it invokes the subclass's overridden _constructor_args() method. Since the subclass constructor hasn't finished running yet, _constructor_args() will attempt to access subclass variables that do not exist yet, resulting in an AttributeError
There was a problem hiding this comment.
Moved the impersonated initialization to before_request. I used a script to reproduce the issue locally and verify that it was fixed.
| self._rab_manager = self._impersonated_credentials._rab_manager | ||
|
|
||
| @property | ||
| def token(self): |
There was a problem hiding this comment.
Is this a necessary change?
Changing token and expiry from normal variables into @property functions introduces a hidden unpickling bug that will silently wipe out saved credentials.
When older saved (pickled) credentials are loaded back into memory, their tokens are restored intact into the object's internal dictionary under the standard 'token' and 'expiry' keys. However, because Python @property functions take priority over the internal dictionary and specifically look for _token and _expiry, accessing creds.token will return None. This silently drops valid saved tokens and forces unnecessary, expensive re-authentication network calls.
There was a problem hiding this comment.
Yes, good catch. I reverted the @Property wrappers entirely so token and expiry stay normal variables.
Instead, now we copy the saved token down to the Service Account object on demand before making HTTP requests.
… execution. When external runners (such as gcloud CLI) load saved access tokens directly onto external account credentials, initial token renewal is skipped. Previously, this left the inner Service Account object uninitialized, forcing background boundary lookups to query outer identity pool endpoints and fail with HTTP 500 errors. This change defers Service Account initialization until an outgoing HTTP request is made (before_request) and copies active tokens downward, guaranteeing correct endpoint routing without extra network calls.
Gaps 1, 2, and 3 are not relevant anymore since I changed the way we were fixing the gcloud impersonation RAB lookup bug. I added checks and tests for gap 4 though! |
This PR resolves issues identified during verification of gcloud Regional Access Boundary (RAB) flows and enables RAB verification by default:
GOOGLE_AUTH_TRUST_BOUNDARY_ENABLED) to execute RAB lookups by default across standard credential classes..rep.mtls.googleapis.com), bypassing redundant RAB lookups on secure transport boundaries.