[codex] Handle missing pyOpenSSL for inferred mTLS#16921
[codex] Handle missing pyOpenSSL for inferred mTLS#16921cneuralnetwork wants to merge 1 commit intogoogleapis:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request refactors client certificate environment variable handling and updates the mTLS configuration logic in both the requests and urllib3 transports. It introduces helper functions to centralize environment checks and modifies the mTLS setup to gracefully disable inferred mTLS if pyOpenSSL is missing, rather than raising an error. A potential NameError was identified in urllib3.py due to an incorrect reference to _mtls_helper.
| try: | ||
| import OpenSSL | ||
| except ImportError as caught_exc: | ||
| if not transport._mtls_helper._is_client_cert_explicitly_enabled(): |
There was a problem hiding this comment.
The use of transport._mtls_helper here appears to be incorrect and will likely result in a NameError at runtime if OpenSSL is missing. In this package, _mtls_helper is typically imported directly from google.auth.transport. For consistency with the changes in packages/google-auth/google/auth/transport/requests.py and to ensure correctness, you should use _mtls_helper directly.
| if not transport._mtls_helper._is_client_cert_explicitly_enabled(): | |
| if not _mtls_helper._is_client_cert_explicitly_enabled(): |
701f4d7 to
c6ec744
Compare
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request refactors mutual TLS (mTLS) environment variable handling and updates the requests and urllib3 transports to gracefully disable mTLS when pyOpenSSL is missing and mTLS is not explicitly enabled. The changes include centralized environment variable lookups, updated docstrings, and new unit tests. Review feedback suggests simplifying the logic for environment variable fallbacks and boolean checks to be more idiomatic.
| use_client_cert = getenv(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) | ||
| if use_client_cert is None or use_client_cert == "": | ||
| use_client_cert = getenv( | ||
| environment_vars.CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE | ||
| ) | ||
| return use_client_cert |
There was a problem hiding this comment.
This logic for falling back between environment variables can be simplified using the or operator, which is more idiomatic in Python for handling None or empty string fallbacks.
return getenv(environment_vars.GOOGLE_API_USE_CLIENT_CERTIFICATE) or getenv(
environment_vars.CLOUDSDK_CONTEXT_AWARE_USE_CLIENT_CERTIFICATE
)| use_client_cert = _get_use_client_cert_env() | ||
| return bool(use_client_cert and use_client_cert.lower() == "true") |
There was a problem hiding this comment.
This can be simplified by inlining the environment variable check and using a default empty string to handle the None case, making the logic more concise.
| use_client_cert = _get_use_client_cert_env() | |
| return bool(use_client_cert and use_client_cert.lower() == "true") | |
| return (_get_use_client_cert_env() or "").lower() == "true" |
Fixes #16920.
Summary
This changes the requests and urllib3 mTLS setup paths so they no longer fail closed when client certificate use was inferred from certificate configuration but
pyOpenSSLis not installed. In that auto-mTLS case, the transports now disable mTLS and continue using the regular TLS channel.Explicit opt-in remains strict: if
GOOGLE_API_USE_CLIENT_CERTIFICATE=trueor the Cloud SDK fallback env var explicitly enables client certs, a missingpyOpenSSLdependency still raisesMutualTLSChannelErroras before.Root Cause
check_use_client_cert()can now infer client certificate use from certificate config. The requests and urllib3 transports then importedOpenSSLbefore checking whether a cert/key could actually be used, so environments without optionalpyOpenSSLcould crash during automatically triggered mTLS configuration.Changes
pyOpenSSLis unavailable.OpenSSLin both requests and urllib3 transports.Validation
/tmp/google-auth-venv/bin/python -m pytest tests/transport/test_requests.py::TestAuthorizedSession::test_configure_mtls_channel_exceptions tests/transport/test_requests.py::TestAuthorizedSession::test_configure_mtls_channel_inferred_missing_openssl tests/transport/test_urllib3.py::TestAuthorizedHttp::test_configure_mtls_channel_exceptions tests/transport/test_urllib3.py::TestAuthorizedHttp::test_configure_mtls_channel_inferred_missing_openssl tests/transport/test__mtls_helper.py::TestCheckUseClientCert(16 passed)/tmp/google-auth-venv/bin/python -m pytest tests/transport/test_requests.py tests/transport/test_urllib3.py tests/transport/test__mtls_helper.py(140 passed, with existing pyOpenSSL deprecation warnings)git diff --checkpython -m compileall -q packages/google-auth/google/auth/transport/_mtls_helper.py packages/google-auth/google/auth/transport/requests.py packages/google-auth/google/auth/transport/urllib3.py packages/google-auth/tests/transport/test_requests.py packages/google-auth/tests/transport/test_urllib3.py