-
Notifications
You must be signed in to change notification settings - Fork 712
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
Conversation
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", |
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 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?
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 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 :)
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:
TLS1.3 PQ output:
Without PQ output:
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.