Skip to content

[2.7] bpo-33570: TLS 1.3 ciphers for OpenSSL 1.1.1 (GH-6976) (GH-8760) #10607

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

Merged
merged 1 commit into from
Feb 15, 2019

Conversation

stratakis
Copy link
Contributor

@stratakis stratakis commented Nov 20, 2018

Change TLS 1.3 cipher suite settings for compatibility with OpenSSL
1.1.1-pre6 and newer. OpenSSL 1.1.1 will have TLS 1.3 cipers enabled by
default.

Also update multissltests to test with latest OpenSSL.

https://bugs.python.org/issue33570

…ythonGH-8760)

Change TLS 1.3 cipher suite settings for compatibility with OpenSSL
1.1.1-pre6 and newer. OpenSSL 1.1.1 will have TLS 1.3 cipers enabled by
default.

Also update multissltests to test with latest OpenSSL.

Signed-off-by: Christian Heimes <christian@python.org>.
(cherry picked from commit 3e630c5)

Co-authored-by: Christian Heimes <christian@python.org>
@stratakis
Copy link
Contributor Author

This PR is based on #8771 however the additional changes to multissltests.py were added.

@stratakis
Copy link
Contributor Author

Some notes: Removed the testing 0.9.8zc and 0.9.8zh versions of openssl.
Added 1.0.2o on the old versions of openssl and 1.0.2p on the recent ones. However maybe the 1.0.2o can just be removed?
Also updated the 1.1.0 series to the latest supported version (1.1.0f -> 1.1.0i).
Added a placeholder for 1.1.1

Updated as well the libressl recent versions to 2.6.5 and 2.7.4 respectively.

]
env = os.environ.copy()
# set rpath
env["LD_RUN_PATH"] = self.lib_dir
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here

if self.system:
is missing which was added on e5f41d2

@stratakis
Copy link
Contributor Author

The '--system', argument is missing as well which was added on e5f41d2. Should it be added?

@stratakis
Copy link
Contributor Author

Also all tests pass when utilizing multissltests.py

@vstinner
Copy link
Member

The '--system', argument is missing as well which was added on e5f41d2. Should it be added?

Yeah, I noticed that. IMHO it can be added later. To be honest, I don't know what is the purpose of this new command line option.

"1.1.0f",
"1.0.2p",
"1.1.0i",
# "1.1.1",
Copy link
Member

Choose a reason for hiding this comment

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

is it skipped on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with this PR, python2 is not yet completely compatible with openssl 1.1.1, it will be with the follow-up PR's.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I preferred to ask.

@vstinner
Copy link
Member

I'm very confused by the change title and description. It says "Change TLS 1.3 cipher suite settings" but I don't see any change in ssl.py nor _ssl.c, only in tests. The main change of this PR is to remove test_default_ciphers() and add a new test_no_shared_ciphers() in test_ssl.

@vstinner
Copy link
Member

Can you mention e8eb6cb in the commit message? Your PR seems to be a backport of this commit from master to 2.7.

@stratakis
Copy link
Contributor Author

I'm very confused by the change title and description. It says "Change TLS 1.3 cipher suite settings" but I don't see any change in ssl.py nor _ssl.c, only in tests. The main change of this PR is to remove test_default_ciphers() and add a new test_no_shared_ciphers() in test_ssl.

The title is taken from the PR title, this commit was cherry-pick from

@vstinner
Copy link
Member

The title is taken from the PR title, this commit was cherry-pick from

Alright, I noticed that later :-) Maybe mention the cherry-picked commit to avoid confusion ;-)

@stratakis
Copy link
Contributor Author

stratakis commented Feb 14, 2019

Can you mention e8eb6cb in the commit message? Your PR seems to be a backport of this commit from master to 2.7.

It is actually a backport of 3e630c5 which is essentially the same but for the 3.6 branch, where this PR is mainly based of, and it's mentioned in the commit message. However the main commit from the master branch is e8eb6cb. I can change the message to point to it, if that's the better case.

@vstinner vstinner merged commit c49f63c into python:2.7 Feb 15, 2019
@vstinner
Copy link
Member

@stratakis: Thanks, I merged your first PR for OpenSSL 1.1.1 in 2.7!

Sorry, I was confused between the commit message and the PR description. PR description didn't contain the cherry-picked commit, whereas commit message did. Since you reused the commit message unmodified, it makes sense to leave it unchanged even if it's surprising (it doesn't change the default list of ciphers, it's done in your second PR).

@stratakis stratakis deleted the 2.7ssl branch June 18, 2020 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants