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

tests: use a fixed SystemTime for certificate validation #50

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

cpu
Copy link
Member

@cpu cpu commented Jan 3, 2024

Fixing the SystemTime that we pass to the certificate verifier for the real world and mock verification tests will ensure that the tests don't start to fail just because the vendored certificates have expired.

Suggested by @complexspaces in #46

@cpu cpu self-assigned this Jan 3, 2024
Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

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

There's two other items that should be fixed/acknowledged here as well:

  • This comment block should be deleted because its no longer accurate.
  • We should see if the OSCP stapling test can be uncommented finally. If it doesn't work right away or needs medium effort to revive it can be punted to a followup issue.

rustls-platform-verifier/src/lib.rs Outdated Show resolved Hide resolved
rustls-platform-verifier/src/lib.rs Outdated Show resolved Hide resolved
@cpu
Copy link
Member Author

cpu commented Jan 3, 2024

We should see if the OSCP stapling test can be uncommented finally. If it doesn't work right away or needs medium effort to revive it can be punted to a followup issue.

let me see if I can get it working. I think at a minimum we'll need to refresh the vendored OCSP resp. The one in-repo expired in 2021:

$ openssl ocsp --respin rustls-platform-verifier/src/tests/verification_real_world/revoked_badssl_com_1.ocsp -noverify -text | grep "Next Update"
    Next Update: Dec  9 01:24:01 2021 GMT

@cpu
Copy link
Member Author

cpu commented Jan 3, 2024

The one in-repo expired in 2021:

Oh, well the revoked.badssl.com endpoint is also serving an expired cert (which I suppose makes sense, they aren't going to re-issue and re-revoke certs to keep the expiry up-to-date). I think we'll need to switch to using a fixed time per testcase for this.

@cpu cpu force-pushed the cpu-test-with-fixed-time_dev branch from 06f8735 to 6ed2f94 Compare January 3, 2024 19:57
Fixing the `SystemTime` that we pass to the certificate verifier for the
mock and real world verification tests will ensure that the tests don't
start to fail just because the vendored certificates have expired.
* Fix the date the files were vendored
* Fix the validity period
* Fix the ref to the tool used to update them
@cpu cpu force-pushed the cpu-test-with-fixed-time_dev branch from 6ed2f94 to 1c03c96 Compare January 3, 2024 19:58
@cpu
Copy link
Member Author

cpu commented Jan 3, 2024

This comment block should be deleted because its no longer accurate.

Fixed.

We should see if the OSCP stapling test can be uncommented finally. If it doesn't work right away or needs medium effort to revive it can be punted to a followup issue.

I started on this, but surprisingly the Windows verifier seems to be returning Ok(()) when I would have expected it to honour the stapled OCSP response and return revoked. This will need more investigation so I've pushed the rest of the feedback in the meantime.

@cpu
Copy link
Member Author

cpu commented Jan 3, 2024

I started on this, but surprisingly the Windows verifier seems to be returning Ok(()) when I would have expected it to honour the stapled OCSP response and return revoked.

Conditionally adding CERT_CHAIN_REVOCATION_CHECK_CACHE_ONLY when we have an OCSP staple doesn't seem to do the trick either. I'm stumped (and tired of iterating in CI 😅) so I'm going to pause on this for now. I'll file an issue for follow-up (edit: #51).

Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

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

Thanks for jumping on that followup issue quick!

@complexspaces complexspaces merged commit dc8de0a into rustls:main Jan 4, 2024
13 checks passed
@cpu cpu deleted the cpu-test-with-fixed-time_dev branch January 4, 2024 13:38
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.

None yet

2 participants