only consume signature budgets during path building

[cpu]

Previously (#164) we updated the signed_data::verify_signed_data fn to take a Budget so that it could directly consume the signature check budget at the place signature verification is done.

While generally a good idea, in practice it means we can't expose signature validation operations on CRLs to crate external consumers that might want to use this functionality (e.g. to verify a CRL on disk has a valid issuer). The budget type is crate-private, and while we could open that up it would be awkward to require providing a budget in the context of validating a single item's signature. The budget concept is really only meaningful in the context of path building.

This commit removes the Budget arg from the signature validation function, and the CRL trait's signature validation function, preferring to consume budget at the call-sites in verify_cert where we're doing path building.

Related #185

[codecov]

Codecov Report

Merging #186 (2d563a3) into main (12fe2bb) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #186      +/-   ##
==========================================
+ Coverage   96.52%   96.53%   +0.01%     
==========================================
  Files          19       19              
  Lines        4492     4507      +15     
==========================================
+ Hits         4336     4351      +15     
  Misses        156      156              

Files Changed	Coverage Δ
src/crl/types.rs	99.53% <ø> (-0.01%)	⬇️
src/signed_data.rs	100.00% <ø> (ø)
src/alg_tests.rs	100.00% <100.00%> (ø)
src/crl/mod.rs	97.50% <100.00%> (+0.01%)	⬆️
src/verify_cert.rs	97.35% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[djc]

I agree that it doesn't make much sense to expose the budget argument to consumers of CertRevocationList::verify_signature(). Can we provide a more specific interface in signed_data, parallel to verify_signed_data() which input arguments that somehow restrict the input to a CertRevocationList implementer?

[cpu]

I'm going to pause this and think about some deeper changes to make this easier to work with. Separately I'll fix the immediate problem in the 0.101.x release.

[cpu]

Closing this to pursue broader changes.

[cpu]

I've put together a wip branch that I want to polish up a bit more + break into smaller commits but the general ideas are:

1. drop the dyn CertRevocationList trait for an enum with an owned and a borrowed variant. I think this is a better fit for what we're trying to accomplish, and removes the need for the private Sealed trait. I'm not sure why I didn't go this way to begin with...
2. express the CRL validation for one-off external sig. validation using a Cert for the issuer, instead of raw SPKI - this should be a lot more ergonomic for users who previously couldn't even get that SPKI data without an external crate. The Budget argument is removed as it should be something crate internal and specific to the validation context.
3. have the CertRevocationList one-off validation function use a new signed_data::verify_crl_signature_unbudgeted fn that can only accept CertRevocationList enums as input, and performs validation without tracking against a budget. It should be pretty hard to use this accidentally from the cert validation context.
4. have the verify_cert.rs CRL signature validation use the existing signed_data::verify_signed_data fn that accepts generic SignedData and tracks against a Budget.
5. move the free-standing authoritative_for fn to be defined on CertRevocationList - this is an unrelated bit of tidying.

@djc I think that accomplishes what you were looking for. No need to look at my messy WIP branch, but if you had feedback on the general approach described above it would be helpful. I'll work on making this reviewable tomorrow.

[djc]

I think this might conflict with the ideas on how to move RevocationOptions into pki-types (because currently that's a webpki type we're leaking into the rustls API)? I guess we could do the verify_crl_signature_unbudgeted() thing even if we keep the CertRevocationList trait instead of enum (if so, no need to conflate the two)?

[cpu]

I guess we could do the verify_crl_signature_unbudgeted() thing even if we keep the CertRevocationList trait instead of enum (if so, no need to conflate the two)?

I think it's more complicated 😓 To make this pattern work we need to be able to accept a CertRevocationList as the argument to the verify function, which means we need to be able to extract SignedData from a CertRevocationList. This means adding a trait fn for that, which also means dragging the SignedData type into the public API. We could do that, and keep the guts pub(crate) to avoid leaking the untrusted dependency into the API, but we're back to having conflict with how to move the trait into pki-types, we'd also need to move SignedData (and would have to rework it to avoid untrusted...).

I'm starting to think the juice isn't worth the squeeze and perhaps we should land 834364c on main. WDYT?

[cpu]

I'm starting to think the juice isn't worth the squeeze and perhaps we should land 834364c on main

#190