8000 fix: correctly enable TLSv1 and TLSv1.1 by liuxiaoy · Pull Request #2510 · nginx-proxy/nginx-proxy · GitHub
[go: up one dir, main page]
Skip to content

fix: correctly enable TLSv1 and TLSv1.1 #2510

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 12, 2024
Merged

fix: correctly enable TLSv1 and TLSv1.1 #2510

merged 3 commits into from
Oct 12, 2024

Conversation

liuxiaoy
Copy link
Contributor

No description provided.

@buchdag
Copy link
Member
buchdag commented Sep 19, 2024

@liuxiaoy hi, could you provide a bit more context on the PR and explanations on the fix ?

@liuxiaoy
Copy link
Contributor Author
liuxiaoy commented Sep 26, 2024 •
edited by buchdag
Loading

Hi, I want my site support TLSv1 TLSv1.1 TLSv1.2 TLSv1.3.
Then I use environment SSL_POLICY=Mozilla-Old.
But the openssl disabled TLSv1 TLSv1.1 default, so we need append :@SECLEVEL=0 to ssl_ciphers in nginx to support TLSv1 really.

@liuxiaoy
Copy link
Contributor Author
liuxiaoy commented Sep 27, 2024 •
edited
Loading

Related links:

nginx/docker-nginx#858 (comment)

nginx/docker-nginx#743 (comment)

@buchdag
Copy link
Member
buchdag commented Sep 27, 2024

Thanks for the links 👍

However, I'm not able to confirm that this work locally using either nmap --script ssl-enum-ciphers, openssl s_client or curl.

Have you used it and confirmed that it does work, and if so how ?

@buchdag buchdag mentioned this pull request Sep 27, 2024
Open
@liuxiaoy
Copy link
Contributor Author

I have test it with the shell below:

curl --tls-max 1.0 https://your.com -Ik
# or
openssl s_client -connect your.com:443 -tls1

or test on the site online like  https://www.ssllabs.com/ssltest/analyze.html?d=your.com&hideResults=on

@liuxiaoy
Copy link
Contributor Author

I used to use a lower version of nginx to solve the tlsv1 problem, which was v1.12. Later, in order to automate the certificate renewal, I found some discussions on this topic online. After experiments, I found that a higher version could also solve the problem, so I proposed this PR

@buchdag
Copy link
Member
buchdag commented Sep 28, 2024

I think I identified why it can' 8000 t validate it, see nginx/docker-nginx#858 (comment) for details, I'll think of a way we could handle this.

@buchdag
Copy link
Member
buchdag commented Sep 30, 2024

@liuxiaoy regarding your message on nginx/docker-nginx#858 : nginx-proxy is built upon the upstream nginx Docker images, so if you can't enable TLSv1 / TLSv1.1 in the base nginx image, you will have the same issue in nginx-proxy, that's not something specific to the later.

@buchdag
Copy link
Member
buchdag commented Sep 30, 2024 •
edited
Loading

The issue comes from the ssl_protocols directive : when used in the http context, it will limit what protocols are available in the server context.

Currently we set ssl_protocols and ssl_ciphers in the http context with the Mozilla-Intermediate SSL policy values, which don't have TLSv1 or TLSv1.1 enabled.

We can either permanently replace this with the Mozilla-Old values, or dynamically use the Mozilla-Old values when at least one container use an SSL_POLICY that has TLSv1 and/or TLSv1.1 enabled. I'm not sure the later is required, since we're always manually setting ssl_ciphers in the server context, those "default" values are actually never used for anything else than the fallback listener.

@buchdag
Copy link
Member
buchdag commented Oct 2, 2024

@liuxiaoy could try image nginxproxy/nginx-proxy:2510 ?

It's based on this change and should correctly enable TLSv1 / TLSv1.1

@buchdag buchdag changed the title Fix nginx.tmpl when enabled TLSv1 TLSv1.1 fix: correctly enable TLSv1 and TLSv1.1 Oct 6, 2024
@buchdag buchdag added the type/fix PR for a bug fix label Oct 6, 2024
@buchdag
Copy link
Member
buchdag commented Oct 6, 2024 •
edited
Loading

@liuxiaoy I took the liberty to update your PR.

@liuxiaoy
Copy link
Contributor Author

@buchdag Sorry for forgetting this. I have tried the nginxproxy/nginx-proxy:2510 and after deployment, I tested it successfully using curl --tls-max 1.0. The online website also tested that This server supports TLS 1.0 and TLS 1.1. Grade capped to B.

If you can't test it there, we can communicate so that you can test it successfully.

@buchdag
Copy link
Member
buchdag commented Oct 12, 2024

I was able to confirm that it work on my side, but I wanted to be sure that it worked for you too.

The only limitation is the fact that it won't work with the Alpine variant of the image as I indicated in the docs (I could not manage to get it to work, if you have any idea on how to do it that would be welcome).

This isn't realistically testable at the moment so I'll merge it as-is.

Thanks for bringing the issue to light @liuxiaoy 👍

@buchdag buchdag merged commit 8417046 into nginx-proxy:main Oct 12, 2024
2 checks passed
@liuxiaoy
Copy link
Contributor Author

@buchdag I have try the same on nginxproxy/nginx-proxy:1.6-alpine，it also works；Just append :@SECLEVEL=0 using vi /etc/nginx/conf.d/default.conf then run nginx -s reload on the contaner.

I also try it ok on nginx:1.27-alpine with config below:

    server {
        listen 443 ssl;
        ssl_certificate /etc/ssl/localhost.crt;
        ssl_certificate_key /etc/ssl/localhost.key;
        ssl_ciphers DEFAULT:@SECLEVEL=0;
        location / { return 200 'OK - $ssl_protocol - $ssl_cipher\n'; }
    }

Test record

curl --tls-max 1.0 https://127.0.0.1:443/ -k                                                                                                                                                                                                                       
OK - TLSv1 - ECDHE-RSA-AES256-SHA

@liuxiaoy liuxiaoy deleted the patch-1 branch October 15, 2024 07:18
@buchdag
Copy link
Member
buchdag commented Oct 16, 2024

@liuxiaoy it does indeed work with the alpine version too, I probably mixed up something while testing.

Thanks for the heads up, I'll update the docs 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buchdag buchdag buchdag approved these changes

Assignees
No one assigned
Labels
type/fix PR for a bug fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@liuxiaoy @buchdag
0