-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(auth): restore pyOpenSSL for ECP offload flow #18085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -45,6 +45,18 @@ | |||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Cast SSL_CTX* to void* | ||||||||||||||||||||||||||||||||
| def _cast_ssl_ctx_to_void_p_pyopenssl(ssl_ctx): | ||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||
| import cffi | ||||||||||||||||||||||||||||||||
| except ImportError as caught_exc: | ||||||||||||||||||||||||||||||||
| raise exceptions.MutualTLSChannelError( | ||||||||||||||||||||||||||||||||
| "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) | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| # Cast SSL_CTX* to void* | ||||||||||||||||||||||||||||||||
| def _cast_ssl_ctx_to_void_p_stdlib(context): | ||||||||||||||||||||||||||||||||
| if not issubclass(type(context), ssl.SSLContext): | ||||||||||||||||||||||||||||||||
|
|
@@ -281,7 +293,7 @@ def attach_to_ssl_context(self, ctx): | |||||||||||||||||||||||||||||||
| 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), | ||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||
|
Comment on lines
293
to
297
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Suggested change
References
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked into this, and it seems like this is a false flag. Second, there isn't really a security risk here. The SSL context is only used to pass OpenSSL's underlying C-level
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||||||||||||||||||||||||
| raise exceptions.MutualTLSChannelError( | ||||||||||||||||||||||||||||||||
| "failed to configure ECP Offload SSL context" | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,9 +39,15 @@ | |
| from google.auth import exceptions | ||
| from google.auth import transport | ||
| from google.auth.transport import _mtls_helper | ||
| import google.auth.transport._mtls_helper | ||
| from google.oauth2 import service_account | ||
|
|
||
| try: | ||
| import OpenSSL.SSL # type: ignore | ||
|
|
||
| _OPENSSL_SSL_ERROR = (OpenSSL.SSL.Error,) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to worry about OpenSSL.crypto.Error? |
||
| except ImportError: | ||
| _OPENSSL_SSL_ERROR = () # type: ignore | ||
|
|
||
| _LOGGER = logging.getLogger(__name__) | ||
|
|
||
| _DEFAULT_TIMEOUT = 120 # in seconds | ||
|
|
@@ -242,7 +248,7 @@ def __init__(self, cert, key, **kwargs): | |
| ValueError, | ||
| RuntimeError, | ||
| TypeError, | ||
| ) as exc: | ||
| ) + _OPENSSL_SSL_ERROR as exc: | ||
| raise exceptions.MutualTLSChannelError( | ||
| "Failed to configure client certificate and key for mTLS." | ||
| ) from exc | ||
|
|
@@ -278,7 +284,7 @@ class _MutualTlsOffloadAdapter(requests.adapters.HTTPAdapter): | |
| } | ||
|
|
||
| Raises: | ||
| 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or if cffi is not installed |
||
| """ | ||
|
|
@@ -290,6 +296,11 @@ def __init__(self, enterprise_cert_file_path): | |
| self.signer = _custom_tls_signer.CustomTlsSigner(enterprise_cert_file_path) | ||
| self.signer.load_libraries() | ||
|
|
||
| if not self.signer.should_use_provider(): | ||
| import urllib3.contrib.pyopenssl | ||
|
|
||
| urllib3.contrib.pyopenssl.inject_into_urllib3() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| poolmanager = create_urllib3_context() | ||
| poolmanager.load_verify_locations(cafile=certifi.where()) | ||
| self.signer.attach_to_ssl_context(poolmanager) | ||
|
|
@@ -467,7 +478,7 @@ def configure_mtls_channel(self, client_cert_callback=None): | |
| creation failed for any reason. The existing session state (such | ||
| as adapter mounts) remains unmodified if this error is raised. | ||
| """ | ||
| use_client_cert = google.auth.transport._mtls_helper.check_use_client_cert() | ||
| use_client_cert = _mtls_helper.check_use_client_cert() | ||
| if not use_client_cert: | ||
| return | ||
|
|
||
|
|
@@ -476,9 +487,7 @@ def configure_mtls_channel(self, client_cert_callback=None): | |
| is_mtls, | ||
| cert, | ||
| key, | ||
| ) = google.auth.transport._mtls_helper.get_client_cert_and_key( | ||
| client_cert_callback | ||
| ) | ||
| ) = _mtls_helper.get_client_cert_and_key(client_cert_callback) | ||
|
|
||
| old_adapter = self.adapters.get("https://") | ||
|
|
||
|
|
@@ -537,7 +546,7 @@ def configure_mtls_channel(self, client_cert_callback=None): | |
| ImportError, | ||
| OSError, | ||
| ValueError, | ||
| ) as caught_exc: | ||
| ) + _OPENSSL_SSL_ERROR as caught_exc: | ||
| new_exc = exceptions.MutualTLSChannelError(caught_exc) | ||
| raise new_exc from caught_exc | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,7 +39,7 @@ | |
|
|
||
| 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"] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Replacing enterprise_cert_extra_require = cryptography_base_require + [
"pyopenssl>=20.0.0",
"cffi>=1.0.0",
] |
||
|
|
||
| urllib3_extra_require = [ | ||
| "urllib3 >= 1.26.15, < 3.0.0", | ||
|
|
@@ -65,6 +65,7 @@ | |
| *reauth_extra_require, | ||
| "responses", | ||
| *urllib3_extra_require, | ||
| *enterprise_cert_extra_require, | ||
| # Async Dependencies | ||
| *aiohttp_extra_require, | ||
| "aioresponses", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"?