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

Default to require_ems in FIPS mode #1772

Merged
merged 1 commit into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion rustls/src/client/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ impl ConfigBuilder<ClientConfig, WantsClientCert> {
enable_secret_extraction: false,
enable_early_data: false,
#[cfg(feature = "tls12")]
require_ems: false,
require_ems: cfg!(feature = "fips"),
}
}
}
17 changes: 15 additions & 2 deletions rustls/src/client/client_conn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,8 @@ pub struct ClientConfig {
/// If set to `true`, requires the server to support the extended
/// master secret extraction method defined in [RFC 7627].
///
/// The default is `false`.
/// The default is `true` if the `fips` crate feature is enabled,
/// `false` otherwise.
///
/// It must be set to `true` to meet FIPS requirement mentioned in section
/// **D.Q Transition of the TLS 1.2 KDF to Support the Extended Master
Expand Down Expand Up @@ -279,8 +280,20 @@ impl ClientConfig {

/// Return true if connections made with this `ClientConfig` will
/// operate in FIPS mode.
///
/// This is different from [`CryptoProvider::fips()`]: [`CryptoProvider::fips()`]
/// is concerned only with cryptography, whereas this _also_ covers TLS-level
/// configuration that NIST recommends.
pub fn fips(&self) -> bool {
self.provider.fips()
#[cfg(feature = "tls12")]
Copy link
Member

Choose a reason for hiding this comment

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

If we're going to make the config-side fips() depend on the config, should we make the CryptoProvider::fips() pub(crate) to make it harder to accidentally rely on the CryptoProvider::fips() where that is not sufficient?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think -- I hope -- that there is not much confusion in semantics between CryptoProvider::fips() and Client/ServerConfig::fips(), even if they were functionally the same up until this commit. Will extend the docs though.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe the distinction needs to be explained in CryptoProvider::fips()? That's where it's risky, if you use that fips() but fail to check the config.

Copy link
Member

Choose a reason for hiding this comment

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

That seems like a good idea to me

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added some clarifying text to CryptoProvider::fips() -- please check.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM

{
self.provider.fips() && self.require_ems
}

#[cfg(not(feature = "tls12"))]
{
self.provider.fips()
}
}

/// We support a given TLS version if it's quoted in the configured
Expand Down
5 changes: 5 additions & 0 deletions rustls/src/crypto/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,11 @@ pub struct CryptoProvider {

impl CryptoProvider {
/// Returns `true` if this `CryptoProvider` is operating in FIPS mode.
///
/// This covers only the cryptographic parts of FIPS approval. There are
/// also TLS protocol-level recommendations made by NIST. You should
/// prefer to call [`ClientConfig::fips()`] or [`ServerConfig::fips()`]
/// which take these into account.
pub fn fips(&self) -> bool {
let Self {
cipher_suites,
Expand Down
3 changes: 2 additions & 1 deletion rustls/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,8 @@
//!
//! - `fips`: enable support for FIPS140-3-approved cryptography, via the aws-lc-rs crate.
//! This feature enables the `aws_lc_rs` feature, which makes the rustls crate depend
//! on [aws-lc-rs](https://github.com/aws/aws-lc-rs).
//! on [aws-lc-rs](https://github.com/aws/aws-lc-rs). It also changes the default
//! for [`ServerConfig::require_ems`] and [`ClientConfig::require_ems`].
//!
//! - `tls12` (enabled by default): enable support for TLS version 1.2. Note that, due to the
//! additive nature of Cargo features and because it is enabled by default, other crates
Expand Down
2 changes: 1 addition & 1 deletion rustls/src/server/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ impl ConfigBuilder<ServerConfig, WantsServerCert> {
send_half_rtt_data: false,
send_tls13_tickets: 4,
#[cfg(feature = "tls12")]
require_ems: false,
require_ems: cfg!(feature = "fips"),
}
}
}
17 changes: 15 additions & 2 deletions rustls/src/server/server_conn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,8 @@ pub struct ServerConfig {
/// If set to `true`, requires the client to support the extended
/// master secret extraction method defined in [RFC 7627].
///
/// The default is `false`.
/// The default is `true` if the "fips" crate feature is enabled,
/// `false` otherwise.
///
/// It must be set to `true` to meet FIPS requirement mentioned in section
/// **D.Q Transition of the TLS 1.2 KDF to Support the Extended Master
Expand Down Expand Up @@ -416,8 +417,20 @@ impl ServerConfig {

/// Return `true` if connections made with this `ServerConfig` will
/// operate in FIPS mode.
///
/// This is different from [`CryptoProvider::fips()`]: [`CryptoProvider::fips()`]
/// is concerned only with cryptography, whereas this _also_ covers TLS-level
/// configuration that NIST recommends.
pub fn fips(&self) -> bool {
self.provider.fips()
#[cfg(feature = "tls12")]
{
self.provider.fips() && self.require_ems
}

#[cfg(not(feature = "tls12"))]
{
self.provider.fips()
}
}

/// We support a given TLS version if it's quoted in the configured
Expand Down
37 changes: 32 additions & 5 deletions rustls/tests/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5063,14 +5063,19 @@ fn test_client_rejects_illegal_tls13_ccs() {

#[cfg(feature = "tls12")]
#[test]
fn test_client_rejects_no_extended_master_secret_extension_when_require_ems() {
fn test_client_rejects_no_extended_master_secret_extension_when_require_ems_or_fips() {
let key_type = KeyType::Rsa;
let mut client_config = make_client_config(key_type);
client_config.require_ems = true;
let server_config = finish_server_config(
if cfg!(feature = "fips") {
assert!(client_config.require_ems);
} else {
client_config.require_ems = true;
}
let mut server_config = finish_server_config(
key_type,
server_config_builder_with_versions(&[&rustls::version::TLS12]),
);
server_config.require_ems = false;
let (client, server) = make_pair_for_configs(client_config, server_config);
let (mut client, mut server) = (client.into(), server.into());
transfer_altered(&mut client, remove_ems_request, &mut server);
Expand All @@ -5086,14 +5091,18 @@ fn test_client_rejects_no_extended_master_secret_extension_when_require_ems() {

#[cfg(feature = "tls12")]
#[test]
fn test_server_rejects_no_extended_master_secret_extension_when_require_ems() {
fn test_server_rejects_no_extended_master_secret_extension_when_require_ems_or_fips() {
let key_type = KeyType::Rsa;
let client_config = make_client_config(key_type);
let mut server_config = finish_server_config(
key_type,
server_config_builder_with_versions(&[&rustls::version::TLS12]),
);
server_config.require_ems = true;
if cfg!(feature = "fips") {
assert!(server_config.require_ems);
} else {
server_config.require_ems = true;
}
let (client, server) = make_pair_for_configs(client_config, server_config);
let (mut client, mut server) = (client.into(), server.into());
transfer_altered(&mut client, remove_ems_request, &mut server);
Expand Down Expand Up @@ -5794,6 +5803,24 @@ fn test_server_fips_service_indicator() {
assert!(make_server_config(KeyType::Rsa).fips());
}

#[cfg(feature = "fips")]
#[test]
fn test_client_fips_service_indicator_includes_require_ems() {
let mut client_config = make_client_config(KeyType::Rsa);
assert!(client_config.fips());
client_config.require_ems = false;
assert!(!client_config.fips());
}

#[cfg(feature = "fips")]
#[test]
fn test_server_fips_service_indicator_includes_require_ems() {
let mut server_config = make_server_config(KeyType::Rsa);
assert!(server_config.fips());
server_config.require_ems = false;
assert!(!server_config.fips());
}

#[cfg(all(not(feature = "ring"), feature = "aws_lc_rs", not(feature = "fips")))]
#[test]
fn test_client_fips_service_indicator() {
Expand Down
Loading