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

kubeadm 710 Switch to a dedicated CA for kubeadm etcd identities #60385

Merged
merged 2 commits into from Mar 1, 2018

Conversation

@stealthybox
Contributor

stealthybox commented Feb 25, 2018

What this PR does / why we need it:
On kubeadm init/kubeadm upgrade, this PR generates an etcd specific CA for signing the following certs:

  • etcd serving cert
  • etcd peer cert
  • apiserver etcd client cert

These certs were previously signed by the kubernetes CA.
The etcd static pod in local.go has also been updated to only mount the /etcd subdir of cfg.CertificatesDir.

New phase command:

kubeadm alpha phase certs etcd-ca

See the linked issue for details on why this change is an important security feature.

Which issue(s) this PR fixes
Fixes kubernetes/kubeadm#710

Special notes for your reviewer:

on the master

this should still fail:

curl localhost:2379/v2/keys  # no output
curl --cacert /etc/kubernetes/pki/etcd/ca.crt https://localhost:2379/v2/keys  # handshake error

this should now fail: (previously would succeed)

cd /etc/kubernetes/pki
curl --cacert etcd/ca.crt --cert apiserver-kubelet-client.crt --key apiserver-kubelet-client.key https://localhost:2379/v2/keys
  # curl: (35) error:14094412:SSL routines:ssl3_read_bytes:sslv3 alert bad certificate

this should still succeed:

cd /etc/kubernetes/pki
curl --cacert etcd/ca.crt --cert apiserver-etcd-client.crt --key apiserver-etcd-client.key https://localhost:2379/v2/keys

Release note:

On cluster provision or upgrade, kubeadm generates an etcd specific CA for all etcd related certificates.
@timothysc

This comment has been minimized.

@timothysc

This comment has been minimized.

Member

timothysc commented Feb 27, 2018

This was an implementation request/bug from the original PR so it meets the criteria for post freeze inclusion.

@detiber

This comment has been minimized.

Member

detiber commented Feb 27, 2018

/lgtm

@xiangpengzhao

just a nit. otherwise LGTM.

@@ -302,6 +322,17 @@ func NewAPIServerKubeletClientCertAndKey(caCert *x509.Certificate, caKey *rsa.Pr
return apiClientCert, apiClientKey, nil
}
// NewEtcdCACertAndKey generate a self signed front proxy CA.

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 27, 2018

Member

Why is it a front proxy CA?

This comment has been minimized.

@stealthybox

stealthybox Feb 28, 2018

Contributor

Fixed -- thanks peter!

@k8s-merge-robot k8s-merge-robot removed the lgtm label Feb 28, 2018

@stealthybox

This comment has been minimized.

Contributor

stealthybox commented Feb 28, 2018

oops... did not notice it was already in the queue 🙃
could somebody review the spelling updates please? thanks!

++ @xiangpengzhao

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 28, 2018

/lgtm

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 28, 2018

/area kubeadm

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 28, 2018

@stealthybox Could you please squash your commits?

@stealthybox

This comment has been minimized.

Contributor

stealthybox commented Feb 28, 2018

@dixudx thanks for reviewing!

I separated the typos so that the CA related commit is easier to understand.
I think it's best to leave it like this. Do you agree?

@dixudx

This comment has been minimized.

Member

dixudx commented Feb 28, 2018

@stealthybox IMO The second commit is more about renaming. But it's okay to keep it as it is.

@detiber

This comment has been minimized.

Member

detiber commented Feb 28, 2018

/lgtm

@@ -136,17 +136,22 @@ func upgradeComponent(component string, waiter apiclient.Waiter, pathMgr StaticP
}
// ensure etcd certs are generated for etcd and kube-apiserver
if component == constants.Etcd || component == constants.KubeAPIServer {

This comment has been minimized.

@kargakis

kargakis Feb 28, 2018

Member

Why CreateEtcdCACertAndKeyFiles needs to run when upgrading the api server?

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Mar 1, 2018

Member

IIUC, CreateEtcdServerCertAndKeyFiles in L145 and CreateAPIServerEtcdClientCertAndKeyFiles in L153 depends on etcd CA to generate their cert and key files, CreateEtcdCACertAndKeyFiles ensures etcd CA files are existing or generated.

This comment has been minimized.

@kargakis

kargakis Mar 1, 2018

Member

Makes sense

/hold cancel

This comment has been minimized.

@stealthybox

stealthybox Mar 2, 2018

Contributor

Peter explained correctly what I was thinking 👍

The happy path would work without it, but if you ever wanted the apiserver to start independently of etcd, it would fail.

@timothysc

/approve

@timothysc timothysc added the approved label Mar 1, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Mar 1, 2018

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: detiber, dixudx, stealthybox, timothysc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@xiangpengzhao

This comment has been minimized.

Member

xiangpengzhao commented Mar 1, 2018

/hold
until @kargakis question #60385 (comment) is finally explained. Hopefully my #60385 (comment) is correct :)

@timothysc

This comment has been minimized.

Member

timothysc commented Mar 1, 2018

xref #60608

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 1, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 1, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 5cff6c9 into kubernetes:master Mar 1, 2018

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce
Details
cla/linuxfoundation stealthybox authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Context retired. Status moved to "pull-kubernetes-integration".
pull-kubernetes-verify Job succeeded.
Details
@andrewrynhard

This comment has been minimized.

Contributor

andrewrynhard commented Mar 17, 2018

This seems to break self-hosting, self-hosted kube-apiserver:

err (open /etc/kubernetes/pki/apiserver-etcd-client.crt: no such file or directory)

@timothysc I'm not sure what the status is for storing certs as secrets in the cluster. I know it is behind a feature gate, but is that something we wan't to continue to do? IIRC there were security concerns with that. I can add apiserver-etcd-client* in a PR if that is the route we are sticking with.

@stealthybox

This comment has been minimized.

Contributor

stealthybox commented Mar 18, 2018

@andrewrynhard do we rely on external etcd for self-hosted?
Is there a case where the self-hosted apiserver contacts an etcd static pod?
I was under the impression that we deprecated etcd-operator.

I know there's upgrade logic that modifies the static pod manifests for self-hosted -- we could ensure the cert is generated and mounted or just remove this option unless a user specifies an external etcd cert.

@andrewrynhard

This comment has been minimized.

Contributor

andrewrynhard commented Mar 18, 2018

@stealthybox sorry, I should have been more specific... this breaks self-hosting if StoreCertsInSecrets is set to true. When StoreCertsInSecrets is set to true, the certs in /etc/kubernetes/pki get stored as secrets in the cluster: https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/selfhosting/selfhosting.go#L63-L73.

@stealthybox

This comment has been minimized.

Contributor

stealthybox commented Mar 18, 2018

I think I found the bug.

@stealthybox

This comment has been minimized.

Contributor

stealthybox commented Mar 18, 2018

@andrewrynhard
would you be willing to open an issue for this including the command and config that reproduces this bug?

I believe the patch should include something like this:

diff --git a/cmd/kubeadm/app/phases/selfhosting/selfhosting_volumes.go b/cmd/kubeadm/app/phases/selfhosting/selfhosting_volumes.go
index 627fb01f04..2446843c34 100644
--- a/cmd/kubeadm/app/phases/selfhosting/selfhosting_volumes.go
+++ b/cmd/kubeadm/app/phases/selfhosting/selfhosting_volumes.go
@@ -279,6 +279,11 @@ func getTLSKeyPairs() []*tlsKeyPair {
                        cert: kubeadmconstants.APIServerKubeletClientCertName,
                        key:  kubeadmconstants.APIServerKubeletClientKeyName,
                },
+               {
+                       name: kubeadmconstants.APIServerEtcdClientCertAndKeyBaseName,
+                       cert: kubeadmconstants.APIServerEtcdClientCertName,
+                       key:  kubeadmconstants.APIServerEtcdClientKeyName,
+               },
                {
                        name: kubeadmconstants.ServiceAccountKeyBaseName,
                        cert: kubeadmconstants.ServiceAccountPublicKeyName,

Probably needs a test as well -- I can help review.

@stealthybox

This comment has been minimized.

Contributor

stealthybox commented Mar 18, 2018

We also need to add a matching &v1.ProjectedVolumeSource to apiServerCertificatesVolumeSource()

@andrewrynhard

This comment has been minimized.

Contributor

andrewrynhard commented Mar 18, 2018

@stealthybox I can create an issue once I’m back to the computer. First glance I think that patch is all we need.

@andrewrynhard

This comment has been minimized.

Contributor

andrewrynhard commented Mar 18, 2018

That patch and the projected volume

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment