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

gh-107562: Lib/test: update test certificates to expire far in the future #107594

Merged
merged 2 commits into from
Oct 9, 2024

Conversation

kanavin
Copy link
Contributor

@kanavin kanavin commented Aug 3, 2023

This allows testing Y2038 with system time set to after that, so that actual Y2038 issues can be exposed, and not masked by expired certificate errors.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Aug 3, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@encukou
Copy link
Member

encukou commented Feb 1, 2024

I'm not comfortable merging this without a SSL expert. (Some candidate experts were pinged as codeowners, but didn't give a yes/no, and didn't prioritize investigating the issue. As for me, I recall there's some kind of Chesterton's Fence in this area, but I can't commit to researching that.)

IMO, until we start running Y2038 tests in CPython's test suite, the way to go is regenerating the certs yourself before running the test suite.
I can review changes to make that easier, like:

  • adding expiration-related option(s) to make_ssl_certs.py
  • moving the expected expiration dates to data files in certdata, rather than using hardcoded values

@kanavin
Copy link
Contributor Author

kanavin commented Feb 1, 2024

As far as I understand problems may arise on legacy 32 bit systems which haven't been reconfigured with 64 bit time_t, as they will misinterpret post-2038 dates in regenerated certs. The pipeline above doesn't seem to include such systems.

I don't know if such retrocomputing systems are important to python upstream: I'd say making it easier to confirm that 32 bit (and 64 bit) systems that do have 64 bit time_t enabled are not going to fail is more important. 32 bit ARM is by far the most problematic - 32 bit x86, mips and ppc are all fading away in use.

If you can get more people to look at this I'd appreciate.

@kanavin kanavin force-pushed the fix-cert-expire branch 2 times, most recently from 971a38f to cb1d9bb Compare April 26, 2024 12:08
@kanavin
Copy link
Contributor Author

kanavin commented Apr 26, 2024

@encukou Note that the existing certificate generation script already sets some of the certificates to expire 7000 days into the future, and therefore the following change:

0876b92

modified them to expire in 2043, and so rendered the whole test suite incompatible with 32 bit time systems.

So this pull request won't make things any worse I think; I have meanwhile rebased it on top of all recent changes.

@encukou
Copy link
Member

encukou commented Apr 29, 2024

To be clear, I'm worried about increasing the expiration from 7000 days in the future to 140000. I seem to vaguely recall @tiran saying he had a reason to not use centuries, back when he was active.

@sethmlarson, do you have any thoughts here? The relevant change is to make_ssl_certs.py.

@gvanrossum
Copy link
Member

Here are some questions. I presume they all have reasonable answers -- I don't want to stand in the way of the future, but recently we are starting to having some concerns about checking blobs of binary data.

Why can't the testing be done on a private fork with these changes?

Has anyone (other than the PR author) double-checked that re-running make_ssl_certs.py produces the same binary blobs that are in the PR?

Why are we checking the output of that script into the tree in the first place?

A possible reason for @tiran's doubts could be that the increase might trigger Y2038 issues in other software?

@kanavin
Copy link
Contributor Author

kanavin commented Apr 29, 2024

Thanks for chiming in, I can give some answers from my perspective.

Why can't the testing be done on a private fork with these changes?

It can, and I've been doing it for a while here:
https://git.yoctoproject.org/poky-contrib/log/?h=akanavin/y2038

You can see all the issues that has uncovered, and that has been reported/fixed by me:
https://git.yoctoproject.org/poky-contrib/commit/?h=akanavin/y2038&id=e73fe14f88ee9a246d8c8e544cd553abbbdeccfe

And indeed, re-setting python test certificates has uncovered a failure in python's test_ssl, which was traced to an issue in openssl, fixed in the just-released 3.3.0:
#101732

If the question is, why does this need to land in python upstream, it's basically to allow others to easily test for actual issues, and not have to fight with certificate expiry errors (which are common across the whole userspace stack). I can't cover everything, and people configure their systems differently, and so may see issues that I don't.

Has anyone (other than the PR author) double-checked that re-running make_ssl_certs.py produces the same binary blobs that are in the PR?

Why are we checking the output of that script into the tree in the first place?

I agree. You shouldn't trust that I'm not 'Jia Tan'. I'm slightly concerned that e.g. openssl has blobs that they don't even know how to regenerate, because the original author has vanished:
openssl/openssl#21671

A possible reason for @tiran's doubts could be that the increase might trigger Y2038 issues in other software?

That's exactly the goal. We want to trigger (and fix) those issues now, and not when the date rolls over and critical infrastructure stops working.

@encukou
Copy link
Member

encukou commented Apr 29, 2024

Has anyone (other than the PR author) double-checked that re-running make_ssl_certs.py produces the same binary blobs that are in the PR?

Unfortunately, the certs include random data and the current date.
(Perhaps we can tell openssl to be deterministic? That would be a great improvement!)

Anyway, a core dev should re-create the certs before merging. I'll add a label so we don't merge before that.

Why are we checking the output of that script into the tree in the first place?

AFAIK, it's because they're generated by the openssl command, while the ssl module only needs the libssl library.

A possible reason for @tiran's doubts could be that the increase might trigger Y2038 issues in other software?

If that's the only reason, and buildbot tests pass for supported platforms, then we should merge this.

@sethmlarson
Copy link
Contributor

@encukou Thanks for the ping. I don't have specific concerns about having a test case for a certificate that expires far in the future. I do know there's been a general trend of certificate lifetimes becoming shorter and shorter, and specifically not longer. This change kinda flies in the face of that trend.

I like the idea of testing for Y2038, but we don't necessarily need to change all of our certificates to achieve that and isn't 7,000 days already enough to set us past 2038?

Regarding "seeded" generation of certificates, I don't think OpenSSL has support but certtool has support for a --seed option when generating certificates. Might be an interesting option to consider?

@kanavin
Copy link
Contributor Author

kanavin commented Apr 30, 2024

Has anyone (other than the PR author) double-checked that re-running make_ssl_certs.py produces the same binary blobs that are in the PR?

Unfortunately, the certs include random data and the current date. (Perhaps we can tell openssl to be deterministic? That would be a great improvement!)

Another option is for the cert generation script to obtain the data after generating the certs, and write it into a reference file next to the cert files. Then the reference file would be used by the tests, instead of having that data hardcoded inside test_ssl.py. It's basically these fields that change:

    'notAfter': 'Aug 17 11:50:48 2407 GMT',
    'notBefore': 'Apr 26 11:50:48 2024 GMT',
    'serialNumber': '7906193B681A42BFF0BFAD94500222617C174ACA',

@kanavin
Copy link
Contributor Author

kanavin commented Apr 30, 2024

@encukou Thanks for the ping. I don't have specific concerns about having a test case for a certificate that expires far in the future. I do know there's been a general trend of certificate lifetimes becoming shorter and shorter, and specifically not longer. This change kinda flies in the face of that trend.

I like the idea of testing for Y2038, but we don't necessarily need to change all of our certificates to achieve that and isn't 7,000 days already enough to set us past 2038?

Setting expiry dates far in the future is intentional. The idea is that when people want to test Y2038 and set their system time to some future date after 2038 they don't have to think about what date specifically to set so that all components' test suites fit into it. Just pick something from the 21st century and it will work. Y2038 testing is done with a complete system, not just isolated components.

Another consideration is that this way, expiry dates don't have to be re-set again in our lifetimes, this is done once, and no one has to deal with it again. Long validity periods don't affect security, as these certs are never deployed anywhere.

@encukou
Copy link
Member

encukou commented May 1, 2024

Another option is for the cert generation script to obtain the data after generating the certs

That's what I meant in #107594 (comment).
make_ssl_certs.py should become a command you can run as part of the build or test.

@kanavin
Copy link
Contributor Author

kanavin commented May 22, 2024

Enhancements to make_ssl_certs that aren't about expiration dates now have their own pull request:

#119401

@kanavin
Copy link
Contributor Author

kanavin commented Sep 6, 2024

#119401 isn't getting any attention from reviewers, sadly, so this can't move forward either (as it needs to be rebased on top of that, once it merges).

@kanavin kanavin force-pushed the fix-cert-expire branch 3 times, most recently from 6ad5725 to 41fa741 Compare September 26, 2024 11:11
@kanavin
Copy link
Contributor Author

kanavin commented Sep 26, 2024

Somehow, simply running make_ssl_certs.py on my build machine (and committing the result to this PR) no longer seems to produce certificates that tests are happy with:

FAIL: test_certificate_chain (test.test_ssl.TestPostHandshakeAuth.test_certificate_chain)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_ssl.py", line 4750, in test_certificate_chain
    self.assertEqual(expected_ee_cert, ee)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
AssertionError: b'0\x[585 chars]00\xa0*(q\x0b\xed\xa0\xed\xde\xcc%\xf2\x14\x05[3204 chars]xac=' != b'0\x[585 chars]00\xafJ\t\x89CI\x9a\x14\x996\x95Ra\xba%\x82\xd[3234 chars]\x04'

I've confirmed this is not related to recent improvements to make_ssl_certs.py by reverting them and still observing this fail.

I've excluded cert regeneration from this PR for now. This needs to be done separately by a reviewer/maintainer anyway to avoid supply chain attacks :)

@kanavin
Copy link
Contributor Author

kanavin commented Sep 26, 2024

@encukou please take a look.

@kanavin
Copy link
Contributor Author

kanavin commented Sep 26, 2024

Somehow, simply running make_ssl_certs.py on my build machine (and committing the result to this PR) no longer seems to produce certificates that tests are happy with:

FAIL: test_certificate_chain (test.test_ssl.TestPostHandshakeAuth.test_certificate_chain)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_ssl.py", line 4750, in test_certificate_chain
    self.assertEqual(expected_ee_cert, ee)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
AssertionError: b'0\x[585 chars]00\xa0*(q\x0b\xed\xa0\xed\xde\xcc%\xf2\x14\x05[3204 chars]xac=' != b'0\x[585 chars]00\xafJ\t\x89CI\x9a\x14\x996\x95Ra\xba%\x82\xd[3234 chars]\x04'

I've confirmed this is not related to recent improvements to make_ssl_certs.py by reverting them and still observing this fail.

I've bisected this to 8ef358d

commit 8ef358dae1959e2aff8b04fb69b8a36d6da6847a
Author: Mateusz Nowak <[email protected]>
Date:   Fri Aug 16 22:27:44 2024 +0200

    gh-118658: Return consistent types from `get_un/verified_chain` in `SSLObject` and `SSLSocket` (#118669)
    
    Co-authored-by: Gregory P. Smith [Google LLC] <[email protected]>

I need to take a break now, but it probably needs to be reverted? Looks like cert3.pem was incorrectly allowed into the tree without a way to regenerate it.

@kanavin
Copy link
Contributor Author

kanavin commented Sep 26, 2024

Come to think of it, running make_ssl_certs.py should also be a part of the CI pipeline at least in one build. So that regressions like this don't happen.

@kanavin
Copy link
Contributor Author

kanavin commented Sep 26, 2024

I need to take a break now, but it probably needs to be reverted? Looks like cert3.pem was incorrectly allowed into the tree without a way to regenerate it.

Ok, I think the regression is resolved with these cert3.pem fixes. The PR for them is #124598

@encukou
Copy link
Member

encukou commented Oct 7, 2024

Took some time as I couldn't make this a high priority, but it looks like the yak is almost shaved now.
Could you update this PR so it only contains the change to the script? If not, I'll do it next week.

I'd like to get the #120762 fix approved too, and then I'd run the resulting script, put updated certs in a separate PR, and merge all three.

…default

This allows testing Y2038 with system time set to after that,
so that actual Y2038 issues can be exposed, and not masked
by expired certificate errors.

Signed-off-by: Alexander Kanavin <[email protected]>
@kanavin
Copy link
Contributor Author

kanavin commented Oct 7, 2024

Took some time as I couldn't make this a high priority, but it looks like the yak is almost shaved now. Could you update this PR so it only contains the change to the script? If not, I'll do it next week.

Done.

@encukou
Copy link
Member

encukou commented Oct 7, 2024

@sethmlarson Well, this PR is in its final and pure form now.

Do your objections still stand after @kanavin's later comments? (“Y2038 testing is done with a complete system”, “Long validity periods don't affect security, as these certs are never deployed anywhere.”)

@sethmlarson
Copy link
Contributor

@encukou This change is fine with me. Since we're using OpenSSL APIs for now this is an okay fix (because OpenSSL doesn't have an opinion about certificate lifetimes), but if we ever adopt system certificate APIs then this approach won't work anymore.

@encukou encukou merged commit 53930cb into python:main Oct 9, 2024
34 of 35 checks passed
efimov-mikhail pushed a commit to efimov-mikhail/cpython that referenced this pull request Oct 9, 2024
…pire far in the future by default (pythonGH-107594)

This allows testing Y2038 with system time set to after that,
so that actual Y2038 issues can be exposed, and not masked
by expired certificate errors.

Signed-off-by: Alexander Kanavin <[email protected]>
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