feat: Add mTLS configuration via x.509 for asynchronous session in google-auth#1958
feat: Add mTLS configuration via x.509 for asynchronous session in google-auth#1958agrawalradhika-cell wants to merge 7 commits intomainfrom
Conversation
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
… handling async and removing encrypted_key complication Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
Summary of ChangesHello @agrawalradhika-cell, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces mTLS support for asynchronous sessions in google-auth. However, a security misconfiguration exists where the is_mtls property can return True even if mTLS was not successfully applied to a custom transport. Additionally, there's a potential NameError if aiohttp is not installed, a typo in an attribute name, and an efficiency issue where a callback is executed twice. Addressing these will improve the robustness, security, and performance of the mTLS support.
| if not _auth_request and AIOHTTP_INSTALLED: | ||
| _auth_request = AiohttpRequest() | ||
| self._is_mtls = False | ||
| self._cached_Cert = None |
There was a problem hiding this comment.
There is a typo in the attribute name here (capital 'C'). It should be _cached_cert to match its usage in configure_mtls_channel at line 174.
self._cached_cert = None| self._is_mtls, | ||
| cert, | ||
| key, | ||
| ) = await mtls.get_client_cert_and_key(client_cert_callback) | ||
|
|
||
| if self._is_mtls: | ||
| self._cached_cert = cert | ||
| ssl_context = await mtls._run_in_executor( | ||
| mtls.make_client_cert_ssl_context, cert, key | ||
| ) | ||
|
|
||
| # Re-create the auth request with the new SSL context | ||
| if isinstance(self._auth_request, AiohttpRequest): | ||
| connector = aiohttp.TCPConnector(ssl=ssl_context) | ||
| new_session = aiohttp.ClientSession(connector=connector) | ||
| await self._auth_request.close() | ||
| self._auth_request = AiohttpRequest(session=new_session) |
There was a problem hiding this comment.
The is_mtls property can return a misleading True value when a custom transport is used. The self._is_mtls attribute is set to True at line 168, but the actual application of mTLS to the transport only occurs if the transport is an instance of AiohttpRequest (lines 180-184). If a user provides a custom auth_request that is not an AiohttpRequest, the transport remains unconfigured for mTLS, yet session.is_mtls will return True. This misleading state could lead users to believe their connection is protected by mTLS when it is not. Furthermore, if aiohttp is not installed, AiohttpRequest will not be defined, leading to a NameError when the isinstance check is performed at line 180. Recommendation: Only set self._is_mtls = True if the mTLS configuration was successfully applied to the transport, and ensure AIOHTTP_INSTALLED is checked before performing the isinstance check to prevent a NameError.
if AIOHTTP_INSTALLED and isinstance(self._auth_request, AiohttpRequest):| try: | ||
| # If it's awaitable, this works. | ||
| cert, key = await client_cert_callback() | ||
| except TypeError: | ||
| # If it's not awaitable (e.g., a tuple), result is already the data. | ||
| cert, key = client_cert_callback() |
There was a problem hiding this comment.
The current implementation calls client_cert_callback() twice if it is a synchronous function: once in the try block (which raises a TypeError when awaited) and again in the except block. This can be inefficient or problematic if the callback has side effects. Consider checking if the result is a coroutine before deciding whether to await it.
| try: | |
| # If it's awaitable, this works. | |
| cert, key = await client_cert_callback() | |
| except TypeError: | |
| # If it's not awaitable (e.g., a tuple), result is already the data. | |
| cert, key = client_cert_callback() | |
| result = client_cert_callback() | |
| if asyncio.iscoroutine(result): | |
| cert, key = await result | |
| else: | |
| cert, key = result |
| google.auth.exceptions.DefaultClientCertSourceError: If the default | ||
| client SSL credentials don't exist or are malformed. |
There was a problem hiding this comment.
The docstring references google.auth.exceptions.DefaultClientCertSourceError, but this exception does not exist in google/auth/exceptions.py. The code actually raises MutualTLSChannelError.
| fd, path = tempfile.mkstemp() | ||
| try: | ||
| with os.fdopen(fd, "wb") as f: | ||
| f.write(content) | ||
| yield path | ||
| finally: | ||
| # Securely delete the file after use. | ||
| if os.path.exists(path): | ||
| os.remove(path) |
There was a problem hiding this comment.
The variable name path shadows the path module imported from os at line 23. While it doesn't break the code here because of how os.path is used later, it's better to use a different name like file_path to avoid confusion and maintain clarity.
| fd, path = tempfile.mkstemp() | |
| try: | |
| with os.fdopen(fd, "wb") as f: | |
| f.write(content) | |
| yield path | |
| finally: | |
| # Securely delete the file after use. | |
| if os.path.exists(path): | |
| os.remove(path) | |
| fd, file_path = tempfile.mkstemp() | |
| try: | |
| with os.fdopen(fd, "wb") as f: | |
| f.write(content) | |
| yield file_path | |
| finally: | |
| # Securely delete the file after use. | |
| if os.path.exists(file_path): | |
| os.remove(file_path) |
1 participant
This pull request introduces support for Mutual TLS (mTLS) in the asynchronous transport layer of the google-auth library. It enables AsyncAuthorizedSession to automatically discover and utilize client certificates for secure communication with Google Cloud APIs. See go/caa:x509-async-support for details.
Please note: Only x.509 creds are in scope of this project currently. Context aware or ECP credentials are not in scope of this project currently.
This PR is second part of #1956