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

Merged
merged 3 commits into from
Dec 31, 2024
Merged

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 (5fd8350).
Report is 4 commits behind head on main.

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

Copy link
Member

@ctz ctz left a comment

Choose a reason for hiding this comment

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

A couple of remaining aws_lc_rs feature mentions:

  • the features table in the top-level crate comment (might be a good venue to mention aws-lc-rs was formerly aws_lc_rs?)
  • the comment against ALL_VERIFICATION_ALGS

cpu and others added 3 commits December 31, 2024 10:04
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
Copy link
Member Author

cpu commented Dec 31, 2024

A couple of remaining aws_lc_rs feature mentions

Good catch, fixed up.

@cpu cpu enabled auto-merge December 31, 2024 15:05
@cpu cpu disabled auto-merge December 31, 2024 15:05
@cpu cpu merged commit 5dc0926 into rustls:main Dec 31, 2024
30 checks passed
@cpu
Copy link
Member Author

cpu commented Dec 31, 2024

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.

Fixed up.

@cpu cpu deleted the cpu-fips-sys branch December 31, 2024 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants