tls: Per-site TLS configs using GetClientConfig, including http2 switch #1389

Merged
merged 14 commits into from Feb 18, 2017

Projects

None yet

2 participants

@wendigo
Collaborator
wendigo commented Jan 30, 2017 edited

Hi,

This PR introduced per vhost tls.Config using GetConfigForClient :)

I've tested it an now it's possible to set tls.Config using tls directive for every subdomain on it's own (i.e. customize curves, tls version, cyphers etc)

As per requested I've also added:

tls {
  http2 off
}

so HTTP/2 can be turned of completely for a subdomain.

Addresses: #1375, #189

wendigo added some commits Jan 30, 2017
@wendigo wendigo Remove manual TLS clone method
d277b53
@wendigo wendigo WiP tls 834cad9
@wendigo wendigo Use GetClientConfig for tls.Config
4d457d8
@wendigo wendigo gofmt -s -w
530fa33
@wendigo wendigo GetConfig 75bb30e
@wendigo wendigo Handshake
819155d
@wendigo wendigo referenced this pull request Jan 30, 2017
Open

Update Caddy to use Go 1.8 #1375

9 of 11 tasks complete
@mholt mholt added the under review label Feb 4, 2017
@mholt
Owner
mholt commented Feb 4, 2017

Awesome, I just got back in town and will take a look at this ASAP!

@mholt mholt referenced this pull request Feb 4, 2017
Closed

Disable HTTP2 for subdomain #189

@wendigo wendigo Merge branch 'master' into go_18
3a5e628
@mholt

This is looking really good. It doesn't surprise me that there are fewer lines of code.

I might have something to say on how we actually wire up the tls.Config to the httpserver.Server, so I'll look at that more next. I feel like there might be a less clumsy way of doing it.

Thanks so much for doing all this work! It's gonna be great when it's merged!

caddytls/handshake.go
+// via ACME.
+//
+// This method is safe for use as a tls.Config.GetCertificate callback.
+func (cg ConfigGroup) GetConfigForClient(clientHello *tls.ClientHelloInfo) (*tls.Config, error) {
@mholt
mholt Feb 16, 2017 Owner

Amazingly, I'm having a hard time finding something other than whitespace to nit-pick about here, but I finally found something. :) The godoc for this function is not relevant...

@mholt
mholt Feb 17, 2017 Owner

@wendigo Ready to merge as soon as this godoc is revised! ๐ŸŽ‰

@wendigo
wendigo Feb 18, 2017 Collaborator

Done :)

@mholt
Owner
mholt commented Feb 16, 2017

I think that it will be good idea to move certificate obtaining from handshake.go (configGroup) to caddytls.Config (as generate will be obtained per config).

You might be right. However, we might still want to keep those methods in handshake.go since that is a significant part of the handshake...

@mholt
Owner
mholt commented Feb 17, 2017

@wendigo I made a merge conflict for you -- you're welcome. ๐Ÿ˜… I just merged @elcore's X25519-related PR, so just make sure this branch sets curve preferences.

wendigo added some commits Feb 17, 2017
@wendigo wendigo Merge branch 'master' into go_18
e58196b
@wendigo wendigo Merge branch 'master' into go_18
86f0db5
@mholt mholt removed the under review label Feb 17, 2017
wendigo added some commits Feb 18, 2017
@wendigo wendigo Merge branch 'master' into go_18
2b19c01
@wendigo wendigo Removed comment
0a7973d
@wendigo wendigo Disable HTTP2 on demand
f166f0a
@wendigo wendigo Remove junk
c794df4
@mholt

Awesomeeeee. We are very close!!

caddytls/setup.go
+ case "off":
+ config.DisableHTTP2 = true
+ case "on":
+ config.DisableHTTP2 = false
@mholt
mholt Feb 18, 2017 Owner

Is there a case where HTTP/2 would need to be turned on, since it's enabled by default? (I would rather not have people thinking they need to explicitly turn on HTTP/2.) If not, let's remove the "on" option.

@wendigo
wendigo Feb 18, 2017 Collaborator

No, there isn't. That's exactly why I've introduced DisableHTTP2 (and used golang default boolean value 'false')

@wendigo
wendigo Feb 18, 2017 Collaborator

Removed "on"

@mholt mholt changed the title from GetClientConfig to tls: Per-site TLS configs using GetClientConfig, including http2 switch Feb 18, 2017
@wendigo wendigo Remove http2 enable (no-op)
82e8fa5
@mholt
mholt approved these changes Feb 18, 2017 View changes
@mholt mholt merged commit 286d8d1 into master Feb 18, 2017

5 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@wendigo wendigo deleted the go_18 branch Feb 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment