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

Clarify s2nc/s2nd PQ output #4702

Merged
merged 8 commits into from
Aug 16, 2024
Merged

Clarify s2nc/s2nd PQ output #4702

merged 8 commits into from
Aug 16, 2024

Conversation

lrstewart
Copy link
Contributor

Description of changes:

#4700 brought up some questions I suspect future users will have too. The output of s2nc/s2nd doesn't make the relationship between KEM / KEM group / curve clear. To fix that, I switched them to only print the PQ information if PQ is configured, and specifically warn about legacy TLS1.2 vs recommended TLS1.3. Most people using s2nc/s2nd with PQ will probably just want an answer to "am I doing PQ now", so I also made that very clear.

TLS1.2 PQ output:

Legacy TLS1.2 KEM: kyber512r3 (PQ key exchange enabled, but TLS1.3 PQ hybrid key exchange strongly recommended instead. See https://github.com/aws/s2n-tls/blob/main/docs/usage-guide/topics/ch15-post-quantum.md)
Curve: secp256r1
Cipher negotiated: ECDHE-KYBER-RSA-AES256-GCM-SHA384

TLS1.3 PQ output:

KEM Group: SecP256r1Kyber768Draft00 (PQ key exchange enabled)
Cipher negotiated: TLS_AES_128_GCM_SHA256

Without PQ output:

Curve: secp256r1
Cipher negotiated: TLS_AES_128_GCM_SHA256

Testing:

This modifies the output used by the integ tests, so I updated them. Since I added the "PQ key exchange enabled" note for human users, I also updated the tests to check for that note as a nice sanity check.

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 Aug 12, 2024
@lrstewart lrstewart marked this pull request as ready for review August 12, 2024 21:33
bin/echo.c Outdated
const char *kem = s2n_connection_get_kem_name(conn);
if (strcmp(kem, "NONE") != 0) {
printf("Legacy TLS1.2 KEM: %s (%s, but TLS1.3 PQ hybrid key exchange strongly recommended instead. "
"See https://github.com/aws/s2n-tls/blob/main/docs/usage-guide/topics/ch15-post-quantum.md)\n",
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't quite tell whether "legacy" refers to TLS 1.2 or the KEM. I think it's referring to the KEM in which case I was expecting the linked usage guide to explain why TLS 1.2 PQ was weaker/flawed relative to TLS 1.3, but it doesn't seem to include that information.

Could we either expand on that information here, or expand on it in the linked usage guide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linked usage guide intentionally doesn't mention TLS1.2 support or list any security policies that would enable it. This comment discussed that choice.

I tried to very briefly explain the recommendation in the output message. While I dislike how long the message is, I prefer only telling people who are already using TLS1.2 PQ over advertising its existence to everyone with the usage guide :)

bin/echo.c Outdated Show resolved Hide resolved
@lrstewart lrstewart enabled auto-merge (squash) August 13, 2024 20:27
@lrstewart lrstewart disabled auto-merge August 13, 2024 20:28
@lrstewart lrstewart enabled auto-merge (squash) August 14, 2024 21:07
@lrstewart lrstewart merged commit e2fcfab into aws:main Aug 16, 2024
35 checks passed
@lrstewart lrstewart deleted the docs_pq branch August 16, 2024 11:38
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.

3 participants