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

[bitnami/nginx] Specify ciphers suites and set strong ciphers #53352

Merged
merged 5 commits into from
Dec 21, 2023
Merged

[bitnami/nginx] Specify ciphers suites and set strong ciphers #53352

merged 5 commits into from
Dec 21, 2023

Conversation

d4rklynk
Copy link
Contributor

@d4rklynk d4rklynk commented Dec 1, 2023

It's better to specify cipher suites to avoid having too many ciphers authorized.

It allows better control of which cipher suites you use.

Only strong ciphers have been chosen.

The cipher have been choosed based on https://english.ncsc.nl/publications/publications/2021/january/19/it-security-guidelines-for-transport-layer-security-2.1

Only a few very very old version of somes OSes will not work (like OX 10.10 that ended 6 years ago).
You can check a handshake simulation example here with these ciphers.

samsepi0l added 3 commits December 1, 2023 11:37
It's better to specify cipher suites to avoid having too many ciphers authorized.

It allow better control of which cipher suites you use.

The cipher have been choosed based on https://english.ncsc.nl/publications/publications/2021/january/19/it-security-guidelines-for-transport-layer-security-2.1

Signed-off-by: samsepi0l <contact@simpleprivacy.fr>
Signed-off-by: samsepi0l <contact@simpleprivacy.fr>
@github-actions github-actions bot added nginx triage Triage is needed labels Dec 1, 2023
@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Dec 1, 2023
@github-actions github-actions bot removed the triage Triage is needed label Dec 1, 2023
@github-actions github-actions bot removed the request for review from javsalgar December 1, 2023 17:53
…lients that don't have AES-NI

Signed-off-by: samsepi0l <contact@simpleprivacy.fr>
@d4rklynk
Copy link
Contributor Author

d4rklynk commented Dec 7, 2023

At the very least, we should set them up as so:

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;
ssl_prefer_server_ciphers off;
ssl_conf_command Options PrioritizeChaCha;

Based on:

These cipher suites might be better for everyone use.

@jotamartos
Copy link
Contributor

Hi @d4rklynk,

Thank you for your contribution and sorry for the delay. Please let us review and test the changes before merging them.

Thanks

@jotamartos
Copy link
Contributor

Hi @d4rklynk,

I just reviewed your changes and proposed some changes according to the information you provided. Could you please review them and ping me here in this main thread so I get notified?

Thanks

@d4rklynk
Copy link
Contributor Author

d4rklynk commented Dec 13, 2023

Hi @jotamartos , sorry, I don't see the changes that you proposed on my end. Could you confirm that you submitted your suggestions?

@jotamartos
Copy link
Contributor

jotamartos commented Dec 18, 2023

Yes, I added the comment and I can see it as well.

Screenshot 2023-12-18 at 14 13 09

The changes I proposed were basically these ones

-           ssl_ciphers        ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256;
-	    ssl_prefer_server_ciphers on;
-	    ssl_conf_command Options PrioritizeChaCha;
+	    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;

@d4rklynk
Copy link
Contributor Author

d4rklynk commented Dec 18, 2023

Hi, when you can, could you please complete your review? You didn't finish the review so I can't see it atm. I'm looking forward. Thanks!

These ciphers are ok imho, I'll let you finish the review so I can accept it.
image

@jotamartos
Copy link
Contributor

So sorry about the review submission 😅 I thought I clicked on it.

…nx.conf

Co-authored-by: Juan José Martos <jotamartos@gmail.com>
Signed-off-by: samsepi0l <contact@samsepi0l.dev>
@d4rklynk
Copy link
Contributor Author

d4rklynk commented Dec 18, 2023

LGTM, Just keep in mind that DHE is weak under somes circumstances -> https://ciphersuite.info/cs/TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256/

Is it relevant to add strong ciphers commented out ? (in case someones needs it)

@d4rklynk d4rklynk changed the title Specify ciphers suites and set strong ciphers [bitnami/nginx] Specify ciphers suites and set strong ciphers Dec 19, 2023
@jotamartos
Copy link
Contributor

Is it relevant to add strong ciphers commented out ? (in case someones needs it)

I think that may confuse users. If the user knows what he's configuring, he'll set the list of ciphers he really wants to use but if he doesn't have enough experience with NGINX, he will probably end up enabling/disabling ciphers he does/doesn't really need.

Let me migrate this information to our systems and will merge this PR after that.

Thanks

Copy link
Contributor

@jotamartos jotamartos left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution!

@jotamartos jotamartos merged commit 4a878fa into bitnami:main Dec 21, 2023
8 checks passed
@d4rklynk d4rklynk deleted the patch-1 branch December 21, 2023 08:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
nginx solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants