-
Notifications
You must be signed in to change notification settings - Fork 706
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 pq to usage guide #4677
Conversation
pub const TESTING_PQ: Policy = policy!("PQ-TLS-1-0-2021-05-26"); | ||
|
||
#[cfg(features = "pq")] | ||
pub const DEFAULT_PQ: Policy = policy!("default_pq"); |
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 worry about replacing TESTING_PQ because 1) it's currently a versioned policy 2) it's a policy that uses an earlier draft of hybrid kex :/
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.
if you don't replace it, s2n-quic should probably be updated to use DEFAULT_PQ
: https://github.com/aws/s2n-quic/blob/cc4e6d023f8edf92fea294dcaea2fd5a1132cb47/quic/s2n-quic-tls/src/lib.rs#L12
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.
Agreed. And s2n-quic is probably a good argument for us needing a "default_pq" policy.
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.
After discussing with @camshaft, I'm just going to delete TESTING_PQ. We've been treating "pq" as unstable anyway. We'll just need to update s2n-quic next time we release s2n-tls.
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.
But after discussing with @WesleyRosenblum, I'm going to delete TESTING_PQ later, not in this PR :)
@@ -1124,13 +1137,15 @@ struct s2n_security_policy_selection security_policy_selection[] = { | |||
{ .version = "default", .security_policy = &security_policy_20240501, .ecc_extension_required = 0, .pq_kem_extension_required = 0 }, | |||
{ .version = "default_tls13", .security_policy = &security_policy_20240503, .ecc_extension_required = 0, .pq_kem_extension_required = 0 }, | |||
{ .version = "default_fips", .security_policy = &security_policy_20240502, .ecc_extension_required = 0, .pq_kem_extension_required = 0 }, | |||
{ .version = "default_pq", .security_policy = &security_policy_20240730, .ecc_extension_required = 0, .pq_kem_extension_required = 0 }, |
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.
Do we want a new default_*
that we have to deal with?
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 don't think we want to deal with it, but I unfortunately do think we need it. Otherwise customers who aren't particularly interested in anything other than enabling PQ are going to have to parse through the security policy tables to try to figure out which one they want.
At worst, it'd have the same deprecation strategy as "default_tls13". If "default" ever includes PQ, we can just alias "default_pq" to "default".
pub const TESTING_PQ: Policy = policy!("PQ-TLS-1-0-2021-05-26"); | ||
|
||
#[cfg(features = "pq")] | ||
pub const DEFAULT_PQ: Policy = policy!("default_pq"); |
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.
if you don't replace it, s2n-quic should probably be updated to use DEFAULT_PQ
: https://github.com/aws/s2n-quic/blob/cc4e6d023f8edf92fea294dcaea2fd5a1132cb47/quic/s2n-quic-tls/src/lib.rs#L12
This reverts commit 6a57e5d.
The interned builds were failing because the binaries couldn't access the libcrypto method to check the version. I tried conditional compilation, but different versions of libcrypto alias the version method to different methods and the check was getting too messy. So instead, I'm just going to call an s2n-tls method. s2nc and s2nd shouldn't do that, but they do that all over the place anyway.
Co-authored-by: maddeleine <59030281+maddeleine@users.noreply.github.com>
Resolved issues:
Based on #4660
Description of changes:
We don't have any documentation on how to use PQ. This PR adds some, including an attempt to document the PQ security policies. I also add a "default_pq" policy to make testing or adopting PQ simpler, and updated s2nc/s2nd to help customers determine their libcrypto version.
Call-outs:
I diverged a little from the policy comparison tables in our main security policy documentation. I didn't want to deviate too much, but I felt compelled to clean up the categories at least a little bit.
Testing:
I added a test to compare default_tls13 and default_pq, since they should only ever differ in their kem_preferences. I worry that it won't be a complete test when we add more fields though :/
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.