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

Update nginx.md #1222

Closed
wants to merge 4 commits into from
Closed

Update nginx.md #1222

wants to merge 4 commits into from

Conversation

jameskimmel
Copy link
Contributor

small corrections

ssl_protocols
Copy link
Member

@felix920506 felix920506 left a comment

Choose a reason for hiding this comment

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

The original config was correct. Change the comment to tell users to comment next line to enable TLS 1.0 and 1.1 as your devices will have to be truly ancient to not support TLS 1.2 or newer.

@jameskimmel
Copy link
Contributor Author

jameskimmel commented Dec 12, 2024

The original config was correct. Change the comment to tell users to comment next line to enable TLS 1.0 and 1.1 as your devices will have to be truly ancient to not support TLS 1.2 or newer.

I don't thinks so.

Version 1.23.4 and later: the default SSL protocols are TLSv1, TLSv1.1, TLSv1.2, and TLSv1.3

Source: https://nginx.org/en/docs/http/configuring_https_servers.html

So my guess is that if you do nothing, aka leave #, you get the defaults, which includes TLSv1.1 and older.

If you uncomment the line, you explicitly enable only 1.2 and newer (which may break older devices)

@felix920506
Copy link
Member

felix920506 commented Dec 12, 2024

Just because it's the "default" doesn't mean it's better. TLS 1.0 and 1.1 have well documented vulnerabilities at this point so disabling them is more secure.

Which may break "older" devices is way too overblown. Your device has to have not received software updates for at least the last 10 years for it to not support TLS 1.2. Ancient is a more fitting description for these devices.

@jameskimmel
Copy link
Contributor Author

jameskimmel commented Dec 12, 2024

Just because it's the "default" doesn't mean it's better. TLS 1.0 and 1.1 have well documented vulnerabilities at this point so disabling them is more secure.

I never said so.
And I agree with you.
But then we would need to change it the other way round.
Make TLS 1.2 and 1.3 the default by not commenting them out and write in a comment that users with older devices can comment that line if they have older incompatible devices.

Something like this:

# Comment the next line if you have older devices that are not compatible with TLS 1.2 or newer. 
# Otherwise you should leave it uncommented for security reasons.
ssl_protocols TLSv1.3 TLSv1.2;

@felix920506
Copy link
Member

Yes that is what I meant in my review comment

@jellyfin-bot
Copy link

Cloudflare Pages deployment

Latest commit 461644b7dd13b53fa52307779c5df5d35bfe519d
Status ✅ Deployed!
Preview URL https://8df06ee2.jellyfin-org.pages.dev
Type 🔀 Preview

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants