Skip to content

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

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions Doc/library/ssl.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1769,6 +1769,10 @@ to speed up repeated connections from the same clients.
a well-known elliptic curve, for example ``prime256v1`` for a widely
supported curve.

For OpenSSL 3.0 and above *curve_name* parameter can be a colon separated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need a "new in version 3.14" admonition.

list of curves, for example ``prime256v1:brainpoolP384r1`` specifies two curves that will be
used on a client hello.

This setting doesn't apply to client sockets. You can also use the
:data:`OP_SINGLE_ECDH_USE` option to further improve security.

Expand Down
20 changes: 19 additions & 1 deletion Lib/test/test_ssl.py
Original file line number Diff line number Diff line change
Expand Up @@ -1375,11 +1375,29 @@ def test_set_ecdh_curve(self):
ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
ctx.set_ecdh_curve("prime256v1")
ctx.set_ecdh_curve(b"prime256v1")
# Only OpenSSL 3 and above supported for multiple curves
if (IS_OPENSSL_3_0_0 >= 3):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IS_OPENSSL_3_0_0 is a boolean, meaning this branch will always fail?

ctx.set_ecdh_curve("prime256v1:brainpoolP384r1")
ctx.set_ecdh_curve(b"prime256v1:brainpoolP384r1")
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")

# Multiple bad curves should cause error for any OpenSSL version
self.assertRaises(ValueError, ctx.set_ecdh_curve, "foo:bar")
self.assertRaises(ValueError, ctx.set_ecdh_curve, b"foo:bar")
self.assertRaises(ValueError, ctx.set_ecdh_curve, "prime256v1:bar")
self.assertRaises(ValueError, ctx.set_ecdh_curve, b"prime256v1:bar")
self.assertRaises(ValueError, ctx.set_ecdh_curve, "foo:prime256v1")
self.assertRaises(ValueError, ctx.set_ecdh_curve, b"foo:prime256v1")
#self.assertRaises(ValueError, ctx.set_ecdh_curve, ":")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's up with the commented out test cases? are these not valuable? it seems like a set of edge cases worth covering to define behavior on unusual inputs.

#self.assertRaises(ValueError, ctx.set_ecdh_curve, b":")
#self.assertRaises(ValueError, ctx.set_ecdh_curve, "::")
#self.assertRaises(ValueError, ctx.set_ecdh_curve, b"::")
#self.assertRaises(ValueError, ctx.set_ecdh_curve, "prime256v1:")
#self.assertRaises(ValueError, ctx.set_ecdh_curve, b"prime256v1:")
#self.assertRaises(ValueError, ctx.set_ecdh_curve, ":prime256v1")
#self.assertRaises(ValueError, ctx.set_ecdh_curve, b":prime256v1")
def test_sni_callback(self):
ctx = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
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.
11 changes: 7 additions & 4 deletions Modules/_ssl.c
Original file line number Diff line number Diff line change
Expand Up @@ -4379,18 +4379,19 @@ _ssl__SSLContext_set_ecdh_curve(PySSLContext *self, PyObject *name)
/*[clinic end generated code: output=23022c196e40d7d2 input=c2bafb6f6e34726b]*/
{
PyObject *name_bytes;
int nid;

if (!PyUnicode_FSConverter(name, &name_bytes))
return NULL;
assert(PyBytes_Check(name_bytes));
#if OPENSSL_VERSION_MAJOR < 3
int nid;
nid = OBJ_sn2nid(PyBytes_AS_STRING(name_bytes));
Py_DECREF(name_bytes);
if (nid == 0) {
PyErr_Format(PyExc_ValueError,
"unknown elliptic curve name %R", name);
return NULL;
}
#if OPENSSL_VERSION_MAJOR < 3
EC_KEY *key = EC_KEY_new_by_curve_name(nid);
if (key == NULL) {
_setSSLError(get_state_ctx(self), NULL, 0, __FILE__, __LINE__);
Expand All @@ -4399,8 +4400,10 @@ _ssl__SSLContext_set_ecdh_curve(PySSLContext *self, PyObject *name)
SSL_CTX_set_tmp_ecdh(self->ctx, key);
EC_KEY_free(key);
#else
if (!SSL_CTX_set1_groups(self->ctx, &nid, 1)) {
_setSSLError(get_state_ctx(self), NULL, 0, __FILE__, __LINE__);
int res = SSL_CTX_set1_groups_list(self->ctx, PyBytes_AS_STRING(name_bytes));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SSL_CTX_set1_groups_list also supports another syntax, adding a ? before the curve name makes it "optional", quoting the docs:

If a group name is preceded with the ? character, it will be ignored if an implementation is missing.

This isn't tested in our test suite, it would be useful to test that so future contributors know that syntax exists.

Py_DECREF(name_bytes);
if (!res) {
PyErr_Format(PyExc_ValueError,"unknown elliptic curves %R", name);
return NULL;
}
#endif
Expand Down
Loading