fix(auth): address concurrency crashes, state desync, and test coverage in async mTLS sessions V2 - #18355
Open
andyrzhao wants to merge 19 commits into
Open
fix(auth): address concurrency crashes, state desync, and test coverage in async mTLS sessions V2#18355andyrzhao wants to merge 19 commits into
andyrzhao wants to merge 19 commits into
Conversation
…overage in async mTLS sessions - Wrap await self._mtls_init_task in asyncio.shield in configure_mtls_channel to prevent external cancellations from destroying the init task - Catch asyncio.CancelledError in request() when _mtls_init_task was cancelled - Force credential refresh in _recover_auth_state when certificate rotation occurs, and conditionally increment _mtls_check_counter on check success - Support reconfiguration in configure_mtls_channel when task is None or done without unsafe task variable resets - Update close() to safely drain and close _old_auth_requests and _auth_request in a robust try...finally structure - Atomically update self._is_mtls and self._cached_cert upon _auth_request swap - Ensure 401 response is closed on timeout and update test assertions - Restore 5 unit tests for configure_mtls_channel and add e2e rotation test
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…eError in cert check, and reference InvalidOperation directly - Match synchronous behavior in mtls.py by setting cached_fingerprint = current_fingerprint when cached_cert is falsy to prevent spurious mTLS reconfigurations on 401 - Catch TypeError in _recover_auth_state parameter checks and update warning log to reflect fallback to credential refresh and retry - Reference exceptions.InvalidOperation directly in refresh exception handler instead of getattr fallback - Add unit tests for TypeError fallback, empty cached cert comparison, and InvalidOperation handling
…t calls and preserve metadata state on error - Distinguish between idempotent default calls and explicit reconfigurations in configure_mtls_channel - Preserve self._is_mtls and self._cached_cert on configuration failure to keep metadata in sync with the active _auth_request - Add unit tests verifying configure_mtls_channel idempotency and state preservation
…y robust across Python versions
…r on refresh - In AsyncAuthorizedSession.request()'s _recover_auth_state(), do not return 401 response on NotImplementedError so that mTLS reconfiguration can fall through to retry - Add unit test test_cert_rotation_credential_refresh_not_implemented_retries
…s_timeout_before_subsequent_retry
Pr 18334 with Patch
Contributor
There was a problem hiding this comment.
Code Review
This pull request enhances the asynchronous mTLS channel configuration and 401 retry/rotation handling in google-auth. Key changes include adding a force parameter to configure_mtls_channel, shielding the initialization task, improving credential refresh logic during reconfiguration, and adding comprehensive unit tests. The review feedback highlights a potential AttributeError when checking cancellation on a None task, suggests cancelling the previous initialization task during reconfiguration to avoid blocking, and recommends mocking time.monotonic locally rather than globally to comply with the repository's style guide.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…esolve mypy error
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
Fixes Add cert rotation handling for aiohttp (Async HTTP) #18227
Overview
This PR resolves concurrency crashes, state desynchronization, and authentication recovery regressions in
AsyncAuthorizedSessionduring asynchronous mTLS initialization and certificate rotation.It builds upon and resolves open gaps identified in #18224 and #18334:
request(), shieldedself._mtls_init_taskwithasyncio.shieldso that external caller cancellation does not cancel background session setup. Propagatesasyncio.CancelledErrorcleanly without leaving the session in a corrupt state.configure_mtls_channel(), awaits any active, in-flight_mtls_init_taskbefore creating a new one, eliminating race conditions that could clobberself._auth_request.client_cert_callbackback to default ADC (client_cert_callback=None).force: bool = Falseparameter toconfigure_mtls_channel()to allow explicit reconfiguration when dynamic callback providers rotate credentials without changing the callback reference._recover_auth_state(),self._mtls_check_counteris now incremented unconditionally at the end of the check block. If parameter checks fail (e.g.ClientCertErrororMutualTLSChannelError), subsequent concurrent tasks waiting on the lock properly detect that the check has already run and bypass redundant validation spikes.NotImplementedErrorhandling so that non-mTLS requests with unrefreshable credentials return 401 immediately rather than triggering a futile retry.exceptions.InvalidOperationhandling so that static credentials (e.g., CBA / static tokens) that cannot be refreshed will still retry if certificate rotation successfully reconfigured the mTLS channel (channel_reconfigured is True).close(), safely drains and closes_old_auth_requestsand the active_auth_requestinfinallyblocks, and cancels any pending initialization tasks without swallowing caller exceptions.mtls.check_parameters_for_unauthorized_response(), correctly defaultscached_fingerprint = current_fingerprintwhencached_certisNone, ensuring equality checks skip redundant reconfigurations when appropriate.