All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

FYI am aware of the test failure. Will be working on update for that. (apologies for delay) + arranging CLA.

The test failure occurs within test_set_ecdh_curve which tests a follows:

    @unittest.skipUnless(ssl.HAS_ECDH, "ECDH disabled on this OpenSSL build")
    def test_set_ecdh_curve(self):
        ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
        ctx.set_ecdh_curve("prime256v1")
        ctx.set_ecdh_curve(b"prime256v1")
        self.assertRaises(TypeError, ctx.set_ecdh_curve)
        self.assertRaises(TypeError, ctx.set_ecdh_curve, None)
        self.assertRaises(ValueError, ctx.set_ecdh_curve, "foo")
        self.assertRaises(ValueError, ctx.set_ecdh_curve, b"foo")

This raises two issues

a) If the change in this PR is to be included, the unit tests should be updated to also include lists of curves - no problem

More relevant is

b) The current tests assert that an exception is raised if an invalid curve is specified (which is not in the initial code)

There are a few ways of handling this

1. I could parse the list of curves, checking each one for validity and raising an exception either on first failure, or after finding all errors. (ie strtok loop, OBJ_snnid checks). This would keep the behaviour effectively the same as the current version, with precisely reported errors, but is the most complex. (and some concern about any loop iterating through the list ad implementation questions, such as is using of string.h / strtok_r ok?)
2. There are lots of places in the ssl code in general where we use _setSSLError and just return NULL without formatting a python error - even in the original code this occurs. The test could be changed to permit this -- but this is a change in behaviour
3. In addition to _setSSLError when we fail to set the curves, I could add something like PyErr_Format(PyExc_ValueError,"unknown elliptic curves %R", name);. This keeps the behaviour closest for the test case, and is the simplest implementation, but does mean other ssl failures now raise an exception where they didn't previously.

Not being very familiar with the code I'm interested in feedback as to what the most appropriate way of handling this would be? Is the additional complexity of the first option the most appropriate & desirable?

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

The current PR proposal builds cleanly. However is an area that needs discussion

Previously an invalid (no NID) curve would result in a value exception, whilst any other kind of SSL error would not, though it would set the ssl error. This passed tests cleanly.

Since we now can have multiple curves, one option is to simple call the underlying SSL function to set the curves. This makes it difficult to distinguish between the two cases, meaning that the code returns a valueerror in both cases (for invalid curves)

In working through this, I also noticed that the thread sanitizer checks FAIL when _setSSLError() is called. Before this code change we still did call this, just in less cases, and via a code path that the unix tests didn't check. With the code change the new code (minus the most recent commit) would set this on every exception (as above). My conclusion is that this function isn't thread safe? So is that a more general bug that needs investigation?

For now the _setSSLError() is removed, however I think it should be reinstated once understood.

Rebased. Would very much appreciate any review comments . Thanks!

Aware of one doc validation error from my news blurb

Adds support for multiple curves to be specified in SSLContext.set_ecdh_curve() for OpenSSL 3.0 and above by setting curve_name to a colon separated list of curves. This allows multiple curves to be passed on a TLS client hello.

/home/runner/work/cpython/cpython/build/NEWS:70: WARNING: py:func reference target not found: warnings.filterswarnings

FYI - I'm looping in @sethmlarson for additional eyeballs on whether or not they think this makes sense given it is a _ssl module change.

Going to also ping @woodruffw and @jvdprng since they're working on an adjacent project.

Going to also ping @woodruffw and @jvdprng since they're working on an adjacent project.

Thank you for the ping!

I'd like to understand the motivation a little better here: is the current cipher suite configuration insufficient? And is there a reason why ssl needs an explicit configuration for this, instead of allowing the client/server to negotiate a compatible cipher suite?

I'd like to understand the motivation a little better here: is the current cipher suite configuration insufficient? And is there a reason why ssl needs an explicit configuration for this, instead of allowing the client/server to negotiate a compatible cipher suite?

This API is for configuring the curves to offer during ECDH key exchange, not the ciphers.

This API is for configuring the curves to offer during ECDH key exchange, not the ciphers.

Whoops!

That makes more sense, although I'm still curious about the intended application here -- IME it's not common for user-level TLS APIs (with the notable exception of OpenSSL) to expose curve selection as a configurable parameter, instead preferring to keep that as a implementation detail of negotiation (with the assumption that the TLS implementation either ensures a baseline level of security or the user configures a higher level "security margin" setting that influences the curve selection.)

TL;DR: My basic concern is that this exposes an API that's uncommon among TLS APIs, is a low level option that is typically only influenced by higher level APIs, and is a potential footgun. But one could easily argue that many of OpenSSL's APIs are footguns and thus there is ample precedent for exposing this 🙂

Quick update - thanks for the review comments - apologies for the delay: just back from a few week's vacation, so will work through & respond specifically. Appreciated.

Regarding use cases, one reason for this is to be able to control whether or not a given connection will attempt to use post-quantum crypto algorithms during the key exchange (such as the hybrid X25519MLKEM768), or plain elliptic curve (such as X25519 by itself). OpenSSL 3.5 defaults to both of these, with a preference for the hybrid PQC algorithm. If an application doesn't want to pay the extra cost for PQC, they could use this mechanism to select only plain EC curves, removing the PQC algorithms from the list.

That said, we might want to think about naming the SSLContext method set_groups() rather than set_ecdh_curve() if we're going to use the OpenSSL SSL_CTX_set1_groups() function here. Alternately, we keep set_ecdh_curve() as only accepting EC curves (or maybe even jus the single curve it accepts now) and make support for multiple groups (including the fancier syntax with things like question mark and star for setting which key shares to generate) only in the new set_groups() method.

@gpshead @sethmlarson @woodruffw I would say I am keen to see this in Python and I think I agree with @ronf that I think it might be better to have this as a different api rather than overloading the set_ecdh_curve() api as it really doesn't describe what is actually happening here. Admittedly it is governed by what OpenSSL expects and can parse and in OpenSSL 3.5 the group string that it can process has become far more powerful. As well as prefixes such as ? and - there is now a prefix * to control which keyshares should be sent, also you can now group curves together by using a tuple separator (to group similar strength mechanisms). So it is really powerful now and allows it to be able to meet NIST requirements for using Quantumsafe Key exchange mechanisms. If you look at various network servers such as HAProxy or NGINX these provide ways to provide your own group string but just passes this to OpenSSL to process and I think People who write Python network servers will want to be able to offer the same ability through their configuration rather than changing the OpenSSL config file or trying to use an ENV var to point to another OpenSSL config file (which may require specialist permissions).

There is a further extension to this which I think is also important and that is observability. It is becoming more important to be able to determine which group was negotiated, which cipher was selected etc to be able to determine the level of security being used to make sure it is adequate and quantumsafe and I would try to include some more apis to access this information so a Python server/client can log this information.

I worked with @planetf1 and know he won't be returning to this, but I would like to see if I can pick it up (if anyone has some pointers around adding and testing new apis the the ssl module that would be great as I see there is some form of code generation going on) and want to guage what everyone thinks so any feedback would be great.

There is a further extension to this which I think is also important and that is observability. It is becoming more important to be able to determine which group was negotiated, which cipher was selected etc to be able to determine the level of security being used to make sure it is adequate and quantumsafe and I would try to include some more apis to access this information so a Python server/client can log this information.

Agreed on this! There's an OpenSSL API SSL_get0_group_name() which can be called to get back the name of the group which was selected for performing the key agreement. Ideally, in addition to changing this code to call SSL_CTX_set1_groups(), there would be a new method which can call SSL_get0_group_name(). Note that this would be called on an SSL object, not an SSL_CTX, though. As such, it would probably need to be available as something like SSLSocket.group(), similar to the SSLSocket.cipher() method which exists today.

I did a little more looking into this, and came up with the following change to add a set_groups() method on SSLContext:

diff --git a/Modules/_ssl.c b/Modules/_ssl.c
index 014e624f6c2..da1ed457e82 100644
--- a/Modules/_ssl.c
+++ b/Modules/_ssl.c
@@ -3403,6 +3403,25 @@ _ssl__SSLContext_get_ciphers_impl(PySSLContext *self)
 }
 
 
+/*[clinic input]
+@critical_section
+_ssl._SSLContext.set_groups
+    grouplist: str
+    /
+[clinic start generated code]*/
+
+static PyObject *
+_ssl__SSLContext_set_groups_impl(PySSLContext *self, const char *grouplist)
+/*[clinic end generated code: output=0b5d05dfd371ffd0 input=2cc64cef21930741]*/
+{
+    if (!SSL_CTX_set1_groups_list(self->ctx, grouplist)) {
+        _setSSLError(get_state_ctx(self), "unrecognized group", 0, __FILE__, __LINE__);
+        return NULL;
+    }
+    Py_RETURN_NONE;
+}
+
+
 static int
 do_protocol_selection(int alpn, unsigned char **out, unsigned char *outlen,
                       const unsigned char *server_protocols, unsigned int server_protocols_len,
@@ -5249,6 +5268,7 @@ static struct PyMethodDef context_methods[] = {
     _SSL__SSLCONTEXT__WRAP_SOCKET_METHODDEF
     _SSL__SSLCONTEXT__WRAP_BIO_METHODDEF
     _SSL__SSLCONTEXT_SET_CIPHERS_METHODDEF
+    _SSL__SSLCONTEXT_SET_GROUPS_METHODDEF
     _SSL__SSLCONTEXT__SET_ALPN_PROTOCOLS_METHODDEF
     _SSL__SSLCONTEXT_LOAD_CERT_CHAIN_METHODDEF
     _SSL__SSLCONTEXT_LOAD_DH_PARAMS_METHODDEF

You also need to run the following to get clinic to auto-generate the necessary changes in Modules/clinic/_ssl.c.h:

Tools/clinic/clinic.py Modules/_ssl.c

More documentation on clinic can be found at https://docs.python.org/3.10/howto/clinic.html.

With this, I was able to pass a string into set_groups() which supported all of the OpenSSL delimiters/flags ('?' for optional, '*' for specifying what to add as key shares, '/' for specifying multiple group tuples at different priorities, and ':' as the separator within each group tuple). See documentation on this at https://docs.openssl.org/master/man3/SSL_CTX_set1_curves/.

This doesn't implement the "SSLSocket.group()" method discussed above, but I'm going to look into that next.

Here's a version which adds SSLSocket.group() to return the selected key agreement group name after the TLS handshake has completed:

diff --git a/Lib/ssl.py b/Lib/ssl.py
index 7e3c4cbd6bb..452118822c1 100644
--- a/Lib/ssl.py
+++ b/Lib/ssl.py
@@ -931,6 +931,10 @@ def cipher(self):
         ssl_version, secret_bits)``."""
         return self._sslobj.cipher()
 
+    def group(self):
+        """Return the currently selected key agreement group name."""
+        return self._sslobj.group()
+
     def shared_ciphers(self):
         """Return a list of ciphers shared by the client during the handshake or
         None if this is not a valid server connection.
@@ -1206,6 +1210,14 @@ def cipher(self):
         else:
             return self._sslobj.cipher()
 
+    @_sslcopydoc
+    def group(self):
+        self._checkClosed()
+        if self._sslobj is None:
+            return None
+        else:
+            return self._sslobj.group()
+
     @_sslcopydoc
     def shared_ciphers(self):
         self._checkClosed()
diff --git a/Modules/_ssl.c b/Modules/_ssl.c
index 014e624f6c2..bc96b7d15ca 100644
--- a/Modules/_ssl.c
+++ b/Modules/_ssl.c
@@ -2142,6 +2142,25 @@ _ssl__SSLSocket_cipher_impl(PySSLSocket *self)
     return cipher_to_tuple(current);
 }
 
+/*[clinic input]
+@critical_section
+_ssl._SSLSocket.group
+[clinic start generated code]*/
+
+static PyObject *
+_ssl__SSLSocket_group_impl(PySSLSocket *self)
+/*[clinic end generated code: output=9c168ee877017b95 input=5f187d8bf0d433b7]*/
+{
+    const char *group_name;
+
+    if (self->ssl == NULL)
+        Py_RETURN_NONE;
+    group_name = SSL_get0_group_name(self->ssl);
+    if (group_name == NULL)
+        Py_RETURN_NONE;
+    return PyUnicode_DecodeFSDefault(group_name);
+}
+
 /*[clinic input]
 @critical_section
 _ssl._SSLSocket.version
@@ -3023,6 +3042,7 @@ static PyMethodDef PySSLMethods[] = {
     _SSL__SSLSOCKET_GETPEERCERT_METHODDEF
     _SSL__SSLSOCKET_GET_CHANNEL_BINDING_METHODDEF
     _SSL__SSLSOCKET_CIPHER_METHODDEF
+    _SSL__SSLSOCKET_GROUP_METHODDEF
     _SSL__SSLSOCKET_SHARED_CIPHERS_METHODDEF
     _SSL__SSLSOCKET_VERSION_METHODDEF
     _SSL__SSLSOCKET_SELECTED_ALPN_PROTOCOL_METHODDEF
@@ -3402,6 +3422,23 @@ _ssl__SSLContext_get_ciphers_impl(PySSLContext *self)
 
 }
 
+/*[clinic input]
+@critical_section
+_ssl._SSLContext.set_groups
+    grouplist: str
+    /
+[clinic start generated code]*/
+
+static PyObject *
+_ssl__SSLContext_set_groups_impl(PySSLContext *self, const char *grouplist)
+/*[clinic end generated code: output=0b5d05dfd371ffd0 input=2cc64cef21930741]*/
+{
+    if (!SSL_CTX_set1_groups_list(self->ctx, grouplist)) {
+        _setSSLError(get_state_ctx(self), "unrecognized group", 0, __FILE__, __LINE__);
+        return NULL;
+    }
+    Py_RETURN_NONE;
+}
 
 static int
 do_protocol_selection(int alpn, unsigned char **out, unsigned char *outlen,
@@ -5249,6 +5286,7 @@ static struct PyMethodDef context_methods[] = {
     _SSL__SSLCONTEXT__WRAP_SOCKET_METHODDEF
     _SSL__SSLCONTEXT__WRAP_BIO_METHODDEF
     _SSL__SSLCONTEXT_SET_CIPHERS_METHODDEF
+    _SSL__SSLCONTEXT_SET_GROUPS_METHODDEF
     _SSL__SSLCONTEXT__SET_ALPN_PROTOCOLS_METHODDEF
     _SSL__SSLCONTEXT_LOAD_CERT_CHAIN_METHODDEF
     _SSL__SSLCONTEXT_LOAD_DH_PARAMS_METHODDEF