-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat: [aiohttp] Add mTLS reconfiguration logic when certificate mismatch for existing credentials & Agent Identity workloads #18224
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
94a1d95
420447c
907cf00
cc850b1
a44acb0
984e47c
30341bc
1c068dc
6fb1e86
2cdfe2d
d734731
97e91d0
d0da58b
825426d
63e587c
71b3bf5
7d92d30
8b2efcf
a4d0405
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 |
|---|---|---|
|
|
@@ -13,8 +13,11 @@ | |
| # limitations under the License. | ||
|
|
||
| import asyncio | ||
| import collections.abc | ||
| from contextlib import asynccontextmanager | ||
| import functools | ||
| import http.client as http_client | ||
| import logging | ||
| import time | ||
| from typing import Mapping, Optional, TYPE_CHECKING, Union | ||
| import warnings | ||
|
|
@@ -37,6 +40,9 @@ | |
| except (ImportError, AttributeError): | ||
| ClientTimeout = None | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
| MTLS_URL_PREFIXES = ["mtls.googleapis.com", "mtls.sandbox.googleapis.com"] | ||
|
|
||
|
|
||
| # Tracks the internal aiohttp installation and usage | ||
| try: | ||
|
|
@@ -148,6 +154,7 @@ def __init__( | |
| "`auth_request` must either be configured or the external package `aiohttp` must be installed to use the default value." | ||
| ) | ||
| self._auth_request = _auth_request | ||
| self._mtls_rotation_lock = asyncio.Lock() | ||
|
|
||
| async def configure_mtls_channel(self, client_cert_callback=None): | ||
| """Configure the client certificate and key for SSL connection. | ||
|
|
@@ -277,7 +284,10 @@ async def request( | |
| google.auth.exceptions.TimeoutError: If the method does not complete within | ||
| the configured `max_allowed_time` or the request exceeds the configured | ||
| `timeout`. | ||
| google.auth.exceptions.MutualTLSChannelError: If mutual TLS | ||
| channel reconfiguration fails for any reason during certificate rotation. | ||
| """ | ||
| _auth_retry_count = kwargs.pop("_auth_retry_count", 0) | ||
| if self._mtls_init_task: | ||
| try: | ||
| await self._mtls_init_task | ||
|
|
@@ -310,8 +320,101 @@ async def request( | |
| url, method, data, headers, actual_timeout, **kwargs | ||
| ) | ||
| ) | ||
|
|
||
| if response.status_code not in transport.DEFAULT_RETRYABLE_STATUS_CODES: | ||
| break | ||
|
|
||
| if response.status_code == http_client.UNAUTHORIZED: | ||
| if _auth_retry_count < 2: | ||
| is_streaming = ( | ||
| data is not None | ||
| and isinstance( | ||
| data, (collections.abc.Iterator, collections.abc.AsyncIterable) | ||
| ) | ||
| or hasattr(data, "read") | ||
| ) | ||
| if getattr(self, "is_mtls", False) and any( | ||
| prefix in url for prefix in MTLS_URL_PREFIXES | ||
| ): | ||
| # Snapshot the stale certificate state BEFORE acquiring the lock. | ||
| # This represents the cert that caused the 401 rejection. | ||
| stale_cert = self._cached_cert | ||
|
|
||
| # Wait in line to acquire the lock | ||
| async with self._mtls_rotation_lock: | ||
| # Check Did another coroutine already reconfigure mTLS | ||
| if self._cached_cert != stale_cert: | ||
| # Yes! Another request already updated the channel | ||
| pass | ||
| else: | ||
| try: | ||
| ( | ||
| call_cert_bytes, | ||
| call_key_bytes, | ||
| cached_fingerprint, | ||
| current_cert_fingerprint, | ||
| ) = await mtls._run_in_executor( | ||
| google.auth.transport._mtls_helper.check_parameters_for_unauthorized_response, | ||
| self._cached_cert, | ||
| ) | ||
| except Exception as e: | ||
| _LOGGER.warning( | ||
| "Failed to check client certificate parameters: %s. Proceeding with original response.", | ||
| e, | ||
| ) | ||
| else: | ||
| if cached_fingerprint != current_cert_fingerprint: | ||
| try: | ||
| _LOGGER.info( | ||
| "Client certificate has changed, reconfiguring mTLS " | ||
| "channel." | ||
| ) | ||
| if ( | ||
| self._mtls_init_task | ||
| and self._mtls_init_task.done() | ||
| ): | ||
| self._mtls_init_task = None | ||
| await self.configure_mtls_channel( | ||
|
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. configure_mtls_channel closes the active ClientSession, which aborts in-flight concurrent requests. Keep old sessions open until their requests finish, or close them in AsyncAuthorizedSession.close(). Add a concurrency regression test for this case. |
||
| lambda: (call_cert_bytes, call_key_bytes) | ||
|
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. Initial setup can use client_cert_callback, but check_parameters_for_unauthorized_response always checks application default credentials. This mismatch can replace custom certificates or skip rotation. Save the initial certificate source, and reuse it for all fingerprint checks and reconfiguration. |
||
| ) | ||
| except Exception as e: | ||
| _LOGGER.error( | ||
| "Failed to reconfigure mTLS channel: %s", e | ||
| ) | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Failed to reconfigure mTLS channel" | ||
| ) from e | ||
| else: | ||
| _LOGGER.info( | ||
| "Skipping reconfiguration of mTLS channel because the client" | ||
| " certificate has not changed." | ||
| ) | ||
| if is_streaming: | ||
| return response | ||
| if hasattr(response, "close"): | ||
| if asyncio.iscoroutinefunction(response.close): | ||
| await response.close() | ||
| else: | ||
| response.close() | ||
| try: | ||
| await self._credentials.refresh(self._auth_request) | ||
|
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. Closing the 401 response before credentials refresh returns an unreadable response if RefreshError occurs. Also, StaticCredentials and AnonymousCredentials raise uncaught InvalidOperation errors during refresh. Close the response only after refresh succeeds. Return the open original response if refresh fails or is unsupported. |
||
| except exceptions.RefreshError as e: | ||
| _LOGGER.debug( | ||
| "Credential refresh failed, returning 401 response. Error: %s", | ||
| e, | ||
| ) | ||
| return response | ||
| kwargs["_auth_retry_count"] = _auth_retry_count + 1 | ||
| return await self.request( | ||
| method, | ||
| url, | ||
| data=data, | ||
| headers=headers, | ||
| max_allowed_time=max_allowed_time, | ||
|
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. max_allowed_time=max_allowed_time resets the timer on each retry instead of limiting total runtime. |
||
| timeout=timeout, | ||
| total_attempts=total_attempts, | ||
| **kwargs, | ||
| ) | ||
| return response | ||
|
|
||
| @functools.wraps(request) | ||
|
|
||
|
agrawalradhika-cell marked this conversation as resolved.
agrawalradhika-cell marked this conversation as resolved.
|
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.
prefix in url searches the entire URL, so paths or query parameters can trigger certificate rotation on standard hosts. Parse and normalize urlsplit(url).hostname, then match only exact hostnames or subdomains of documented mTLS endpoints.