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

caddytls: add TLS 1.3 support #2399

Merged
merged 2 commits into from
Feb 26, 2019
Merged

caddytls: add TLS 1.3 support #2399

merged 2 commits into from
Feb 26, 2019

Conversation

crvv
Copy link

@crvv crvv commented Dec 20, 2018

1. What does this change do, exactly?

add TLS 1.3

2. Please link to the relevant issues.

#2080

3. Which documentation changes (if any) need to be made because of this PR?

https://caddyserver.com/docs/tls

  1. "tls1.3" is a new suppoted protocol if caddy is built by Go 1.12 or newer.
  2. TLS 1.3 cipher suites are not configurable.

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later

If caddy should be able to be built by Go 1.11 after the releasing of 1.12, I think this PR can be accepted now.
If not, the new file with build tag "go1.12" is useless and the change can be applied to config.go directly when Go 1.12 is released.

@mholt
Copy link
Member

mholt commented Dec 20, 2018

Nice, thanks for jumping on this. (Realizing that, of course, this is a temporary solution, as once Go 1.12 is released, we can just update it in-place.)

While we're at it, can we remove the CBC ciphers from the defaultCiphers lists? I think we should take this opportunity to modernize Caddy's TLS config to be up to snuff with the latest: TLS 1.2 and 1.3.

@crvv
Copy link
Author

crvv commented Dec 20, 2018

I tested this on SSL Labs. The result is https://www.ssllabs.com/ssltest/analyze.html?d=tw.crvv.me&hideResults=on

With Let's Encrypt RSA certificates, the failed clients are IE 11 on Windows 7/8 and Safari 6/7/8

Is that OK?

@abiosoft
Copy link

I do not think this should be an issue.

I tested this on SSL Labs. The result is https://www.ssllabs.com/ssltest/analyze.html?d=tw.crvv.me&hideResults=on

With Let's Encrypt RSA certificates, the failed clients are IE 11 on Windows 7/8 and Safari 6/7/8

Is that OK?

@elcore
Copy link
Collaborator

elcore commented Dec 22, 2018

While we're at it, can we remove the CBC ciphers from the defaultCiphers lists? I think we should take this opportunity to modernize Caddy's TLS config to be up to snuff with the latest: TLS 1.2 and 1.3.

We definitely should add TLS 1.3 cipher suites 😄

@crvv
Copy link
Author

crvv commented Dec 23, 2018

@elcore
TLS 1.3 ciphers are not configurable via Config.CipherSuites.
Please see golang/go#29349 for details.

@elcore
Copy link
Collaborator

elcore commented Dec 23, 2018

@elcore
TLS 1.3 ciphers are not configurable via Config.CipherSuites.
Please see golang/go#29349 for details.

Oh... Thank you!

@crvv crvv force-pushed the master branch 2 times, most recently from 412ef95 to fe52358 Compare January 17, 2019 10:25
@brink668
Copy link

Is there an ETA on this?

@mholt
Copy link
Member

mholt commented Jan 28, 2019

To use Caddy with TLS 1.3 today, go ahead and build from @crvv's branch -- and when Go 1.12 comes out next month, this PR should be updated to make TLS 1.3 the default (without the build constraint which it currently has), and then we'll merge in the PR.

@mholt
Copy link
Member

mholt commented Feb 12, 2019

@crvv Will we need to set an env variable to enable TLS 1.3 by default? golang/go#30055

@crvv
Copy link
Author

crvv commented Feb 13, 2019

We need to set that.
But let's wait for some responses to my comments in that issue.

@mholt
Copy link
Member

mholt commented Feb 13, 2019

@crvv Great! Looks like a doc change will be merged, and I'm good with it if we include that Setenv in this PR to enable it by default.

@high3eam
Copy link

Made some tests today, compiling caddy with go1.12rc1 and this patch.
Result: TLS 1.3 did not work anymore.

Switched then back to go1.12beta2 and applied the same patch and boom, TLS 1.3 works again... Wonder where that comes from.

@mholt
Copy link
Member

mholt commented Feb 17, 2019

@henrocker I think it's because rc1 now requires an environment variable to be set. See the issue I linked to in a comment above. Would love it if you could help figure out if that's what is needed!

@high3eam
Copy link

Thanks for pointing me out. Gonna do some further tests tomorrow and will report back if the envvar fixes this. Thanks

@crvv crvv force-pushed the master branch 2 times, most recently from 35345c3 to 9436e12 Compare February 19, 2019 09:54
@high3eam
Copy link

high3eam commented Feb 20, 2019

Applied patch and built Caddy with go1.12rc1:

$ GODEBUG=tls13=1 go run build.go

Command to run Caddy:

$ GODEBUG=tls13=1 ./caddy

TLS1.3 works! :-)

When running caddy without the GODEBUG var, then TLS1.2 is served

@mholt
Copy link
Member

mholt commented Feb 20, 2019

@henrocker Any chance we can set that env var in code instead of the user having to do it?

@high3eam
Copy link

Maybe something like golang/go#30055 (comment) could be done, but since my understanding in golang is limited, I'm afraid I can only do some tests, if there's something new to try out.

@@ -34,6 +34,9 @@ import (
)

func init() {
// opt-in TLS 1.3 for Go1.12
os.Setenv("GODEBUG", os.Getenv("GODEBUG")+",tls13=1")
Copy link
Author

Choose a reason for hiding this comment

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

@henrocker @mholt
I have added this env in code.

Copy link

Choose a reason for hiding this comment

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

@mholt I can confirm that compiling Caddy with go1.12rc1 and @crvv's change successfully enables TLS 1.3 support in Caddy without the need to manually set an env variable in the OS.

Copy link
Member

Choose a reason for hiding this comment

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

Excellent!

Can we put a TODO in the comment so we remember to revisit this later (i.e. after Go 1.13)?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I have added the TODO.

@mholt
Copy link
Member

mholt commented Feb 26, 2019

Go 1.12 is released; just waiting for CI systems to pick up the new version.

@mholt
Copy link
Member

mholt commented Feb 26, 2019

AppVeyor is refusing to pick up the merge that upgrades to Go 1.12, but the tests on Travis are passing, so I'm merging this sucker.

Thanks everyone!! How exciting.

@mholt mholt merged commit 72d0deb into caddyserver:master Feb 26, 2019
@mholt mholt mentioned this pull request Feb 26, 2019
@kleveralt
Copy link

kleveralt commented Feb 27, 2019

tls1.0 and tls1.1 is no longer available?

I use this config but only tls1.2 and tls1.3 are supported, since this commit...
protocols tls1.0 tls1.3

@whitestrake
Copy link
Collaborator

The documentation does state that "Supported protocols and default protocol versions may be changed at any time", but if that's the case, it should be updated at the next opportunity to reflect the current available protocols.

@mholt
Copy link
Member

mholt commented Feb 27, 2019

TLS 1.0 and 1.1 are supported but not enabled by default, you have to enable those manually, but I highly, highly, strongly, super duper discourage that. (That didn't change with this commit. It has been that way for well over a year.)

@kleveralt
Copy link

I understand, but ssllabs.com replys only tls1.2 and 1.3 are supported with this cofig,
tls {
protocols tls1.0 tls1.3
}

image

@mholt
Copy link
Member

mholt commented Feb 27, 2019

Oh, I see what you mean. Hmm. Can you open a new issue please?

@crvv
Copy link
Author

crvv commented Feb 27, 2019

With this PR, there are only AEAD ciphers in the default cipher suites.
AEAD is new in TLS 1.2
So TLS 1.0 and 1.1 won't work with default cipher suites.

If you want TLS 1.0 and 1.1, you must specify cipher suites you want in the Caddyfile.

@mholt
Copy link
Member

mholt commented Feb 27, 2019

Annnd we have a winner, totally forgot about that. 😅 That would probably be the reason! Thanks @crvv.

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.

8 participants