migrate group approver to use subject access reviews #45619

mikedanese · 2017-05-10T21:46:13Z

WIP, needs test and changes to kubeadm

depends on #45514

kube-controller-manager has dropped support for the `--insecure-experimental-approve-all-kubelet-csrs-for-group` flag. Instead, the `csrapproving` controller uses authorization checks to determine whether to approve certificate signing requests:
* requests for a TLS client certificate for any node are approved if the CSR creator has `create` permission on the `certificatesigningrequests` resource and `nodeclient` subresource in the `certificates.k8s.io` API group
* requests from a node for a TLS client certificate for itself are approved if the CSR creator has `create` permission on the `certificatesigningrequests` resource and the `selfnodeclient` subresource in the `certificates.k8s.io` API group
* requests from a node for a TLS serving certificate for itself are approved if the CSR creator has `create` permission on the `certificatesigningrequests` resource and the `selfnodeserver` subresource in the `certificates.k8s.io` API group

k8s-reviewable · 2017-05-10T21:46:20Z

This change is

k8s-github-robot · 2017-05-10T21:46:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese

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

~~cmd/gke-certificates-controller/OWNERS~~ [mikedanese]
~~cmd/kube-controller-manager/OWNERS~~ [mikedanese]
~~pkg/apis/componentconfig/OWNERS~~ [mikedanese]
~~pkg/controller/OWNERS~~ [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-05-10T21:46:39Z

cmd/kube-controller-manager/app/options/options.go

@@ -185,7 +185,7 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
 	fs.StringVar(&s.ServiceAccountKeyFile, "service-account-private-key-file", s.ServiceAccountKeyFile, "Filename containing a PEM-encoded private RSA or ECDSA key used to sign service account tokens.")
 	fs.StringVar(&s.ClusterSigningCertFile, "cluster-signing-cert-file", s.ClusterSigningCertFile, "Filename containing a PEM-encoded X509 CA certificate used to issue cluster-scoped certificates")
 	fs.StringVar(&s.ClusterSigningKeyFile, "cluster-signing-key-file", s.ClusterSigningKeyFile, "Filename containing a PEM-encoded RSA or ECDSA private key used to sign cluster-scoped certificates")
-	fs.StringVar(&s.ApproveAllKubeletCSRsForGroup, "insecure-experimental-approve-all-kubelet-csrs-for-group", s.ApproveAllKubeletCSRsForGroup, "The group for which the controller-manager will auto approve all CSRs for kubelet client certificates.")
+	fs.BoolVar(&s.EnableKubeletCSRApprover, "enable-kubelet-csr-approver", s.EnableKubeletCSRApprover, "Enable the kubelet CSR approver which approves kubelet CSRs based on subject access reviews")


Do we need an option for this or can we just leave it on?

When the auto-approving controller is its own controller, you would use --controllers=... to decide whether to run it or not. I wouldn't enable it by default yet. For example, if it was named autoapprove-kubelet-csrs and was not on by default, you would specify --controllers=*,autoapprove-kubelet-csrs to run all on-by-default controllers and the autoapprover.

oh, I see this already splits it out

Ok, I'll drop it.

liggitt · 2017-05-15T14:27:13Z

cmd/kube-controller-manager/app/controllermanager.go

@@ -311,7 +311,8 @@ func NewControllerInitializers() map[string]InitFunc {
 	controllers["disruption"] = startDisruptionController
 	controllers["statefuleset"] = startStatefulSetController
 	controllers["cronjob"] = startCronJobController
-	controllers["certificatesigningrequests"] = startCSRController
+	controllers["certificatesigningrequestssigning"] = startCSRSigningController
+	controllers["certificatesigningrequestsapproving"] = startCSRApprovingController


Changing the controller name needs a release note since people can address it via --controllers. If we're changing it, shorter might be better? Something like csrsigner / csrautoapprover?

liggitt · 2017-05-15T14:28:17Z

pkg/apis/componentconfig/types.go

-	ApproveAllKubeletCSRsForGroup string
+	// enableKubeletCSRApprover tells the CSR controller to approve
+	// kubelet CSRs based on SubjectAccessReviews.
+	EnableKubeletCSRApprover bool


not needed, use controller name in Controllers

liggitt · 2017-05-15T14:30:20Z

pkg/controller/certificates/approver/groupapprove.go

+		utilruntime.HandleError(fmt.Errorf("unable to parse csr %q: %v", csr.Name, err))
+		return csr, nil
+	}
+	if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {


I wouldn't expect this to be true for node serving certs

I attempted to match the behavior here: https://github.com/kubernetes/kubernetes/pull/45059/files#diff-bf28da68f62a8df6e99e447c4351122dR1060

liggitt · 2017-05-15T14:30:36Z

pkg/controller/certificates/approver/groupapprove.go

+	if !reflect.DeepEqual([]string{"system:nodes"}, x509cr.Subject.Organization) {
+		return csr, nil
+	}
+	if len(x509cr.DNSNames)+len(x509cr.EmailAddresses)+len(x509cr.IPAddresses) != 0 {


this will not be true for node serving certs

Same as above. I will adjust when #45059 settles.

liggitt · 2017-05-15T14:52:47Z

pkg/controller/certificates/approver/groupapprove.go

+	if err != nil {
+		utilruntime.HandleError(fmt.Errorf("unable to parse csr %q: %v", csr.Name, err))
+		return csr, nil
+	}


I expected this to be structured more in terms of recognizers and associated permission required.

type CSRRecognizer interface { Matches(csrObj, parsedCSR) bool } recognizers := []struct{ recognizer CSRRecognizer permission authorizer.ResourceAttributes }{ { recognizer: isSelfNodeClientCert, permission: authorizer.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Subresource: "selfnodeclient"}, }, { recognizer: isNodeClientCert, permission: authorizer.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Subresource: "nodeclient"}, }, { recognizer: isSelfNodeClientCert, permission: authorizer.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Subresource: "selfnodeclient"}, }, { recognizer: isNodeClientCert, permission: authorizer.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Subresource: "nodeclient"}, }, } func autoApprove(csrObj) { // short-circuit on pre-approved parsedCSR, err := parse(csrObj) // short-circuit on error for _, r := range recognizers { if !r.recognizer.Matches(csrObj, parsedCSR) { continue } approved, err := authorize(csrObj, r.permission) if err != nil { // accumulate error in case we ultimately fail? continue } if approved { // record approval } } }

liggitt · 2017-05-15T15:09:06Z

pkg/controller/certificates/approver/groupapprove.go

+			return csr, nil
+		}
+	}
+	if hasExactUsages(csr, kubeletServerUsages) {


have you thought about how we'd verify what hostnames and subjectaltnames a kubelet is allowed to ask for in its serving cert? In non-cloud-provider setups, the only info we have about the node is self-reported today in node status (which isn't even available until the kubelet self-registers, which is probably after the serving cert bootstrap). In cloud-provider cases, are we able to ask the cloud provider for the hostnames/dns names/IP addresses for an arbitrary node?

ericchiang · 2017-05-17T19:26:29Z

cc @kubernetes/sig-auth-pr-reviews

closes #45030?

mikedanese · 2017-05-23T01:24:31Z

@ericchiang yest this closes #45030

mikedanese · 2017-05-23T01:33:41Z

pkg/controller/certificates/approver/groupapprove.go

+	if !hasExactUsages(csr, kubeletServerUsages) {
+		return false
+	}
+	//TODO(jcbsmpsn): implement the rest of this


@jcbsmpsn I expect you will want to fill this in once we know how kubelet certs are going to look.

mikedanese · 2017-05-23T22:07:06Z

@liggitt Please rereview this (especially the second commit)

liggitt · 2017-05-29T17:09:58Z

The behavior was alpha and the flag was marked experimental. I'd consider marking the flag as deprecated and making it inert for a release

luxas · 2017-05-30T07:23:38Z

pkg/controller/certificates/approver/sarapprove.go

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


Is there a canonical constant on this anywhere?

luxas · 2017-05-30T07:23:48Z

pkg/controller/certificates/approver/sarapprove.go

+	if !hasExactUsages(csr, kubeletClientUsages) {
+		return false
+	}
+	if !strings.HasPrefix(x509cr.Subject.CommonName, "system:node:") {


...or this?

This existed in the previous code.

luxas · 2017-05-30T07:24:15Z

pkg/controller/certificates/approver/sarapprove.go

+	if !hasExactUsages(csr, kubeletServerUsages) {
+		return false
+	}
+	//TODO(jcbsmpsn): implement the rest of this


critical for v1.7?

No it is not.

luxas · 2017-05-30T07:26:19Z

I'm fine with one release cycle as the deprecation period, so we'll remove this in v1.8 then...

liggitt · 2017-05-30T15:05:34Z

cmd/kube-controller-manager/app/options/options.go

@@ -192,7 +192,6 @@ func (s *CMServer) AddFlags(fs *pflag.FlagSet, allControllers []string, disabled
 	fs.StringVar(&s.ServiceAccountKeyFile, "service-account-private-key-file", s.ServiceAccountKeyFile, "Filename containing a PEM-encoded private RSA or ECDSA key used to sign service account tokens.")
 	fs.StringVar(&s.ClusterSigningCertFile, "cluster-signing-cert-file", s.ClusterSigningCertFile, "Filename containing a PEM-encoded X509 CA certificate used to issue cluster-scoped certificates")
 	fs.StringVar(&s.ClusterSigningKeyFile, "cluster-signing-key-file", s.ClusterSigningKeyFile, "Filename containing a PEM-encoded RSA or ECDSA private key used to sign cluster-scoped certificates")
-	fs.StringVar(&s.ApproveAllKubeletCSRsForGroup, "insecure-experimental-approve-all-kubelet-csrs-for-group", s.ApproveAllKubeletCSRsForGroup, "The group for which the controller-manager will auto approve all CSRs for kubelet client certificates.")


@mikedanese please re-add the flag, make it inert (it doesn't need to bind to anything), mark it as deprecated and describe the replacement mechanism. We'll remove it completely in 1.8.

mikedanese · 2017-05-30T18:26:31Z

since the permission checks are in the core controller manager, I would sort of expect corresponding single-permission clusterroles to match, so that consumers (like kubeadm) would just need to bind one of those roles to their bootstrapper identity or nodes. @deads2k, would that match your expectations?

This is not neccessary to get in on the first pass.

liggitt · 2017-05-30T18:35:07Z

nit on short-circuiting CSRs that already have a cert issued in status, LGTM otherwise as a first-pass

mikedanese · 2017-05-30T18:36:27Z

Ok, will addresss and add lgtm label.

mikedanese · 2017-05-30T18:44:19Z

There is a bit of good feedback that isn't necessary for 1.7 and that I won't be able to get to before code freeze. I opened #46638 to get back to it.

mikedanese · 2017-05-30T20:13:16Z

@k8s-bot pull-kubernetes-unit test this

k8s-github-robot · 2017-05-31T02:48:57Z

Automatic merge from submit-queue (batch tested with PRs 46635, 45619, 46637, 45059, 46415)

liggitt · 2017-05-31T05:28:28Z

thanks, can you open doc PRs against the TLS bootstrapping topic and the authorization topic (to call out the subresources used to authorize)

@pipejakob

Automatic merge from submit-queue (batch tested with PRs 46897, 46899, 46864, 46854, 46875) kubeadm: Make kubeadm use the right CSR approver for the right version **What this PR does / why we need it**: fixes regression caused in: kubernetes#45619 **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes # Fixes: kubernetes/kubeadm#289 **Special notes for your reviewer**: cc @pipejakob our e2e CI should probably go green after this change **Release note**: ```release-note NONE ``` @mikedanese @pipejakob @timothysc @liggitt

mikedanese self-assigned this May 10, 2017

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

mikedanese commented May 10, 2017

View reviewed changes

liggitt self-assigned this May 11, 2017

liggitt reviewed May 15, 2017

View reviewed changes

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label May 17, 2017

mikedanese force-pushed the cert-sar branch from 949d989 to 3e40e22 Compare May 23, 2017 01:21

mikedanese force-pushed the cert-sar branch from 3e40e22 to 39d9424 Compare May 23, 2017 01:31

mikedanese commented May 23, 2017

View reviewed changes

mikedanese force-pushed the cert-sar branch from 39d9424 to bf224cc Compare May 23, 2017 02:37

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 23, 2017

mikedanese force-pushed the cert-sar branch from bf224cc to 92ab528 Compare May 23, 2017 14:43

mikedanese mentioned this pull request May 23, 2017

refactor certificate controller to break it into two parts #45514

Merged

mikedanese force-pushed the cert-sar branch 2 times, most recently from b518a7e to d4f7b99 Compare May 23, 2017 22:06

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

luxas reviewed May 30, 2017

View reviewed changes

luxas modified the milestone: v1.7 May 30, 2017

liggitt reviewed May 30, 2017

View reviewed changes

mikedanese force-pushed the cert-sar branch from 37e7f24 to a78ccb6 Compare May 30, 2017 18:23

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

mikedanese force-pushed the cert-sar branch from a78ccb6 to 041ccf1 Compare May 30, 2017 18:26

mikedanese mentioned this pull request May 30, 2017

enable kubelet csr bootstrap in GCE/GKE #40760

Merged

mikedanese added 2 commits May 30, 2017 11:43

migrate group approver to use subject access reviews

66b4b99

make kubeadm use new sar approver

fb4c020

mikedanese force-pushed the cert-sar branch from 041ccf1 to fb4c020 Compare May 30, 2017 18:43

mikedanese mentioned this pull request May 30, 2017

address outstanding feedback in SAR approver controller review #46638

Closed

mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2017

k8s-github-robot merged commit 4e531f6 into kubernetes:master May 31, 2017

mikedanese deleted the cert-sar branch May 31, 2017 03:18

This was referenced Jun 2, 2017

kubeadm: Make kubeadm use the right CSR approver for the right version #46864

Merged

Deploying v1.6 clusters with kubeadm at HEAD is broken kubernetes/kubeadm#289

Closed

ericchiang mentioned this pull request Jun 21, 2017

move group approver to use a subject access review #45030

Closed

jamiehannaford mentioned this pull request Oct 10, 2017

Key/Cert rotation for Kubernetes clients. #4672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

migrate group approver to use subject access reviews #45619

migrate group approver to use subject access reviews #45619

mikedanese commented May 10, 2017 •

edited by liggitt

Loading

k8s-reviewable commented May 10, 2017

k8s-github-robot commented May 10, 2017

mikedanese May 10, 2017

liggitt May 15, 2017 •

edited

Loading

liggitt May 15, 2017

mikedanese May 15, 2017

liggitt May 15, 2017

liggitt May 15, 2017

liggitt May 15, 2017

mikedanese May 15, 2017

liggitt May 15, 2017

mikedanese May 15, 2017

liggitt May 15, 2017

liggitt May 15, 2017 •

edited

Loading

ericchiang commented May 17, 2017

mikedanese commented May 23, 2017

mikedanese May 23, 2017

mikedanese commented May 23, 2017

liggitt commented May 29, 2017

luxas May 30, 2017

luxas May 30, 2017

mikedanese May 30, 2017

luxas May 30, 2017

mikedanese May 30, 2017

luxas commented May 30, 2017

liggitt May 30, 2017

mikedanese May 30, 2017

mikedanese commented May 30, 2017

liggitt commented May 30, 2017

mikedanese commented May 30, 2017

mikedanese commented May 30, 2017 •

edited

Loading

mikedanese commented May 30, 2017 •

edited by rmmh

Loading

k8s-github-robot commented May 31, 2017

liggitt commented May 31, 2017

migrate group approver to use subject access reviews #45619

migrate group approver to use subject access reviews #45619

Conversation

mikedanese commented May 10, 2017 • edited by liggitt Loading

k8s-reviewable commented May 10, 2017

k8s-github-robot commented May 10, 2017

Choose a reason for hiding this comment

liggitt May 15, 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

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

liggitt May 15, 2017 • edited Loading

Choose a reason for hiding this comment

ericchiang commented May 17, 2017

mikedanese commented May 23, 2017

Choose a reason for hiding this comment

mikedanese commented May 23, 2017

liggitt commented May 29, 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

luxas commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented May 30, 2017

liggitt commented May 30, 2017

mikedanese commented May 30, 2017

mikedanese commented May 30, 2017 • edited Loading

mikedanese commented May 30, 2017 • edited by rmmh Loading

k8s-github-robot commented May 31, 2017

liggitt commented May 31, 2017

mikedanese commented May 10, 2017 •

edited by liggitt

Loading

liggitt May 15, 2017 •

edited

Loading

liggitt May 15, 2017 •

edited

Loading

mikedanese commented May 30, 2017 •

edited

Loading

mikedanese commented May 30, 2017 •

edited by rmmh

Loading