fix(auth): restore pyOpenSSL for ECP offload flow - #18085
Conversation
Resolves an issue where Enterprise Certificate Proxy (ECP) offload failed with "failed to configure ECP Offload SSL context" after pyOpenSSL was removed in googleapis#16976. The `tls_offload` C library requires a PyOpenSSL context pointer and does not support standard library CPython `ssl.SSLContext`. This change: - Restores `_cast_ssl_ctx_to_void_p_pyopenssl` in `_custom_tls_signer.py` for the offload branch. - Conditionally calls `urllib3.contrib.pyopenssl.inject_into_urllib3()` in `_MutualTlsOffloadAdapter` only when the offload flow is used, leaving the ECP Provider flow on standard library `ssl.SSLContext`. - Restores `pyopenssl` and `cffi` in `enterprise_cert` extra requirements in `setup.py`. - Updates unit tests Fixes googleapis#17791
|
I've verified the fix on the ECP offload flow locally. I still need to verify that there was no regression on the ECP provider flow. |
![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 support for pyOpenSSL ECP by casting the SSL context using cffi and pyOpenSSL, injecting pyOpenSSL into urllib3 when necessary, and updating the setup dependencies and tests accordingly. The review feedback highlights two critical issues: first, the cryptography dependency should be combined with, rather than replaced by, the new pyOpenSSL and cffi dependencies in setup.py; second, strict type checking should be enforced on the SSL context in _custom_tls_signer.py to prevent security risks from duck-typed wrapper objects, while gracefully returning False if the type is unexpected to maintain backwards compatibility.
| reauth_extra_require = ["pyu2f>=0.1.5"] | ||
|
|
||
| enterprise_cert_extra_require = cryptography_base_require | ||
| enterprise_cert_extra_require = ["pyopenssl>=20.0.0", "cffi>=1.0.0"] |
There was a problem hiding this comment.
Replacing cryptography_base_require entirely with ["pyopenssl>=20.0.0", "cffi>=1.0.0"] removes the cryptography dependency from the enterprise_cert extra. Since the enterprise certificate functionality still relies on cryptography for parsing and handling certificates, we should combine them instead of replacing.
enterprise_cert_extra_require = cryptography_base_require + [
"pyopenssl>=20.0.0",
"cffi>=1.0.0",
]| if not self._offload_lib.ConfigureSslContext( | ||
| self._sign_callback, | ||
| ctypes.c_char_p(self._cert), | ||
| _cast_ssl_ctx_to_void_p_stdlib(ctx), | ||
| _cast_ssl_ctx_to_void_p_pyopenssl(ctx._ctx._context), | ||
| ): |
There was a problem hiding this comment.
To prevent security risks such as passphrase leakage from arbitrary duck-typed wrapper objects, we should enforce strict type checking on the SSL context instead of using duck typing. Additionally, to maintain backwards compatibility and avoid introducing breaking changes, we should gracefully return False (or fall back) instead of raising an exception if the context is not of the expected type.
| if not self._offload_lib.ConfigureSslContext( | |
| self._sign_callback, | |
| ctypes.c_char_p(self._cert), | |
| _cast_ssl_ctx_to_void_p_stdlib(ctx), | |
| _cast_ssl_ctx_to_void_p_pyopenssl(ctx._ctx._context), | |
| ): | |
| if not isinstance(ctx, OpenSSL.SSL.Context): | |
| return False | |
| ssl_ctx = ctx._ctx._context | |
| if not self._offload_lib.ConfigureSslContext( | |
| self._sign_callback, | |
| ctypes.c_char_p(self._cert), | |
| _cast_ssl_ctx_to_void_p_pyopenssl(ssl_ctx), | |
| ): |
References
- When passing sensitive cryptographic material (such as private keys and passphrases) to an SSL context, enforce strict type checking (e.g., isinstance(ctx, ssl.SSLContext)) instead of duck typing. This prevents security risks, such as passphrase leakage, that could be introduced by arbitrary duck-typed wrapper objects.
- Do not replace historical graceful fallback behaviors (such as returning False/falling back to standard TLS) with exceptions if doing so would introduce breaking changes for downstream users and violate backwards compatibility.
There was a problem hiding this comment.
I looked into this, and it seems like this is a false flag.
First, the recommended solution would not work, because ctx is a urllib3.contrib.pyopenssl.PyOpenSSLContext wrapper object (where the OpenSSL.SSL.Context instance lives at ctx._ctx). Enforcing isinstance(ctx, OpenSSL.SSL.Context) directly on ctx evaluates to False, which would disable ECP offload configuration entirely. Also attach_to_ssl_context is designed to raise exceptions.MutualTLSChannelError whenever ECP configuration fails. Transport callers (such as requests.py) expect an exception on failure and do not evaluate boolean return values, so returning False would cause setup failures to fail silently.
Second, there isn't really a security risk here. The SSL context is only used to pass OpenSSL's underlying C-level SSL_CTX* pointer (ctx._ctx._context) to the C++ offload library. Private key operations and signing are offloaded via _sign_callback (communicating with hardware/TPM), so no private keys or passphrases are stored on or accessed from ctx.
There was a problem hiding this comment.
While I think I understand the conclusion of this being, largely, a false flag - I do think there may be some validity here with regards to the existing change leading to the possibility of an AttributeError being raised and escaping. We probably should be more defensive about extracting this.
| "cffi is required for pyOpenSSL ECP support." | ||
| ) from caught_exc | ||
|
|
||
| return ctypes.cast(int(cffi.FFI().cast("intptr_t", ssl_ctx)), ctypes.c_void_p) |
There was a problem hiding this comment.
Do we need to use an unsigned ptr here (uintptr_t) to avoid overflow if the address of ssl_ctx has a most significant bit of "1"?
| if not self._offload_lib.ConfigureSslContext( | ||
| self._sign_callback, | ||
| ctypes.c_char_p(self._cert), | ||
| _cast_ssl_ctx_to_void_p_stdlib(ctx), | ||
| _cast_ssl_ctx_to_void_p_pyopenssl(ctx._ctx._context), | ||
| ): |
There was a problem hiding this comment.
While I think I understand the conclusion of this being, largely, a false flag - I do think there may be some validity here with regards to the existing change leading to the possibility of an AttributeError being raised and escaping. We probably should be more defensive about extracting this.
| try: | ||
| import OpenSSL.SSL # type: ignore | ||
|
|
||
| _OPENSSL_SSL_ERROR = (OpenSSL.SSL.Error,) |
There was a problem hiding this comment.
Do we need to worry about OpenSSL.crypto.Error?
| ImportError: if certifi is not installed | ||
| ImportError: if certifi or pyOpenSSL is not installed | ||
| google.auth.exceptions.MutualTLSChannelError: If mutual TLS channel | ||
| creation failed for any reason. |
There was a problem hiding this comment.
or if cffi is not installed
| if not self.signer.should_use_provider(): | ||
| import urllib3.contrib.pyopenssl | ||
|
|
||
| urllib3.contrib.pyopenssl.inject_into_urllib3() |
There was a problem hiding this comment.
I believe this has a process-wide side effect where it permanently switches all subsequent HTTPS requests made anywhere across the Python process to PyOpenSSL.
2 participants
Resolves an issue where Enterprise Certificate Proxy (ECP) offload failed with "failed to configure ECP Offload SSL context" after pyOpenSSL was removed in #16976.
The
tls_offloadC library requires a PyOpenSSL context pointer and does not support standard library CPythonssl.SSLContext. This change:_cast_ssl_ctx_to_void_p_pyopensslin_custom_tls_signer.pyfor the offload branch.urllib3.contrib.pyopenssl.inject_into_urllib3()in_MutualTlsOffloadAdapteronly when the offload flow is used, leaving the ECP Provider flow on standard libraryssl.SSLContext.pyopensslandcffiinenterprise_certextra requirements insetup.py.Fixes #17791