FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-109945: Enable spec of multiple curves/groups for TLS by planetf1 · Pull Request #119244 · python/cpython · GitHub

  1. python
  2. cpython

Repository navigation

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.

Sign up for GitHub

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

Closed
planetf1 wants to merge 13 commits into from
+31 −5
Conversation 35 Commits 13 Checks 0 Files changed 4
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
403b917
gh-109945 Enable spec of multiple curves/groups for TLS
planetf1 May 20, 2024
8ac3842
gh-109945 Additional testcases for ssl
planetf1 Jun 4, 2024
65490a2
gh-109945 Refine exception & add more tests
planetf1 Jun 3, 2024
8ec8608
📜🤖 Added by blurb_it.
blurb-it[bot] Jun 4, 2024
ebad50c
gh-109945 update documentation
planetf1 Jun 4, 2024
af71eee
gh-109945 fix incorrect reference management of string
planetf1 Jun 4, 2024
8459b66
gh-109945 fix openssl<3 tests
planetf1 Jun 4, 2024
1af4b23
gh-109945 update docs to clarify change is OpenSSL 3 and above
planetf1 Jun 4, 2024
b48e5bc
gh-109945 Corrected typo/whitespace in docs
planetf1 Jun 4, 2024
42ec0c8
gh-109945 Correct thread issue
planetf1 Jun 4, 2024
67fd580
gh-109945 do not call _setSSLError
planetf1 Jun 4, 2024
fae2620
gh-109945 remove commented code
planetf1 Jul 17, 2024
8a2cbde
gh-109945 update NEWS to correct function call format check
planetf1 Jul 17, 2024
File filter

Filter by extension

Filter by extension .c  (1) .py  (1) .rst  (2) All 3 file types selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
4 changes: 4 additions & 0 deletions Doc/library/ssl.rst
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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

sethmlarson Jul 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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):

sethmlarson Jul 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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, ":")

gpshead Jul 19, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2024-06-04-07-28-18.gh-issue-109945.02TejW.rst
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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
Show comments
View file
Open in desktop
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
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));

sethmlarson Jul 19, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Back | FazBrowse Home | New Git URL