feat: Add helper methods for asynchronous x.509 certificate discovery#1956
feat: Add helper methods for asynchronous x.509 certificate discovery#1956agrawalradhika-cell wants to merge 6 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>
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 asynchronous helper methods for mTLS certificate discovery, a valuable addition for non-blocking I/O during the mTLS handshake process. However, a high-severity Path Traversal vulnerability was identified in google/auth/aio/transport/mtls.py where user-provided file paths are not properly validated, potentially allowing attackers to read arbitrary files and disclose sensitive information. Additionally, the review suggests improving consistency with existing synchronous helpers, enhancing robustness by handling async callbacks, and refining the API by removing unused parameters to make the new aio module more reliable and intuitive.
… handling async and removing encrypted_key complication Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
|
Note - Python3.9 check is failing due to some nox dependency missing for python 3.9, IIUC unrelated to my change |
| """ | ||
| if _check_config_path(CERTIFICATE_CONFIGURATION_DEFAULT_PATH) is not None: | ||
| return True | ||
| cert_config_path = getenv("GOOGLE_API_CERTIFICATE_CONFIG") |
There was a problem hiding this comment.
shouldn't we use the value from env var if set and fallback to default?
There was a problem hiding this comment.
Based on AIP-4114 - Users should enable Device Certificate Authentication through ADC instead of manual configuration via client options.
Thus, ~/.config/gcloud/certificate_config.json checks if the certs are present at standard location, and
if the certs are not present at default location, then certs are checked for specific custom config.
Same logic in synchronous operations - see url
There was a problem hiding this comment.
Are you sure that's right? That section uses the word should, not must.
If we put the default check above the env var check, that completely removes the ability to manually override, which doesn't seem like the intended approach to me, even if automatic detection is the best practice
But I just skimmed the doc, so let me know if I'm missing something
| return await loop.run_in_executor(None, func, *args) | ||
|
|
||
|
|
||
| def has_default_client_cert_source(): |
There was a problem hiding this comment.
can we reuse this method from sync implementation?
There was a problem hiding this comment.
Actually, has_default_client_cert_source from synchronous flow checks for CONTEXT_AWARE_METADATA path too, which is not implemented for async flow yet. Sync method flow would return True if the certificate was present in that location, which would be misleading in a way that Async flow wouldn't implement mTLS configuration in that case
There was a problem hiding this comment.
Can we add a flag to the sync helper called check_context_aware_metadata, and turn it off when calling from async? It seems like that could cut down on logic duplication
There was a problem hiding this comment.
And wait, don't has_default_client_cert_source and _get_cert_config_path also already do essentially the same thing? Can we condense this some more?
I know some of this logic was already in place, but I think we need to be careful about duplication when adding async, otherwise it can get confusing fast
daniel-sanche
changed the title
| from google.auth import exceptions | ||
| import google.auth.transport._mtls_helper | ||
|
|
||
| CERTIFICATE_CONFIGURATION_DEFAULT_PATH = "~/.config/gcloud/certificate_config.json" |
There was a problem hiding this comment.
This seems weird to me... What environment are we expecting to run? Should we be setting this up to work with windows? Or is this for Cloud Shell or some other Google-managed environment?
There was a problem hiding this comment.
It is the well-known location, which is for all OS.
Please note there is also GOOGLE_API_CERTIFICATE_CONFIG taken into consideration as well to cater to custom cert provided by user.
There was a problem hiding this comment.
The ~ threw me off, but it looks like it's being passed into path.expanduser.
Assuming that path is right:
I'd probably suggest using path.expanduser here, so it's already converted (it can always be turned back into a string for _check_config_path). Or at the very least, add a comment like # default gcloud config path, to be used with path.expanduser?
These docs seem to say that windows uses %APPDATA%\gcloud though, which would not be found by this. Are you sure this is right?
There was a problem hiding this comment.
I realize now that this is coming from _mtls_helper. Maybe windows isn't in scope?'
It would be good if we can import this variable from the helper instead of duplciating it. But even better if we can share the sync logic so this isn't needed
| """ | ||
| if _check_config_path(CERTIFICATE_CONFIGURATION_DEFAULT_PATH) is not None: | ||
| return True | ||
| cert_config_path = getenv("GOOGLE_API_CERTIFICATE_CONFIG") |
There was a problem hiding this comment.
Are you sure that's right? That section uses the word should, not must.
If we put the default check above the env var check, that completely removes the ability to manually override, which doesn't seem like the intended approach to me, even if automatic detection is the best practice
But I just skimmed the doc, so let me know if I'm missing something
Signed-off-by: Radhika Agrawal <agrawalradhika@google.com>
|
|
||
| async def callback(): | ||
| try: | ||
| _, cert_bytes, key_bytes = await get_client_cert_and_key() |
There was a problem hiding this comment.
It seems a little risky to me that it uses has_default_client_cert_source to check the location of the keys, but then uses a completely different code path to actually read them. I worry these could get out of sync in the future. Especially since we have logic split between the sync and async implementations
It looks like has_default_client_source reads:
- CERTIFICATE_CONFIGURATION_DEFAULT_PATH
- GOOGLE_API_CERTIFICATE_CONFIG
and get_client_cert_and_key uses _get_client_cert_config_path(None), which reads
- GOOGLE_API_CERTIFICATE_CONFIG
- CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH
- CERTIFICATE_CONFIGURATION_DEFAULT_PATH
Didn't you mention that CLOUDSDK_CONTEXT_AWARE_CERTIFICATE_CONFIG_FILE_PATH shouldn't be used by the async code?
There was a problem hiding this comment.
It would be great if we could refactor the logic that describes where to find the cert into one helper, like this:
def _found_cert_path(include_context_aware=True) -> str | None:
# a single source of truth where we search for the cert locations, and return the place it was found, or None
def has_default_client_cert_source():
return _found_cert_path() is not None
async def has_default_client_cert_source():
return _found_cert_path(include_context_aware=False) is not None
def get_client_cert_and_key():
cert_path = _found_cert_path
# read certs from path
That way, checking for existence vs reading; and async vs sync, it would all flow through the same logic
daniel-sanche
left a comment
daniel-sanche
left a comment
There was a problem hiding this comment.
Hopefully my comments aren't too confusing, I was kind of learning the codebase as I went
In general, my feed back is that it would be great if we can re-use logic already in the sync implemtation as much as possible. I see a lot of implementations that are 90% the same as what we implemented elsewhere, with minor critical differences. And that can be very hard to build upon safely in the future
| 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.
Sorry, actually looking at this again, I think we do need to use a result variable, like you had before. Otherwise if it's sync, it'll be called twice:
if client_cert_callback:
result = client_cert_callback()
try:
# handle coroutine callback
cert, key = await result
except TypeError:
# unpack non-async callback result
cert, key = result
3 participants
This PR introduces the google.auth.aio.transport.mtls module, providing asynchronous helper methods for mTLS certificate discovery go/caa:x509-async-support. These helpers are designed to be for x.509 certs discovery and non-blocking, ensuring that disk I/O operations are async during mTLS handshake process. Plus, added unit tests respectively for the helper functions.
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.
Next Steps: Will create a followup PR will that will utilize these helpers to implement
configure_mtls_channelwithin theAsyncAuthorizedSessionclass.