kubeadm: Add support for using an external CA whose key is never stored in the cluster #50832

nckturner · 2017-08-17T06:17:05Z

We allow a kubeadm user to use an external CA by checking to see if ca.key is missing and skipping cert checks and kubeconfig generation if ca.key is missing. We also pass an empty arg --cluster-signing-key-file="" to kube controller manager so that the csr signer doesn't start.

What this PR does / why we need it:

This PR allows the kubeadm certs phase and kubeconfig phase to be skipped if the ca.key is missing but all other certs are present.

Which issue this PR fixes :

Fixes kubernetes/kubeadm/issues/280

Special notes for your reviewer:

@luxas @mikedanese @fabriziopandini

Release note:

kubeadm: Add support for using an external CA whose key is never stored in the cluster

k8s-ci-robot · 2017-08-17T06:17:12Z

Hi @nckturner. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

dims · 2017-08-17T10:52:22Z

/ok-to-test

luxas

Basically LGTM; some asks and nits + unit (which I know you're working on already 👍)

Thanks for this contribution!

luxas · 2017-08-17T15:01:30Z

cmd/kubeadm/app/cmd/init.go

+		}
+
+	} else {
+		fmt.Printf("[externalca] No ca.key detected, assuming external CA.  Skipping certs and kubeconfig.\n")


nit: fmt.Println
nit: No ca.key detected, but all other files generated from it are available, so using external CA mode. Won't generate certs nor kubeconfig files (or something like that)

luxas · 2017-08-17T15:09:15Z

cmd/kubeadm/app/phases/controlplane/manifests.go

 		"use-service-account-credentials":  "true",
 		"controllers":                      "*,bootstrapsigner,tokencleaner",
 	}

+	// If using external CA, pass empty string to controller manager instead of ca.key path, so that the csrsigning controller fails to start
+	if certphase.UsingExternalCA(cfg) {
+		defaultArguments["cluster-signing-key-file"] = ""


for clarity; I'd always set cluster-signing-key-file and cluster-signing-cert-file in defaultArguments
and in this special case; unset both (although one only is technically needed, it's safest to reset both of them

Makes sense, fixed.

luxas · 2017-08-17T15:10:45Z

cmd/kubeadm/app/phases/certs/certs.go

+		return false
+	}
+
+	if !ValidatePrivateKey(cfg.CertificatesDir, kubeadmconstants.ServiceAccountKeyBaseName, "service account") {


What about sa.pub? can we make a case for that one as well?

I added a case to superficially check for its existence, tell me what you think.

luxas · 2017-08-17T15:11:01Z

cmd/kubeadm/app/phases/certs/certs.go

+	return true
+}
+
+func ValidateCACert(pkiDir string, baseName string, UXName string) bool {


Please add a comment for all these

luxas · 2017-08-17T15:11:25Z

cmd/kubeadm/app/phases/certs/certs.go

+	return true
+}
+
+func ValidateCACertAndKey(pkiDir string, baseName string, UXName string) bool {


And unit test all these new funcs you are creating.

Done, though tell me if I should refactor them into table driven tests.

Table driven tests are common practice, please write such ones.
Nearly all unit tests in cmd/kubeadm are table driven

luxas · 2017-08-17T15:13:37Z

From the verify job:

I0817 11:02:41.808] Run ./hack/update-bazel.sh
...
W0817 11:20:11.506] Errors from golint:
W0817 11:20:11.507] cmd/kubeadm/app/phases/certs/certs.go:162:1: exported function ValidateCACert should have comment or be unexported
W0817 11:20:11.510] cmd/kubeadm/app/phases/certs/certs.go:178:1: exported function ValidateCACertAndKey should have comment or be unexported
W0817 11:20:11.510] cmd/kubeadm/app/phases/certs/certs.go:191:1: exported function ValidateSignedCert should have comment or be unexported
W0817 11:20:11.511] cmd/kubeadm/app/phases/certs/certs.go:214:1: exported function ValidatePrivateKey should have comment or be unexported

Please fix.

luxas · 2017-08-17T15:15:05Z

This definitely deserves a release note 😄

mikedanese · 2017-08-17T15:56:43Z

cmd/kubeadm/app/phases/certs/certs.go

+	return true
+}
+
+func ValidateSignedCert(pkiDir string, CABaseName string, baseName string, UXName string) bool {


don't start go local variables with capitals. caBaseName...

fabriziopandini · 2017-08-17T15:57:41Z

:+1

mikedanese · 2017-08-17T16:00:13Z

cmd/kubeadm/app/phases/certs/certs.go

+		return false
+	}
+
+	if !ValidateSignedCert(cfg.CertificatesDir, kubeadmconstants.CACertAndKeyBaseName, kubeadmconstants.APIServerCertAndKeyBaseName, "API server") {


make this table driven. https://play.golang.org/p/UJY4McemtD

Can you explain what you are suggesting a little more? Do you mean this entire function?

mikedanese · 2017-08-17T16:06:03Z

cmd/kubeadm/app/phases/certs/certs.go

+	// Check CA Cert
+	caCert, err := pkiutil.TryLoadCertFromDisk(pkiDir, baseName)
+	if err != nil {
+		fmt.Printf("failure loading certificate for %s: %v", UXName, err)


raw prints without linebreaks are going to look pretty funky in the output.

fabriziopandini · 2017-08-17T16:58:24Z

cmd/kubeadm/app/cmd/init.go

-	if err := cmdphases.CreatePKIAssets(i.cfg); err != nil {
-		return err
-	}
+	if !certphase.UsingExternalCA(i.cfg) {


According to the issue, it is expected to have also required kubeconfig files on disk. Is kubeadm going to test this precondition?

Yes, that should also be checked, good catch

luxas

Left some nits, please fix at least most of them. Other than that, LGTM

luxas · 2017-08-29T19:28:36Z

cmd/kubeadm/app/cmd/init.go

-		return err
-	}
+	if res, err := certsphase.UsingExternalCA(i.cfg); !res {
+		fmt.Printf("[externalca] Not using external CA mode: %v\n", err)


I don't think we should print anything here, this is the "normal" state anyway...

Removed the print, if someone wants to see why external CA mode is not active they would have to add print statements to this step. (a logger with verbosity levels would be nice but it seems to be not used in kubeadm)

luxas · 2017-08-29T19:29:22Z

cmd/kubeadm/app/cmd/init.go

-	if err := certsphase.CreatePKIAssets(i.cfg); err != nil {
-		return err
-	}
+	if res, err := certsphase.UsingExternalCA(i.cfg); !res {


err seems to be ignored here?

I'd say its not an error at this step, so I changed it to be a string representing the reason that we are not in "Using External CA".

luxas · 2017-08-29T19:33:33Z

cmd/kubeadm/app/phases/certs/pkiutil/pki_helpers.go

+	}
+
+	var p *rsa.PublicKey
+	p = pubKeys[0].(*rsa.PublicKey)


Any reason to not use := here?

luxas · 2017-08-29T19:36:20Z

cmd/kubeadm/app/phases/certs/pkiutil/pki_helpers.go

+
+	var key *rsa.PrivateKey
+	// Allow RSA format only
+	if k, ok := privKey.(*rsa.PrivateKey); ok {


nit on style, this can be moved out of the if clause and you can check for !ok, return in that case, otherwise just use := afterwards.

Generally I'm trying to minimize the amount of indentation in funcs and avoid if/else when possible. This is still a nit though ;)

luxas

WDYT about the delete(... comment?

luxas · 2017-08-29T21:52:34Z

cmd/kubeadm/app/phases/certs/certs.go

+func UsingExternalCA(cfg *kubeadmapi.MasterConfiguration) (bool, string) {
+
+	if err := validateCACert(certKeyLocation{cfg.CertificatesDir, kubeadmconstants.CACertAndKeyBaseName, "", "CA"}); err != nil {
+		return false, err.Error()


I actually preferred returning an error like you did in the earlier version, but I'm not blocking this PR on that

luxas · 2017-08-29T21:55:08Z

cmd/kubeadm/app/phases/certs/certs.go

+		return err
+	}
+
+	_, err := pkiutil.TryLoadKeyFromDisk(l.pkiDir, l.caBaseName)


Not needed for this PR; but in some future PR we should make sure the public key in the cert and key match each other (we have other places that do similar functionality like this so...)

luxas · 2017-08-29T21:59:41Z

cmd/kubeadm/app/phases/controlplane/manifests.go

+	// If using external CA, pass empty string to controller manager instead of ca.key/ca.crt path,
+	// so that the csrsigning controller fails to start
+	if res, _ := certphase.UsingExternalCA(cfg); res {
+		defaultArguments["cluster-signing-key-file"] = ""


Gah, should have spotted this earlier, I realize delete(defaultArguments, "cluster...") is probably preferable over this method... WDYT?

It would be great to get an unit test for this as well if possible...

The nice thing about setting to "" is that (in controller manager logs) you get:

Skipping "csrsigning"

instead of:

Failed to start certificate controller: error reading CA cert file "/etc/kubernetes/ca/ca.pem": open /etc/kubernetes/ca/ca.pem: no such file or directory

when using delete() which I think is slightly better, but I will switch them if you prefer.

With that context (thanks for reminding me about the UX), it makes sense to pass ""

luxas

Thanks for this!

/lgtm

luxas · 2017-08-31T09:41:40Z

cmd/kubeadm/app/phases/controlplane/manifests.go

+	// If using external CA, pass empty string to controller manager instead of ca.key/ca.crt path,
+	// so that the csrsigning controller fails to start
+	if res, _ := certphase.UsingExternalCA(cfg); res {
+		defaultArguments["cluster-signing-key-file"] = ""


With that context (thanks for reminding me about the UX), it makes sense to pass ""

fejta-bot · 2017-08-31T15:00:51Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

We allow a kubeadm user to use an external CA by checking to see if ca.key is missing and skipping cert checks and kubeconfig generation if ca.key is missing.

nckturner · 2017-08-31T17:17:18Z

@luxas @mikedanese I squashed my commits, can I get another lgtm?

mikedanese · 2017-08-31T17:46:45Z

/lgtm

k8s-github-robot · 2017-08-31T17:46:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, mikedanese, nckturner

Associated issue: 280

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas,mikedanese]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

mikedanese · 2017-09-02T04:52:07Z

/retest

fejta-bot · 2017-09-02T14:57:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

mikedanese · 2017-09-02T20:24:22Z

/retest

k8s-github-robot · 2017-09-03T01:19:16Z

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

k8s-github-robot · 2017-09-03T02:26:19Z

Automatic merge from submit-queue (batch tested with PRs 50832, 51119, 51636, 48921, 51712)

k8s-ci-robot · 2017-09-03T02:35:22Z

@nckturner: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`e0ab0b5`	link	`/test pull-kubernetes-e2e-kops-aws`
pull-kubernetes-e2e-gce-etcd3	`e0ab0b5`	link	`/test pull-kubernetes-e2e-gce-etcd3`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fabriziopandini · 2017-09-03T07:30:49Z

🎉 well done!

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2017

k8s-github-robot assigned mikedanese and luxas Aug 17, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 17, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 17, 2017

luxas reviewed Aug 17, 2017

View reviewed changes

luxas changed the title ~~Kubeadm external CA check~~ kubeadm: Add support for using an external CA whose key is never stored in the cluster Aug 17, 2017

luxas added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 17, 2017

luxas added this to the v1.8 milestone Aug 17, 2017

mikedanese reviewed Aug 17, 2017

View reviewed changes

fabriziopandini reviewed Aug 17, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2017

nckturner force-pushed the external-ca branch from 7777aee to c4b1948 Compare August 21, 2017 22:25

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 21, 2017

nckturner force-pushed the external-ca branch from 53fd4e2 to 533d7c1 Compare August 29, 2017 18:25

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2017

luxas reviewed Aug 29, 2017

View reviewed changes

nckturner force-pushed the external-ca branch from 533d7c1 to 739df46 Compare August 29, 2017 21:11

luxas reviewed Aug 29, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

luxas approved these changes Aug 31, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017

Adds check for external CA

e0ab0b5

We allow a kubeadm user to use an external CA by checking to see if ca.key is missing and skipping cert checks and kubeconfig generation if ca.key is missing.

nckturner force-pushed the external-ca branch from c4d87f6 to e0ab0b5 Compare August 31, 2017 17:13

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

mikedanese added the ok-to-merge label Sep 2, 2017

k8s-github-robot merged commit 2164f09 into kubernetes:master Sep 3, 2017

kubeadm: Add support for using an external CA whose key is never stored in the cluster #50832

kubeadm: Add support for using an external CA whose key is never stored in the cluster #50832

Conversation

nckturner commented Aug 17, 2017 • edited by luxas Loading

k8s-ci-robot commented Aug 17, 2017

dims commented Aug 17, 2017

luxas left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nckturner Aug 21, 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

Choose a reason for hiding this comment

luxas commented Aug 17, 2017

luxas commented Aug 17, 2017

mikedanese Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini commented Aug 17, 2017

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

Choose a reason for hiding this comment

luxas left a 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

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

luxas left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Aug 31, 2017

nckturner commented Aug 31, 2017

mikedanese commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

mikedanese commented Sep 2, 2017

fejta-bot commented Sep 2, 2017

mikedanese commented Sep 2, 2017

k8s-github-robot commented Sep 3, 2017

k8s-github-robot commented Sep 3, 2017

k8s-ci-robot commented Sep 3, 2017

fabriziopandini commented Sep 3, 2017

nckturner commented Aug 17, 2017 •

edited by luxas

Loading

nckturner Aug 21, 2017 •

edited

Loading

mikedanese Aug 17, 2017 •

edited

Loading