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

Conversation

Projects
None yet
9 participants
@crvv
Copy link
Collaborator

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

This comment has been minimized.

Copy link
Owner

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 crvv force-pushed the crvv:master branch from 3cd7ba7 to 4b79e90 Dec 20, 2018

@crvv

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
Collaborator

commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 23, 2018

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

@crvv crvv force-pushed the crvv:master branch from 8bb1203 to 262aa6f Dec 23, 2018

@elcore

This comment has been minimized.

Copy link
Collaborator

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 crvv:master branch 2 times, most recently from 412ef95 to fe52358 Jan 17, 2019

@brink668

This comment has been minimized.

Copy link

commented Jan 27, 2019

Is there an ETA on this?

@mholt

This comment has been minimized.

Copy link
Owner

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.

@crvv crvv force-pushed the crvv:master branch from fe52358 to d4379f4 Jan 30, 2019

@crvv crvv force-pushed the crvv:master branch from d4379f4 to 7cf42d7 Feb 9, 2019

@mholt

This comment has been minimized.

Copy link
Owner

commented Feb 12, 2019

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

@crvv

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 13, 2019

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

@mholt

This comment has been minimized.

Copy link
Owner

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.

@Henrocker

This comment has been minimized.

Copy link

commented Feb 17, 2019

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

This comment has been minimized.

Copy link
Owner

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!

@Henrocker

This comment has been minimized.

Copy link

commented Feb 17, 2019

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 crvv:master branch 2 times, most recently from 35345c3 to 9436e12 Feb 19, 2019

@Henrocker

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Owner

commented Feb 20, 2019

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

@Henrocker

This comment has been minimized.

Copy link

commented Feb 20, 2019

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")

This comment has been minimized.

Copy link
@crvv

crvv Feb 20, 2019

Author Collaborator

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

This comment has been minimized.

Copy link
@whalehub

whalehub Feb 20, 2019

Contributor

@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.

This comment has been minimized.

Copy link
@mholt

mholt Feb 20, 2019

Owner

Excellent!

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

This comment has been minimized.

Copy link
@crvv

crvv Feb 22, 2019

Author Collaborator

Yes, I have added the TODO.

@crvv crvv force-pushed the crvv:master branch from 9436e12 to 5edf0f7 Feb 22, 2019

@mholt

This comment has been minimized.

Copy link
Owner

commented Feb 26, 2019

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

@mholt

This comment has been minimized.

Copy link
Owner

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 approved these changes Feb 26, 2019

@mholt mholt merged commit 72d0deb into mholt:master Feb 26, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@mholt mholt referenced this pull request Feb 26, 2019

Closed

TLS 1.3 #2080

@kleveralt

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Collaborator

commented Feb 27, 2019

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

This comment has been minimized.

Copy link
Owner

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

This comment has been minimized.

Copy link

commented Feb 27, 2019

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

This comment has been minimized.

Copy link
Owner

commented Feb 27, 2019

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

@crvv

This comment has been minimized.

Copy link
Collaborator Author

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

This comment has been minimized.

Copy link
Owner

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
You can’t perform that action at this time.