fix(transport): propagate mTLS adapter to auth session and fix connection leaks#17689
fix(transport): propagate mTLS adapter to auth session and fix connection leaks#17689nbayati wants to merge 2 commits into
Conversation
…tion leaks When `configure_mtls_channel` is called, the newly created mTLS HTTPAdapter is mounted to the `"https://"` prefix. Previously, the existing adapter was replaced but never explicitly closed, which orphaned the underlying urllib3 connection pools and could cause file descriptor exhaustion. This commit addresses the connection leak by retrieving the old adapter and safely calling `.close()` on it before replacing it. Additionally, the mTLS adapter is now correctly propagated to the internal `_auth_request_session`. This ensures that out-of-band IAM signing requests and token refreshes securely traverse the mTLS channel when authenticating against strict Certificate-Based Access (CBA) endpoints. Finally, a documentation warning was added to clarify that dynamically reconfiguring the channel mutates the underlying session and is not natively thread-safe.
![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 updates the configure_mtls_channel method in requests.py to properly close old adapters when mounting a new mTLS adapter, and extends this configuration to _auth_request_session if it exists. It also adds comprehensive unit tests and a thread-safety warning in the docstring. The review feedback correctly points out a potential AttributeError when configure_mtls_channel is called on a base Session instance where _auth_request_session is not defined, and provides an actionable code suggestion to safely access the attribute using getattr.
| "https://{}/".format(self._default_host) if self._default_host else None | ||
| ) | ||
|
|
||
| def configure_mtls_channel(self, client_cert_callback=None): |
There was a problem hiding this comment.
Do we also need to update configure_mtls_channel in urllib3.py?
There was a problem hiding this comment.
Yes, good catch. I've updated urllib3.py to match.
| if old_auth_adapter is not None and old_auth_adapter is not new_adapter: | ||
| old_auth_adapter.close() | ||
|
|
||
| self._auth_request_session.mount("https://", new_adapter) |
There was a problem hiding this comment.
I could be misreading things here - but does this unintentionally drop the retries configured originally via # Using an adapter to make HTTP requests robust to network errors. # This adapter retrys HTTP requests when network errors occur # and the requests seems safely retryable. retry_adapter = requests.adapters.HTTPAdapter(max_retries=3) self._auth_request_session.mount("https://", retry_adapter) (around line 421).
There was a problem hiding this comment.
Perhaps also worth noting, IIUC, while we want the retries on the self._auth_request_session adapter, we DO NOT want the retries on the adapter mounted to self up above which would leak into the main user session (so we may want a separate "new_auth_adapter" or similar?)
There was a problem hiding this comment.
Thanks for flagging this! I've updated the logic to fetch the existing configs from both the old adapter and the old auth adapter, passing them into the new adapters so the original retry configurations are preserved.
| If the callback is None, application default SSL credentials | ||
| will be used. | ||
|
|
||
| .. warning:: |
There was a problem hiding this comment.
IIUC the problem is we are closing one adapater while opening (and attaching) a new one. Gemini seems to claim that .mount updates in a way that in CPython is atomic - so in theory if we mount first and then close (instead of what is currently done by closing first and then mounting) we may improve the race case when a closing adapter is attempted to be used.
There was a problem hiding this comment.
Done. The code now mounts the new adapter (self.mount()) before calling .close() on the old adapter. I applied this same atomic ordering in urllib3.py as well.
…ions - Mount new adapters before closing old ones to prevent race conditions in both requests.py and urllib3.py - Preserve original `max_retries` configurations when replacing adapters - Separate main session adapters from auth session adapters to prevent retry configs from leaking into the user's session
3 participants
This PR is fixing two issues found in the auth library:
Adapter Connection Leaks
Whenever the
configure_mtls_channel()method was called, the code would create a new mTLSHTTPAdapterand mount it to"https://". However, it never closed the old adapter it was replacing. Because of how the underlyingrequestsandurllib3libraries work, the old adapter's connection pool was left open, creating dangling sockets and eventually causing file descriptor exhaustion.The Fix: I updated the method to retrieve the existing
"https://"adapter (viaself.get_adapter()) before replacing it. If an old adapter is found, we now explicitly callold_adapter.close()to shut down its connection pool and cleanly release the sockets back to the OS.Missing Auth Request Session Update (Issue google-auth: mTLS token refresh blindspot due to unconfigured internal helper transport #17680)
The
AuthorizedSessionclass maintains an internal session called_auth_request_session. This hidden session is specifically responsible for doing background work, like fetching new OAuth tokens or signing IAM requests. When mTLS was enabled, the new secure adapter was only applied to the main user-facing session, completely missing this internal session. As a result, critical token refreshes were bypassing mTLS and failing when they hit strict Certificate-Based Access (CBA) endpoints.The Fix: I added logic to check if
self._auth_request_sessionexists during the mTLS configuration. If it does, we now apply the exact same cleanup (closing its old adapter) and then explicitly mount the new mTLS adapter to it. This guarantees that all background token refreshes are routed securely over the mTLS channel.Also added a documentation warning to clarify that dynamically reconfiguring the channel mutates the underlying session and is not natively thread-safe.