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

Fix Rust 1.75 clippy finding, expired real world test certificate #46

Merged
merged 2 commits into from
Dec 29, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Dec 29, 2023

  • The Let's Encrypt test certificate expired, breaking the real world verification test suite.
  • Rust 1.75 is flagging the pub use tests::ffi::*; in src/lib.rs that's behind the ffi-testing feature flag as unused. This branch allows that unused import.

Running `cargo clippy-ci` using Rust 1.75 fails:

```
error: unused import: `tests::ffi::*`
  --> src/lib.rs:28:9
   |
28 | pub use tests::ffi::*;
   |         ^^^^^^^^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(unused_imports)]`
```

This commit adds a `allow(unused_imports)` gated on the same
`ffi-testing` feature that the `pub use` statement is conditional upon.
@cpu cpu self-assigned this Dec 29, 2023
@cpu
Copy link
Member Author

cpu commented Dec 29, 2023

The Let's Encrypt test certificate expired, breaking the real world verification test suite.

Since this is going to keep happening every ~90d (who thought automated certificate issuance was a good idea? Sounds crazy to me! 🤪) we should probably fix this more robustly. Some ideas:

  1. We could use something like ci: add cert-update cron workflow #47 to keep the test cert up-to-date (but then have to contend with CI permissions).
  2. We could switch to a "live" test case that depends on a remote server we assume is correctly rotating the cert every ~60d instead of using a vendored copy. This is likely to flake in different ways - Rustls has a similar test suite and moved it to a separate daily CI task for that reason.
  3. We remove the LE test case deciding its more trouble than its worth.
  4. Status quo: we let CI fail and a human can use our shiny new Rust script to update the test files.

Edit: and of course, this also applies to the 1Password test certs - they just expire at a slower rate.

@ctz
Copy link
Member

ctz commented Dec 29, 2023

Would running the tests under faketime work?

edit: and, by implication, only on linux.

@cpu
Copy link
Member Author

cpu commented Dec 29, 2023

edit: and, by implication, only on linux.

For another repo that would probably be an OK trade-off but I think for this one the platform coverage is important :-(

@complexspaces
Copy link
Collaborator

complexspaces commented Dec 29, 2023

I don't think that we need any platform-specific time-mocking. There's just a current defficency in both the mock and real world tests where we pass in SystemTime::now() to the platform verifier instead of a hardcoded datetime. You can see that here and here.

If we update these to use a hardcoded date, such as today for example, the certificates should last much longer and only be prone to root expiry. I apologize for not calling that out earlier, I only remembered it when thinking about a better solution then something Linux-specific.

@cpu
Copy link
Member Author

cpu commented Dec 29, 2023

If we update these to use a hardcoded date, such as today for example, the certificates should last much longer and only be prone to root expiry. I apologize for not calling that out earlier, I only remembered it when thinking about a better solution then something Linux-specific.

Ah! That does sound best. Can we merge this to fix CI and I will follow up on that approach?

@complexspaces complexspaces merged commit 85c3820 into rustls:main Dec 29, 2023
13 checks passed
@complexspaces
Copy link
Collaborator

That sounds good to me. I agree its better to get CI unbroken first.

@cpu cpu deleted the cpu-ci-fixes_dev branch December 29, 2023 20:32
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

3 participants