-
-
Notifications
You must be signed in to change notification settings - Fork 33.1k
gh-137197: Add SSLContext.set_ciphersuites to set TLS 1.3 ciphers #137198
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
Conversation
{ | ||
int ret = SSL_CTX_set_cipher_list(self->ctx, cipherlist); | ||
if (ret == 0) { | ||
/* Clearing the error queue is necessary on some OpenSSL versions, |
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.
We shouldn't remove this without understanding which OpenSSL versions it was referring to and whether they are still in use by CPython builds (1.1.x-ish API'd AWS-LC at a minimum, otherwise OpenSSL 3.0+).
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.
The clearing of the error queue is still happening here. I just took advantage of an existing helper function setSSLError
to take care of this:
static PyObject *
_setSSLError (_sslmodulestate *state, const char *errstr, int errcode, const char *filename, int lineno)
{
if (errstr == NULL)
errcode = ERR_peek_last_error();
else
errcode = 0;
fill_and_set_sslerror(state, NULL, state->PySSLErrorObject, errcode, errstr, lineno, errcode);
ERR_clear_error();
return NULL;
}
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.
Let's keep it that way and change it in a follow-up PR instead. Actually, the errcode
parameter for _setSSLError
is redundant as it's always overwritten.
Clarify when to use the original set_ciphers (TLS 1.2 and earlier) vs. the new set_ciphersuites (TLS 1.3) methods and that both can be used at once.
f2cda88
to
d3375f1
Compare
I think all the remaining review comments should be addressed in 48e5164. Let me know if you'd like any other changes. |
This commit reworks the set_ciphersuites() test cases, moving them into their own class to avoid any changes to existing tests. It also makes the cipher selection dynamic to avoid potentially trying to use a cipher not available in some environments.
@picnixz, any further changes required? The build failure appears to be something in the test runner, as the only thing changed in the last checkin was capitalization in the docs. Prior to that, all tests succeeded. |
I'm not available before next week but I'll have a look. |
That'd be great - thanks! I spent some time today working on adding methods to manage signature algorithms and will create a new issue & PR for those as soon as these changes are merged. |
Doc/library/ssl.rst
Outdated
ciphers yet, but :meth:`SSLContext.get_ciphers` returns them. | ||
- TLS 1.3 uses a disjunct set of cipher suites. All AES-GCM and ChaCha20 | ||
cipher suites are enabled by default. To restrict which TLS1.3 ciphers | ||
are allowed, the method :meth:`SSLContext.set_ciphersuites` should be |
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.
For posterity: I really hate that OpenSSL named it SSL_CTX_set_ciphersuites
for TLS 1.3 and later, and SSL_CTX_set_cipher_list
for TLS 1.2 and below. I hope that this won't be annoying to users.
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.
I was surprised what they did here at first and I'm not a big fan about the naming chosen, but I can see why they didn't want to try and extend the existing command. It allows all kinds of partial matches on the five individual elements that make up a cipher suite in TLS 1.2 and earlier, plus other special keywords like LOW/MEDIUM/HIGH. All of that is gone with TLS 1.3, and the new function is much simpler and only supports an exact match against the very small number of cipher suites defined in TLS 1.3.
I don't really know why they didn't include "_list" in the name of the OpenSSL function, given that this seems to be the convention for other such calls that take a string argument.
The good news here is that things like post-quantum crypto are going to drive migration to TLS 1.3, and at some point setting TLS 1.2 cipher suites will no longer matter. At that point, the previous set_ciphers()
can be deprecated and eventually removed, with only set_ciphersuites()
remaining.
{ | ||
int ret = SSL_CTX_set_cipher_list(self->ctx, cipherlist); | ||
if (ret == 0) { | ||
/* Clearing the error queue is necessary on some OpenSSL versions, |
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.
Let's keep it that way and change it in a follow-up PR instead. Actually, the errcode
parameter for _setSSLError
is redundant as it's always overwritten.
Thanks for your review! I think I've addressed this round of review comments, including reverting the change to _ssl__SSLContext_set_ciphers_impl for now. Let me know if anything else is needed. |
Just checking in - are there any further changes you'd like me to make? |
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.
Just a few nitpicks.
Misc/NEWS.d/next/Library/2025-07-29-05-12-50.gh-issue-137197.bMK3sO.rst
Outdated
Show resolved
Hide resolved
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.
A final nitpick and we're good to go I think
I've gone ahead and created issue #138252 in preparation for the next round of changes, to add support for getting and setting TLS signature algorithms. I have those changes ready, but will hold off until this change is merged before I submit the PR, to avoid any merge conflicts. |
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.
I'm going to amend it directly and merge this.
self.assertRaises(ssl.SSLEOFError, sslobj.read) | ||
|
||
|
||
@unittest.skipUnless(has_tls_version('TLSv1_3'), "TLS 1.3 is not available") |
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.
Note to myself: change the requires_tls_version
decorator so that it works.
min_version=ssl.TLSVersion.TLSv1_2, | ||
max_version=ssl.TLSVersion.TLSv1_2) as s: | ||
s.connect(self.server_addr) | ||
self.assertEqual(s.cipher()[1], 'TLSv1.2') |
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 test is asserting that the default TLSv1_2
cipher suite is actually being used right? I'm fine with the current test but I'd like to fortify it (which I'll do) in the future where we have a ciphersuite for TLSv1.3 and a ciphersuite for TLSv1.2 and if we use min/max versions, then we're going to be using the TLSv1.2 ciphersuite we specified.
This commit adds a new method SSLContext.set_ciphersuites which can be used to set TLS 1.3 cipher suites. It also updates the documentation, unit tests, and "what's new" text. A NEWS blurb will be added shortly.
📚 Documentation preview 📚: https://cpython-previews--137198.org.readthedocs.build/