gh-136306: Address additional review comments

Author: ronf

Doc/library/ssl.rst

@@ -1641,7 +1641,7 @@ to speed up repeated connections from the same clients.
 
    .. versionadded:: 3.6
 
-.. method:: SSLContext.get_groups()
+.. method:: SSLContext.get_groups(*, include_aliases=False)
 
    Get a list of groups implemented for key agreement, taking into account
    the SSLContext's current TLS ``minimum_version`` and ``maximum_version``
@@ -1653,6 +1653,11 @@ to speed up repeated connections from the same clients.
        >>> ctx.get_groups()
        ['secp256r1', 'secp384r1', 'secp521r1', 'x25519', 'x448', 'brainpoolP256r1tls13', 'brainpoolP384r1tls13', 'brainpoolP512r1tls13', 'ffdhe2048', 'ffdhe3072', 'ffdhe4096', 'ffdhe6144', 'ffdhe8192', 'MLKEM512', 'MLKEM768', 'MLKEM1024', 'SecP256r1MLKEM768', 'X25519MLKEM768', 'SecP384r1MLKEM1024']
 
+   By default, this method returns only the preferred IANA names for the
+   available groups. However, if the ``include_aliases`` parameter is set to
+   :const:`True` this method will also return any associated aliases such as
+   the ECDH curve names supported in older versions of OpenSSL.
+
    .. versionadded:: next
 
 .. method:: SSLContext.set_default_verify_paths()

Doc/whatsnew/3.15.rst

@@ -158,6 +158,23 @@ ssl
   supports "External PSKs" in TLSv1.3, as described in RFC 9258.
   (Contributed by Will Childs-Klein in :gh:`133624`.)
 
+* Added new methods for managing groups used for SSL key agreement
+
+   * :meth:`SSLContext.set_groups` sets the groups allowed for doing
+     key agreement, extending the previous :meth:`set_ecdh_curve` method.
+     This new API provides the ability to list multiple groups and
+     supports fixed-field and post-quantum groups in addition to ECDH
+     curves. This method can also be used to control what key shares
+     are sent in the TLS handshake.
+   * :meth:`SSLSocket.group` returns the group selected for doing key
+     agreement on the current connection after the TLS handshake completes.
+     This call requires OpenSSL 3.2 or later.
+   * :meth:`SSLContext.get_groups` returns a list of all available key
+     agreement groups compatible with the minimum and maximum TLS versions
+     currently set in the context. This call requires OpenSSL 3.5 or later.
+
+   (Contributed by Ron Frederick in :gh:`136306`)
+
 
 tarfile
 -------

Lib/test/test_ssl.py

@@ -48,6 +48,8 @@
 PROTOCOLS = sorted(ssl._PROTOCOL_NAMES)
 HOST = socket_helper.HOST
 IS_OPENSSL_3_0_0 = ssl.OPENSSL_VERSION_INFO >= (3, 0, 0)
+CAN_GET_SELECTED_OPENSSL_GROUP = ssl.OPENSSL_VERSION_INFO >= (3, 2)
+CAN_GET_AVAILABLE_OPENSSL_GROUPS = ssl.OPENSSL_VERSION_INFO >= (3, 5)
 PY_SSL_DEFAULT_CIPHERS = sysconfig.get_config_var('PY_SSL_DEFAULT_CIPHERS')
 
 PROTOCOL_TO_TLS_VERSION = {}
@@ -960,14 +962,25 @@ def test_get_ciphers(self):
             len(intersection), 2, f"\ngot: {sorted(names)}\nexpected: {sorted(expected)}"
         )
 
-    def test_groups(self):
+    def test_set_groups(self):
         ctx = ssl.create_default_context()
-        self.assertIsNone(ctx.set_groups('P-256'))
+
+        # Test valid group list
         self.assertIsNone(ctx.set_groups('P-256:X25519'))
 
-        if ssl.OPENSSL_VERSION_INFO >= (3, 5):
-            self.assertNotIn('P-256', ctx.get_groups())
-            self.assertIn('P-256', ctx.get_groups(include_aliases=True))
+        # Test invalid group list
+        self.assertRaises(ssl.SSLError, ctx.set_groups, 'P-256:xxx')
+
+    @unittest.skipUnless(CAN_GET_AVAILABLE_OPENSSL_GROUPS,
+                         "OpenSSL version doesn't support getting groups")
+    def test_get_groups(self):
+        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())
+
+        # Aliases like P-256 sbould be returned when include_aliases is set
+        self.assertIn('P-256', ctx.get_groups(include_aliases=True))
 
     def test_options(self):
         # Test default SSLContext options
@@ -2710,7 +2723,7 @@ def server_params_test(client_context, server_context, indata=b"FOO\n",
                 'session_reused': s.session_reused,
                 'session': s.session,
             })
-            if ssl.OPENSSL_VERSION_INFO >= (3, 2):
+            if CAN_GET_SELECTED_OPENSSL_GROUP:
                 stats.update({'group': s.group()})
             s.close()
         stats['server_alpn_protocols'] = server.selected_alpn_protocols
@@ -4146,7 +4159,7 @@ def test_groups(self):
         stats = server_params_test(client_context, server_context,
                                    chatty=True, connectionchatty=True,
                                    sni_name=hostname)
-        if ssl.OPENSSL_VERSION_INFO >= (3, 2):
+        if CAN_GET_SELECTED_OPENSSL_GROUP:
             self.assertEqual(stats['group'], "secp384r1")
 
         # server auto, client secp384r1
@@ -4156,7 +4169,7 @@ def test_groups(self):
         stats = server_params_test(client_context, server_context,
                                    chatty=True, connectionchatty=True,
                                    sni_name=hostname)
-        if ssl.OPENSSL_VERSION_INFO >= (3, 2):
+        if CAN_GET_SELECTED_OPENSSL_GROUP:
             self.assertEqual(stats['group'], "secp384r1")
 
         # server / client curve mismatch

Modules/_ssl.c

@@ -3462,46 +3462,55 @@ _ssl__SSLContext_get_groups_impl(PySSLContext *self, int include_aliases)
 /*[clinic end generated code: output=6d6209dd1051529b input=3e8ee5deb277dcc5]*/
 {
 #if OPENSSL_VERSION_NUMBER >= 0x30500000L
-    STACK_OF(OPENSSL_CSTRING) *groups;
+    STACK_OF(OPENSSL_CSTRING) *groups = NULL;
     const char *group;
     size_t i, num;
-    PyObject *item, *result;
+    PyObject *item, *result = NULL;
 
     if ((groups = sk_OPENSSL_CSTRING_new_null()) == NULL) {
         _setSSLError(get_state_ctx(self), "Can't allocate stack", 0, __FILE__, __LINE__);
-        return NULL;
+	goto error;
     }
 
+    /*
+     * Note: The "groups" stack is dynamically allocated, but the strings
+     * returned in the stack are references to internal constants which
+     * should NOT be modified or freed. They should also be plain ASCII,
+     * so there should be no decoding issue when converting to Unicode.
+     */
+
     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);
-        return NULL;
+        goto error;
     }
 
     num = sk_OPENSSL_CSTRING_num(groups);
     result = PyList_New(num);
     if (result == NULL) {
         _setSSLError(get_state_ctx(self), "Can't allocate list", 0, __FILE__, __LINE__);
-        sk_OPENSSL_CSTRING_free(groups);
-        return NULL;
+        goto error;
     }
 
     for (i = 0; i < num; ++i) {
         group = sk_OPENSSL_CSTRING_value(groups, i);
+	assert(group != NULL);
+
         item = PyUnicode_DecodeFSDefault(group);
 
         if (item == NULL) {
             _setSSLError(get_state_ctx(self), "Can't allocate group name", 0, __FILE__, __LINE__);
-            Py_XDECREF(result);
-            sk_OPENSSL_CSTRING_free(groups);
-            return NULL;
+            goto error;
         }
 
-        PyList_SET_ITEM(result, i, PyUnicode_DecodeFSDefault(group));
+        PyList_SET_ITEM(result, i, item);
     }
 
     sk_OPENSSL_CSTRING_free(groups);
     return result;
+error:
+    Py_XDECREF(result);
+    sk_OPENSSL_CSTRING_free(groups);
+    return NULL;
 #else
     PyErr_SetString(PyExc_NotImplementedError,
                     "Getting implemented groups requires OpenSSL 3.5 or later.");