feat[bindings]: fips feature flag #4527
Conversation
toidiu
changed the title
| if #[cfg(feature = "fips")] { | ||
| assert_eq!(init::fips_mode().unwrap(), FipsMode::Enabled); | ||
| } else { | ||
| assert_eq!(init::fips_mode().unwrap(), FipsMode::Disabled); |
There was a problem hiding this comment.
Is it safe to make this disabled assertion? Couldn't someone use the aws-lc-rs FIPS feature flag to enable FIPS mode, rather than the s2n-tls feature flag? In this case, we wouldn't expect the FIPS feature, but we would still be in FIPS mode.
There was a problem hiding this comment.
If someone enables the fips feature flag in aws-lc-rs, but not in s2n-tls then I would consider that odd behavior. At that point this test might actually help them catch the discrepancy and either enable/disable fips for their application.
There was a problem hiding this comment.
Ah got it, okay. I guess I was thinking that someone might have aws-lc-rs as a direct dependency in addition to s2n-tls, or have multiple dependencies with an aws-lc-rs dependency, in which case just setting the fips flag for aws-lc-rs might make sense. But if this would be a strange thing to do, and they definitely should instead be setting the s2n-tls feature flag, then the test makes sense.
| if #[cfg(feature = "fips")] { | ||
| assert_eq!(init::fips_mode().unwrap(), FipsMode::Enabled); | ||
| } else { | ||
| assert_eq!(init::fips_mode().unwrap(), FipsMode::Disabled); |
There was a problem hiding this comment.
Ah got it, okay. I guess I was thinking that someone might have aws-lc-rs as a direct dependency in addition to s2n-tls, or have multiple dependencies with an aws-lc-rs dependency, in which case just setting the fips flag for aws-lc-rs might make sense. But if this would be a strange thing to do, and they definitely should instead be setting the s2n-tls feature flag, then the test makes sense.
| #[derive(Debug, PartialEq, Copy, Clone)] | ||
| pub enum FipsMode { | ||
| Disabled, | ||
| Enabled, | ||
| } |
There was a problem hiding this comment.
Can someone confirm that this should be non-exhaustive.
There was a problem hiding this comment.
@goatgoose confirmed that we might add some other states to this.
There was a problem hiding this comment.
Yeah - this was discussed when adding the FIPS getter. We decided to use an enum in case we want to add a new value in the future, rather than returning only true/false. So making this non-exhaustive makes sense.
bindings/rust/s2n-tls/src/enums.rs
Outdated
| match input { | ||
| s2n_fips_mode::FIPS_MODE_DISABLED => FipsMode::Disabled, | ||
| s2n_fips_mode::FIPS_MODE_ENABLED => FipsMode::Enabled, | ||
| _ => unreachable!(), |
There was a problem hiding this comment.
sorry, I was thinking that fn fips_mode() didn't need to return an error, since I wasn't sure why it would fail. But now I see that s2n_get_fips_mode fails if s2n is not initialized, so there is a failure mode. And I guess fn fips_mode() should fail too if s2n_get_fips_mode started returning some new enum variant we didn't account for in the bindings yet. Anyway, I think this should probably go back to what you have before with TryFrom
Description of changes:
This PR adds a new
fipsfeature flag which builds the bindings in FIPS mode. We also exposes the underlying C function to retrieve thefips_mode()so Applications can check their FIPS status at runtime.aws-lc-rs exposes a
fipsfeature flag, which can be set to use aws-lc in FIPS mode. The changes in this PR simply propagate the fips flag down to aws-lc-rs so that s2n-tls is built with a FIPS compliant libcrypto.Testing:
Added testing to ensure s2n-tls correctly enters FIPS mode when built with aws-lc-rs with fips enabled
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.