-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(google-auth): fail loudly when dynamically disabling mTLS on active sessions #18005
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -182,6 +182,13 @@ async def _do_configure(): | |||||
| 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 contexts | ||||||
| # remain attached while auth checks believe mTLS is disabled. | ||||||
| if getattr(self, "_is_mtls", False): | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| raise exceptions.MutualTLSChannelError( | ||||||
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please update this to reference AsyncAuthorizedSession instead of AuthorizedSession so async users aren't directed to the synchronous requests transport class.
Suggested change
|
||||||
| ) | ||||||
|
Comment on lines
+188
to
+191
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising a References
|
||||||
| return | ||||||
|
|
||||||
| try: | ||||||
|
|
@@ -191,6 +198,12 @@ async def _do_configure(): | |||||
| key, | ||||||
| ) = await mtls.get_client_cert_and_key(client_cert_callback) | ||||||
|
|
||||||
| # Prevent mid-lifecycle transition from mTLS-enabled to mTLS-disabled state. | ||||||
| if getattr(self, "_is_mtls", False) and not is_mtls: | ||||||
| raise exceptions.MutualTLSChannelError( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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(...))). |
||||||
| "Cannot disable mTLS on an active session. A new AuthorizedSession must be created." | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here
Suggested change
|
||||||
| ) | ||||||
|
Comment on lines
+202
to
+205
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising a References
|
||||||
|
|
||||||
| if is_mtls: | ||||||
| # Re-create the auth request with the new SSL context | ||||||
| if AIOHTTP_INSTALLED and isinstance( | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -469,6 +469,13 @@ def configure_mtls_channel(self, client_cert_callback=None): | |||||
| """ | ||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit
Suggested change
|
||||||
| # 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." | ||||||
| ) | ||||||
|
Comment on lines
+475
to
+478
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising a References
|
||||||
| return | ||||||
|
|
||||||
| try: | ||||||
|
|
@@ -480,6 +487,12 @@ def configure_mtls_channel(self, client_cert_callback=None): | |||||
| client_cert_callback | ||||||
| ) | ||||||
|
|
||||||
| # 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." | ||||||
| ) | ||||||
|
Comment on lines
+491
to
+494
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising a References
|
||||||
|
|
||||||
| old_adapter = self.adapters.get("https://") | ||||||
|
|
||||||
| kwargs = {} | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -350,13 +350,26 @@ def configure_mtls_channel(self, client_cert_callback=None): | |||||
| """ | ||||||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
|
||||||
| # pools 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 AuthorizedHttp must be created." | ||||||
| ) | ||||||
|
Comment on lines
+356
to
+359
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising a References
|
||||||
| return False | ||||||
|
|
||||||
| try: | ||||||
| found_cert_key, cert, key = transport._mtls_helper.get_client_cert_and_key( | ||||||
| client_cert_callback | ||||||
| ) | ||||||
|
|
||||||
| # Prevent mid-lifecycle transition from mTLS-enabled to mTLS-disabled state. | ||||||
| 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." | ||||||
| ) | ||||||
|
Comment on lines
+368
to
+371
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Raising a References
|
||||||
|
|
||||||
| if found_cert_key: | ||||||
| new_http = _make_mutual_tls_http(cert, key) | ||||||
| new_is_mtls = True | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: