fix(google-auth): fail loudly when dynamically disabling mTLS on active sessions - #18005
Draft
gorthitk wants to merge 3 commits into
Draft
fix(google-auth): fail loudly when dynamically disabling mTLS on active sessions#18005gorthitk wants to merge 3 commits into
gorthitk wants to merge 3 commits into
Conversation
…ve sessions (googleapis#17761) When mTLS is previously enabled on an active transport session (requests, urllib3, or aiohttp), attempting to disable mTLS mid-lifecycle in configure_mtls_channel() now raises a MutualTLSChannelError instead of exiting early or replacing adapters. This prevents thread-safety violations from connection pool mutation and eliminates zombie state mismatches where active sessions continue sending client certificates while auth checks treat mTLS as disabled.
Contributor
There was a problem hiding this comment.
Code Review
This pull request prevents mid-lifecycle transitions from mTLS-enabled to mTLS-disabled states on active sessions across the aio, requests, and urllib3 transports by raising a MutualTLSChannelError. However, the review feedback highlights that raising this exception replaces historical graceful fallback behaviors (such as falling back to standard TLS or returning False), which introduces breaking changes for downstream users and violates backwards compatibility.
nbayati
reviewed
Aug 7, 2026
| # Dynamically disabling mTLS on an active session is unsafe in concurrent | ||
| # environments and can cause a zombie state mismatch where mTLS contexts | ||
| # remain attached while auth checks believe mTLS is disabled. | ||
| if getattr(self, "_is_mtls", False): |
Contributor
There was a problem hiding this comment.
self._is_mtls is explicitly initialized to False in init across AsyncAuthorizedSession, AuthorizedSession, and AuthorizedHttp. Using getattr(self, "_is_mtls", False) is unnecessary, we can access self._is_mtls directly.
| # remain attached while auth checks believe mTLS is disabled. | ||
| if getattr(self, "_is_mtls", False): | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." |
Contributor
There was a problem hiding this comment.
Please update this to reference AsyncAuthorizedSession instead of AuthorizedSession so async users aren't directed to the synchronous requests transport class.
Suggested change
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." | |
| "Cannot disable mTLS on an active session. A new AsyncAuthorizedSession must be created." |
| # Prevent mid-lifecycle transition from mTLS-enabled to mTLS-disabled state. | ||
| if getattr(self, "_is_mtls", False) and not is_mtls: | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." |
Contributor
There was a problem hiding this comment.
Same here
Suggested change
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." | |
| "Cannot disable mTLS on an active session. A new AsyncAuthorizedSession must be created." |
| use_client_cert = google.auth.transport._mtls_helper.check_use_client_cert() | ||
| if not use_client_cert: | ||
| # Dynamically disabling mTLS on an active session is unsafe in concurrent | ||
| # environments and can cause a zombie state mismatch where mTLS adapters |
Contributor
There was a problem hiding this comment.
nit
Suggested change
| # environments and can cause a zombie state mismatch where mTLS adapters | |
| # environments and can cause a state mismatch where mTLS adapters |
| use_client_cert = transport._mtls_helper.check_use_client_cert() | ||
| if not use_client_cert: | ||
| # Dynamically disabling mTLS on an active session is unsafe in concurrent | ||
| # environments and can cause a zombie state mismatch where mTLS connection |
Contributor
There was a problem hiding this comment.
nit:
Suggested change
| # environments and can cause a zombie state mismatch where mTLS connection | |
| # environments and can cause a state mismatch where mTLS connection |
parthea
marked this pull request as draft
August 10, 2026 19:17
Contributor
|
Hi @gorthitk, I'm going to mark this as draft since presubmits are failing but please feel free to mark it ready for review once checks are green |
added 2 commits
August 11, 2026 08:52
…tly in mTLS checks - Add _is_mtls_configured() helper across AuthorizedSession, AuthorizedHttp, and AsyncAuthorizedSession. - Inspect adapter types (_MutualTlsAdapter, _MutualTlsOffloadAdapter), PoolManager connection_pool_kw ssl_context, connector SSL context, and cached certificates directly. - Add unit tests verifying desynchronized state detection across all transports.
…yncAuthorizedSession
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
3 participants
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.
When mTLS is previously enabled on an active transport session (requests, urllib3, or aiohttp), attempting to disable mTLS mid-lifecycle in configure_mtls_channel() now raises a MutualTLSChannelError instead of exiting early or replacing adapters.
This prevents thread-safety violations from connection pool mutation and eliminates zombie state mismatches where active sessions continue sending client certificates while auth checks treat mTLS as disabled.
Fixes #17761