Optionally support aws-lc-rs by ctz · Pull Request #158 · rustls/webpki

ctz · 2023-08-30T14:38:42Z

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, because aws-lc-rs does not gate RSA on the alloc feature. That was a detail I think worth getting right, but means quite a bit of similarity between aws_ls_rs_algs.rs and ring_algs.rs. But I am using a similar trick to reuse the test code.)

codecov · 2023-08-30T14:43:07Z

Codecov Report

Merging #158 (45b74d0) into main (16a9127) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #158   +/-   ##
=======================================
  Coverage   96.33%   96.34%           
=======================================
  Files          17       19    +2     
  Lines        4524     4536   +12     
=======================================
+ Hits         4358     4370   +12     
  Misses        166      166

Files Changed	Coverage Δ
src/ring_algs.rs	`100.00% <ø> (ø)`
src/test_utils.rs	`100.00% <ø> (ø)`
src/alg_tests.rs	`100.00% <100.00%> (ø)`
src/aws_lc_rs_algs.rs	`100.00% <100.00%> (ø)`
src/verify_cert.rs	`97.35% <100.00%> (-0.01%)`	⬇️

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

cpu

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?

cpu · 2023-08-30T15:17:52Z

src/lib.rs

+/// 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] = &[


Thank you! It's been gross seeing variations of this replicating across various tests 😅

src/ring_algs.rs

djc · 2023-08-31T07:50:35Z

tests/signatures.rs

-        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,


Could/should these (and the other long lists in tests) use ALL_?

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

tests/signatures.rs

Cargo.toml

cpu · 2023-09-14T14:31:08Z

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 · 2023-09-15T13:28:10Z

nb. coverage job is broken due to nightly breaking time; see time-rs/time#618

ctz · 2023-09-15T14:36:45Z

Apologies, CI is telling me this is not completely baked!

cpu · 2023-09-18T18:28:43Z

src/verify_cert.rs

 }

-#[cfg(test)]
+#[cfg(all(test, feature = "alloc", feature = "ring"))]


Thanks 👍 I also moved to a module-wide requirement for alloc in #179

cpu · 2023-09-18T18:34:08Z

src/test_utils.rs

    rcgen::Certificate::from_params(ca_params).unwrap()
 }

+#[cfg_attr(not(feature = "ring"), allow(dead_code))]


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 🤞

src/verify_cert.rs

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

cpu reviewed Aug 30, 2023

View reviewed changes

djc reviewed Aug 31, 2023

View reviewed changes

ctz mentioned this pull request Sep 13, 2023

tests: local test speed optimizations, add cargo hack feature powerset #176

Merged

cpu mentioned this pull request Sep 14, 2023

0.102 release planning #159

Closed

13 tasks

ctz force-pushed the jbp-aws-lc-rs-support branch 4 times, most recently from 9759e1a to 84436a0 Compare September 15, 2023 13:22

ctz marked this pull request as ready for review September 15, 2023 13:26

ctz marked this pull request as draft September 15, 2023 14:36

ctz force-pushed the jbp-aws-lc-rs-support branch 3 times, most recently from 5d69116 to de84af0 Compare September 15, 2023 15:46

ctz marked this pull request as ready for review September 15, 2023 15:50

cpu approved these changes Sep 18, 2023

View reviewed changes

ctz added 11 commits September 20, 2023 10:32

Defend cargo test without ring

6ff143a

verify_cert.rs: don't try tests without ring

68680e1

client_auth.rs: don't try without ring

6a3baae

test_utils.rs: make_end_entity is not referenced without ring

8313d38

Extract algorithm tests from ring_algs.rs

3f1c857

Extract feature-gated configuration from alg_tests.rs

437519b

Export array of all predefined SignatureVerificationAlgorithms

d139225

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.

tests/signatures.rs: import algorithms at top

bf2d36a

Export *ring*-backed signature algorithms in crate::ring.

eaed18c

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.

Add aws-lc-rs as optional dependency

8890ddc

Export SignatureVerificationAlgorithms backed by it in webpki::aws_lc_rs

Enable test coverage with aws-lc-rs

3e672ae

When run with --feature aws_lc_rs but not --feature ring, use aws-lc-rs to run the same set of tests.

Exercise aws-lc-rs in CI

45b74d0

- 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 September 20, 2023 09:55

ctz added this pull request to the merge queue Sep 20, 2023

Merged via the queue into main with commit 4902b1d Sep 20, 2023

ctz deleted the jbp-aws-lc-rs-support branch September 20, 2023 10:37

Conversation

ctz commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cpu left a comment

Choose a reason for hiding this comment

Uh oh!

cpu Aug 30, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

djc Aug 31, 2023

Choose a reason for hiding this comment

Uh oh!

ctz Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cpu commented Sep 14, 2023

Uh oh!

ctz commented Sep 15, 2023

Uh oh!

ctz commented Sep 15, 2023

Uh oh!

cpu Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

cpu Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ctz commented Aug 30, 2023 •

edited

Loading

codecov bot commented Aug 30, 2023 •

edited

Loading