fix(google-auth): fail loudly when dynamically disabling mTLS on active sessions - #18005
fix(google-auth): fail loudly when dynamically disabling mTLS on active sessions#18005gorthitk wants to merge 1 commit 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.
gorthitk
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
| if getattr(self, "_is_mtls", False): | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." | ||
| ) |
There was a problem hiding this comment.
Raising a MutualTLSChannelError exception here replaces the historical graceful fallback behavior with an exception. This introduces breaking changes for downstream users and violates backwards compatibility.
References
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
| 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." | ||
| ) |
There was a problem hiding this comment.
Raising a MutualTLSChannelError exception here replaces the historical graceful fallback behavior with an exception. This introduces breaking changes for downstream users and violates backwards compatibility.
References
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
| if getattr(self, "_is_mtls", False): | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." | ||
| ) |
There was a problem hiding this comment.
Raising a MutualTLSChannelError exception here replaces the historical graceful fallback behavior (falling back to standard TLS/HTTPAdapter) with an exception. This introduces breaking changes for downstream users and violates backwards compatibility.
References
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
| 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." | ||
| ) |
There was a problem hiding this comment.
Raising a MutualTLSChannelError exception here replaces the historical graceful fallback behavior with an exception. This introduces breaking changes for downstream users and violates backwards compatibility.
References
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
| if getattr(self, "_is_mtls", False): | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Cannot disable mTLS on an active session. A new AuthorizedHttp must be created." | ||
| ) |
There was a problem hiding this comment.
Raising a MutualTLSChannelError exception here replaces the historical graceful fallback behavior (returning False and falling back to standard TLS/PoolManager) with an exception. This introduces breaking changes for downstream users and violates backwards compatibility.
References
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
| if getattr(self, "_is_mtls", False) and not found_cert_key: | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Cannot disable mTLS on an active session. A new AuthorizedHttp must be created." | ||
| ) |
There was a problem hiding this comment.
Raising a MutualTLSChannelError exception here replaces the historical graceful fallback behavior with an exception. This introduces breaking changes for downstream users and violates backwards compatibility.
References
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
| ) | ||
| 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 contexts |
There was a problem hiding this comment.
nit:
| # environments and can cause a zombie state mismatch where mTLS contexts | |
| # environments and can cause a state mismatch where mTLS contexts |
| # 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): |
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." |
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.
| "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." |
There was a problem hiding this comment.
Same here
| "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 |
There was a problem hiding this comment.
nit
| # 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 |
There was a problem hiding this comment.
nit:
| # environments and can cause a zombie state mismatch where mTLS connection | |
| # environments and can cause a state mismatch where mTLS connection |
| # 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): |
There was a problem hiding this comment.
Checking is_mtls is a good start, but I think it might not be enough. Since this is a flag that we set manually when attempting to establish mTLS, it might not always accurately reflect the transport state. We have previously identified and resolved bugs where is_mtls was out of sync with the reality of the transport. Relying on this check on its own makes me a bit nervous, especially since we're changing the behavior to raise an exception where we used to silently pass. Can we verify the transport adapter type or SSL context presence directly?
This applies to the other transports as well.
|
|
||
| # Prevent mid-lifecycle transition from mTLS-enabled to mTLS-disabled state. | ||
| if getattr(self, "_is_mtls", False) and not is_mtls: | ||
| raise exceptions.MutualTLSChannelError( |
There was a problem hiding this comment.
Raising exceptions.MutualTLSChannelError here causes it to be caught by the generic except Exception as caught_exc: handler at line 243. The handler then wraps it into MutualTLSChannelError(caught_exc), resulting in a double-wrapped exception (MutualTLSChannelError(MutualTLSChannelError(...))).
2 participants
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