Optionally support aws-lc-rs #158
Conversation
Codecov Report
@@ Coverage Diff @@
## main #158 +/- ##
=======================================
Coverage 96.33% 96.34%
=======================================
Files 17 19 +2
Lines 4524 4536 +12
=======================================
+ Hits 4358 4370 +12
Misses 166 166
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
This looks great 🚀
Build currently known-broken on windows. This will be stuck as a draft until then.
Is there a path forward in the event Windows support in the upstream crate isn't prioritized?
The first commit is somewhat odd/novel, to allow a single file to be used twice with different dependencies, resolved via super::lib. Interested in any alternatives to that!
It seems like a reasonable approach to me but I'm not sure I have the experience to see any lurking danger from the novelty. Was there a particular reason you were interested in alternatives that would be good to know about?
src/lib.rs
Outdated
| /// An array of all the verification algorithms exported by this crate. | ||
| /// | ||
| /// This will be empty if the crate is built without the `ring` feature. | ||
| pub static ALL_VERIFICATION_ALGS: &[&dyn SignatureVerificationAlgorithm] = &[ |
There was a problem hiding this comment.
Thank you! It's been gross seeing variations of this replicating across various tests 😅
tests/signatures.rs
Outdated
| webpki::RSA_PSS_2048_8192_SHA256_LEGACY_KEY, | ||
| webpki::RSA_PSS_2048_8192_SHA384_LEGACY_KEY, | ||
| webpki::RSA_PSS_2048_8192_SHA512_LEGACY_KEY, | ||
| webpki::ring::ECDSA_P256_SHA256, |
There was a problem hiding this comment.
Could/should these (and the other long lists in tests) use ALL_?
There was a problem hiding this comment.
some of these lists are things like "all the algorithms except RSA_PSS_2048_8192_SHA256_LEGACY_KEY" which could be addressed by filtering the ALL_ list and removing the desired one? probably not going to do that in this PR, but will keep it in mind for a later refactor
|
Could this be revived in a non-draft status now that the upstream fixed Windows support or are we waiting for a release tag there? 🤔 I think it's important to get this into the blocking list for #159 |
ctz
force-pushed
the
jbp-aws-lc-rs-support
branch
4 times, most recently
from
9759e1a
to
84436a0
Compare
|
nb. coverage job is broken due to nightly breaking |
|
Apologies, CI is telling me this is not completely baked! |
ctz
force-pushed
the
jbp-aws-lc-rs-support
branch
3 times, most recently
from
5d69116
to
de84af0
Compare
src/verify_cert.rs
Outdated
| @@ -495,10 +495,9 @@ enum Role { | |||
| EndEntity, | |||
| } | |||
|
|
|||
| #[cfg(test)] | |||
| #[cfg(all(test, feature = "alloc", feature = "ring"))] | |||
There was a problem hiding this comment.
Thanks 👍 I also moved to a module-wide requirement for alloc in #179
| @@ -27,6 +27,7 @@ pub(crate) fn make_issuer( | |||
| rcgen::Certificate::from_params(ca_params).unwrap() | |||
| } | |||
|
|
|||
| #[cfg_attr(not(feature = "ring"), allow(dead_code))] | |||
There was a problem hiding this comment.
I think with the refactoring I did in #178 this will fall away because end_entity.rs's printable_string_common_name test will use the make_end_entity helper from a test that doesn't require ring 🤞
This avoids quite a bit of repetition in tests, and addresses the case that a consuming crate wants to use the full set of predefined algorithms but doesn't much care what they are.
This is a breaking change -- previously these were in the crate root. This is necessary to keep crate features additive: the `ring` crate feature provides this set of items.
Export SignatureVerificationAlgorithms backed by it in webpki::aws_lc_rs
When run with --feature aws_lc_rs but not --feature ring, use aws-lc-rs to run the same set of tests.
- ensure `cargo package` works with --all-features, otherwise optional modules could be missing from the list in Cargo.toml. - install nasm on windows for aws-lc-rs - run tests against aws-lc-rs on all platforms
ctz
force-pushed
the
jbp-aws-lc-rs-support
branch
from
de84af0
to
45b74d0
Compare
This passes the same set of testing as ring. I haven't tested performance.
(I've removed some of the novel code-reuse via
super::present in prior versions of this PR, becauseaws-lc-rsdoes not gate RSA on theallocfeature. That was a detail I think worth getting right, but means quite a bit of similarity betweenaws_ls_rs_algs.rsandring_algs.rs. But I am using a similar trick to reuse the test code.)