-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
gh-89730: EncryptedClientHello support in ssl module #135435
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?
Conversation
Exposes options for clients to use Encrypted Client Hello (ECH) when establishing TLS connections. It is up to the user to source the ECH public keys before making use of these options.
PyObject *retval = PyTuple_New(3); | ||
PyTuple_SetItem(retval, 0, PyLong_FromLong(status)); |
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.
PyTuple_New
can fail, and use PyTuple_SET_ITEM
to avoid error checking there.
PyTuple_SetItem(retval, 0, PyLong_FromLong(status)); | ||
|
||
if (inner_sni != NULL) { | ||
PyTuple_SetItem(retval, 1, PyUnicode_FromString(inner_sni)); |
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.
PyUnicode_FromString
can fail.
@@ -2073,6 +2077,111 @@ cipher_to_dict(const SSL_CIPHER *cipher) | |||
); | |||
} | |||
|
|||
/*[clinic input] |
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.
I'm pretty sure you need to decorate these with @critical_section
for free-threading.
@@ -70,6 +70,10 @@ | |||
#include "openssl/bio.h" | |||
#include "openssl/dh.h" | |||
|
|||
#if __has_include("openssl/ech.h") |
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.
Check #ifdef __has_include
.
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.
Also use the macro OPENSSL_ECH
that you defined (which should be renamed)
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.
As you mentioned, the ECH feature is not yet available in OpenSSL (https://github.com/openssl/openssl/tree/feature/ech). I would like to defer this until it's released.
In addition:
- please remove blank lines as much as possile. Too many blank lines hinder the reading flow.
- check return codes from OpenSSL (SSL_ech_get1_retry_config may fail for instance, so you should check if it succeeded, and if not, correctly handle the return code)
- we need tests
@@ -214,7 +214,6 @@ purposes. | |||
The context now uses :data:`VERIFY_X509_PARTIAL_CHAIN` and | |||
:data:`VERIFY_X509_STRICT` in its default verify flags. | |||
|
|||
|
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.
Keep this line.
:class:`enum.IntEnum` collection of Encrypted Client Hello (ECH) statuses | ||
returned by :meth:`SSLSocket.get_ech_status`. | ||
|
||
.. versionadded:: TODO XXX |
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.
.. versionadded:: TODO XXX | |
.. versionadded:: next |
And ditto for the rest
Adds support for Encrypted Client Hello (ECH) to the ssl module. Clients may | ||
use the options exposed to establish TLS connections using ECH. Third-party | ||
libraries like dnspython can be used to query for HTTPS and SVCB records | ||
that contain the public keys required to use ECH with specific servers. If | ||
no public key is available, an option is available to "GREASE" the | ||
connection, and it is possible to retrieve the public key from the retry | ||
configuration sent by servers that support ECH as they terminate the initial | ||
connection. |
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.
This description should also be put in What's New rather here.
@@ -0,0 +1,8 @@ | |||
Adds support for Encrypted Client Hello (ECH) to the ssl module. Clients may | |||
use the options exposed to establish TLS connections using ECH. Third-party | |||
libraries like dnspython can be used to query for HTTPS and SVCB records |
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.
libraries like dnspython can be used to query for HTTPS and SVCB records | |
libraries like ``dnspython`` can be used to query for HTTPS and SVCB records |
@@ -70,6 +70,10 @@ | |||
#include "openssl/bio.h" | |||
#include "openssl/dh.h" | |||
|
|||
#if __has_include("openssl/ech.h") |
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.
Also use the macro OPENSSL_ECH
that you defined (which should be renamed)
if ((es_in = BIO_new_mem_buf(ech_config->buf, ech_config->len)) == NULL | ||
|| (es = OSSL_ECHSTORE_new(NULL, NULL)) == NULL | ||
|| OSSL_ECHSTORE_read_echconfiglist(es, es_in) != 1 | ||
|| SSL_CTX_set1_echstore(self->ctx, es) != 1) { | ||
_setSSLError(get_state_ctx(self), NULL, 0, __FILE__, __LINE__); |
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.
Break down this big if into multiple ones and use a goto error with an error label doing the cleanup.
@@ -459,7 +464,7 @@ def wrap_socket(self, sock, server_side=False, | |||
suppress_ragged_eofs=suppress_ragged_eofs, | |||
server_hostname=server_hostname, | |||
context=self, | |||
session=session | |||
session=session, |
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.
Revert this change, it's not needed.
@property | ||
def outer_server_hostname(self) -> str: | ||
"""The server name used in the outer ClientHello.""" | ||
if self._sslobj: | ||
return self._sslobj.get_ech_status()[2] | ||
else: | ||
raise ValueError("No SSL wrapper around " + str(self)) |
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.
@property | |
def outer_server_hostname(self) -> str: | |
"""The server name used in the outer ClientHello.""" | |
if self._sslobj: | |
return self._sslobj.get_ech_status()[2] | |
else: | |
raise ValueError("No SSL wrapper around " + str(self)) | |
@property | |
def outer_server_hostname(self): | |
"""The server name used in the outer ClientHello.""" | |
return self._sslobj.get_ech_status()[2] |
We don't use type annotations in the standard library, it's left to typeshed. And let's just propagate an AttributeError as for the other properties.
def outer_server_hostname(self) -> str: | ||
"""The server name used in the outer ClientHello.""" | ||
if self._sslobj: | ||
return self._sslobj.get_ech_status()[2] | ||
else: | ||
raise ValueError("No SSL wrapper around " + str(self)) |
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.
def outer_server_hostname(self) -> str: | |
"""The server name used in the outer ClientHello.""" | |
if self._sslobj: | |
return self._sslobj.get_ech_status()[2] | |
else: | |
raise ValueError("No SSL wrapper around " + str(self)) | |
def outer_server_hostname(self): | |
"""The server name used in the outer ClientHello.""" | |
if self._sslobj: | |
return self._sslobj.get_ech_status()[2] |
Other properties actually return None if sslobj is None.
protos = bytearray() | ||
for protocol in alpn_protocols: | ||
b = bytes(protocol, 'ascii') | ||
if len(b) == 0 or len(b) > 255: |
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.
if len(b) == 0 or len(b) > 255: | |
if not b or len(b) > 255: |
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
Exposes options for clients to use Encrypted Client Hello (ECH) when establishing TLS connections. It is up to the user to source the ECH public keys before making use of these options.
To use these options requires a version of openssl that includes these options, which currently are on a feature branch:
https://github.com/openssl/openssl/tree/feature/ech
We do not expect that the API here will have any substaintial changes when it is merged.
For testing, clone the above repository, checkout the
feature/ech
branch, and build. Then use this openssl installation when building cPython:Additionally I have added to the documentation an example of the use of these options, which is built at: https://irl.github.io/cpython/library/ssl.html#encrypted-client-hello
📚 Documentation preview 📚: https://cpython-previews--135435.org.readthedocs.build/