Remove x509-verify; inline signature verification in is_link_valid

[lukevalenta]

Summary

Removes the x509-verify dependency and replaces it with a focused inline implementation in is_link_valid (x509_util) that supports exactly the algorithms present in real-world TLS/PKI certificate chains. This is a subset of the changes from #204 for updating the RustCrypto ecosystem (x509-verify uses but is not part of RustCrypto, and is not kept up-to-date with release candidate versions).

Supported algorithms:

• ECDSA P-256 (ecdsa-with-SHA256)
• ECDSA P-384 (ecdsa-with-SHA384)
• ECDSA P-521 (ecdsa-with-SHA512) (notably, was not enabled in the existing x509-verify feature set)
• RSA PKCS#1 v1.5 (SHA-1, SHA-256, SHA-384, SHA-512)

Intentionally excluded (none appear in any configured root pool, and none were enabled in our existing x509-verify feature set):

• Ed25519 (not permitted in TLS by CA/Browser Forum Baseline Requirements)
• DSA, MD2, MD5, k256, P-192, P-224

Other changes:

• Add p384 and p521 as an explicit workspace dependencies (previously only transitive deps via x509-verify)
• Replace x509_verify::spki::Error with spki::Error directly in static_ct_api::StaticCTError — x509-verify was only re-exporting spki
• Remove x509-verify from integration_tests and from the cargo-machete ignored list; also remove signature from that list (was never directly used)

[lukevalenta]

/bonk review

[ask-bonk]

Review

Clean removal of x509-verify with a well-scoped inline replacement. Clippy is clean, all 46 tests pass (x509_util: 26, static_ct_api: 20).

Code-level observations

ECDSA_WITH_SHA_512 fallback logic (lines 469-472): The comment says "P-521 is unsupported, so try P-256 then P-384", but ecdsa-with-SHA512 with a P-256 key would be unusual verging on non-existent — SHA-512 is truncated to 256 bits on P-256, and I'm not aware of any real CA chain doing this. Practically this is harmless since p256::ecdsa::VerifyingKey::try_from will reject a P-384 SPKI (and vice versa), so the fallback chain is safe — it just burns a failed parse. If this is based on a specific real-world observation feel free to keep it; otherwise verify_p384 alone would be the more obvious choice for SHA-512.

use signature::Verifier as _; appears three times — once in is_link_valid (line 450, inside the ECDSA branch but before the match arms where it's actually used) and once in each of verify_p256 (line 508) and verify_p384 (line 520). The import inside is_link_valid is dead — it's only needed in the RSA branch and the two helper functions. Consider removing the one at line 450 and keeping it only where vk.verify() is actually called (the two helpers and the RSA match arms). Clippy doesn't catch it because use … as _ is excluded from unused-import warnings, but it's still noise.

Minor: redundant double-match on sig_alg for RSA (lines 473-501). The outer match already binds the three RSA OIDs, then an inner match re-dispatches on the same variable. A slight restructure could avoid this — e.g., three separate outer arms or a helper — but this is a readability nit, not a correctness issue.

Everything else looks good

• spki::Error replacing x509_verify::spki::Error in StaticCTError — correct, same type.
• x509_cert::Certificate replacing x509_verify::x509_cert::Certificate in test imports — correct, x509-verify was just re-exporting.
• p384 promoted to explicit workspace dep with ecdsa feature — matches what the code uses.
• x509-verify and signature removed from cargo-machete ignored list — signature is now a real transitive dep via x509_util, no longer needs ignoring.
• Cargo.lock shrinks by removing dsa, k256, md-5, md2, p192, p224, p521, x509-ocsp, and x509-verify — expected given the feature list that was enabled.

[ask-bonk]

Review posted on PR #209. Here's a summary of my findings:

Verification results: Clippy clean, all 46 tests pass across x509_util and static_ct_api.

Key feedback points:

1. The ECDSA_WITH_SHA_512 fallback tries P-256 before P-384 — SHA-512 with P-256 is extremely unlikely in real CA chains, so verify_p384 alone would be the more natural choice (though the current code is safe since SPKI parsing rejects mismatched curves).
2. use signature::Verifier as _; at line 450 inside is_link_valid is dead — it's only needed in the two helper functions and the RSA branch. Clippy doesn't flag as _ imports, but it's unnecessary noise.
3. Minor readability nit on the double match sig_alg for RSA (outer match + inner match on the same variable).

Overall the PR is a clean, well-motivated removal of x509-verify with a focused inline replacement that covers exactly the algorithms needed.

github run

[lukevalenta]

/bonk review

[ask-bonk]

Verified: clippy clean, all 46 tests pass (26 x509_util, 20 static_ct_api).

This is a clean, well-scoped removal. The inline replacement in is_link_valid is straightforward and covers exactly the algorithm set needed for real-world CT/MTC chains. The helper functions (verify_p256, verify_p384, verify_rsa) are well-factored and the generic RSA approach avoids repetition.

Agree with the prior review's observations — the ECDSA_WITH_SHA_512 → verify_p384 mapping is the right call (SHA-512 with P-256 is essentially non-existent in practice), and the code is safe even in the theoretical fallback case since SPKI parsing rejects curve mismatches. The other points (dead use signature::Verifier as _ at line 450 in the old revision, redundant double-match on RSA) look like they were already addressed in the current version — the helpers now each import Verifier locally where needed, and the RSA dispatch is clean with three separate outer match arms.

The dependency changes are all correct:

• p384 promoted to explicit workspace dep with ecdsa feature
• spki added directly to static_ct_api (was previously re-exported by x509-verify)
• x509-verify and signature removed from cargo-machete ignore list
• Cargo.lock shrinks appropriately (removes dsa, k256, md-5, md2, p192, p224, p521, x509-ocsp, x509-verify)

LGTM.

[ask-bonk]

PR #209 has been approved. The changes are clean and correct — x509-verify is replaced with a focused inline implementation that covers exactly the signature algorithms present in real-world TLS/PKI certificate chains (ECDSA P-256, ECDSA P-384, RSA PKCS#1 v1.5 with SHA-256/384/512). All 46 tests pass and clippy is clean.

github run

[lukevalenta]

/bonk review

[ask-bonk]

Review

Verified: all 48 tests pass (28 x509_util, 20 static_ct_api), clippy is clean with -D warnings.

This is a well-executed removal of x509-verify. The inline replacement is focused and correct — it covers exactly the signature algorithms present in real-world TLS/PKI chains. The updates addressing Lekensteyn's feedback (adding P-521 support and SHA-1 RSA) are sound.

Code observations

verify_p521 workaround (lines 500-520): The SEC1 workaround for p521 0.13.x is well-documented and correct. One subtlety: unlike verify_p256 and verify_p384 which use TryFrom<SubjectPublicKeyInfoRef> (which validates both the algorithm OID and the key bytes), verify_p521 extracts raw subject_public_key bytes and passes them to from_sec1_bytes, which only validates the point is on the P-521 curve. This means a hypothetical SPKI with algorithm OID id-ecPublicKey + named curve secp256r1 but containing a valid 133-byte P-521 point would pass verify_p521 — which cannot happen in practice (key sizes differ), but worth noting. This is fine and will be resolved naturally when upgrading to p521 >=0.14.

Algorithm dispatch (lines 461-471): Clean and easy to follow. The ECDSA_WITH_SHA_512 → verify_p521 mapping is now correct, consistent with the real-world certificate data Lekensteyn identified (e-Szigno TLS Root CA 2023).

Helper functions (lines 474-537): The use signature::Verifier as _; imports are placed correctly — one per helper where .verify() is actually called. The generic RSA helper avoids repetition nicely.

Test coverage: The new SHA-1 RSA and P-521 ECDSA chain tests exercise the newly added code paths end-to-end through validate_chain_lax, which is good coverage.

Dependency changes: All correct — p384, p521, sha1 promoted to explicit workspace deps; spki added directly to static_ct_api; x509-verify and signature removed from cargo-machete ignore list; Cargo.lock shrinks as expected.

LGTM.

[ask-bonk]

I've reviewed and approved PR #209. Here's a summary:

Verification: All 48 tests pass (28 x509_util + 20 static_ct_api), clippy is clean.

Assessment: This is a clean, well-scoped removal of x509-verify with a focused inline replacement. The key changes since the initial review feedback:

• P-521 support added — ECDSA_WITH_SHA_512 now correctly maps to verify_p521, addressing Lekensteyn's finding that the e-Szigno TLS Root CA 2023 uses this combination.
• SHA-1 RSA support added — sha1WithRSAEncryption is now handled via verify_rsa::<sha1::Sha1>, covering the 18 root CAs in Mozilla's trust store that use this algorithm.
• New test chains — SHA-1 RSA and P-521 ECDSA synthetic chains exercise the newly added code paths.

The one minor subtlety I noted is that verify_p521 uses from_sec1_bytes (due to p521 0.13.x limitations) rather than TryFrom<SubjectPublicKeyInfoRef>, which means it doesn't validate the SPKI algorithm OID — but this is safe since P-521 points are 133 bytes and won't be confused with other curves, and it will be resolved when upgrading to p521 >=0.14.

github run

[rozbb]

LGTM! I got a little confused bc your first comment said that P-521 and SHA1 are excluded, but they are in fact supported

[lukevalenta]

LGTM! I got a little confused bc your first comment said that P-521 and SHA1 are excluded, but they are in fact supported

Oops, I've fixed the description!

[lukevalenta]

I'm going to go ahead and merge to unblock other changes, but happy to address any other feedback in followup PRs!