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. |
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.
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