remove briansmith/webpki, tidy up rustls-webpki harness, add ML-DSA support - #648
Merged
Conversation
Fixes clippy warnings in the rust-webpki and rust-rustls harnesses.
The Rust harness and helper code was not covered by any formatting or lint checks in CI, allowing non-compliant code to accumulate. Add rustfmt and clippy jobs to the lint workflow using the runner rust so regressions are caught going forward.
Remove the harness for the original `webpki` crate, along with its workspace membership and Makefile targets. The upstream crate has not seen a commit in roughly two years, or a release in almost three, while the `rustls-webpki` fork is well maintained and in use throughout the Rust ecosystem for all supported rustls versions.
Sync with the style of the rustls/webpki x509_limbo test harness: import the pki-types and webpki items the harness uses rather than fully qualifying their paths, and alias rustls-pki-types to pki-types in Cargo.toml to match how the upstream harness names it.
Sync with the rustls/webpki x509_limbo test harness: testcases without an expected peer name were previously reported as skipped after an otherwise successful chain validation. Upstream instead treats the name check as conditional, validating the chain and only verifying the subject name when the testcase specifies one.
Sync with the structure of the rustls/webpki x509_limbo test harness: move chain building and name verification into a run_validation helper returning a Result, with evaluate_testcase mapping the outcome onto a testcase result after the feature skip checks. Along the way this adopts upstream's error handling: * Leaf parse failures now include the underlying webpki error. * CRL DER parse failures fail the testcase instead of panicking the whole harness run. * Unparseable trust anchors are dropped silently, with an explicit "trust anchor extraction failed" error when none survive, replacing the stderr warning. * An invalid expected peer name fails the testcase instead of panicking.
Sync with the rustls/webpki x509_limbo test harness: use the PemObject API from rustls-pki-types to decode certificate and CRL PEM, dropping the third-party pem dependency. The single-use CRL helper is inlined at its only call site, and a malformed CRL PEM now fails the testcase rather than panicking the harness.
Sync with the rustls/webpki x509_limbo test harness's setup ordering: trust anchors first, then intermediates, CRLs and revocation options, with the leaf certificate and validation time constructed last, just ahead of verification.
Sync with the rustls/webpki x509_limbo test harness: avoid eagerly evaluating Utc::now() when the testcase provides a validation time.
Sync with the rustls/webpki x509_limbo test harness's builder usage. RevocationOptionsBuilder is Copy and its with_* methods return a new builder, so the unchained calls discarded every customization: notably the intended ExpirationPolicy::Enforce was lost, leaving the default of ignoring CRL nextUpdate. Chain the calls (via bool::then, as upstream does) so the configuration actually applies. No testcase in the current corpus changes result, but expired CRLs are now enforced as intended.
Sync with the rustls/webpki x509_limbo test harness: verify using the crate's full set of verification algorithms rather than a hand-picked subset. This picks up Ed25519, the mixed curve/hash ECDSA combinations and the absent-params RSA variants that the manual list omitted.
Sync with the rustls/webpki x509_limbo test harness, which verifies with aws-lc-rs based signature verification algorithms. This broadens ALL_VERIFICATION_ALGS with the additional ECDSA curve/hash combinations (including P-521) that the ring backend lacks, and is a prerequisite for ML-DSA support, which is only implemented for aws-lc-rs. The cc lockfile entry moves forward to satisfy aws-lc-sys.
Enable rustls-webpki's aws-lc-rs-unstable feature so the ML-DSA-44, ML-DSA-65 and ML-DSA-87 verification algorithms join ALL_VERIFICATION_ALGS, matching the ML-DSA coverage recently added to the rustls/webpki x509_limbo test harness. The rfc9881::ml-dsa-44/65/87 testcases now validate successfully and ml-dsa-44-bad-signature fails signature verification as expected. The ml-dsa-44-key-encipherment and ml-dsa-44-key-agreement testcases succeed against an expected FAILURE, matching the known divergence recorded in rustls/webpki's exceptions (webpki does not enforce the RFC 9881 key usage restrictions).
alex
approved these changes
Jul 29, 2026
Contributor
|
Review these changes very carefully before continuing! Sampled regressions: https://github.com/C2SP/x509-limbo/actions/runs/30467541040 |
Regressions found.
Member
Author
FWIW these match what I expected: three expected success test cases that now succeed (rfc9881::ml-dsa-65, rfc9881::ml-dsa-44 and rfc9881::ml-dsa-87), two expected failure success test cases that unexpectedly succeed based on our omitted KU enforcement (rfc9881::ml-dsa-44-key-encipherment, rfc9881::ml-dsa-44-key-agreement). I'm going to merge this but will happily avoid that in the future, or follow-up with more changes, if there's any additional feedback. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I recommend reviewing commit-by-commit but the broad set of changes can be summarized as three topics:
I removed the
briansmith/webpkiharness (calledrust-webpkiin this repo). We talked about this previously and there was some support. My take is that this repo hasn't seen commits or a release in years, it was mainly consumed throughrustls, and the entirerustlsecosystem has migrated to the actively maintainedrustls-webpki. I think maintaining the first-party harness here isn't great value/effort tradeoff and it's not clear who would be taking the results and using them to inform development, or benefiting from the fixes.I tidied up the
rustls-webpkiharness (and added some Rust CI coverage, trying to match project convention). Besides fixing clippy findings/formatting issues this was mainly oriented around removing divergences between what's here and what's upstream to hopefully make it easier to keep them in sync going forward. This also fixed an oversight in the CRL configuration that made it effectively a no-op.I extended the harness to support MLDSA, to support the new testcases that were added. Like upstream, we still see two failures related to KU enforcement that are accepted as-is.
As broader meta-commentary in case you wonder "Should we remove the in-repo rustls-webpki harness?", I think it's helpful to have both harnesses (here, and in
rustls/webpki) because they serve slightly different purposes. Inrustls/webpkiI see the harness as wanting to catch new issues/regressions during development, and we maintain our own configuration for places we expect to diverge. In this repo, I think having the first partyrustls-webpkiharness is helpful for immediate feedback w.r.t new test cases. I'm happy to help maintain this side over time to facilitate that.