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

feat(s2n-quic): provider-crypto-fips feature flag #2194

Merged
merged 4 commits into from
May 9, 2024
Merged

Conversation

toidiu
Copy link
Contributor

@toidiu toidiu commented Apr 30, 2024

Description of changes:

This PR exposed a new provider-fips-crypto feature flag and makes the necessary changes for s2n-quic to use FIPS-approved cryptography for encryption.

We want to support fips mode for:

  • if s2n-quic-crypto fips flag is set (compile time check)
  • target_os != windows (compile time check)
  • negotiated cipher is not CHACHA20_POLY1305 (runtime check)

To avoid branching on the fips and windows, I throw a compiler error if the fips flag is set on windows:

#[cfg(all(feature = "fips", target_os = "windows"))]
std::compile_error!("feature `fips` is not supported on windows");

To avoid branching on the CHACHA20_POLY1305 I don't derive FipsKeys when using this cipher. This was a macro level change (key!) rather than code branching, which helps with readability imo.

Call-outs:

A lot of the changes are simply converting the encrypt function to take a &mut self, which I captured in the first commit. The interesting changes are in the second commit FipsKey.

Testing:

New CI task to test with provider-fips-crypto feature enabled

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@toidiu toidiu force-pushed the ak-fipsKeyUsage branch 3 times, most recently from 443a42d to 847fef8 Compare May 1, 2024 21:54
@toidiu toidiu changed the title Ak fips key usage feat[s2n-quic]: provider-fips-crypto feature flag May 1, 2024
quic/s2n-quic/src/lib.rs Outdated Show resolved Hide resolved
@toidiu toidiu marked this pull request as ready for review May 1, 2024 23:04
@@ -15,7 +15,7 @@ default = [
"provider-address-token-default",
"provider-tls-default",
]

provider-fips-crypto = ["s2n-quic-crypto/fips"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why provider? This feature seems different than the others in that its not changing out an actual provider

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to "extend" the s2n provider to enable fips? Something like provider-tls-s2n-fips?

Copy link
Contributor

Choose a reason for hiding this comment

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

its not just TLS that changes with this flag though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are also going to end up supporting rustls so should avoid mentioning s2n-tls.

At the moment the changes include

  • indirectly passing fips to aws-lc-rs, which runs TLS in fips mode
  • making the AEAD implementation fips compliant, which is used directly by QUIC for encryption (not TLS)

Therefore provider-fips-crypto seemed like a good name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at the other features in s2n-quic:

  • provider-address-token-default: brings in dependencies needed for the default address token provider
  • provider-tls-default: enables the default TLS provider based on platform detection
  • provider-event-tracing: enabled tracing provider by default

They all have to do with an existing provider, but for fips, its not actually changing anything about any of the providers, thats why I wasn't sure provider makes sense in the name here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On a related note: running into some usage issues with provider-tls-default and fips.

I listed 2 options, neither of which seem amazing.


I have a slight preference for "provider" since fips affects the behavior of the tls provider. All our current features are also prefixed with "provider".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline and "provider-crypto-fips" seems like a good fit. 'fips' touches more than just tls so that doesnt feel right. We dont have a crypto provider today but its possible to image that we could have one in the future so this naming would fit with configuring cypto functionality.

Thoughts @camshaft

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to work and allows us to keep target based dependency selection: 5baaa57

@toidiu toidiu changed the title feat[s2n-quic]: provider-fips-crypto feature flag feat[s2n-quic]: provider-crypto-fips feature flag May 3, 2024
@toidiu toidiu force-pushed the ak-fipsKeyUsage branch 4 times, most recently from 2a7c73b to 796b96b Compare May 6, 2024 03:16
quic/s2n-quic-crypto/src/cipher_suite/ring.rs Outdated Show resolved Hide resolved
quic/s2n-quic-crypto/src/cipher_suite/ring.rs Outdated Show resolved Hide resolved
Comment on lines +84 to +88
#[inline]
#[allow(dead_code)] // this is to maintain compatibility between implementations
pub fn update_pmtu(&mut self, _secret: &[u8; KEY_LEN], _mtu: u16) {
unimplemented!();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this probably could be removed now that the custom AES code is gone. Maybe a follow up task though, since I think there are a few places that MTU isn't needed anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, also created an issue so we don't forget: #2204

quic/s2n-quic-tls-default/Cargo.toml Outdated Show resolved Hide resolved
quic/s2n-quic/src/lib.rs Outdated Show resolved Hide resolved
quic/s2n-quic/src/lib.rs Show resolved Hide resolved
Comment on lines 127 to +134
key!(aes128_gcm, aead::AES_128_GCM, 128 / 8, 16);
key!(aes256_gcm, aead::AES_256_GCM, 256 / 8, 16);
key!(chacha20_poly1305, aead::CHACHA20_POLY1305, 256 / 8, 16);
// FipsKey is backed by TlsRecordSealingKey/TlsRecordOpeningKey which doesn't
// support CHACHA20_POLY1305.
//
// https://docs.rs/aws-lc-rs/latest/aws_lc_rs/aead/struct.TlsRecordSealingKey.html
// https://docs.rs/aws-lc-rs/latest/aws_lc_rs/aead/struct.TlsRecordOpeningKey.html
key_no_fips_support!(chacha20_poly1305, aead::CHACHA20_POLY1305, 256 / 8, 16);
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum May 7, 2024

Choose a reason for hiding this comment

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

could also do something like

cfg_if::cfg_if! {
  if #[cfg(feature = "fips")] {
    key_fips!(aes128_gcm, aead::AES_128_GCM, 128 / 8, 16);
    key_fips!(aes256_gcm, aead::AES_256_GCM, 256 / 8, 16);
  } else {
    key_not_fips!(aes128_gcm, aead::AES_128_GCM, 128 / 8, 16);
    key_not_fips!(aes256_gcm, aead::AES_256_GCM, 256 / 8, 16);
  }
}
key_not_fips!(chacha20_poly1305, aead::CHACHA20_POLY1305, 256 / 8, 16);

@toidiu toidiu changed the title feat[s2n-quic]: provider-crypto-fips feature flag feat(s2n-quic): provider-crypto-fips feature flag May 9, 2024
@toidiu toidiu merged commit 286adde into main May 9, 2024
130 checks passed
@toidiu toidiu deleted the ak-fipsKeyUsage branch May 9, 2024 18:34
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