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

Generate CSRs for kubeadm #70809

Merged
merged 1 commit into from Nov 15, 2018

Conversation

@liztio
Copy link
Member

liztio commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Member

neolit123 commented Nov 8, 2018

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

defer os.Remove(f.Name())

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

This comment has been minimized.

@neolit123

neolit123 Nov 8, 2018

Member

error should start lowercase.
CSR upper case?

@liztio liztio force-pushed the liztio:csr branch from 46d8b69 to 2e03c7c Nov 8, 2018

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

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Nov 9, 2018

kubeadm alpha certs renew ?

@liztio

This comment has been minimized.

Copy link
Member

liztio commented Nov 9, 2018

@timothysc that one's included too.

@liztio

This comment has been minimized.

Copy link
Member

liztio commented Nov 9, 2018

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

@liztio

This comment has been minimized.

Copy link
Member

liztio commented Nov 9, 2018

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

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 12, 2018

@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.

@@ -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) {

This comment has been minimized.

@mikedanese

mikedanese Nov 13, 2018

Member

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.

This comment has been minimized.

@awly

awly Nov 13, 2018

Contributor

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.

This comment has been minimized.

@liztio

liztio Nov 15, 2018

Member

Ack. Everything removed from staging.


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)

This comment has been minimized.

@atoato88

atoato88 Nov 14, 2018

Contributor

Is it correct that csr -> CSR ?

csr, _, err := NewCSR(&kubeadmCert, cfg)

if err != nil {
t.Errorf("invalid signature on csr: %v", err)

This comment has been minimized.

@atoato88

atoato88 Nov 14, 2018

Contributor

Is it correct that csr -> CSR ?

@rosti
Copy link
Contributor

rosti left a comment

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

@@ -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

This comment has been minimized.

@rosti

rosti Nov 14, 2018

Contributor

Unfinished comment?

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

This comment has been minimized.

@rosti

rosti Nov 14, 2018

Contributor

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

@@ -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 {

This comment has been minimized.

@rosti

rosti Nov 14, 2018

Contributor

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

@@ -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)

This comment has been minimized.

@rosti

rosti Nov 14, 2018

Contributor

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).

This comment has been minimized.

@liztio

liztio Nov 15, 2018

Member

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 liztio force-pushed the liztio:csr branch from 4a31173 to ef9cab1 Nov 14, 2018

@liztio liztio force-pushed the liztio:csr branch from ef9cab1 to 7227efe Nov 15, 2018

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

var (
csrOnly bool

This comment has been minimized.

@liztio

liztio Nov 15, 2018

Member

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

@liztio liztio force-pushed the liztio:csr branch 3 times, most recently from d22d781 to b6e97e5 Nov 15, 2018

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Nov 15, 2018

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

@liztio liztio force-pushed the liztio:csr branch from b6e97e5 to 00faf0c Nov 15, 2018

@timothysc
Copy link
Member

timothysc left a comment

Minor comments.

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

This comment has been minimized.

@timothysc

timothysc Nov 15, 2018

Member

There is no change here that relates to your PR.

@@ -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) {

This comment has been minimized.

@timothysc

timothysc Nov 15, 2018

Member

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

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

This comment has been minimized.

@timothysc

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

This comment has been minimized.

@timothysc

timothysc Nov 15, 2018

Member

Wrap for consistency, also to denote location for debugging.

return err
}


This comment has been minimized.

@timothysc

timothysc Nov 15, 2018

Member

nix spare line


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

This comment has been minimized.

@timothysc

timothysc Nov 15, 2018

Member

same re: Wrap.

@liztio liztio force-pushed the liztio:csr branch from 00faf0c to 2b5cbf5 Nov 15, 2018

done.

@liztio liztio force-pushed the liztio:csr branch from 2b5cbf5 to 5fc1a9a Nov 15, 2018

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Nov 15, 2018

/test pull-kubernetes-e2e-kops-aws

@neolit123
Copy link
Member

neolit123 left a comment

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

@timothysc

This comment has been minimized.

Copy link
Member

timothysc commented Nov 15, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 15, 2018

@timothysc timothysc added the approved label Nov 15, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 15, 2018

[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:

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 k8s-ci-robot merged commit 3d3cc63 into kubernetes:master Nov 15, 2018

18 checks passed

cla/linuxfoundation liztio 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-100-performance 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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment