gh-136306: Add support for SSL groups #136307

ronf · 2025-07-04T22:40:00Z

This is an initial implementation of the feature proposed in issue #136306.

Issue: Add support in SSL module for getting/setting groups used for key agreement #136306

📚 Documentation preview 📚: https://cpython-previews--136307.org.readthedocs.build/

This is an initial implementation of the feature proposed in issue python#136306.

picnixz

A first round of comments

picnixz · 2025-07-05T07:16:37Z

Doc/library/ssl.rst

+       >>> ctx.get_groups()
+       ['secp256r1', 'secp384r1', 'secp521r1', 'x25519', 'x448', 'brainpoolP256r1tls13', 'brainpoolP384r1tls13', 'brainpoolP512r1tls13', 'ffdhe2048', 'ffdhe3072', 'ffdhe4096', 'ffdhe6144', 'ffdhe8192', 'MLKEM512', 'MLKEM768', 'MLKEM1024', 'SecP256r1MLKEM768', 'X25519MLKEM768', 'SecP384r1MLKEM1024'
+
+   .. versionadded:: 3.15


Suggested change

.. versionadded:: 3.15

.. versionadded:: next

picnixz · 2025-07-05T07:17:33Z

Doc/library/ssl.rst

+   values.
+
+   Example::


Suggested change

values.

Example::

values. For example::

And remove the indentation in the example (I don't think it's necessary)

picnixz · 2025-07-05T07:17:51Z

Doc/library/ssl.rst

+   <https://docs.openssl.org/master/man3/SSL_CTX_set1_groups_list/>`_.
+
+   .. note::
+      when connected, the :meth:`SSLSocket.group` method of SSL sockets will


Suggested change

when connected, the :meth:`SSLSocket.group` method of SSL sockets will

When connected, the :meth:`SSLSocket.group` method of SSL sockets will

picnixz · 2025-07-05T07:18:59Z

Doc/library/ssl.rst

@@ -1789,6 +1817,10 @@ to speed up repeated connections from the same clients.

   .. versionadded:: 3.3

+   .. deprecated:: 3.15


This should be done in a separate PR as it would be better to raise a warning. Leave it supported for now.

Understood - reverted for now.

Misc/NEWS.d/next/Library/2025-07-04-23-45-00.gh-issue-136306.O1YLIU.rst

picnixz · 2025-07-05T07:41:59Z

Modules/_ssl.c

+    if (self->ssl == NULL)
+        Py_RETURN_NONE;
+    group_name = SSL_get0_group_name(self->ssl);
+    if (group_name == NULL)


Please follow PEP-7 for new code (add braces).

Looks like the indentation is required. Got a doc build error without it.

picnixz

Some other comments (sorry I'm on mobile so it's hard)

picnixz · 2025-07-05T13:51:30Z

Doc/library/ssl.rst

+   the SSLContext's current TLS ``minimum_version`` and ``maximum_version``
+   values.  For example::
+
+   >>> ctx = ssl.create_default_context()


I am a bad reviewer but I just saw that you actually copied the example style of the other example. Sorry to ask you to revert it... I would prefer to update it in a follow-up PR both examplesn(or more) on this page. Again I am very sorry (GH didn't show me enough context)

I actually had to revert this to get the docs to build. In the doc preview, though, it looks like the block isn't actually indented. So, I think we may be good here if I understand correctly what you were looking for.

One thing I didn't like here is that the long list of returned values doesn't wrap. Should I add hard line breaks in there to manually wrap the returned value across multiple lines for readability, even though the real return value wouldn't?

Doc/library/ssl.rst

Lib/test/test_ssl.py

picnixz · 2025-07-05T13:56:18Z

Lib/test/test_ssl.py

+        self.assertIsNone(ctx.set_groups('P-256:X25519'))
+
+        if ssl.OPENSSL_VERSION_INFO >= (3, 5):
+            self.assertNotIn('P-256', ctx.get_groups())


Can we create multiple contexts to check this? Like:

1 context for which we set P-256 and then check get_groups w and w/o aliases and 1 context for which we set P-256:X25519 and then check get_groups w and w/o aliases? something like

def test(self): ctx = ... ctx.set_groups('P-256') # checks ctx = ... ctx.set_groups('P-256:X25519') # checks

The values returned by ctx.get_groups() are affected only be ctx.minimum_version and ctx.maximum_version, and not by calls to ctx.set_groups(). There is a separate set of tests added in ThreadedTests.test_groups() which set the client and server contexts to different values and see what the resulting selected value is by querying SSLObject.group() after the handshake completes, including a test where the connection fails due to no overlapping groups. This is following the pattern of the test_ecdh_curve() test just above that.

Ok then maybe only use either P-256 or P-256:X25519 in the set_groups call (namely only one such call) if possible and comment what we are testing (don't use docstrings). The name "test_groups" is not sufficiently clear at first glance.

Ok - I've split out the test methods into separate test_set_groups() and test_get_groups(), and also added some comments within each of those. This also allowed me to do a skipUnless on test_get_groups() on platforms with too old an OpenSSL.

picnixz · 2025-07-05T14:01:12Z

Lib/test/test_ssl.py

+        stats = server_params_test(client_context, server_context,
+                                   chatty=True, connectionchatty=True,
+                                   sni_name=hostname)
+        if ssl.OPENSSL_VERSION_INFO >= (3, 2):


Let's add a constant (in the file) say OPENSSL_CAN_QUERY_GROUPS = ssl.OPENSSL_VERSION_INFO >= (3,2)

In this case, I think two constants would be required. The SSLObject.group() call requires 3.2 or later, but the SSLContext.get_groups() call requires 3.5 or later.

Yes please do so. It'll ease future maintenance especially if we then support other libcrypto implementations

Adding CAN_GET_SELECTED_OPENSSL_GROUP and CAN_GET_AVAILABLE_OPENSSL_GROUPS.

Misc/NEWS.d/next/Library/2025-07-04-23-45-00.gh-issue-136306.O1YLIU.rst

picnixz · 2025-07-05T14:04:07Z

Modules/_ssl.c

+
+    for (i = 0; i < num; ++i) {
+        group = sk_OPENSSL_CSTRING_value(groups, i);
+        PyList_SET_ITEM(result, i, PyUnicode_DecodeFSDefault(group));


This call can fail (decode call) so you need to handle the failure

The values returned here are constant strings coming from OpenSSL, and they should always be plain ASCII. So, the decode should always succeed in this case. I can put in a check here, but it would effectively be dead code unless there was some kind of bug in OpenSSL. The function _ssl__SSLSocket_compression_impl contains another example of this.

We need it if Python runs out of memory unfortunately. It comverts a const char* to a PyObject* so we need such guards.

Ah, gotcha. Change made.

The values returned here are constant strings coming from OpenSSL, and they should always be plain ASCII.

In this case, add an assert with a comment saying that those strings are statically allocated on OpenSSL's side. If they are dynamically fetched, checking for NULL would still be preferred but an assert doesn't hurt

I've added an assert about group elements being non-NULL, as well as a comment about these items being internal constants which shouldn't be modified or freed. I also added a comment about the strings being ASCII, so there should be no decoding errors (though an allocation check on the decoded string is still being handled in the previous commit.

picnixz · 2025-07-05T16:44:35Z

Modules/_ssl.c

+/*[clinic end generated code: output=6d6209dd1051529b input=3e8ee5deb277dcc5]*/
+{
+#if OPENSSL_VERSION_NUMBER >= 0x30500000L
+    STACK_OF(OPENSSL_CSTRING) *groups;


Can we have heap allocation instead here? or is there some magic in OpenSSL?

The word "stack" here is just what OpenSSL calls one of its data types (used for a list of items). The actual allocation is in the call to sk_OPENSSL_CSTRING_new_null just below this (with a check for allocation failure).

picnixz · 2025-07-05T16:45:53Z

Modules/_ssl.c

+            return NULL;
+        }
+
+        PyList_SET_ITEM(result, i, PyUnicode_DecodeFSDefault(group));


Suggested change

PyList_SET_ITEM(result, i, PyUnicode_DecodeFSDefault(group));

PyList_SET_ITEM(result, i, item);

Thanks for catching this!

picnixz · 2025-07-05T16:46:47Z

Modules/_ssl.c

+
+    if (!SSL_CTX_get0_implemented_groups(self->ctx, include_aliases, groups)) {
+        _setSSLError(get_state_ctx(self), "Can't get groups", 0, __FILE__, __LINE__);
+        sk_OPENSSL_CSTRING_free(groups);


Is the free() call a no-op on NULLs? I'd suggest having an error label responsible for XDECREF the result and free the groups (and don't forget to initialize them to NULL in this case).

From what I read, it is safe to call things like sk_OPENSSL_CSTRING_free() on a null pointer. For PyObjects, though, there's Py_DECREF() and Py_XDECREF(). The latter is safe to use on a potential NULL pointer.

I've gone ahead and moved all the freeing to an "error" label as you suggested. Commit will be in shortly.

From what I read, it is safe to call things like sk_OPENSSL_CSTRING_free() on a null pointer. For PyObjects, though, there's Py_DECREF() and Py_XDECREF(). The latter is supposedly safe on NULL pointers, but the former is not.

ronf · 2025-07-05T22:49:28Z

I'm not sure why the latest round of tests failed - The only change in this last commit was a doc file change, so I'm guessing it's a transient CI failure. Is there a way to trigger it to retry?

Doc/library/ssl.rst

picnixz · 2025-07-06T08:05:46Z

Doc/library/ssl.rst

+       >>> ctx.minimum_version=ssl.TLSVersion.TLSv1_3
+       >>> ctx.maximum_version=ssl.TLSVersion.TLSv1_3
+       >>> ctx.get_groups()
+       ['secp256r1', 'secp384r1', 'secp521r1', 'x25519', 'x448', 'brainpoolP256r1tls13', 'brainpoolP384r1tls13', 'brainpoolP512r1tls13', 'ffdhe2048', 'ffdhe3072', 'ffdhe4096', 'ffdhe6144', 'ffdhe8192', 'MLKEM512', 'MLKEM768', 'MLKEM1024', 'SecP256r1MLKEM768', 'X25519MLKEM768', 'SecP384r1MLKEM1024']


To reduce this, just use ... and add a +doctest: SKIP comment

picnixz · 2025-07-06T08:06:15Z

Doc/library/ssl.rst

+       >>> ctx.minimum_version=ssl.TLSVersion.TLSv1_3
+       >>> ctx.maximum_version=ssl.TLSVersion.TLSv1_3


Suggested change

>>> ctx.minimum_version=ssl.TLSVersion.TLSv1_3

>>> ctx.maximum_version=ssl.TLSVersion.TLSv1_3

>>> ctx.minimum_version = ssl.TLSVersion.TLSv1_3

>>> ctx.maximum_version = ssl.TLSVersion.TLSv1_3

picnixz · 2025-07-06T08:09:01Z

Lib/test/test_ssl.py

+        ctx = ssl.create_default_context()
+
+        # P-256 isn't an IANA name, so it shouldn't be returned by default
+        self.assertNotIn('P-256', ctx.get_groups())


Is there a default group that is known to always be returned?

The set of returned versions varies depending on the OpenSSL version. However, with the default of include_aliases=False, it should be guaranteed that P-256 will not be included since it is an alias. With include_aliases=True, I believe it should be included in all versions, going all the way back to OpenSSL 1.1.1, and I did some local testing to confirm that.

picnixz · 2025-07-06T08:12:16Z

Modules/_ssl.c

+
+    if (self->ssl == NULL) {
+        Py_RETURN_NONE;
+    }
+
+    group_name = SSL_get0_group_name(self->ssl);
+    if (group_name == NULL) {
+        Py_RETURN_NONE;
+    }
+


Suggested change

if (self->ssl == NULL) {

Py_RETURN_NONE;

}

group_name = SSL_get0_group_name(self->ssl);

if (group_name == NULL) {

Py_RETURN_NONE;

}

if (self->ssl == NULL) {

Py_RETURN_NONE;

}

group_name = SSL_get0_group_name(self->ssl);

if (group_name == NULL) {

Py_RETURN_NONE;

}

You can write more compact code in general

What did you have in mind here? With the group_name assignment, it becomes difficult to combine the blocks above and below,e even though they both do a Py_RETURN_NONE. I'm happy to remove the blank lines if you prefer that. The style guide wasn't completely clear on that and the existing code is a mixture of cases containing blank lines after close braces and not.

Modules/_ssl.c

picnixz · 2025-07-06T08:18:40Z

Modules/_ssl.c

+    }
+
+    /*
+     * Note: The "groups" stack is dynamically allocated, but the strings


This comment should be split into parts:

comment about constness should be above the assert.

comment about the dynamic memory should be at the declaration of stack_of

comment about non-failure for decoding should in the if (item == NULL).

Ok - I've made an attempt at this, though I'm not sure it completely matches what you had in mind. In particular, it was difficult to split the comment on the lack of allocation of strings inside the stack from the points later about not needing a NULL check on "group" since there's no allocation. It also meant I had to add a comment about the fact that item could be NULL due to an allocation error, even though there's no issue with decoding errors.

These and other requested changes are now in.

picnixz · 2025-07-06T08:22:34Z

Is there a way to trigger it to retry?

You can make an empty commit or ask a triager to rerun the CI (but there is none apart from me that is watching the PR I think)

picnixz

This looks great. I was planning to add some PQ support to Python now that ML-KEM/DEM were standardized but this is a good start as well. I'll have a look at the tests when I'm on my laptop again (Friday) but you can consider this PR to be approved unless I find something by then.

Doc/library/ssl.rst

ronf · 2025-07-07T13:40:16Z

This looks great. I was planning to add some PQ support to Python now that ML-KEM/DEM were standardized but this is a good start as well. I'll have a look at the tests when I'm on my laptop again (Friday) but you can consider this PR to be approved unless I find something by then.

Thanks very much! There seems to be less urgency in providing support for ML-DSA and SLH-DSA for authentication than there was for ML-KEM for key agreement mainly because of the "capture now, decrypt later" concerns, but I'd be happy to contribute to an effort around supporting those as well. I'd actually like to learn more about what OpenSSL APIs actually need to change here.

picnixz · 2025-07-07T16:14:48Z

I am currently modernizing the use of OpenSSL HMAC in hashlib and I actually wondered whether we used APIs that were deprecated in 3.0. If you want, you can help me here, at least for the SSL module (I'd prefer that we work on separate modules to avoid merge conflicts), namely look at the calls we make to OpenSSL and check if there are deprecated calls in 3.x. If there are, open an issue and a PR that modernize such calls (for HMAC, we moved from using the old HMAC_* interface to the more generic EVP_MAC interface)

ronf · 2025-07-07T21:58:04Z

If you want, you can help me here, at least for the SSL module (I'd prefer that we work on separate modules to avoid merge conflicts), namely look at the calls we make to OpenSSL and check if there are deprecated calls in 3.x.

I don't know if I'll have the cycles to actually take on fixing all the issues that we find, but I gave this a quick look to get a sense for the scope of the problem. There aren't as many changes as I was expecting to find, but it's still fairly complicated if we want to continue to support all the way back to OpenSSL 1.1.1 while avoiding functions deprecated in OpenSSL 3.x.

In many cases, the old API calls still exist in 3.x but the new API will only work on 3.x. I'm thinking leaving the old calls in place may be our best bet for now, to avoid conditional compilation. An example is replacing all references to BIO_new() with BIO_new_ex() where we pass in an explicit NULL argument for the library context. It doesn't seem worth a #if everywhere that appears as long as 3.x continues to provide a wrapper for the old BIO_new() call. A similar issue shows up around one use of BIO_new_mem_buf().

Here are some of the items I've found:

It looks like OpenSSL has split the cipher setting function into two separate functions for TLS 1.2 and below vs. TLS 1.3. The code currently calls SSL_CTX_set_cipher_list() which is TLS 1.2-only and there's another function SSL_CTX_set_ciphersuites() for TLS 1. Since we don't call the latter function right now, I think that means there's no way currently to set a non-default TLS 1.3 cipher suite list. I'm thinking this is a good candidate for treating as a bug.
There are some calls (d2i_X509_bio and i2d_X509) for reading certs that should be changed to use the OSSL_DECODER and OSSL_ENCODER APIs.
There's code reading DH params with PEM_read_DHparams() which returns a low-level DH object. I think should change to use PEM_read_bio_Parameters(), which returns an EVP_PKEY. There's also a call to DH_free that would change to EVP_PKEY_free().
Another place is setting DH params with SSL_CTX_set_tmp_dh(), and that should change to SSL_CTX_set0_tmp_dh_pkey(), which operates on EVP_PKEYs.
There's code using SSL_CTX_set_psk_client_callback() and SSL_CTX_set_psk_server_callback() which could be changed to use the OpenSSL 3.x SSL_CTX_set_psk_use_session_callback() and SSL_CTX_set_psk_find_session_callback() APIs. However, I think those new APIs only work on TLS 1.3, so this may be another case where we need to keep both around, or just continue to use the deprecated API for now, since I think it is possible to use that for both TLS 1.2 and TLS 1.3 connections if you're careful. There were some notes on this in the OpenSSL migration guide.

pythongh-136306: Initial cut at SSL groups support

662d66e

This is an initial implementation of the feature proposed in issue python#136306.

ronf requested review from gpshead and picnixz as code owners July 4, 2025 22:40

bedevere-app bot added the awaiting review label Jul 4, 2025

bedevere-app bot mentioned this pull request Jul 4, 2025

Add support in SSL module for getting/setting groups used for key agreement #136306

Open

ronf and others added 2 commits July 4, 2025 16:29

Fix regen and lint issues

9acb2c3

📜🤖 Added by blurb_it.

03072c4

ronf mentioned this pull request Jul 4, 2025

gh-109945: Enable spec of multiple curves/groups for TLS #119244

Open

3 tasks

picnixz changed the title ~~gh-136306: Initial cut at SSL groups support~~ gh-136306: Add support for SSL groups Jul 5, 2025

picnixz reviewed Jul 5, 2025

View reviewed changes

ronf added 2 commits July 5, 2025 06:39

pythongh-136306: Address first round of comments

187ff2e

pythongh-136306: Add back indentation of example

452bdec

Looks like the indentation is required. Got a doc build error without it.

picnixz reviewed Jul 5, 2025

View reviewed changes

pythongh-136306: Handle another allocation failure in get_groups()

9b4066b

picnixz reviewed Jul 5, 2025

View reviewed changes

ronf added 3 commits July 5, 2025 14:44

pythongh-136306: Address additional review comments

a6ad433

pythongh-136306: Fix method references in whatsnew entry.

aecc96b

pythongh-136306: Add missing documentation for SSLSocket.group()

05c75a5

picnixz reviewed Jul 6, 2025

View reviewed changes

pythongh-136306: Address additional review comments

304c223

picnixz reviewed Jul 7, 2025

View reviewed changes

Doc/library/ssl.rst Outdated Show resolved Hide resolved

Doc/library/ssl.rst Outdated Show resolved Hide resolved

Doc/library/ssl.rst Show resolved Hide resolved

Doc/library/ssl.rst Outdated Show resolved Hide resolved

ronf added 2 commits July 7, 2025 06:24

pythongh-136306: Minor doc updates

b516200

pythongh-136306: Missed one

e0fdd25

	when connected, the :meth:`SSLSocket.group` method of SSL sockets will
	When connected, the :meth:`SSLSocket.group` method of SSL sockets will

		@@ -1789,6 +1817,10 @@ to speed up repeated connections from the same clients.

		.. versionadded:: 3.3

		.. deprecated:: 3.15

	PyList_SET_ITEM(result, i, PyUnicode_DecodeFSDefault(group));
	PyList_SET_ITEM(result, i, item);

		>>> ctx.minimum_version=ssl.TLSVersion.TLSv1_3
		>>> ctx.maximum_version=ssl.TLSVersion.TLSv1_3

Uh oh!

gh-136306: Add support for SSL groups #136307

Are you sure you want to change the base?

gh-136306: Add support for SSL groups #136307

Conversation

ronf commented Jul 4, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

picnixz Jul 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronf commented Jul 4, 2025 •

edited by github-actions bot

Loading

picnixz Jul 5, 2025 •

edited

Loading