Generate CSRs for kubeadm #70809

liztio · 2018-11-08T17:18:15Z

What type of PR is this?

/kind feature

What this PR does / why we need it:
Generating CSRs makes it much easier to integrate kubeadm's certificates into a larger organisation. Previously adminstrators would have to carefully set up certificates in the correct configuration. With a CSR they can simply sign them appropriately and move them to the directory.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #kubernetes/kubeadm#794

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add option to create CSRs instead of certificates for kubeadm init phase certs and kubeadm alpha certs renew

neolit123 · 2018-11-08T17:23:01Z

/kind feature
/priority important-longterm
/assign @timothysc

neolit123 · 2018-11-08T17:24:49Z

staging/src/k8s.io/client-go/util/cert/pem_test.go

+	defer os.Remove(f.Name())
+
+	if _, err := CertificateRequestFromFile(f.Name()); err == nil {
+		t.Fatalf("Expected error reading csr from empty file, got none")


error should start lowercase.
CSR upper case?

timothysc · 2018-11-09T00:22:25Z

kubeadm alpha certs renew ?

liztio · 2018-11-09T14:42:32Z

@timothysc that one's included too.

liztio · 2018-11-09T14:43:11Z

/test pull-kubernetes-kubemark-e2e-gce-big
/test pull-kubernetes-integration
/test pull-kubernetes-e2e-kops-aws

liztio · 2018-11-09T15:50:33Z

/test pull-kubernetes-e2e-kops-aws pull-kubernetes-kubemark-e2e-gce-big

k8s-ci-robot · 2018-11-12T21:06:54Z

@fedebongio: GitHub didn't allow me to request PR reviews from the following users: client-go, changes, only, for, the.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @roycaihw for the client-go changes only

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.

mikedanese · 2018-11-13T02:23:34Z

staging/src/k8s.io/client-go/util/cert/cert.go

@@ -87,6 +87,26 @@ func NewSelfSignedCACert(cfg Config, key crypto.Signer) (*x509.Certificate, erro
 	return x509.ParseCertificate(certDERBytes)
 }

+// NewCSR creates a new CSR
+func NewCSR(cfg Config, key crypto.Signer) (*x509.CertificateRequest, error) {


the certificates packages in client-go are kind of a kitchen sink and we are working on removing a lot of the surface. @awly would you keep these functions here? I'm wondering if adding this to kubeadm utilities would be preferable.

I'd move this to pkiutil.
Everything under client-go is intended for clients of k8s that do not want to vendor the entire k8s codebase. We should keep only things that are relevant for clients here.

Ack. Everything removed from staging.

atoato88 · 2018-11-14T04:40:42Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

+
+	csrPath := pathForCSR(csrDir, name)
+	if err := certutil.WriteCSR(csrPath, certutil.EncodeCSRPEM(csr)); err != nil {
+		return errors.Wrapf(err, "unable to write csr to file %s", csrPath)


Is it correct that csr -> CSR ?

atoato88 · 2018-11-14T04:40:56Z

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

+	csr, _, err := NewCSR(&kubeadmCert, cfg)
+
+	if err != nil {
+		t.Errorf("invalid signature on csr: %v", err)


Is it correct that csr -> CSR ?

rosti

Thanks @liztio !
Only some tiny nits on my behalf.

rosti · 2018-11-14T10:42:40Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

@@ -65,6 +65,21 @@ func NewCertAndKey(caCert *x509.Certificate, caKey *rsa.PrivateKey, config *cert
 	return cert, key, nil
 }

+// NewCSRAndKey generates a new CSR and key that could be signed to create the given


Unfinished comment?

rosti · 2018-11-14T10:43:51Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

+		return nil, nil, errors.Wrap(err, "unable to create private key")
+	}
+	csr, err := certutil.NewCSR(*config, key)
+


Strange NL choice here. Probably move the empty line before the NewCSR call?

rosti · 2018-11-14T10:47:08Z

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

@@ -276,6 +295,30 @@ func writeKeyFilesIfNotExist(pkiDir string, baseName string, key *rsa.PrivateKey
 	return nil
 }

+func writeCSRFilesIfNotExist(csrDir string, baseName string, csr *x509.CertificateRequest, key *rsa.PrivateKey) error {


I'd rather put a big comment on top of this one, to prevent people from accidentally calling it.

rosti · 2018-11-14T10:50:37Z

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

@@ -276,6 +295,30 @@ func writeKeyFilesIfNotExist(pkiDir string, baseName string, key *rsa.PrivateKey
 	return nil
 }

+func writeCSRFilesIfNotExist(csrDir string, baseName string, csr *x509.CertificateRequest, key *rsa.PrivateKey) error {
+	if pkiutil.CSROrKeyExist(csrDir, baseName) {
+		_, _, err := pkiutil.TryLoadCSRAndKeyFromDisk(csrDir, baseName)


Do we really have to load the key here? In the wake of Meltdown and Spectre I am a bit hesitant to load a key in memory, just to check if it exists. Probably a good old os.Stat of a *.key file can do the trick here?
It's better to keep it in this way in the testing code though (but not in production code).

I'd prefer to keep this consistent with the other similar methods. Maybe file an issue against kubernetes/kubeadm to fix this everywhere? It should be pretty straightforward

liztio · 2018-11-15T00:10:44Z

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

@@ -47,6 +48,11 @@ var (
 		` + cmdutil.AlphaDisclaimer)
 )

+var (
+	csrOnly bool


I don't love that we have to do this, but I don't see any other way to plum this

timothysc · 2018-11-15T02:29:49Z

It looks like this needs a rebase now that @fabriziopandini 's PR has merged.
/hold cancel

timothysc

Minor comments.

timothysc · 2018-11-15T17:10:29Z

cmd/kubeadm/app/cmd/init.go

-	client                clientset.Interface
-	waiter                apiclient.Waiter
-	outputWriter          io.Writer
+


There is no change here that relates to your PR.

timothysc · 2018-11-15T17:10:55Z

cmd/kubeadm/app/cmd/init.go

@@ -236,23 +237,23 @@ func AddInitConfigFlags(flagSet *flag.FlagSet, cfg *kubeadmapiv1beta1.InitConfig
 }

 // AddInitOtherFlags adds init flags that are not bound to a configuration file to the given flagset
-func AddInitOtherFlags(flagSet *flag.FlagSet, cfgPath *string, skipTokenPrint, dryRun *bool, ignorePreflightErrors *[]string) {
+func AddInitOtherFlags(flagSet *flag.FlagSet, initOpts *initOptions) {


I'm ok with this, but this seems orthogonal to the purpose of the PR.

timothysc · 2018-11-15T17:12:36Z

cmd/kubeadm/app/cmd/phases/certs_test.go

@@ -0,0 +1,77 @@
+/*
+Copyright 2017 The Kubernetes Authors.


timothysc · 2018-11-15T17:16:19Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

+
+	csrPath := pathForCSR(csrDir, name)
+	if err := os.MkdirAll(filepath.Dir(csrPath), os.FileMode(0755)); err != nil {
+			return err


Wrap for consistency, also to denote location for debugging.

timothysc · 2018-11-15T17:16:28Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

+			return err
+	}
+
+


nix spare line

timothysc · 2018-11-15T17:16:50Z

cmd/kubeadm/app/util/pkiutil/pki_helpers.go

+
+	key, err := TryLoadKeyFromDisk(pkiPath, name)
+	if err != nil {
+		return nil, nil, err


same re: Wrap.

done.

timothysc · 2018-11-15T20:05:31Z

/test pull-kubernetes-e2e-kops-aws

neolit123

LGTM
thanks for all the updates @liztio . i don't see anything blocking.

timothysc · 2018-11-15T20:39:11Z

/lgtm

k8s-ci-robot · 2018-11-15T20:53:57Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: liztio

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:

cmd/kubeadm/OWNERS

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

k8s-ci-robot requested review from kad and mikedanese November 8, 2018 17:18

k8s-ci-robot assigned timothysc Nov 8, 2018

neolit123 reviewed Nov 8, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2018

liztio force-pushed the csr branch from 46d8b69 to 2e03c7c Compare November 8, 2018 20:57

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2018

liztio changed the title ~~[WIP] Generate CSRs for kubeadm~~ Generate CSRs for kubeadm Nov 8, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 8, 2018

k8s-ci-robot requested a review from roycaihw November 12, 2018 21:06

mikedanese reviewed Nov 13, 2018

View reviewed changes

atoato88 reviewed Nov 14, 2018

View reviewed changes

rosti reviewed Nov 14, 2018

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2018

liztio force-pushed the csr branch from 4a31173 to ef9cab1 Compare November 14, 2018 23:46

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2018

liztio force-pushed the csr branch from ef9cab1 to 7227efe Compare November 15, 2018 00:04

liztio commented Nov 15, 2018

View reviewed changes

liztio force-pushed the csr branch 3 times, most recently from d22d781 to b6e97e5 Compare November 15, 2018 00:55

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2018

liztio force-pushed the csr branch from b6e97e5 to 00faf0c Compare November 15, 2018 17:08

timothysc previously requested changes Nov 15, 2018

View reviewed changes

liztio force-pushed the csr branch from 00faf0c to 2b5cbf5 Compare November 15, 2018 17:41

Option to generate CSRs instead of issued certificates

5fc1a9a

liztio force-pushed the csr branch from 2b5cbf5 to 5fc1a9a Compare November 15, 2018 18:48

neolit123 approved these changes Nov 15, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 15, 2018

timothysc added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 15, 2018

k8s-ci-robot merged commit 3d3cc63 into kubernetes:master Nov 15, 2018

neolit123 mentioned this pull request Nov 19, 2018

Add option to generate CSRs to kubeadm alpha phase certs kubernetes/kubeadm#794

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate CSRs for kubeadm #70809

Generate CSRs for kubeadm #70809

liztio commented Nov 8, 2018

neolit123 commented Nov 8, 2018

neolit123 Nov 8, 2018

timothysc commented Nov 9, 2018

liztio commented Nov 9, 2018

liztio commented Nov 9, 2018

liztio commented Nov 9, 2018

k8s-ci-robot commented Nov 12, 2018

mikedanese Nov 13, 2018

awly Nov 13, 2018

liztio Nov 15, 2018

atoato88 Nov 14, 2018

atoato88 Nov 14, 2018

rosti left a comment

rosti Nov 14, 2018

rosti Nov 14, 2018

rosti Nov 14, 2018

rosti Nov 14, 2018

liztio Nov 15, 2018

liztio Nov 15, 2018

timothysc commented Nov 15, 2018

timothysc left a comment

timothysc Nov 15, 2018

timothysc Nov 15, 2018

timothysc Nov 15, 2018

timothysc Nov 15, 2018

timothysc Nov 15, 2018

timothysc Nov 15, 2018

timothysc commented Nov 15, 2018

neolit123 left a comment

timothysc commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018

Generate CSRs for kubeadm #70809

Generate CSRs for kubeadm #70809

Conversation

liztio commented Nov 8, 2018

neolit123 commented Nov 8, 2018

Choose a reason for hiding this comment

timothysc commented Nov 9, 2018

liztio commented Nov 9, 2018

liztio commented Nov 9, 2018

liztio commented Nov 9, 2018

k8s-ci-robot commented Nov 12, 2018

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

rosti 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

timothysc commented Nov 15, 2018

timothysc 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

timothysc commented Nov 15, 2018

neolit123 left a comment

Choose a reason for hiding this comment

timothysc commented Nov 15, 2018

k8s-ci-robot commented Nov 15, 2018