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

Add new security policy #3895

Merged
merged 5 commits into from
Mar 22, 2023
Merged

Add new security policy #3895

merged 5 commits into from
Mar 22, 2023

Conversation

lrstewart
Copy link
Contributor

Description of changes:

Adds a new versioned security policy intended to be FIPS compliant and support >=TLS1.2. Basically, I intend this policy as a sane default for customers that don't need to worry about supporting older peers. In particular, this should be useful for customers that own both the clients and servers in their system.

Call-outs:

I don't think these "sane defaults" should be too controversial, but I did:

  • Exclude DHE. It requires additional configuration that customers need to manage, and ECDHE should really be preferred at this point.
  • Prefer PSS over PKCS1 for RSA signatures, even for TLS1.2. I think this is the right choice, since PSS is more secure.
  • Include CBC mode. I went back and forth on this one; GCM is preferred, but FIPS still includes CBC, so I ultimately decided to include it.

Testing:

I added some simple handshake tests to verify that handshake are possible between the new policy and existing defaults.

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

@github-actions github-actions bot added the s2n-core team label Mar 20, 2023
Copy link
Contributor

@torben-hansen torben-hansen left a comment

Choose a reason for hiding this comment

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

looks good from my point of view

docs/USAGE-GUIDE.md Outdated Show resolved Hide resolved
@toidiu
Copy link
Contributor

toidiu commented Mar 20, 2023

Are you working from a FIPS RFC/compliance document when choosing these options?

@toidiu
Copy link
Contributor

toidiu commented Mar 20, 2023

Are you working from a FIPS RFC/compliance document when choosing these options?

* These curves were chosen based on the following specification:
* https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-52r2.pdf
*/
const struct s2n_ecc_named_curve *const s2n_ecc_pref_list_default_fips[] = {

I believe the document is https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-52r2.pdf

Should the name of the policy then be some derivation of NIST.SP.800-52r2?

@lrstewart
Copy link
Contributor Author

lrstewart commented Mar 20, 2023

Are you working from a FIPS RFC/compliance document when choosing these options?

* These curves were chosen based on the following specification:
* https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-52r2.pdf
*/
const struct s2n_ecc_named_curve *const s2n_ecc_pref_list_default_fips[] = {

I believe the document is https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-52r2.pdf

Should the name of the policy then be some derivation of NIST.SP.800-52r2?

This policy is not explicitly following that standard document. It conforms to FIPS requirements, but only provides a subset of the available options. For example, it doesn't support DHE. We also won't update it, even in a way compliant with NIST.SP.800-52r2. It's intended as a default rather than a fips-only option; customers looking for any sane policy should choose it, rather than customer looking for FIPS. Therefore, I think it needs to be a traditionally named versioned policy.

/* TLS1.2 with ECDSA */
&s2n_ecdhe_ecdsa_with_aes_128_gcm_sha256,
&s2n_ecdhe_ecdsa_with_aes_256_gcm_sha384,
&s2n_ecdhe_ecdsa_with_aes_128_cbc_sha256,
Copy link
Contributor

Choose a reason for hiding this comment

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

aes-cbc-hmac256 is typically a lot slower than aes-cbc-hmac-sha1 but pretty much all modern clients that care about performance support GCM anyway

@lrstewart lrstewart enabled auto-merge (squash) March 21, 2023 22:53
@lrstewart lrstewart merged commit 404b56b into aws:main Mar 22, 2023
@lrstewart lrstewart deleted the new_policy branch March 22, 2023 01:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants