Add optional SSL for Gatekeeper #262
Conversation
Interesting, we could also just offer it as a configuration option via the
I hadn't thought of this, it'd definitely be neat, I wonder if there would be a good way for us to do it through some sort of plugin or external tool. |
gatekeeper/http/http.go
Outdated
| server := &http.Server{ | ||
| Addr: cfg.GatekeeperAddress, | ||
| } | ||
|
|
||
| if certpath == "" || keypath == "" { |
There was a problem hiding this comment.
shouldn't this be an && as they are both required?
There was a problem hiding this comment.
or better: have tlsKeypair error appropriately if both are not provided (or above someplace)..
gatekeeper/http/http.go
Outdated
|
|
||
| tlsConfig := &tls.Config{ | ||
| MinVersion: tls.VersionTLS12, | ||
| CurvePreferences: []tls.CurveID{tls.CurveP521, tls.CurveP384, tls.CurveP256}, |
There was a problem hiding this comment.
Have we checked the right configuration for these curves (e.g. are they the widely recommended ones)? If I remember correclty these curves have been backdoored by the NSA (since they're NIST curves).
There was a problem hiding this comment.
I found this configuration based on some research online, but it may be better to remove CurvePreferences (maybe even CipherSuites, I think PreferServerCipherSuites can be removed safely in any case) from the config. Based on https://github.com/golang/go/blob/46f4bfb2d17a3ccb4b3207d086a90cac3c00ea2f/src/crypto/tls/common.go#L688 it looks like the defaults are []CurveID{X25519, CurveP256, CurveP384, CurveP521} for CurvePreferences. Defaults for CipherSuites depend on the hardware (https://github.com/golang/go/blob/46f4bfb2d17a3ccb4b3207d086a90cac3c00ea2f/src/crypto/tls/common.go#L917)
|
This looks great, can you rebase it? |
|
@ianlivingstone -- just added the --ca flag for bootstrapping against a custom CA. I used square/certstrap to create a fake CA to test against. If that looks good, I can rebase. Regarding something like LetsEncrypt, the more I think about it, the less sense it might make. There's a few scenarios where it would make sense (if a user doesn't have any certificates and doesn't know to/want to get certs from somewhere), but overall something like lego would be an easy way for anyone to get a LetsEncrypt certificate. There would also be some additional configuration that would need to be provided to gatekeeper (probably LetsEncrypt email address and hostname(s) for certificate) which might confuse things. That said, the acme package from lego simplifies things like verification, and could help with the auto-TLS stuff on Gatekeeper. |
| if !strings.HasSuffix(host, "/") { | ||
| host += "/" | ||
| } | ||
|
|
||
| caPool, err := x509.SystemCertPool() | ||
| if err != nil { |
There was a problem hiding this comment.
Should we log the error? As in, "Could not load System Cert Pool, error: %s, creating custom certificate pool"?
Also, thinking about this more, I think this would be fatal if a user didn't specify the --ca flag?
There was a problem hiding this comment.
Also, thinking about this more, I think this would be fatal if a user didn't specify the --ca flag?
Not sure here. There are a few cases I see:
- No
--keyand--certgiven ongatekeepercmd will start the Gatekeeper without SSL. Maybe they should be required too (--cawould probably need them to be required to be consistent). --keyand--certspecify a certificate that is signed by the system bundle (or an intermediate cert), then--cawouldn't be required.
Thinking on what OpenShift does, I think (1) makes sense. In OpenShift, they generate a CA cert (we'd have the user generate one, maybe?), and then use that as a basis of the certificates in the system. That way, there's no reliance on having a certain CA be trusted or not in your system. That should force Gatekeeper over SSL in all cases (which is probably the right thing)
There was a problem hiding this comment.
For testing and development purposes, it makes sense for the Gatekeeper to not require SSL. However, in all other cases SSL should be required.
So, perhaps, we just add a log to create noise (a WARN) about the fact that SSL is not enabled.
Regarding the error, in the case where --ca is not provided and we can't get the SystemCertPool, we'd end up swallowing the error. Instead, I think this should be fatal instead of silently swallowed with an empty cert pool. Perhaps I'm missing some context?
|
@enmand can you rebase this branch and then we can merge it in :) |
Add org/role flags for Gatekeper defaults Use SSL if certpath and keypath are given error in tleKeypair if no certificate or key is provided Use defaults for CurvePreferences, CipherSuites and remove PreferServerCipherSuites Add --ca for specifying a custom CA bundle to use during bootstrapping Log error if we can't load the system cert pool
This pull request starts to add optional SSL for the Gatekeeper bootstrap tool. This would start progress on #248.
This adds --cert, --key (
TORUS_GATEKEEPER_CERTandTORUS_GATEKEEPER_CERT_KEY) to thetorus gatekeeper startcommand, and--ca(TORUS_BOOTSTRAP_CA) to thetorus machines bootstrapto start the service with TLS.Some considerations: the certificate would need to be signed by a CA in the bootstrapping system's CA bundle. There may be cases where a user would use a self-signed cert, or a cert signed by an internal CA. In that case, the certificates would need to be added to the system's certificate bundle, or else the bootstrapping will fail on a bad certificate. The
--caflag will override using the system's certificate bundle.It might be something to explore to do some automatic TLS through something like LetsEncrypt, if no certificates are provided, so that Gatekeeper is always over SSL.