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

New AWS SSL policies #2274

Merged
merged 5 commits into from Dec 8, 2023
Merged

New AWS SSL policies #2274

merged 5 commits into from Dec 8, 2023

Conversation

buchdag
Copy link
Member

@buchdag buchdag commented Jul 12, 2023

New SSL policies from #1644 by @patrickdk77.

@buchdag buchdag added the type/feat PR for a new feature label Jul 12, 2023
@buchdag buchdag requested a review from rhansen July 12, 2023 21:47
@buchdag buchdag self-assigned this Jul 12, 2023
nginx.tmpl Outdated
@@ -161,6 +160,30 @@
ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3;
ssl_ciphers 'ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:DHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES256-SHA256:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA';
ssl_prefer_server_ciphers on;
{{- else if eq .ssl_policy "AWS-FS-1-2-Res-2020-10" }}
ssl_protocols TLSv1.2 TLSv1.3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm reading the AWS docs correctly, none of these newly added policies support TLSv1.3. If the addition of TLSv1.3 is intentional, please add a comment explaining why it differs from the AWS policy.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not, I lifted the code from #1644 without even thinking to double check.

Furthermore AWS now have a grand total of 18 SSL policies, and I'm wondering is those should be added too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just learned that the v1.3 and <=v1.2 cipher suites are disjoint sets. None of these new policies mention any TLSv1.3 cipher suites. It's unclear to me how nginx/OpenSSL behaves in this case. I see three possibilities:

  • TLSv1.3 is effectively disabled, as if it was excluded from ssl_protocols.
  • TLSv1.3 negotiations always fail due to lack of acceptable cipher suites. (Perhaps causing clients to retry with TLS v1.2?)
  • The default set of TLSv1.3 cipher suites is used.

Regardless, I think that TLSv1.3 should be removed from these new policies. I'm hesitant to remove it from the existing policies without testing because that might break compatibility.

Furthermore AWS now have a grand total of 18 SSL policies, and I'm wondering is those should be added too.

That would be nice, although that can be done in a future PR if you don't feel like doing it now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Regardless, I think that TLSv1.3 should be removed from these new policies.

Agreed, done ✅

I'm hesitant to remove it from the existing policies without testing because that might break compatibility.

I think I'll remove TLSv1.3 from older AWS policies when I'll add the new ones that do support TLSv1.3

nginx.tmpl Show resolved Hide resolved
@buchdag buchdag merged commit 2ed3297 into main Dec 8, 2023
2 checks passed
@buchdag buchdag deleted the ssl-policies branch December 8, 2023 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feat PR for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants