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?

picnixz · 2025-07-05T22:06:38Z

Doc/library/ssl.rst

+      When connected, the :meth:`SSLSocket.group` method of SSL sockets will
+      return the group used for key agreement on that connection.
+
+   .. versionadded:: 3.15


Suggested change

.. versionadded:: 3.15

.. versionadded:: next

picnixz · 2025-07-06T08:04:39Z

Doc/library/ssl.rst

+.. method:: SSLSocket.group()
+
+   Return the group used for doing key agreement on this connection. If no
+   connection has been established, returns ``None``.


We're missing the versionadded

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.

picnixz · 2025-07-06T08:14:47Z

Modules/_ssl.c

+/*[clinic input]
+@critical_section
+_ssl._SSLContext.set_groups
+    grouplist: str


Would it make sense to actually support a list of strings and then we join the list into a single string and pass it to OpenSSL? Maybe expose it as a pure Python method as well? And I suggest we call this method set_groups_list() and the pure python one set_groups(). (since get_groups() returns a list and not a string I think it makes sense). Tell me what you think.

Actually, to support the full functionality, a string is required here. While the string can be a single group or a colon-separated list of groups in the simplest cases, the string format actually supports a much more complex structure. It's actually a slash-separated list of "group tuples" representing different priority levels. Each group tuple is then a colon-separated list of groups which are equal in priority. There are other modifiers as well in front of group names (minus, question mark, asterisk) as well. This is all documented in the link I included in the doc (https://docs.openssl.org/master/man3/SSL_CTX_set1_groups_list).

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)

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

	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

Uh oh!

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

ronf commented Jul 4, 2025 •

edited by github-actions bot

Loading

picnixz Jul 5, 2025 •

edited

Loading