Add support for generating and using etcd TLS assets. #433

diegs · 2017-04-14T17:58:48Z

This change adds rendering options to allow the apiserver to connect
to etcd using TLS. This applies to both the temporary and self-hosted
control plane.

There are also some options (mostly intended for development) to help
generate the etcd (client/server) certificates. Obviously this is only
useful if etcd is not already up. I updated the quickstart/init-master.sh
script to use this to demonstrate the new functionality.

Self-hosted etcd is not supported at this time.

Partial fix for #403, fixes #244, #389.

diegs · 2017-04-14T19:16:19Z

rktbot run tests

aaronlevy · 2017-04-14T21:04:50Z

cmd/bootkube/render.go

@@ -63,6 +71,7 @@ func init() {
 	cmdRender.Flags().BoolVar(&renderOpts.selfHostKubelet, "experimental-self-hosted-kubelet", false, "(Experimental) Create a self-hosted kubelet daemonset.")
 	cmdRender.Flags().StringVar(&renderOpts.cloudProvider, "cloud-provider", "", "The provider for cloud services.  Empty string for no provider")
 	cmdRender.Flags().BoolVar(&renderOpts.selfHostedEtcd, "experimental-self-hosted-etcd", false, "(Experimental) Create self-hosted etcd assets.")
+	cmdRender.Flags().BoolVar(&renderOpts.etcdUseTLS, "etcd-use-tls", false, "If true, uses TLS for etcd. Implicitly true if --etcd-ca-path,--etcd-certificate-path,--etcd-private-key-path are set. If true but those flags are not set etcd TLS certificates will be generated. Not supported if --experimental-self-hosted-etcd=true.")


Should we just force etcd to always use TLS? I think if the TLS assets are provided, we use them. And if not, we generate them for the user.

In the case of self-hosted etcd - we can't support this yet, but maybe we just warn the user if they try and set the asset flags and say it's not supported yet?

Recapping our offline discussion: yes, we should always use TLS. I'll remove the flag and always either use or generate certs. I'll also generate a set of peer certs if none were provided, and update all the hack scripts to always use the certs.

For self-hosted, I'll simply fail for now if the flags are specified.

If I'm following along, this sounds like it might be an all or nothing hurdle for provisioning tools. We'll need to add path units to block etcd3 from starting until the generated TLS credentials are in place (when there is not secret store). Tectonic will need to distribute those credentials as well in ways that are appropriate for the platform.

If its reasonable, I'd recommend bootkube allow etcd TLS but not require it until v0.5.0.

/cc @philips thoughts on this? I was approaching it from we only allow etcd+TLS (same as we only allow kubelet/api+TLS) -- but should we still allow this to be optional?

Also, etcd would just be in a failure loop until the assets exist, don't know that we need to explicitly coordinate on the paths (but we could I guess)

@aaronlevy @dghubble the generation of etcd assets here is mainly intended as a convenience to help with testing. I was under the assumption that Tectonic would generate etcd certs upstream and simply pass them into bootkube. That's the main use production use case I would envision.

I can keep support for insecure etcd as well if it's a huge pain point for Tectonic, but I agree with Aaron that it feels like a stopgap we should try to discourage as much as possible.

Sure, etcd can crash loop until the credentials are in place on the host. Bare-Metal can scp these into place. Cloud platforms can pull them from KMS / Swift / S3. The story is similar to how the kubeconfig is placed. The challenges are not new. I just want to point out the risk here is that these clients need to make infrastructure specific changes to be able to upgrade to the new version of bootkube that ships with this change. We absolutely want required etcd TLS soon, no disagreement there.

It will be harder to add later -- so my vote would still be to require it (and that we make all the necessary changes in various installers as well).

I reworked it so the render phase will accept http:// etcd endpoints (and not set the TLS flags). However, the default flag still uses TLS, and when using TLS bootkube will create certificates for etcd if none were provided.

aaronlevy · 2017-04-14T21:09:36Z

hack/quickstart/init-master.sh

-Environment="ETCD_ADVERTISE_CLIENT_URLS=http://${COREOS_PRIVATE_IPV4}:2379"
-Environment="ETCD_LISTEN_CLIENT_URLS=http://0.0.0.0:2379"
+Environment="ETCD_ADVERTISE_CLIENT_URLS=https://${COREOS_PRIVATE_IPV4}:2379"
+Environment="ETCD_SSL_DIR=/etc/etcd/tls"


Do we need this if we're providing the assets already?

Note sure I fully follow your question, but yes, we have to set the ETCD_SSL_DIR environment variable to mount /etc/etcd/tls under /etc/ssl/certs in the etcd container so etcd can see the certs (referenced below).

aaronlevy · 2017-04-14T21:40:53Z

/cc @xiang90 @hongchaodeng

aaronlevy · 2017-04-14T21:42:10Z

or @heyitsanthony for etcd sanity check (others may be in diff timezone)

This change adds rendering options to allow the apiserver to connect to etcd using TLS. This applies to both the temporary and self-hosted control plane. There are also some options (mostly intended for development) to help generate the etcd (client/server) certificates. Obviously this is only useful if etcd is not already up. Self-hosted etcd is not supported at this time.

The default is still TLS-enabled, and we print a notification that we're are generating etcd TLS certificates if they weren't provided.

aaronlevy · 2017-04-17T21:29:57Z

cmd/bootkube/render.go

@@ -55,6 +59,9 @@ func init() {
 	cmdRender.Flags().StringVar(&renderOpts.assetDir, "asset-dir", "", "Output path for rendered assets")
 	cmdRender.Flags().StringVar(&renderOpts.caCertificatePath, "ca-certificate-path", "", "Path to an existing PEM encoded CA. If provided, TLS assets will be generated using this certificate authority.")
 	cmdRender.Flags().StringVar(&renderOpts.caPrivateKeyPath, "ca-private-key-path", "", "Path to an existing Certificate Authority RSA private key. Required if --ca-certificate is set.")
+	cmdRender.Flags().StringVar(&renderOpts.etcdCAPath, "etcd-ca-path", "", "Path to an existing PEM encoded CA that will be used for TLS-enabled communication between the apiserver and etcd. Must be used in conjunction with --etcd-certificate-path and --etcd-private-key-path, and must have etcd configured to use TLS with matching secrets.")
+	cmdRender.Flags().StringVar(&renderOpts.etcdCertificatePath, "etcd-certificate-path", "", "Path to an existing certificate that will be used for TLS-enabled communication between the apiserver and etcd. Must be used in conjunction with --etcd-ca-path and --etcd-private-key-path, and must have etcd configured to use TLS with matching secrets.")


Allowing this makes the use of these three flags (ca, cert, key) have varying behaviors. E.g. if etcd-certificate-path is provided, it's assumed that the key is for that cert, not the CA. But if it is not provided, the key is for the CA not the cert.

I'm thinking we could even remove this flag and only allow the option of providing a CA/key to use while generating certs for the etcd nodes. Only allowing the ca cert/key to be provided is more inline with how we generate the rest of the certs (user provides CA, which we use). Providing your own cert is something that is very easy to get wrong as well (wrong CN/SAN, etc.) which is why we've gone the route of "provide CA, but we'll generate).

Also, along those lines I'm wondering if we need to allow for providing a different etcd-specific CA altogether. The flexibility can't hurt -- just thinking aloud.

What about if we changed this to just allow:

--etcd-ca-certificate-path= --etcd-ca-key-path=

Then use those to generate the assets we need.

Or, now that we're assuming cert generation based on etcd server scheme -- if we didn't care about supporting a separate etcd-specific CA - we wouldn't really need any new flags (just use the existing --ca-certificate-x-path flags).

Sorry for the back-forth on this. Obviously more considerations than I initially thought.

/cc @dghubble @ericchiang any thoughts here? Specifically, do we want a diff CA for etcd? Is it safe to assume that given a CA we can generate the assets in the default case?

Specifically, do we want a diff CA for etcd?

For the "bring your own etcd" case we're supporting, I think it's important that these could be different.

This is definitely another mode of operation we could support, either in addition to or instead of the other modes.

I guess my thought was that if you were bringing your own etcd cluster you'd have the CA and certs for that already. The CA could certainly be the same one as what is used to generate the kubelet certs.

So, in total we have the following possible modes of operation:

user provides etcd ca, certs

user provides etcd ca, we use it to generate etcd certs

user provides kubelet ca, we use it to generate etcd certs

user provides no ca, we generate ca (shared for both kubelet and etcd) & use it to generate etcd certs

right now we support 1, 3, and 4. we could trivially add support for 2. what you describe would be to remove support for 1, and maybe or maybe not support 2.

I'm cool with whatever you think makes sense :)

Actually, in-person discussion - I misread this. The behavior is that all three "bring your own flags" must be set:

--etcd-ca-path --etcd-private-key-path --etcd-certificate-path

Or the we will generate the assets for you using the provided CA or we will generate one for you.

I think this is great -- covers the three common cases:

Bring your own etcd

Bring your own CA, but generate assets for me

Generate CA, generate all assets for me

tldr: ignore my previous comments.

This allows the tests to pass after merging kubernetes-retired/bootkube#433. Since work is in progress to change pluton to use the hack scripts, this is only a temporary fix, and it's easier than porting over all the TLS certificate placement changes here too.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 14, 2017

diegs force-pushed the etcd-tls branch 2 times, most recently from 9c2175c to efe544b Compare April 14, 2017 19:04

diegs force-pushed the etcd-tls branch 3 times, most recently from 3eec07b to 7c78d39 Compare April 14, 2017 19:35

diegs changed the title ~~WIP: Add support for generating and using etcd TLS assets.~~ Add support for generating and using etcd TLS assets. Apr 14, 2017

diegs requested review from aaronlevy and yifan-gu April 14, 2017 19:39

aaronlevy reviewed Apr 14, 2017

View reviewed changes

Diego Pontoriero added 3 commits April 17, 2017 11:27

Always secure etcd using TLS, except when self-hosting.

4615f6d

Enable TLS for single-node and mult-node quickstart.

93d9d9a

diegs force-pushed the etcd-tls branch from 7c78d39 to 93d9d9a Compare April 17, 2017 19:56

Add back support for http:// etcd endpoints.

1af5409

The default is still TLS-enabled, and we print a notification that we're are generating etcd TLS certificates if they weren't provided.

aaronlevy reviewed Apr 17, 2017

View reviewed changes

aaronlevy approved these changes Apr 17, 2017

View reviewed changes

diegs mentioned this pull request Apr 17, 2017

Add flag to disable etcd TLS for pluton. coreos/mantle#563

Merged

diegs merged commit 45ef63b into kubernetes-retired:master Apr 18, 2017

diegs deleted the etcd-tls branch April 18, 2017 00:54

diegs mentioned this pull request Apr 18, 2017

Support etcd+TLS #403

Closed

This was referenced Apr 18, 2017

Allow talking to secure etcd (authentication through client certs) #245

Closed

API server connects to etcd over TLS coreos/tectonic-installer#233

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for generating and using etcd TLS assets. #433

Add support for generating and using etcd TLS assets. #433

diegs commented Apr 14, 2017 •

edited

Loading

diegs commented Apr 14, 2017

aaronlevy Apr 14, 2017

diegs Apr 14, 2017

dghubble Apr 17, 2017 •

edited

Loading

aaronlevy Apr 17, 2017

aaronlevy Apr 17, 2017

diegs Apr 17, 2017

dghubble Apr 17, 2017 •

edited

Loading

aaronlevy Apr 17, 2017

dghubble Apr 17, 2017

diegs Apr 17, 2017

aaronlevy Apr 14, 2017

diegs Apr 14, 2017 •

edited

Loading

aaronlevy commented Apr 14, 2017

aaronlevy commented Apr 14, 2017

aaronlevy Apr 17, 2017

ericchiang Apr 17, 2017

diegs Apr 17, 2017 •

edited

Loading

aaronlevy Apr 17, 2017

Add support for generating and using etcd TLS assets. #433

Add support for generating and using etcd TLS assets. #433

Conversation

diegs commented Apr 14, 2017 • edited Loading

diegs commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dghubble Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dghubble Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

aaronlevy commented Apr 14, 2017

aaronlevy commented Apr 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs Apr 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegs commented Apr 14, 2017 •

edited

Loading

dghubble Apr 17, 2017 •

edited

Loading

dghubble Apr 17, 2017 •

edited

Loading

diegs Apr 14, 2017 •

edited

Loading

diegs Apr 17, 2017 •

edited

Loading