Skip to content

feat(auth): make RAB feature production ready#17390

Open
nbayati wants to merge 9 commits into
googleapis:mainfrom
nbayati:rab-follow-up
Open

feat(auth): make RAB feature production ready#17390
nbayati wants to merge 9 commits into
googleapis:mainfrom
nbayati:rab-follow-up

Conversation

@nbayati

@nbayati nbayati commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

This PR resolves issues identified during verification of gcloud Regional Access Boundary (RAB) flows and enables RAB verification by default:

  • Removes the client-side environment variable feature gate (GOOGLE_AUTH_TRUST_BOUNDARY_ENABLED) to execute RAB lookups by default across standard credential classes.
  • Updates the Python auth SDK to recognize mTLS regional endpoints (.rep.mtls.googleapis.com), bypassing redundant RAB lookups on secure transport boundaries.
  • Defers Service Account impersonation setup until HTTP request execution before_request, propagating active cached tokens downward onto the inner credential to guarantee that access tokens restored across external CLI entrypoints correctly delegate regional access boundary (RAB) lookups to target Service Account endpoints without forcing redundant STS network renewal.

nbayati added 4 commits June 7, 2026 00:25
…lookup

Initialize impersonated credentials inside ExternalAccountCredentials.__init__() when an impersonation URL is set. This ensures that RAB lookup targets the Service Account endpoint.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nbayati nbayati marked this pull request as ready for review June 8, 2026 19:44
@nbayati nbayati requested review from a team as code owners June 8, 2026 19:44
@nbayati nbayati requested a review from daniel-sanche June 8, 2026 19:46

@lsirac lsirac left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Backward Compatibility with Pickled States:

    • The Gap: No test verifies unpickling a credential object serialized in a previous release.
    • The Risk: Because token and expiry are 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.
  2. 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 fatal AttributeError on subclass instantiation.
    • Recommendation: Add a test verifying instantiation of a mock custom subclass.
  3. 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 AttributeError instead of raising a clean exceptions.InvalidValue validation error.
    • Recommendation: Add a test asserting that invalid configurations raise exceptions.InvalidValue.
  4. Programmatic Suppliers in from_info:

    • The Gap: No test verifies passing programmatic suppliers to the static from_info method.
    • The Risk: from_info in both aws.py and identity_pool.py unconditionally updates kwargs with None from 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the impersonated initialization to before_request. I used a script to reproduce the issue locally and verify that it was fixed.

Comment thread packages/google-auth/google/auth/aws.py Outdated
Comment thread packages/google-auth/google/auth/identity_pool.py Outdated
self._rab_manager = self._impersonated_credentials._rab_manager

@property
def token(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/google-auth/google/auth/credentials.py
nbayati added 2 commits June 10, 2026 01:10
… 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.
@nbayati

nbayati commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author
  • Backward Compatibility with Pickled States:

    • The Gap: No test verifies unpickling a credential object serialized in a previous release.
    • The Risk: Because token and expiry are 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 fatal AttributeError on 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 AttributeError instead of raising a clean exceptions.InvalidValue validation 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_info method.
    • The Risk: from_info in both aws.py and identity_pool.py unconditionally updates kwargs with None from 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.

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!

@nbayati nbayati requested a review from lsirac June 10, 2026 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants