-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add aws-lc-rs-fips feature, adjust sys dep #308
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #308 +/- ##
=======================================
Coverage 97.23% 97.23%
=======================================
Files 20 20
Lines 4225 4225
=======================================
Hits 4108 4108
Misses 117 117 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these changes are good. We could consider naming the feature aws-lc-rs-fips
instead?
And, given we currently have a semver-incompatible version bump, switching from underscore to hyphen spelling which IIRC we decided ought to be the way forward?
We made a mistake using underscores in the original Rustls and Webpki features. We patched over this in Rustls with an alias. Since we're making semver incompat changes, let's fix it here properly.
Previously we unconditionally used the `aws-lc-sys` and `prebuilt-nasm` features of the `aws-lc-rs` dep, meaning we always brought along `aws-lc-sys` (note the `prebuilt-nasm` feature customizes that dep). However, when a user is looking for a FIPS crypto provider we want to avoid bringing in `aws-lc-sys` and instead use `aws-lc-rs/fips` to get `aws-lc-fips-sys`. This commit makes the `aws-lc-rs` feature of `webpki` activate the "usual" config: `aws-lc-rs/aws-lc-sys` w/ `aws-lc-rs/prebuilt-nasm` to have `aws-lc-sys` with prebuilt assmebly to avoid the nasm dep. A new `aws-lc-rs-fips` feature is added for `webpki` that activates the FIPS specific config: `aws-lc-rs/fips`. The `aws-lc-sys` and `prebuilt-nasm` features are **not** activated.
SGTM. Updated the name throughout.
Make sense. I broke that out as a separate commit up-front since it's a much more invasive diff. I also cherry-picked Ctz's version bump from #302 to make the semver check in CI happy.
There will be a few CI jobs left stuck in 'expected' based on the |
It appears that GitHub has a newer way to set these up called rule sets, maybe that has gotten better at these kinds of things? (Agreed that the disjointness of workflow job names and their protection status is pretty annoying.) |
Previously we unconditionally used the
aws-lc-sys
andprebuilt-nasm
features of theaws-lc-rs
dep, meaning we always brought alongaws-lc-sys
(note theprebuilt-nasm
feature customizes that dep).However, when a user is looking for a FIPS crypto provider we want to avoid bringing in
aws-lc-sys
and instead useaws-lc-rs/fips
to getaws-lc-fips-sys
.This commit makes the
aws-lc-rs
feature ofwebpki
activate the "usual" config:aws-lc-rs/aws-lc-sys
w/aws-lc-rs/prebuilt-nasm
to haveaws-lc-sys
with prebuilt assmebly to avoid the nasm dep.A new
aws-lc-rs-fips
feature is added forwebpki
that activates the FIPS specific config:aws-lc-rs/fips
. Theaws-lc-sys
andprebuilt-nasm
features are not activated.Updates #307