feat: support "verify" with FIPS crypto backend

[domodwyer]

Hi there! First off, thanks for this library and the careful thought put into the security aspects.

I'm working on a system that is deployed in FedRAMP environments, which requires FIPS certified crypto backends, and fortunately the aws-lc-rs crate used by x509-parser is already supported! However it's configured to use the non-FIPS backend when enabling the verify-aws feature today.

This PR adds a new feature flag verify-aws-fips to go alongside the existing verify-aws feature flag, enabling the FIPS-approved aws-lc-rs backend.

There's no code changes, just feature flag toggling 👍

---

• feat: support "verify" with FIPS crypto backend (8f355a0)

  Allow verifying cryptographic signatures using the AWS-LC's FIPS crypto
  backend.
  
  This change allows this library to be used in FedRAMP / US Gov
  deployments, which have a hard requirement on using FIPS-approved crypto
  modules only.
  
  The "verify-aws-fips" feature flag is functionally identical to using
  "verify-aws", but it selects the FIPS backend in aws-lc-rs.
  

[cpu]

Thanks for the PR (& self-review), I will try to review it soon but I'm supportive of the general direction.

Security audit / security_audit (pull_request) - Failing after 3m

FWIW you can ignore this failing CI job, it's being addressed in #223

[domodwyer]

Awesome! No rush.

Thanks for the link - I see you're keeping support for a MSRV of 1.67.1 in the linked PR, but aws-lc-rs wants at least 1.70:

error: package aws-lc-fips-sys v0.13.11 cannot be built because it requires rustc 1.70.0 or newer, while the currently active rustc version is 1.67.1

Should I gate the CI checks agains the FIPS backend with if: matrix.rust != '1.67.1'?

[cpu]

Thanks for the link - I see you're keeping support for a MSRV of 1.67.1 in the linked PR, but aws-lc-rs wants at least 1.70

I think this will ultimately come down to @chifflier's preference. IMHO I think an MSRV of 1.67 is too stale and many crates in the ecosystem with larger use have more aggressive MSRVs (e.g. Tokio at 1.71, aws-lc-rs at 1.70, rustls at 1.83).

I would personally be in favour of taking an MSRV increase that was more in-line with those projects. 1.71 seems like an OK target and was a rust version released ~2 years ago.

[domodwyer]

It's a shame there's no way of getting hold of rust version metadata for downloads from crates.io in order to make an informed decision - I agree it's quite a low MSRV and I doubt there's a meaningful fraction of users on <1.71 these days.

I will leave it as-is for now, and make any changes after review / when there's a MSRV plan 👍

[cpu]

Thanks! Putting aside the question on how to handle MSRV this looks good to me otherwise.

[cpu]

1.71 seems like an OK target and was a rust version released ~2 years ago.

Another point in favour: in would allow taking a dev dep on wasm-bindgen & associated bits for #231

[DarkmatterVale]

Hey all! Interested in seeing this deployed in the next release. Where does this stand?

[cpu]

Where does this stand?

This branch is targeting main, so unless backported to the x509-parser-0.18 branch it would go out in a 0.19 beta, which as I understand things is blocked on #223

[domodwyer]

I'd be happy to backport to 0.18 if that helps? Would we have the same MSRV problem?

[cpu]

I'd be happy to backport to 0.18 if that helps?

It seems reasonable from my perspective, but I can't publish the crate so I think we're still blocked on chifflier (even setting aside the MSRV issue).

Would we have the same MSRV problem?

Yes, I believe so.

[domodwyer]

It's a shame there's no way of getting hold of rust version metadata for downloads from crates.io in order to make an informed decision - I agree it's quite a low MSRV and I doubt there's a meaningful fraction of users on <1.71 these days.

So fun fact, crates.io has a public Datadog dashboard that tracks requests by cargo version: https://p.datadoghq.com/sb/3a172e20-e9e1-11ed-80e3-da7ad0900002-973f4c1011257befa8598303217bfe3a?fromUser=false&refresh_mode=sliding&from_ts=1771511854139&to_ts=1772116654139&live=true

A quick check of this week of data shows that the highest ranking <1.71 version is 1.70 at 0.36%, and then after that, 1.65 at 0.07% of requests.

[chifflier]

The MSRV has been upgraded to 1.85, so this part is now solved.
I'll see for merging the PR.
Thanks!

[chifflier]

Hi,
I checkout out the PR and fixed conflicts introduced when merging other PRs. I am trying to build (cargo build --all-features) but get an error with aws-lc-fips-sys:

warning: aws-lc-fips-sys@0.13.16: Emitting configuration: cargo:rustc-cfg=x86_64_unknown_linux_gnu
warning: aws-lc-fips-sys@0.13.16: Only dynamic crypto library found in system install; using dynamic linking.
warning: aws-lc-fips-sys@0.13.16: Only dynamic crypto library found in system install; using dynamic linking.
warning: aws-lc-fips-sys@0.13.16: Building with: CMake
warning: aws-lc-fips-sys@0.13.16: Symbol Prefix: Some("aws_lc_fips_0_13_16")
warning: aws-lc-fips-sys@0.13.16: Target platform: 'x86_64-unknown-linux-gnu'
warning: aws-lc-fips-sys@0.13.16: Compilation of 'c11.c' succeeded - Ok(["/home/user/CODE/RUST/ASN1/x509-parser/target/debug/build/aws-lc-fips-sys-a6bbfe71212b012c/out/out-c11/7dfda64fdf5a526c-c11.o"]).
error: failed to run custom build command for `aws-lc-fips-sys v0.13.16`

... lots of lines ...

  In file included from /home/user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/aws-lc-fips-sys-0.13.16/aws-lc/crypto/fipsmodule/bcm.c:41:
  /home/user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/aws-lc-fips-sys-0.13.16/aws-lc/crypto/fipsmodule/../internal.h: In function 'OPENSSL_memchr':
  /home/user/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/aws-lc-fips-sys-0.13.16/aws-lc/crypto/fipsmodule/../internal.h:921:10: error: return discards 'const' qualifier from pointer target type [-Werror=discarded-qualifiers]
    921 |   return memchr(s, c, n);
        |          ^~~~~~
  At top level:
  cc1: note: unrecognized command-line option '-Wno-c11-extensions' may have been intended to silence earlier diagnostics
  cc1: all warnings being treated as errors

Any idea on how to fix this? Note: the C compiler used seems to be gcc 16, according to the logs

This also brings more problems:

• the build requires an executable TMPDIR (I could live with that, but only if this is required for this feature, not in --all-features build (see next point)
• the --all-features becomes easily broken, and will add all dependencies in the result. Do someone have an idea on how to either override what is done in Cargo.toml for the --all-features? Otherwise, we may have to assume this and make all these features mutually exclusive. @cpu any thoughts?

Thanks

[cpu]

Hi, I pushed some revisions to this branch. It seemed easier than blocking on more back/forth given it has sat for a while. Sorry about that. Let me know if you'd prefer I don't push to your branch in the future.

Any idea on how to fix this? Note: the C compiler used seems to be gcc 16

That failure is upstream, see aws/aws-lc-rs#569

The FIPS module source is frozen for certification, so new-compiler fixes lag behind the non-FIPS aws-lc-sys. Until it's fixed upstream you can either build with clang (set CC=clang CXX=clang++) or use an older gcc (<= 13). GitHub's ubuntu-latest runners build it fine today using that latter strategy. FIPS is pain and nobody should subject themselves to it unless they have no choice :P

the build requires an executable TMPDIR

This is also inherent to the aws-lc-fips-sys build, so on the upside only users that opt-in have to deal with it (and the heavy extra build deps, e.g. Go). Might be worth an upstream issue if it's something you want to work around.

we may have to assume this and make all these features mutually exclusive

I'd recommend against. For rustls, rcgen, hyper-rustls, tokio-rustls, instant-acme, and rustls-ffi we model fips as an additive feature, and none use true mutually exclusive features / or a compile_error!.

Enabling verify-aws + verify-aws-fips together is how we manage rustls' fips configuration: both sys crates are compiled but aws-lc-rs's own cfg selects the FIPS one, so --all-features stays correct, just heavyweight. Mutual exclusion would instead make --all-features fail to compile for every consumer. Ultimately feature flags kind of suck for this use-case but it's what we have.

the --all-features becomes easily broken, and will add all dependencies in the result

I tried to address this a bit on the branch based on other projects:

• docs.rs metadata now lists explicit features instead of all-features = true, excluding verify-aws-fips (docs.rs can't run the Go/CMake build; rustls and instant-acme do the same, with the same comment). The feature still shows up in the doc-cfg annotations. It's a pain to keep this in sync, but I don't think there's a better way.
• New CI job: cargo hack --feature-powerset --mutually-exclusive-features verify-aws,verify-aws-fips --all-targets, with -D warnings. This is a "spiritual --all-features", it checks every feature combination except pairing the two aws-lc-rs backends. It already caught two real gate bugs (P-521 OID import and test fixture not gated to the aws features, also fixed on-branch).
• Redundant --all-features runs consolidated (dropped the duplicate test job and the stable row of check-all-features), cutting several aws-lc-fips-sys builds per CI run.
• verify-aws now uses aws-lc-rs's blessed non-fips alias (this is the same dependency graph, just more explicit). Oops: undid this, it actually enables the mutual-exclusivity I wanted to avoid.

I also rebased on master and adjusted for the P-521 support that landed in the meantime (and fixed a typo in the CI config).

PTAL!

[domodwyer]

Looks great to me.

Hi, I pushed some revisions to this branch. It seemed easier than blocking on more back/forth given it has sat for a while. Sorry about that. Let me know if you'd prefer I don't push to your branch in the future.

Absolutely fine by me - thank you for taking the time to get this into shape.

[chifflier]

Hi, I pushed some revisions to this branch. It seemed easier than blocking on more back/forth given it has sat for a while. Sorry about that. Let me know if you'd prefer I don't push to your branch in the future.

Hi @cpu,
Thanks a lot for your work! I tested locally, and everything looks good to me.

The FIPS module source is frozen for certification, so new-compiler fixes lag behind the non-FIPS aws-lc-sys. Until it's fixed upstream you can either build with clang (set CC=clang CXX=clang++) or use an older gcc (<= 13). GitHub's ubuntu-latest runners build it fine today using that latter strategy. FIPS is pain and nobody should subject themselves to it unless they have no choice :P

Indeed, this is quite annoying. Let's just hope this does not crash the CI too often in the future

we may have to assume this and make all these features mutually exclusive

I'd recommend against. For rustls, rcgen, hyper-rustls, tokio-rustls, instant-acme, and rustls-ffi we model fips as an additive feature, and none use true mutually exclusive features / or a compile_error!.

Yes, I'd like to avoid making things exclusive. I think we'll keep the existing for now.
If this becomes too complex in the feature, we may switch to a different approach, like having a common Trait for cryptographic operations required to verify signatures and have multiple implementers depending on the features.

the --all-features becomes easily broken, and will add all dependencies in the result

I tried to address this a bit on the branch based on other projects:

* `docs.rs` metadata now lists explicit features instead of `all-features = true`, excluding `verify-aws-fips` (`docs.rs` can't run the Go/CMake build; rustls and instant-acme do the same, with the same comment). The feature still shows up in the doc-cfg annotations. It's a pain to keep this in sync, but I don't think there's a better way.

I have no better idea, so it's fine

* New CI job: `cargo hack --feature-powerset --mutually-exclusive-features verify-aws,verify-aws-fips --all-targets`, with `-D warnings`. This is a "spiritual --all-features", it checks every feature combination except pairing the two `aws-lc-rs` backends. It already caught two real gate bugs (P-521 OID import and test fixture not gated to the aws features, also fixed on-branch).

Thanks

* Redundant `--all-features` runs consolidated (dropped the duplicate test job and the stable row of check-all-features), cutting several `aws-lc-fips-sys` builds per CI run.

Fine

* ~`verify-aws` now uses `aws-lc-rs`'s blessed `non-fips` alias (this is the same dependency graph, just more explicit).~ _Oops: undid this, it actually enables the mutual-exclusivity I wanted to avoid_.

Indeed, if we want to keep features additive, I think the best is to keep aws-lc-rs non-FIPS by default, and FIPS is the relevant feature is enabled.

I also rebased on master and adjusted for the P-521 support that landed in the meantime (and fixed a typo in the CI config).

Thanks a lot!

[chifflier]

Applied
Thank you @domodwyer and @cpu

[cpu]

If this becomes too complex in the feature, we may switch to a different approach, like having a common Trait for cryptographic operations required to verify signatures and have multiple implementers depending on the features.

FWIW that's closer to what I was recommending in this other thread about RustCrypto. I personally wouldn't be keen to add support for another cryptography library above/beyond ring and aws-lc-rs without doing that first.