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

docs: add more warnings about security policy defaults #4507

Merged
merged 6 commits into from
Apr 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
22 changes: 22 additions & 0 deletions bindings/rust/s2n-tls/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@ impl Config {
/// Returns a Config object with pre-defined defaults.
///
/// Use the [`Builder`] if custom configuration is desired.
///
/// # Warning
///
/// By default, the newly created Config uses the default security policy.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// By default, the newly created Config uses the default security policy.
/// By default, the newly created Config uses the default security policy. This policy changes across library versions and could break connections.

Should this short version of the warning mention that the policy changes? Unless I already knew about the default policy/had read the other documentation, I probably wouldn't understand why this was a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idk, the changing isn't the only issue. The default policy might also just not have the options they're expecting / wanting. Like, at the moment it still has 1.0 and most customers probably don't want 1.0.

I was hoping I just needed to point out policies even exist so customers might read the other documentation :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya, if we think the default is going to remain stuck at 1.0, then I wonder if it would be worth exploring the option of totally removing the default from the bindings?

But still think we should merge this PR, since the new warning is definitely better than what we have now 😄

/// Consider changing this depending on your security and availability requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "compatibility" maybe more accurate here or availability what is intended?

Suggested change
/// Consider changing this depending on your security and availability requirements
/// Consider changing this depending on your security and compatibility requirements

/// by using [`Builder`] and [`Builder::set_security_policy`].
/// See the s2n-tls usage guide:
/// <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>
pub fn new() -> Self {
Self::default()
}
Expand Down Expand Up @@ -158,6 +166,13 @@ pub struct Builder {
}

impl Builder {
/// # Warning
///
/// By default, the newly created Builder uses the default security policy.
/// Consider changing this depending on your security and availability requirements
/// by calling [`Builder::set_security_policy`].
/// See the s2n-tls usage guide:
/// <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>
pub fn new() -> Self {
crate::init::init();
let config = unsafe { s2n_config_new_minimal().into_result() }.unwrap();
Expand Down Expand Up @@ -754,6 +769,13 @@ impl Builder {
}
}

/// # Warning
///
/// The newly created Builder uses the default security policy.
/// Consider changing this depending on your security and availability requirements
/// by using [`Builder::new`] instead and calling [`Builder::set_security_policy`].
/// See the s2n-tls usage guide:
/// <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>
impl Default for Builder {
fn default() -> Self {
Self::new()
Expand Down
9 changes: 9 additions & 0 deletions bindings/rust/s2n-tls/src/connection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,15 @@ unsafe impl Send for Connection {}
unsafe impl Sync for Connection {}

impl Connection {
/// # Warning
///
/// The newly created connection uses the default security policy.
/// Consider changing this depending on your security and availability requirements
/// by calling [`Connection::set_security_policy`].
/// Alternatively, you can use [`crate::config::Builder`], [`crate::config::Builder::set_security_policy`],
/// and [`Connection::set_config`] to set the policy on the Config instead of on the Connection.
/// See the s2n-tls usage guide:
/// <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>
pub fn new(mode: Mode) -> Self {
crate::init::init();

Expand Down
47 changes: 42 additions & 5 deletions bindings/rust/s2n-tls/src/security.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

//! Security options like cipher suites, signature algorithms, versions, etc.
//!
//! See <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>

use crate::error::Error;
use core::fmt;
use std::ffi::{CStr, CString};
Expand All @@ -25,6 +29,8 @@ impl Policy {
}
}

/// See the s2n-tls usage guide for details on available policies:
/// <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>
pub fn from_version(version: &str) -> Result<Policy, Error> {
let cstr = CString::new(version).map_err(|_| Error::INVALID_INPUT)?;
let context = Context::Owned(cstr);
Expand All @@ -39,16 +45,47 @@ impl fmt::Debug for Policy {
}

macro_rules! policy {
($name:ident, $version:expr) => {
pub const $name: Policy = Policy(Context::Static(concat!($version, "\0").as_bytes()));
($version:expr) => {
Policy(Context::Static(concat!($version, "\0").as_bytes()))
};
}

policy!(DEFAULT, "default");
policy!(DEFAULT_TLS13, "default_tls13");
/// Default policy
///
/// # Warning
///
/// Cipher suites, curves, signature algorithms, or other security policy options
/// may be added or removed from "default" in order to keep it up to date with
/// current security best practices.
///
/// That means that updating the library may cause the policy to change. If peers
/// are expected to be reasonably modern and support standard options, then this
/// should not be a problem. But if peers rely on a deprecated option that is removed,
/// they may be unable to connect.
///
/// If you instead need a static, versioned policy, choose one according to the s2n-tls usage guide:
/// <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>
pub const DEFAULT: Policy = policy!("default");

/// Default policy supporting TLS1.3
///
/// # Warning
///
/// Cipher suites, curves, signature algorithms, or other security policy options
/// may be added or removed from "default_tls13" in order to keep it up to date with
/// current security best practices.
///
/// That means that updating the library may cause the policy to change. If peers
/// are expected to be reasonably modern and support standard options, then this
/// should not be a problem. But if peers rely on a deprecated option that is removed,
/// they may be unable to connect.
///
/// If you instead need a static, versioned policy, choose one according to the s2n-tls usage guide:
/// <https://aws.github.io/s2n-tls/usage-guide/ch06-security-policies.html>
pub const DEFAULT_TLS13: Policy = policy!("default_tls13");

#[cfg(feature = "pq")]
policy!(TESTING_PQ, "PQ-TLS-1-0-2021-05-26");
pub const TESTING_PQ: Policy = policy!("PQ-TLS-1-0-2021-05-26");

pub const ALL_POLICIES: &[Policy] = &[
DEFAULT,
Expand Down
14 changes: 9 additions & 5 deletions docs/usage-guide/topics/ch06-security-policies.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,24 +40,26 @@ The following chart maps the security policy version to protocol version and cip
| 20200207 | | | | X | | X | X | | | | X | |
| rfc9151 | | | X | X | | X | | | | X | X | X |

The "default", "default_tls13", and "default_fips" versions are special in that they will be updated with future s2n-tls changes and ciphersuites and protocol versions may be added and removed, or their internal order of preference might change. Numbered versions are fixed and will never change.
In general, customers prefer to use numbered versions for production use cases to prevent impact from library updates.
The "default", "default_tls13", and "default_fips" versions are special in that they will be updated with future s2n-tls changes to keep up-to-date with current security best practices. Ciphersuites, protocol versions, and other options may be added or removed, or their internal order of preference might change. **Warning**: this means that the default policies may change as a result of library updates, which could break peers that rely on legacy options.

In contrast, numbered or dated versions are fixed and will never change.

"20230317" offers more limited but more secure options than the default policies. Consider it if you don't need or want to support less secure legacy options like TLS1.1 or SHA1. It is also FIPS compliant and supports TLS1.3. If you need a version of this policy that doesn't support TLS1.3, choose "20240331" instead.

"20160411" follows the same general preference order as "default". The main difference is it has a CBC cipher suite at the top. This is to accommodate certain Java clients that have poor GCM implementations. Users of s2n-tls who have found GCM to be hurting performance for their clients should consider this version.

"rfc9151" is derived from [Commercial National Security Algorithm (CNSA) Suite Profile for TLS and DTLS 1.2 and 1.3](https://datatracker.ietf.org/doc/html/rfc9151). This policy restricts the algorithms allowed for signatures on certificates in the certificate chain to RSA or ECDSA with sha384, which may require you to update your certificates.
Like the default policies, this policy may also change if the source RFC definition changes.

s2n-tls does not expose an API to control the order of preference for each ciphersuite or protocol version. s2n-tls follows the following order:

*NOTE*: All ChaCha20-Poly1305 cipher suites will not be available if s2n-tls is not built with an Openssl 1.1.1 libcrypto. The underlying encrypt/decrypt functions are not available in older versions.

1. Always prefer the highest protocol version supported
2. Always use forward secrecy where possible. Prefer ECDHE over DHE.
3. Prefer encryption ciphers in the following order: AES128, AES256, ChaCha20, 3DES, RC4.
4. Prefer record authentication modes in the following order: GCM, Poly1305, SHA256, SHA1, MD5.

*NOTE*: ChaCha20-Poly1305 cipher suites will not be available if s2n-tls is built with an older libcrypto like openssl-1.0.2. The underlying encrypt/decrypt functions are not available in older versions.

#### ChaCha20 Boosting

s2n-tls usually prefers AES over ChaCha20. However, some clients-- particularly mobile or IOT devices-- do not support AES hardware acceleration, making AES less efficient and performant than ChaCha20. In this case, clients will indicate their preference for ChaCha20 by listing it first during cipher suite negotiation. Usually s2n-tls servers ignore client preferences, but s2n-tls offers "ChaCha20 boosted" security policies that will choose ChaCha20 over AES if the client indicates a preference for ChaCha20. This is available in the "CloudFront-TLS-1-2-2021-ChaCha20-Boosted" policy, which is identical to the "CloudFront-TLS-1-2-2021" policy listed above but with ChaCha20 Boosting enabled.
Expand Down Expand Up @@ -90,7 +92,9 @@ s2n-tls usually prefers AES over ChaCha20. However, some clients-- particularly
| 20200207 | | X | | X |
| rfc9151 | X | X | | X |

Note that legacy SHA-1 algorithms are not supported in TLS1.3. Legacy SHA-1 algorithms will be supported only if TLS1.2 has been negotiated and the security policy allows them.
*NOTE*: Legacy SHA-1 algorithms are not supported in TLS1.3. Legacy SHA-1 algorithms will be supported only if TLS1.2 has been negotiated and the security policy allows them.

*NOTE*: RSA-PSS certificates will not be available if s2n-tls is built with an older libcrypto like openssl-1.0.2. RSA certificate signatures with PSS padding (RSA-PSS-RSAE) will still be available, but RSA-PSS certificate signatures with PSS padding (RSA-PSS-PSS) will not be available.

### Chart: Security policy version to supported curves/groups

Expand Down
Loading