FIPS support

[BiagioFesta]

Following the same direction of rustls, from the latest version (0.23.0):

Default cryptography provider changed to aws-lc-rs.
[...]
Support for FIPS validated mode with aws-lc-rs

This PR is composed by 2 commits:

• Adding fips feature to enable aws_lc_rs/fips
• Changing default crypto backend from ring to aws_lc_rs

[cpu]

Generally looks good, thank you for picking this up.

I think there are some CI tweaks that should accompany this change.

1. it looks like we have an explicit CI task targetting the aws_lc_rs feature. I think that should probably be flipped to use ring now that the default features builds are using aws_lc_rs
2. I think we don't have any coverage for the fips feature option. I think it makes sense to add that, and I suspect it will break the Windows build until we also add Ninja alongside nasm for the Windows/FIPS combo.

[est31]

There is multiple reasons why I think this is a bad idea.

First, the title of the PR is wrong: we already optionally support FIPS by letting people opt-in to aws_lc_rs. There is nothing inherent to FIPS certification that is worthwhile, people who need it to check some box on a form can use it, but everyone else is well off without it. The advantage here isn't as big as the title makes it out to be. I think only a fraction is interested in FIPS certification.

The PR rustls/rustls#1780 has actually given a worthwhile advantage (for rustls), better performance. When one compares different TLS implementations I think performance is way more important than when one compares different certificate generator implementations. rcgen has the luxury of being able to not be as performant, because unlike a TLS library, it's not processing all of that program's communications, so it should be less in the hot path.

Second, the aws_lc_rs library relies heavily on a large C codebase , which is yet another fork of OpenSSL. Is that really what we should strive towards? It's too challenging to implement SHA-2 in Rust so we have to use C for it?(edit: my point comes across better with this wording removed) There have been CVEs in openssl's FIPS module, e.g. this one.

My goal for rcgen is not to become a C bindings crate, the goal is more ambitious, to have a working stack as fully in Rust as possible.

Beyond the philosophical impacts there is also practical concerns like requiring cmake and nasm. It should be quite easy to list all the units required for compilation and then compile them using the cc crate. This is what Ring is doing as well.

It also hurts the image of Rust in the outside, when the first thing people read who are trying out Rust programs is instructions on how to install C build systems. Should rustup be shipping cmake now? This hurts especially when it comes to defaults of fundamental crates (like rustls) because most people are not turning off default features.

So I think we should just close this and revisit it only once aws_lc_rs has divested from C and C build systems. At the very least they should drop the cmake requirement (but making nasm optional wouldn't hurt). But even then it's a stretch to switch the default.

[est31]

Hmm seems there is a PR to remove the cmake requirement: aws/aws-lc-rs#317

And there's an issue for nasm (already been linked to in the support PR): aws/aws-lc#1477

[djc]

@BiagioFesta do you have a concrete use case for this? I'm sympathetic to @est31's concerns and think it might make sense to with ring as the default for this crate.

[BiagioFesta]

I appreciate the feedback.

I share the same concern about incorporating heavy C build systems as hard dependencies in a Rust project, especially since this can be particularly challenging on Windows.
I encountered similar difficulties with a personal project of mine, where I opted to reimplement a compression algorithm in pure Rust to avoid relying on C-bindings. Many users struggled to compile on Windows, even for a single C-header library.

That's why I split up this PR into two distinct commits, as mentioned in the description itself.

---

First, the title of the PR is incorrect: we already optionally support FIPS by allowing people to opt-in to aws_lc_rs.

However, I'm not entirely clear on this point. With this PR, we added support for aws_lc_rs, but not FIPS.

From the description:

[...] The main reason behind this is to provide FIPS compliance in the future via aws-lc [...]

aws_lc_rs itself does not imply FIPS compliance. FIPS certification involves a frozen code base that has been validated and approved by NIST.

aws_lc_rs?/fips utilizes that particular commit, enabling the module.

---

In conclusion, I don't have a concrete use case for having aws_lc_rs as the default crypto backend.
I've added that commit on top of this PR, following the same approach as with rustls (as it's often the case that those using rcgen also depend on rustls, but that's a probably wrong though of mine).

If you agree, we can remove e3529f5 from this PR.

What about FIPS, tho? I still think that's something useful to have as an opt-in feature. And concrete examples here trivial to explain.

[est31]

Oh yeah the fips changes are fine.

I was more concerned about the second commit.

Also, could you remove the lockfile changes, they look like a cargo update to me? To get Cargo.toml changes into Cargo.lock, one can do cargo check instead for example. To the extent that MSRV is respected, I'm fine with a general cargo update (there hasn't been one for some time) but I'd like it to be in a separate PR.

[djc]

Also, could you remove the lockfile changes, they look like a cargo update to me? To get Cargo.toml changes into Cargo.lock, one can do cargo check instead for example. To the extent that MSRV is respected, I'm fine with a general cargo update (there hasn't been one for some time) but I'd like it to be in a separate PR.

Did this in aws/aws-lc-rs#235.

[BiagioFesta]

Is there something else we need to add to the CI?

[est31]

Thanks for the PR @BiagioFesta !