Enable cert generation using Let's Encrypt #3878
Conversation
*Instead of passing cert and key to serve over tls, the enable_autocert flag sets up ATC to get a certificate from Let's Encrypt Signed-off-by: Mark James Hender <mhender@pivotal.io>
atc/atccmd/command.go
Outdated
| Cache: cache, | ||
| HostPolicy: autocert.HostWhitelist(cmd.ExternalURL.URL.Hostname()), | ||
| Client: &acme.Client{DirectoryURL: cmd.ACMEURL.URL.String()}, | ||
| ForceRSA: true, |
There was a problem hiding this comment.
nit: looks like this is deprecated and ignored now: https://godoc.org/golang.org/x/crypto/acme/autocert#Manager
atc/atccmd/command.go
Outdated
| tlsConfig.NextProtos = append(tlsConfig.NextProtos, acme.ALPNProto) | ||
| tlsConfig.GetCertificate = m.GetCertificate | ||
| } else { | ||
| tlsLogger.Info("configuring tls using provided certs") |
There was a problem hiding this comment.
| tlsLogger.Info("configuring tls using provided certs") | |
| tlsLogger.Debug("loading-tls-certs") |
atc/atccmd/command.go
Outdated
| } | ||
| tlsLogger := logger.Session("tls-enabled") | ||
| if cmd.EnableAutocert { | ||
| tlsLogger.Info("starting autocert manager") |
There was a problem hiding this comment.
These log lines should match our style guide (and probably be at Debug level?):
| tlsLogger.Info("starting autocert manager") | |
| tlsLogger.Debug("using-autocert-manager") |
atc/atccmd/command.go
Outdated
| errors.New("cannot enable autocert if --tls-cert or --tls-key are set"), | ||
| ) | ||
| } | ||
| if cmd.TLSBindPort != 443 { |
There was a problem hiding this comment.
Where does this requirement come from? 🤔 This may be problematic for setups that run web on a higher port (>1024) and use a load balancer to forward to it.
There was a problem hiding this comment.
This is probably a misunderstanding reading the autocerts Listeners which only listens on port 443. I have done some testing to make sure the ALPN challenge isn't restricted to having the external port on 443 so this check can be removed.
go.mod
Outdated
| @@ -45,6 +45,7 @@ require ( | |||
| github.com/cloudfoundry/bosh-utils v0.0.0-20181224171034-c2cf699102bd // indirect | |||
| github.com/cloudfoundry/go-socks5 v0.0.0-20180221174514-54f73bdb8a8e // indirect | |||
| github.com/cloudfoundry/socks5-proxy v0.0.0-20180530211953-3659db090cb2 // indirect | |||
| github.com/concourse/atc v4.2.2+incompatible | |||
There was a problem hiding this comment.
This looks bogus - bad import somewhere?
henderjm
force-pushed
the
lets-encrypt
branch
2 times, most recently
from
6d41b27
to
bbccc29
Compare
* Remove deprecated attribute ForceRSA * Standardise logging * Don't check for secure port 443 Signed-off-by: Mark James Hender <mhender@pivotal.io>
jamieklassen
added
the
release/documented
concourse/concourse#3878 Signed-off-by: Jamie Klassen <cklassen@pivotal.io>
|
Whilst I 100% welcome this feature (thanks @henderjm!), could I ask: @vito @topherbullock what's changed since @takeyourhatoff implemented this back in vmware-archive/atc#199? What caused the change of heart? :-) |
|
@jpluscplusm I think it just took time to warm up to the idea of Let's Encrypt. It still seemed pretty new and unknown to us at the time. At this point the benefits of making TLS so easy to set up outweigh the technical things we pointed out back in the day. |
Instead of passing cert and key to serve over tls, the enable_autocert flag sets up ATC to get a certificate from Let's Encrypt.
Based on this PR