-
Notifications
You must be signed in to change notification settings - Fork 419
Add ability to TLS 1.3 cipher suites on SSL Context #1432
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
Needs test + changelog entry. |
0f13c17
to
406e319
Compare
should be possible to rebase on main to resolve the CI issues |
182450c
to
c6fe5df
Compare
src/OpenSSL/SSL.py
Outdated
|
||
.. versionadded:: 25.2.0 | ||
""" | ||
ciphersuites = _text_to_bytes_and_warn("ciphersuites", ciphersuites) |
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.
No warning for new APIs, we should strictly enforce types.
tests/test_ssl.py
Outdated
) | ||
def test_set_cipher_list( | ||
def test_set_ciphersuites( |
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 need tests for both, you can't get rid of the old test.
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.
my mistake I copied the old and then messed up the existing one.
07815b9
to
09a3e1f
Compare
src/OpenSSL/SSL.py
Outdated
@@ -1501,6 +1504,29 @@ def set_cipher_list(self, cipher_list: bytes) -> None: | |||
], | |||
) | |||
|
|||
@_require_not_used | |||
def set_ciphersuites(self, ciphersuites: bytes) -> None: |
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.
@reaperhulk should this be set_tls13_ciphersuites
, or just match the OpenSSL name?
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.
Yeah, I think we should put tls13
in there even though it's inconsistent with OpenSSL itself. Marginally less confusing to consumers or people reading code who haven't memorized the docstrings for every function.
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 changed the PR to add the tls13 in there. I hope we don't have to rename it again when TLS 1.4/2.0 comes out
09a3e1f
to
39218cf
Compare
No description provided.