Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

cpu
Copy link
Member

@cpu cpu commented Dec 24, 2024

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.

Updates #307

@cpu

This comment was marked as outdated.

Copy link

codecov bot commented Dec 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.23%. Comparing base (dad66f2) to head (b0cfb44).

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@djc djc left a 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?

cpu and others added 3 commits December 28, 2024 10:46
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.
@cpu cpu self-assigned this Dec 28, 2024
@cpu cpu changed the title add fips feature, adjust aws-lc-rs sys dep add aws-lc-rs-fips feature, adjust sys dep Dec 28, 2024
@cpu
Copy link
Member Author

cpu commented Dec 28, 2024

I think these changes are good. We could consider naming the feature aws-lc-rs-fips instead?

SGTM. Updated the name throughout.

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?

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.

Build+test (--no-default-features --features alloc,std,aws_lc_rs, stable, macos-latest) Expected

There will be a few CI jobs left stuck in 'expected' based on the aws_lc_rs -> aws-lc-rs rename in the title (I really hate this about GH branch protection rules....). Will need to admin merge & fix afterwards.

@djc
Copy link
Member

djc commented Dec 28, 2024

(I really hate this about GH branch protection rules....)

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.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants