-
Notifications
You must be signed in to change notification settings - Fork 726
Add fixed version of the rfc9151 policy #5277
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
base: main
Are you sure you want to change the base?
Conversation
27c21cd
to
d33048a
Compare
This avoids any future changes rolling out silently for customers who want to pin to a particular policy.
d33048a
to
70b11e1
Compare
@@ -66,7 +66,7 @@ For previous defaults, see the "Default Policy History" section below. | |||
"default_fips" does not currently support TLS1.3. If you need a policy that supports both FIPS and TLS1.3, choose "20230317". We plan to add TLS1.3 support to both "default" and "default_fips" in the future. | |||
|
|||
"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. | |||
Like the default policies, this policy may also change if the source RFC definition changes. The "20250429" policy is the current fixed policy corresponding to "rfc9151". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add a little "history" table for RFC9151 like we have for the default policies? https://github.com/aws/s2n-tls/blob/70b11e11826489798b30e5b80b6fda4a77e3d937/docs/usage-guide/topics/ch06-security-policies.md#default-policy-history I'd rather not add it to that actual table though, since it's unlikely to follow the same update schedule as the default policies. But maybe a separate table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed an update that revises how we display this information for both the default and rfc9151 policy (all named policies). Let me know what you think.
EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_rfc9151, "20250211", ecdsa_sha384_chain_and_key)); | ||
EXPECT_OK(s2n_test_security_policies_compatible(&security_policy_20250429, "default_tls13", ecdsa_sha384_chain_and_key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this test actually want a non-fixed RFC9151? Same question for below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current API for s2n_test_security_policies_compatible doesn't really let us chose anything else for the base policy (not the one that's by string).
Technically, what the test wants is the full set of all past/future rfc9151 policies, but that's hard to express in the current framework. If we did change rfc9151 we'd ideally extend this test to include coverage for both the previous & next version of it. I'm not sure of a good way to indicate that -- open to thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we did for the default policies:
s2n-tls/tests/unit/s2n_security_policies_test.c
Lines 860 to 922 in ed81463
/* Sanity check that changes to default security policies are not completely | |
* backwards incompatible. | |
* | |
* If we get into a situation where the current default has NO options in | |
* common with a past version of the default, then updating s2n-tls becomes | |
* very dangerous. Fleets with a mix of the old default and the new default | |
* may be unable to communicate. | |
* | |
* This check only performs one basic handshake, so isn't exhaustive. | |
*/ | |
{ | |
/* "default" */ | |
{ | |
const struct s2n_security_policy *versioned_policies[] = { | |
&security_policy_20170210, | |
&security_policy_20240501, | |
}; | |
const struct s2n_supported_cert supported_certs[] = { | |
{ .cert = rsa_chain_and_key }, | |
{ .cert = ecdsa_chain_and_key, .start_index = 1 }, | |
}; | |
EXPECT_OK(s2n_test_default_backwards_compatible("default", | |
versioned_policies, s2n_array_len(versioned_policies), | |
supported_certs, s2n_array_len(supported_certs))); | |
}; | |
/* "default_tls13" */ | |
if (s2n_is_rsa_pss_certs_supported()) { | |
const struct s2n_security_policy *versioned_policies[] = { | |
&security_policy_20240417, | |
&security_policy_20240503, | |
}; | |
const struct s2n_supported_cert supported_certs[] = { | |
{ .cert = rsa_chain_and_key }, | |
{ .cert = ecdsa_chain_and_key }, | |
{ .cert = rsa_pss_chain_and_key }, | |
}; | |
EXPECT_OK(s2n_test_default_backwards_compatible("default_tls13", | |
versioned_policies, s2n_array_len(versioned_policies), | |
supported_certs, s2n_array_len(supported_certs))); | |
}; | |
/* "default_fips" */ | |
{ | |
const struct s2n_security_policy *versioned_policies[] = { | |
&security_policy_20240416, | |
&security_policy_20240502, | |
}; | |
const struct s2n_supported_cert supported_certs[] = { | |
{ .cert = rsa_chain_and_key }, | |
{ .cert = ecdsa_chain_and_key }, | |
}; | |
EXPECT_OK(s2n_test_default_backwards_compatible("default_fips", | |
versioned_policies, s2n_array_len(versioned_policies), | |
supported_certs, s2n_array_len(supported_certs))); | |
}; | |
}; |
In contrast, numbered or dated versions are fixed and will never change. | ||
|
||
The numbered equivalents of the default policies are currently: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you were aiming for with this section, but I don't think you finished?
If you were trying to remove all the default mappings to rely on the history info below, I don't think I agree with that change. I like the "fixed" versions being very obvious up here, and the full history is much more verbose and less useful.
* "default" | ||
* v1.4.16 - 20240501 | ||
* Older - 20170210 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I prefer the table format. I worry the bulleted list is less readable and won't grow well. But with only two entries for each it probably doesn't really matter yet?
Release Summary:
Resolved issues:
n/a
Description of changes:
This avoids any future changes rolling out silently for customers who want to pin to a particular policy.
Call-outs:
I updated the rfc9151 components in-place (I think they're not in public APIs at all?).
Testing:
I didn't add any new tests since this is essentially just a rename of the existing policy. I don't think we have tests for every numbered policy today so that decision seems consistent (happy to add something if asked).
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.