Closed 1Dragoon closed 5 months ago
Hi there,
Thanks for the PR! I have some thoughts on this after seeing a couple other repos implement similar. Sorry for the volume of feedback. @chifflier may have his own thoughts and you should prefer his opinions over mine in the event they conflict.
Easy stuff up-front:
You need to do a cargo fmt
pass on your diff to fix one of the CI failures. It looks like there's some other clippy stuff that's unrelated, I will try to fix that on main
shortly (Edit: https://github.com/rusticata/x509-parser/pull/155)
I think the README.md coverage of the verify
and validate
features needs a touch-up to mention the aws-lc-rs option
A unit test wouldn't hurt :-)
More involved feedback on the overall approach:
verify
feature, and separate ring
and aws-lc-rs
features. This is closer to how Rustls and Rcgen handle activating those crypto provider dependencies. It would be unfortunate if down the road we wanted to add other features that require a backing cryptography library and need to bifurcate each of those features similar to verify
and verify-aws-lc-rs
.I think the best end state would be that neither the verify
or ring
features are enabled by default to match the status quo, activating verify
with no default features should be a compile error about a required source of cryptography, activating verify
with no default features and ring
should use ring, activating verify
with no default features and aws-lc-rs
should use aws-lc-rs, and activating verify
with both ring
and aws-lc-rs
should use ring
. I believe this would be a breaking change since previously activating verify
would implicitly activate ring
and I'm suggesting that should be made explicit.
cargo hack
to check the powerset works correctly. In Rcgen we have a bunch of extra CI tasks. It looks like right now in this repo CI tests --all-features
but not --no-default-features
or a build that activates the aws-lc-rs feature but not the ring feature. I think landing this work would require figuring out a strategy to exercise more feature combinations.In order to build, you have to have cmake and nasm installed
Quick note here: the nasm
requirement is only needed on Windows. Linux and MacOS shouldn't require it.
Unfortunately some stuff has come up and I'm super busy and haven't had time to pick this up again, probably won't have time for some months to come. Somebody else can take it over or simply close it meanwhile.
Totally understand. Thanks for leaving an update.
I'm supportive of the goal of adding aws-lc-rs support but I also don't have time to adopt the branch at the moment. I'm going to close this, but if you or someone else finds the time to pick it up again I'd be happy to help review.
Thanks again,
Main motivation for this is I need ECDSA with SHA512 for some certs I need to verify. Problem with this is ring doesn't support it. However, aws-lc-rs, which is designed to be a drop-in replacement for ring, does support it, but it comes with a few stipulations:
Let me know what you think.