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 Caddy to use Go 1.8 #1375

Closed
9 of 10 tasks
mholt opened this issue Jan 25, 2017 · 19 comments
Closed
9 of 10 tasks

Update Caddy to use Go 1.8 #1375

mholt opened this issue Jan 25, 2017 · 19 comments
Labels
feature ⚙️ New feature or request help wanted 🆘 Extra attention is needed

Comments

@mholt
Copy link
Member

mholt commented Jan 25, 2017

Go 1.8 introduces some nice improvements to the crypto packages and Caddy should begin using them. There are also some updates to net/http and other packages we should use.

Specifically:

I'm looking into server push next chance I get -- others are welcome to help contribute to the crypto changes.

@mholt mholt added feature ⚙️ New feature or request help wanted 🆘 Extra attention is needed labels Jan 25, 2017
@elcore
Copy link
Collaborator

elcore commented Jan 25, 2017

@mholt

Add support for curve X25519 and ChaCha20-Poly1305. Ensure curve X25519 is prioritized if hardware has AES-GCM support, otherwise prefer a ChaCha20 suite.

You mean "Add support for curve X25519 and ChaCha20-Poly1305. Ensure AES-GCM is prioritized if hardware has AES-GCM support, otherwise prefer a ChaCha20 suite.", right?

"Basically" like my implementation here https://github.com/elcore/caddy

Use Config.GetConfigForClient() to dynamically select a TLS config for the specific site being connected to, rather than having to reduce them all into a single TLS config in config.go:MakeTLSConfig.

So, the user should not be able to configure the TLS Configuration, it should be dynamically configured???

P.S.: Just asking, I am so sorry, but I don´t have enough time to work on this new feature.

@mholt
Copy link
Member Author

mholt commented Jan 25, 2017

So, the user should not be able to configure the TLS Configuration, it should be dynamically configured???

No; the user can still configure TLS, but right now we have to combine all sites' tls directives to a single one (if they're all on the same listener) because there was no way to load a TLS config per handshake before. But now there will be.

@elcore
Copy link
Collaborator

elcore commented Jan 25, 2017

No; the user can still configure TLS

Okay

[...] but right now we have to combine all sites' tls directives to a single one (if they're all on the same listener) because there was no way to load a TLS config per handshake before. [...]

I know 😄

Thank you for the clarification

@mholt
Copy link
Member Author

mholt commented Jan 25, 2017

@elcore In regards to:

You mean "Add support for curve X25519 and ChaCha20-Poly1305. Ensure AES-GCM is prioritized if hardware has AES-GCM support, otherwise prefer a ChaCha20 suite.", right?

Yes, sorry, I meant what the Go release notes say: "ChaCha20-Poly1305 is now prioritized unless hardware support for AES-GCM is present."

I updated my original post.

@wendigo
Copy link

wendigo commented Jan 30, 2017

Is it possible to modify .travis.yml to drop go1.7 and build only for 1.8 (so go1.8 builds will pass)?

@wendigo
Copy link

wendigo commented Jan 30, 2017

I've started working on (#1389):

  • Use Config.Clone() instead of our own function, cloneTLSClientConfig, in reverseproxy.go
  • Use Config.GetConfigForClient() to dynamically select a TLS config for the specific site being connected to, rather than having to reduce them all into a single TLS config in config.go:MakeTLSConfig

@mholt
Copy link
Member Author

mholt commented Jan 31, 2017

@wendigo

Is it possible to modify .travis.yml to drop go1.7 and build only for 1.8 (so go1.8 builds will pass)?

Yes! We'll drop support for Go 1.7 when Go 1.8 is released. Won't be long now.

And thank you for working on those :) That's super helpful.

@injust
Copy link

injust commented Feb 21, 2017

Now that go 1.8 has been released, any ETA on a version of Caddy built with it?

@mholt
Copy link
Member Author

mholt commented Feb 21, 2017

@injust Not yet, but hopefully soon!

@suedadam
Copy link

suedadam commented Apr 5, 2017

I'm down to help, where do you want help @mholt ?

@mholt
Copy link
Member Author

mholt commented Apr 5, 2017

@suedadam Thanks! We're almost done. How would you like to help with the VerifyPeerCertificate feature? Basically, this would allow a user to specify a specific TLS client certificate to allow through, rather than how it is now, which is any certificate signed by the specified CA. I'm not sure how involved this is, depends what you're up to. But that's the only thing on this list that is really remaining at this point.

I've already decided that we're going to hold off on auto-push for now, it's complicated.

None of the remaining TODO items will delay the release on April 20, so we're on track for that. 👍

@suedadam
Copy link

suedadam commented Apr 5, 2017

@mholt Seems relatively simple at first glance. Would you like me to append the functionality to "basicauth"? Haven't looked too much into the organization of this project so any pointers to where to start the creation and addition of it would help a ton

(I'll try and do most of it tonight)

@mholt
Copy link
Member Author

mholt commented Apr 5, 2017

@suedadam Not quite, this is at the transport layer, not the application layer. This belongs in the tls directive. Right now, we have the "clients" subdirective but it only lets you list entire CAs to accept certificates from. It should be changed so that you list specific client certificates to allow. (Does that make sense?)

@suedadam
Copy link

suedadam commented Apr 5, 2017

@mholt Gotcha, that makes perfect sense. Thanks!

@thibaultmeyer
Copy link

thibaultmeyer commented Apr 28, 2017

Use Config.VerifyPeerCertificate() to give site owner more control over authentication with client certificates. At least, I think that's what this would let us do. TLS client auth is a wonderful and too-underused technology. Let's help make it more popular. (See golang/go#16363)

Yeah, it could by nice to have access to, at least, that information :

  • Result (useful in case of Client Auth is enabled but not mandatory) : “SUCCESS”, “FAILED:reason”, or “NONE”
  • Client certificate Fingerprint
  • Client subject DN
  • Issuer subject DN
  • Serial number of the client certificate

As variables, we can pass these data to the proxy via extra headers.

Related forum threads :

@mholt
Copy link
Member Author

mholt commented Jun 30, 2017

Okay, here's an elaboration of what needs to happen to finish TLS client authentication and close this issue:

Right now we can only verify client certificates based on issuing CA, but it would be nice to be able to authorize only specific client certificates based on their public key. This'll probably involve a slight change to the tls Caddyfile syntax:

tls {
     clients allowed_client_key1.pem allowed_client_key2.pem ...
}

and maybe we can read in all the keys in a bundle in a single file, and allow all of them:

tls {
    clients allowed_clients.pem
}

Do we want to continue supporting authorized CAs (current behavior)? If we do, then we should introduce a new tls subdirective called client_ca or something. /cc @carlisia

@zikes
Copy link

zikes commented Jul 1, 2017

Would it be worthwhile to consider multiple clients subdirectives, such as with proxy upstream?

@mholt
Copy link
Member Author

mholt commented Jul 1, 2017

@zikes Yes, good point, my intent is that clients subdirective could be specified multiple times if there are many that won't fit on a single line.

@mholt
Copy link
Member Author

mholt commented Mar 27, 2018

This was all done long ago, except for adding improved support for verifying (accepting/rejecting) client certificates, and I think that can be done in a new issue if there's demand for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request help wanted 🆘 Extra attention is needed
Projects
None yet
Development

No branches or pull requests

7 participants