Support TLS-enabled servers for the server-acl-init command #183
Conversation
ishustava
force-pushed
the
enable-tls-acl-init
branch
3 times, most recently
from
bc9a747 to
15dcaae
Compare
| "Path to the PEM-encoded CA certificate of the Consul cluster.") | ||
| c.flags.StringVar(&c.flagConsulTLSServerName, "consul-tls-server-name", "", | ||
| "The server name to set as the SNI header when sending HTTPS requests to Consul.") | ||
| c.flags.BoolVar(&c.flagUseHTTPS, "use-https", false, |
There was a problem hiding this comment.
If we set consul-ca-cert or consul-tls-server-name with use-https=false, does it work? What about vice versa? Wondering if we should error out if not all the flags are set.
There was a problem hiding this comment.
Yeah, good question. If you set use-https but choose to add your CA to the system trusted CAs, then this should still work. If you set ca cert and tls server name, but don't enable https, it won't have any effect because the job will just use HTTP for all connections, so I don't want to error here.
| // It returns file names in this order: | ||
| // CA certificate, server certificate, and server key. | ||
| // Note that it's the responsibility of the caller to | ||
| // remove the temporary files created by this function. |
There was a problem hiding this comment.
you could also return a function that the callers can run in a defer that cleans up the files:
func() a (string, func()) {
return "a", func() {
os.Remove("a")
}
}
file, cleanup := a()
defer cleanup()There was a problem hiding this comment.
Oh, that's an awesome idea. I'll do that!
- add new flags, -use-https, -consul-ca-cert, and -consul-tls-server-name. - Bump Consul version to 1.6.2 to use tlsutil certificate generation functions in tests.
ishustava
force-pushed
the
enable-tls-acl-init
branch
from
85616b8 to
ddca80d
Compare
ishustava
added
type/enhancement
This PR makes the following changes
-use-https,-consul-ca-cert, and-consul-tls-server-name. The last one is specifically useful for the Helm chart since our certificates don't have Pod IPs as SANs. This allows us to set an SNI header with a custom server name.tlsutilpackage for certificate generation in tests.Ideally, I wanted to change this job to use DNS names of the servers for bootstrapping rather than IPs. But I didn't know how to write tests for it without requiring root privileges to run them. More specifically, we could programmatically add Kubernetes DNS names to
etc/hostsbut that will requiresudoon the machine where the tests are run. This seemed a bit awkward.