Skip to content
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

Auto approve kubelet server certificate signing requests. #46884

Conversation

jcbsmpsn
Copy link
Contributor

@jcbsmpsn jcbsmpsn commented Jun 2, 2017

Fixes #47208

Release note:

Adds an approval work flow to the the certificate approver that will approve certificate signing requests from kubelets that meet all the criteria of kubelet server certificates.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 2, 2017
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 2, 2017
capi.UsageServerAuth,
}

func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't check it is from a node

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from fb0968a to 0aebbe3 Compare June 5, 2017 17:09
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 5, 2017
@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 0aebbe3 to 1e4a7ef Compare June 6, 2017 18:45
@@ -187,14 +187,25 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert

var kubeletServerUsages = []capi.KeyUsage{
capi.UsageKeyEncipherment,
capi.UsageDigitalSignature,
capi.UsageSigning,
capi.UsageServerAuth,
}

func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feature flag this so it returns false if the kubelet server cert rotation feature is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@mikedanese
Copy link
Member

mikedanese commented Jun 7, 2017

You need this:

diff --git a/cluster/addons/rbac/kubelet-certificate-management.yaml b/cluster/addons/rbac/kubelet-certificate-management.yaml
index 4b4b3d7381..83f1187d57 100644
--- a/cluster/addons/rbac/kubelet-certificate-management.yaml
+++ b/cluster/addons/rbac/kubelet-certificate-management.yaml
@@ -57,5 +57,6 @@ rules:
   - "certificates.k8s.io"
   resources:
   - certificatesigningrequests/selfnodeclient
+  - certificatesigningrequests/selfnodeserver
   verbs:
   - "create"

@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 1e4a7ef to 993a783 Compare June 8, 2017 04:02
capi.UsageServerAuth,
}

func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
if !hasExactUsages(csr, kubeletServerUsages) {
if utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invert this. If feature not enabled return false.

@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 993a783 to b34dc6e Compare June 9, 2017 14:44
@mikedanese
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017
@mikedanese
Copy link
Member

/approve

@kubernetes kubernetes deleted a comment from k8s-github-robot Jun 9, 2017
//
// Signing allows the certificate to be used to verify
// digital signatures used during TLS negotiation.
certificates.UsageSigning,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what key usage or ext key usage does this correspond to?

Copy link
Member

@mikedanese mikedanese Jun 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, digital signature...

	"signing":             x509.KeyUsageDigitalSignature,
	"digital signature":   x509.KeyUsageDigitalSignature,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we actually change the kubelet to use the usage that corresponds to the RFC? "signing" is pretty ambiguous

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to UsageDigitalSignature is both locations.

@@ -187,14 +189,28 @@ func isSelfNodeClientCert(csr *capi.CertificateSigningRequest, x509cr *x509.Cert

var kubeletServerUsages = []capi.KeyUsage{
capi.UsageKeyEncipherment,
capi.UsageDigitalSignature,
capi.UsageSigning,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer this to keep the usage that matches the term used in the RFC

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed back to UsageDigitalSignature.

@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from b34dc6e to dacfede Compare June 9, 2017 17:56
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017
@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from dacfede to d2f9643 Compare June 9, 2017 18:57
@mikedanese
Copy link
Member

mikedanese commented Jun 9, 2017

/lgtm
/assign @derekwaynecarr
for kubelet approval

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017
@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from d2f9643 to 0b4f825 Compare June 9, 2017 20:39
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017
Copy link

@timstclair timstclair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@timstclair
Copy link

/approve

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2017
@jcbsmpsn
Copy link
Contributor Author

/unassign @derekwaynecarr

@@ -1124,16 +1124,25 @@ func initializeServerCertificateManager(kubeClient clientset.Interface, kubeCfg
CertificateSigningRequestClient: certSigningRequestClient,
Template: &x509.CertificateRequest{
Subject: pkix.Name{
CommonName: string(nodeName),
CommonName: fmt.Sprintf("system:node:%s", nodeName),
Copy link
Member

@liggitt liggitt Jun 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really like to see this change make 1.7. I'd prefer not to have a mix of CSR formats to recognize unnecessarily

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on it.

@mikedanese
Copy link
Member

@dchen1107 this fixes the alpha e2e tests and only touches alpha codepaths. Please add 1.7 milestone

@mikedanese mikedanese changed the title Auto approve kubelet certificate signing requests. Auto approve kubelet server certificate signing requests. Jun 13, 2017
@mikedanese mikedanese added this to the v1.7 milestone Jun 15, 2017
@mikedanese
Copy link
Member

We need this to fix broken e2es.

)

func init() {
utilfeature.DefaultFeatureGate.Set(string(features.RotateKubeletServerCertificate) + "=true")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't expecting this... the approver is permission based... I only expected the kubelet rotation to be gated by the flag. Also, setting this one way in an init() function means it is always enabled when testing, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is only enabled in this package test, yes.

@@ -192,9 +194,23 @@ var kubeletServerUsages = []capi.KeyUsage{
}

func isSelfNodeServerCert(csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
if !utilfeature.DefaultFeatureGate.Enabled(features.RotateKubeletServerCertificate) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expect the approver to be flag gated this way, only the kubelet function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you want to gate it in the apiserver, gate adding isSelfNodeServerCert in recognizers(), and you can unit test isSelfNodeServerCert without messing with feature flag enablement in init()

Copy link
Member

@mikedanese mikedanese Jun 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommended to @jcbsmpsn that we do this. I really don't want to be tied to the behavior of isSelfNodeServer until we have thought through kubelets self reporting their SANs. What we currently allow is too weak, and this is an API.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, then move the gate to recognizers(), and remove the gate-twiddling from the init()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds good to me.

@dchen1107
Copy link
Member

I am fine with kubelet change, which is trivial anyway. But I am not comfortable with the changes in pkg/controller/certificates/approver simply due to the lack of the knowledge from my side. I will leave @liggitt to make the final call.

@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from 0b4f825 to e1b05e0 Compare June 15, 2017 22:14
@liggitt
Copy link
Member

liggitt commented Jun 16, 2017

I0615 15:22:27.510] Verifying hack/make-rules/../../hack/verify-bazel.sh
I0615 15:22:33.070] --- /tmp/actual-file-768259978	2017-06-15 15:22:33.058595771 -0700
I0615 15:22:33.070] +++ /tmp/expected-file-304640865	2017-06-15 15:22:33.058595771 -0700
I0615 15:22:33.070] @@ -17,10 +17,8 @@
I0615 15:22:33.071]          "//pkg/apis/authorization/v1beta1:go_default_library",
I0615 15:22:33.071]          "//pkg/apis/certificates/v1beta1:go_default_library",
I0615 15:22:33.071]          "//pkg/client/clientset_generated/clientset/fake:go_default_library",
I0615 15:22:33.071] -        "//pkg/features:go_default_library",
I0615 15:22:33.071]          "//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
I0615 15:22:33.072]          "//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
I0615 15:22:33.072] -        "//vendor/k8s.io/apiserver/pkg/util/feature:go_default_library",
I0615 15:22:33.072]          "//vendor/k8s.io/client-go/testing:go_default_library",
I0615 15:22:33.072]      ],
I0615 15:22:33.072]  )
W0615 15:22:33.173] DRY-RUN: wrote "pkg/controller/certificates/approver/BUILD"
W0615 15:22:33.728] 
W0615 15:22:33.728] 1 BUILD files not up-to-date.
I0615 15:22:33.828] 
I0615 15:22:33.829] Run ./hack/update-bazel.sh
I0615 15:22:33.829] FAILED   hack/make-rules/../../hack/verify-bazel.sh	6s

@jcbsmpsn jcbsmpsn force-pushed the autoapprover-kubelet-server-certificate branch from e1b05e0 to 334de1c Compare June 16, 2017 15:47
@mikedanese
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2017
@liggitt
Copy link
Member

liggitt commented Jun 16, 2017

/lgtm

function is gated and off by default, and this helps fix the alpha features CI

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcbsmpsn, liggitt, mikedanese, timstclair

Associated issue: 47208

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

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

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46884, 47557)

@k8s-github-robot k8s-github-robot merged commit aa7458a into kubernetes:master Jun 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants