Skip to content
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

Update make_tls_certs.py, work with openssl 3 (#8701) #8707

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AdamWill
Copy link
Contributor

make_tls_certs.py has not been updated significantly since 2018, and the certs it generates are not good enough for openssl 3:

E ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Basic Constraints of CA cert not marked critical (_ssl.c:1020)

This resyncs the generation script with the current version of cpython's make_ssl_certs.py, on which it is based. I dropped various superficial changes which were made (wrapping, spacing, quote style), because they make diffing it against the original to see what's really different unnecessarily hard.

This also updates all the certificates, of course, which makes the tests work against openssl 3.

Closes #8701

  • Tests added / passed
  • Passes pre-commit run --all-files

make_tls_certs.py has not been updated significantly since 2018,
and the certs it generates are not good enough for openssl 3:

E           ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: Basic Constraints of CA cert not marked critical (_ssl.c:1020)

This resyncs the generation script with the current version of
cpython's make_ssl_certs.py, on which it is based. I dropped
various superficial changes which were made (wrapping, spacing,
quote style), because they make diffing it against the original
to see what's *really* different unnecessarily hard.

This also updates all the certificates, of course, which makes
the tests work against openssl 3.

Signed-off-by: Adam Williamson <[email protected]>
@AdamWill AdamWill requested a review from fjetter as a code owner June 19, 2024 22:36
@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

Admins can comment ok to test to allow this one PR to run or add to allowlist to allow all future PRs from the same author to run.

@AdamWill
Copy link
Contributor Author

oh, this also incorporates a fix I had to do to the upstream one to make it work with openssl 3.2 - python/cpython#120764

Copy link
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

    29 files  ±0      29 suites  ±0   10h 56m 27s ⏱️ - 35m 52s
 4 061 tests ±0   3 956 ✅  - 5     97 💤 ±0  8 ❌ +5 
55 939 runs  ±0  53 768 ✅  - 4  2 162 💤  - 1  9 ❌ +5 

For more details on these failures, see this check.

Results for commit 221875f. ± Comparison against base commit aeebb2d.

@AdamWill
Copy link
Contributor Author

on a quick look, the failures look like flakes, not to do with this PR...

@fjetter
Copy link
Member

fjetter commented Jun 20, 2024

add to allowlist

@fjetter
Copy link
Member

fjetter commented Jun 20, 2024

Apologies for the CI failures. Our CI is indeed haunted by flaky tests. Changes proposed LGTM but maybe we'll wait for a bit to get a review on the CPython fix

@AdamWill
Copy link
Contributor Author

If you care about keeping the tests running on ancient openssl, it would also be good to check whether they still do. It's actually hard for me to do that as it's not easily possible to spin up a Fedora build environment with something pre-openssl 3 any more :P But at least the TLS tests passed in the old CI environments, I guess that's a good sign.

@AdamWill
Copy link
Contributor Author

Hmm, reviewing the PR, we do lose the change to make the certs valid after 2037. It should be easy to change that by changing the enddate variable, I can change that if desired.

@fjetter
Copy link
Member

fjetter commented Jun 24, 2024

If you care about keeping the tests running on ancient openssl,

how ancient? (I think if CPython doesn't test more, I'm good with this)

Hmm, reviewing the PR, we do lose the change to make the certs valid after 2037. It should be easy to change that by changing the enddate variable, I can change that if desired.

Well, I assume this is only relevant if we won't update this file in the next 13 years. I'm fine with this risk.

@AdamWill
Copy link
Contributor Author

Well, I suspect it might have been done so you can test 2038 problems by testing with the system date set past 2038. I guess I can kick it out to 2045 or something, or just to the dates that we were using before (I think the validity was set to 1000 years with the old version).

@AdamWill
Copy link
Contributor Author

how ancient? (I think if CPython doesn't test more, I'm good with this)

I actually don't know, I can't test with anything older than 3 :) I'd kinda expect it to be fine, really, just wanted to flag it up as a potential issue. But yeah, this is all just from cpython, so I'd be surprised if it wasn't OK.

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.

Tests fail with openssl 3 due to test certificates not meeting current standards
3 participants