Approve Kubelet serving CSR #7

awly · 2018-04-19T18:25:00Z

Validate same as client CSRs, plus check DNSNames SAN.

DNSNames should contain GCE VM name. Approver checks it against cluster
project in VM metadata (customer project, not master project).
Optionally it also checks that VM belongs to cluster's NodePool.

awly · 2018-04-19T18:25:20Z

/assign @mikedanese

awly · 2018-04-19T18:26:14Z

It looks like k8s-ci-robot in this repo doesn't run the tests? cc @calebamiles

mikedanese · 2018-04-19T20:23:19Z

Can you do the vendor changes in a separate commit?

awly · 2018-04-19T21:39:56Z

Remove cluster membership check. Only match csr.Username to CN and check (DNS name + IP address) against GCE API.

Split into vendoring and code commits.

PTAL @mikedanese

mikedanese · 2018-04-19T22:35:03Z

cmd/gke-certificates-controller/app/gke_approver.go

 		return false
 	}
 	if csr.Spec.Username != x509cr.Subject.CommonName {
 		return false
 	}
 	return true
 }
+
+func validateNodeServerCert(opts approverOptions, csr *capi.CertificateSigningRequest, x509cr *x509.CertificateRequest) bool {
+	client, err := google.DefaultClient(context.Background())


Does this have a global token source or do we create one per call?

Good point, this was creating one per call.
Switched to a single shared token source.

mikedanese · 2018-04-19T22:46:55Z

cmd/gke-certificates-controller/app/gke_approver.go

+		for _, iface := range inst.NetworkInterfaces {
+			for _, ip := range x509cr.IPAddresses {
+				if ip.String() == iface.NetworkIP {
+					return nil


why return? don't we meed to validate all ips in the CSR, not just the first?

You're right.
For some reason I thought "if any dns/ip combo passes it's sufficient". Brain fart.

mikedanese · 2018-04-19T22:48:36Z

cmd/gke-certificates-controller/app/gke_approver.go

+	a.zone = strings.TrimSpace(string(val))
+
+	// Get the token source for Application Default Credentials.
+	a.tokenSource, err = google.DefaultTokenSource(context.Background())


Need an AltTokenSource on masters. URLs are in the GCEConfig.

https://github.com/kubernetes/kubernetes/blob/f18a921a0308c4ecd2e7e8b7e30e39a8216a3447/pkg/cloudprovider/providers/gce/token_source.go#L103

Done. Is this for quota or permission reasons?

mikedanese · 2018-04-19T22:51:04Z

cmd/gke-certificates-controller/app/gke_approver.go

+	a.projectID = gceConfig.Global.ProjectID
+
+	// Get cluster zone from metadata server.
+	req, err := http.NewRequest(http.MethodGet, "http://metadata.google.internal/computeMetadata/v1/instance/attributes/cluster-location", nil)


https://godoc.org/cloud.google.com/go/compute/metadata#Get

mikedanese · 2018-04-23T15:34:41Z

cmd/gke-certificates-controller/app/gke_approver.go

 	validators []csrValidator
 }

-func newGKEApprover(client clientset.Interface) *gkeApprover {
+func newGKEApprover(opts approverOptions, client clientset.Interface) *gkeApprover {
 	return &gkeApprover{client: client, validators: validators}


mikedanese · 2018-04-23T15:36:41Z

cmd/gke-certificates-controller/app/gke_approver.go

+		name:       "kubelet server certificate SubjectAccessReview",
+		recognize:  isNodeServerCert,
+		validate:   validateNodeServerCert,
+		permission: authorization.ResourceAttributes{Group: "certificates.k8s.io", Resource: "certificatesigningrequests", Verb: "create", Subresource: "selfnodeserver"},


We might want to lump this with selfnodeclient. I'm not sure if the separate permission buys us much.

mikedanese · 2018-04-23T15:42:12Z

cmd/gke-certificates-controller/app/gke_approver.go

+
+outer:
+	for _, n := range x509cr.DNSNames {
+		inst, err := compute.NewInstancesService(cs).Get(opts.projectID, opts.zone, n).Do()


Move this out of the loop

mikedanese · 2018-04-23T15:43:11Z

cmd/gke-certificates-controller/app/gke_approver.go

+	if len(x509cr.DNSNames) == 0 {
+		return errors.New("no SAN DNS names")
+	}
+	if len(x509cr.IPAddresses) == 0 {


Also consider if len(dnsNames) + len(ipAddrs) == 0 { return err }

Also consider capping the number of SANs at something more than reasonable, e.g. 10 or 20.

Also only allow dns and ip sans.

mikedanese · 2018-04-23T15:47:01Z

cmd/gke-certificates-controller/app/gke_approver.go

+	if err != nil {
+		return a, err
+	}
+	a.zone = strings.TrimSpace(zone)


This isn't always going to be a zone. If it's a regional cluster, this will be a region.

Done.
Loading list of all zones at startup and matching to location by prefix.
If it's a zone, only one will match.
If it's a region, all zones in it will match via prefix in name.

awly · 2018-04-23T20:22:51Z

PTAL @mikedanese

mikedanese · 2018-04-23T20:26:41Z

cmd/gke-certificates-controller/app/gke_approver.go

+	for _, n := range x509cr.DNSNames {
+	inner:
+		for _, z := range opts.zones {
+			inst, err := srv.Get(opts.projectID, z, n).Do()


Can you use the common name to find the instance, and find the instance outside the loop?

I think we need to loop over zones regardless. The checks above limit how many requests we make.

awly · 2018-04-23T20:46:22Z

Changed cluster zones lookup to use VM's region instead of cluster-location.
PTAL

mikedanese · 2018-04-24T00:27:49Z

cmd/gke-certificates-controller/app/gke_approver.go

+	numDNS, numIPs := len(x509cr.DNSNames), len(x509cr.IPAddresses)
+	switch {
+	case numDNS != numIPs:
+		return fmt.Errorf("got %d DNS names and %d IPs, want equal numbers", numDNS, numIPs)


I'm not sure this makes sense. There's aren't 1 to 1.

Done.
Also changed the check below to disallow len(IPs) > 0 && len(DNS) == 0 (yay unit tests!)

Validate same as client CSRs, plus check DNSNames/IPAddresses SAN. DNSNames should contain GCE VM name and IPAddresses should match that VM's IP. Approver checks it against cluster project in VM metadata (customer project, not master project).

awly · 2018-04-24T17:49:05Z

Squashed all the fixes. PTAL

mikedanese · 2018-04-24T20:58:04Z

/lgtm

k8s-ci-robot · 2018-04-24T20:58:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese

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:

~~OWNERS~~ [mikedanese]

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

UPSTREAM: <carry>: GCP CCM fixes

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 19, 2018

k8s-ci-robot requested review from pipejakob and roberthbailey April 19, 2018 18:25

k8s-ci-robot assigned mikedanese Apr 19, 2018

awly force-pushed the node-server-cert branch 2 times, most recently from e4fd823 to d94a8b5 Compare April 19, 2018 21:38

mikedanese reviewed Apr 19, 2018

View reviewed changes

mikedanese reviewed Apr 23, 2018

View reviewed changes

mikedanese reviewed Apr 24, 2018

View reviewed changes

Andrew Lytvynov added 2 commits April 24, 2018 10:48

Approve Kubelet serving CSR

206aff2

Validate same as client CSRs, plus check DNSNames/IPAddresses SAN. DNSNames should contain GCE VM name and IPAddresses should match that VM's IP. Approver checks it against cluster project in VM metadata (customer project, not master project).

Vendor new dependencies

0f313c7

awly force-pushed the node-server-cert branch from 5463eed to 0f313c7 Compare April 24, 2018 17:48

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 24, 2018

k8s-ci-robot merged commit ebdac3f into kubernetes:master Apr 24, 2018

awly deleted the node-server-cert branch April 24, 2018 21:24

awly mentioned this pull request May 24, 2018

gke-certificates-controller: approve kubelet serving certificates #6

Closed

nrb pushed a commit to nrb/cloud-provider-gcp that referenced this pull request Jul 26, 2023

Merge pull request kubernetes#7 from Fedosin/fixes

83bfc8c

UPSTREAM: <carry>: GCP CCM fixes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Approve Kubelet serving CSR #7

Approve Kubelet serving CSR #7

awly commented Apr 19, 2018

awly commented Apr 19, 2018

awly commented Apr 19, 2018

mikedanese commented Apr 19, 2018

awly commented Apr 19, 2018

mikedanese Apr 19, 2018

awly Apr 19, 2018

mikedanese Apr 19, 2018

awly Apr 19, 2018

mikedanese Apr 19, 2018

awly Apr 19, 2018

mikedanese Apr 19, 2018

awly Apr 19, 2018

mikedanese Apr 23, 2018

awly Apr 23, 2018

mikedanese Apr 23, 2018

awly Apr 23, 2018

mikedanese Apr 23, 2018

awly Apr 23, 2018

mikedanese Apr 23, 2018 •

edited

Loading

mikedanese Apr 23, 2018

mikedanese Apr 23, 2018

awly Apr 23, 2018

mikedanese Apr 23, 2018

awly Apr 23, 2018

awly commented Apr 23, 2018

mikedanese Apr 23, 2018

awly Apr 23, 2018

awly commented Apr 23, 2018

mikedanese Apr 24, 2018

awly Apr 24, 2018

awly commented Apr 24, 2018

mikedanese commented Apr 24, 2018

k8s-ci-robot commented Apr 24, 2018

Approve Kubelet serving CSR #7

Approve Kubelet serving CSR #7

Conversation

awly commented Apr 19, 2018

awly commented Apr 19, 2018

awly commented Apr 19, 2018

mikedanese commented Apr 19, 2018

awly commented Apr 19, 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

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

mikedanese Apr 23, 2018 • 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

awly commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly commented Apr 24, 2018

mikedanese commented Apr 24, 2018

k8s-ci-robot commented Apr 24, 2018

mikedanese Apr 23, 2018 •

edited

Loading