-
-
Notifications
You must be signed in to change notification settings - Fork 32.2k
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
base: main
Are you sure you want to change the base?
Changes from all commits
403b917
8ac3842
65490a2
8ec8608
ebad50c
af71eee
8459b66
1af4b23
b48e5bc
42ec0c8
67fd580
fae2620
8a2cbde
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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, ":") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
||
|
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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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__); | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SSL_CTX_set1_groups_list also supports another syntax, adding a
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 | ||
|
There was a problem hiding this comment.
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.