Run node authorizer in kops-controller #7780

justinsb · 2019-10-14T03:36:36Z

We create a GRPC interface and use it to serve the kubelet bootstrap
certificate; we consume it directly from nodeup.

Based heavily on the existing AWS node-authorizer implementation.

k8s-ci-robot · 2019-10-14T03:37:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb

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~~ [justinsb]

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

johngmyers · 2020-02-02T18:58:28Z

This should update docs/architecture/kops-controller.md to document the new service provided by kops-controller.

justinsb · 2020-02-16T20:27:13Z

Thanks for the suggestion @johngmyers - added a small section to the architecture doc. As we add rules, we might want to fill in the details of how node validation is performed.

Right now, there is no node validation, and therefore this is very much not enabled by default. Nonetheless, I'm marking not-WIP because I think it is the first step to enabling a lot of the other functionality (a lot of my other WIP PRs).

johngmyers

In general, I am against adding "trust everything" approvers. They are, in the best case, useless. How can we be confident we have the machinery to do a reasonable validation without some code that can validate something?

johngmyers · 2020-02-16T20:36:55Z

cmd/kops-controller/pkg/nodebootstrap/client/client.go

+func (c *nodeBootstrapClient) Close() error {
+	if c.connection != nil {
+		if err := c.connection.Close(); err != nil {
+			return fmt.Errorf("error closing GRPC connection: %v", err)


Why is the style to use this instead of errors.Wrap()?

Yeah - xerrors is now looking good, so maybe we should do that. I've started using it in new go projects, but I'm not sure whether it's time to put it into bigger existing projects (like kops) or whether we should wait for official support in go.

johngmyers · 2020-02-16T20:56:40Z

nodeup/pkg/model/kops_controller.go

+		Path:     filepath.Join(pkiDir, "server.key"),
+		Contents: fi.NewBytesResource(pkiutil.EncodePrivateKeyPEM(serverKey)),
+		Type:     nodetasks.FileType_File,
+		Mode:     s("0600"),


Do you need to specify the file owner?

Looks like it's fixed.

johngmyers · 2020-02-16T21:54:13Z

cmd/kops-controller/pkg/nodebootstrap/client/client.go

+	Server string `json:"server"`
+
+	// CACertificate is the CA certificate for the GRPC server
+	CACertificate []byte `json:"caCertificate"`


Consider making this a list of CAs certificates, to permit rotation.

Actually looks like it already does :-)

But renaming to CACertificates to make this more self-documenting.

We should probably try to plumb this through the rest of the system though - we haven't totally ignored it, but I'm reasonably sure there are a few places where we likely drop the ball.

johngmyers · 2020-02-16T21:57:07Z

cmd/kops-controller/pkg/nodebootstrap/client/client.go

+}
+
+func (c *nodeBootstrapClient) CreateKubeletBootstrapToken(ctx context.Context) (pb.Token, error) {
+	request := &pb.CreateKubeletBootstrapTokenRequest{}


I don't see how the server would be able to authorize anything without some sort of cloudprovider-specific credential in the request.

Right - so we have NodeInfo which is empty today, but is where I think we can put this information.

The ideal is TPM, which GCE has, but AFAIK no other clouds do. But pkg/authorizers/aws/aws.go has some good checks based on the AWS identity document, and we can also cross-check against the IP address.

One cloud provider is enough; doesn't matter which. We should at least have the structure for where the provider-specific code goes.

johngmyers · 2020-02-16T21:58:18Z

cmd/kops-controller/pkg/nodebootstrap/client/client.go

+
+	if response.Token == nil || response.Token.BearerToken == "" {
+		return pb.Token{}, fmt.Errorf("created bootstrap token, but response was empty")
+	}


I was hoping this would also return a TLS server certificate for the kubelet, but I guess that's another PR.

So the process here is sort of tricky - we're creating a bootstrap token and then the kubelet exchanges that for a kubelet server certificate.

I did debate just generating the kubelet server certificate; a few things made me not do it:

It's different; so we'd have to think more carefully about our failure scenarios. Here we're just doing the normal bootstrap flow - arguably a more secure version of it because we can use shorter-lived tokens.

I wasn't sure if kubelet certiificate rotation worked if we didn't start with a bootstrap token.

This way kops-controller doesn't need the CA keypair. (Although we could always do a CSR flow to avoid that)

We can always generate a kubelet certificate directly in future, though - it wouldn't be a huge change!

We could do a CSR flow with a pre-approved request, to allow for external or hardware-key signing controllers.

johngmyers · 2020-02-16T22:04:51Z

cmd/kops-controller/pkg/nodebootstrap/server.go

+		return nil, err
+	}
+
+	// @step: add the secret to the namespace


Why is this storing the created token in a Secret? I'm concerned a compromised node could impersonate a node in a more privileged instancegroup. Or a pod with permission to read these could escalate to impersonating a node.

Answering narrowly, this is how bootstrap tokens work.

Broadly, I do think your concerns are valid. We can make our bootstrap tokens short lived, we can use the NodeAuthorizer to reduce impact, but it's still a concern. The k8s secret also has all the information, whereas presumably it would be safer to store only the hash of the token (requiring an attacker to find a hash collision in a relatively short TTL).

Perhaps then we should move to just generating the kubelet TLS certificate, and trying to figure out what happens during rotation... WDYT?

Short lived doesn't help, as the compromised node could presumably set a watch for kube-system secrets and snatch it almost as soon as it's created.

But yes, this weakness appears to be designed in upstream of us. Just generating the TLS certificate might be simpler.

johngmyers · 2020-02-16T22:14:19Z

upup/models/cloudup/resources/addons/kops-controller.addons.k8s.io/k8s-1.16.yaml.template

+
+---
+
+# permits the node access to create a CSR


What is using these three ClusterRoleBindings?

Ugh .. good point on the RoleBinding to kops-controller - we should always have those events permissions in kops-controller, at least.

On the ClusterRoleBindings, these are the magic ClusterRoleBindings that (1) allow a kubelet bootstrap token to create a CSR request, and (2) tell kube-controller-manager to auto-approve CSR requests and (3) tell kube-controller-manager to auto approve CSR renewal requests. Some docs are here: https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet-tls-bootstrapping/#approval

If the user/group isn't specified, don't try to change it.

kops-controller now exposes a GRPC service, which is used to more securely provide nodes their kubelet bootstrap tokens.

johngmyers · 2020-02-16T22:40:28Z

pkg/proto/nodebootstrap/bootstrap.proto

+  NodeInfo node_info = 1;
+}
+
+// NodeInfo allows a node to provide any secrets etc that can help verify its identity


A better name would be NodeCredential

Will change - thank you!

johngmyers · 2020-02-16T23:22:00Z

upup/pkg/fi/nodeup/nodetasks/service.go

-			klog.Warningf("Unhandled type %T in Service::GetDependencies: %v", v, v)
-			deps = append(deps, v)
+			switch fmt.Sprintf("%T", v) {
+			case "*model.KubeletBootstrapKubeconfigTask":


Why not add this type to the outer switch? And why test the type by converting to string?

I'll add a comment, but it's because of go packages not allowing circular references..

johngmyers · 2020-02-17T00:49:54Z

/test pull-kops-verify-staticcheck

justinsb · 2020-02-17T03:12:37Z

I can certainly work on adding more trust code. We do have the AWS trust code already from the node-authorizer - it's based on the instance identity document. GCE has vTPM which ideally we would use. But we also have some similar logic when we map the instance to a Node.

In general I think we would only turn this on for platforms where we could make a reasonable case

For clouds that will be based on cross-checking with the control plane and verifying some form of cloud-provided identifier or authentication.

For metal I imagine it'll ideally be based on TPM, but will probably also be based on some pre-shared per-host key (maybe even the SSH hostkey, or at least something similar?)

I can certainly work on implementing some of these authorizers e.g. for AWS. I'll do that in a separate PR that builds on this.

Honestly, your concerns about bootstrap tokens are well made and are more concerning in my mind :-) I'm going to look at generating the kubelet certificate directly.

justinsb · 2020-02-17T13:09:21Z

I had a go at using client-certificates instead of bootstrap tokens and it does work: #8580

I also was able to validate certificate rotation by setting kubeControllerManager: experimentalClusterSigningDuration: 10m0s. For kubelet to rotate its certificates using the CSR flow, we must also pass --rotate-certificates to kubelet. (I added #8581 to map that flag, regardless of whether we need it here) It looks like the CSR request uses the system:certificates.k8s.io:certificatesigningrequests:selfnodeclient, which is much more narrowly scoped than system:certificates.k8s.io:certificatesigningrequests:nodeclient. I think we would not need the system:certificates.k8s.io:certificatesigningrequests:nodeclient binding.

This does raise questions though:

Should we use the CSR flow when generating certificates, so we don't need to map the ca.key into kops-controller (Answer: probably, as pointed out by @johngmyers )
Do we want to do the kubelet bootstrap & rotation flows at all? Should nodeup just write the kubelet kubeconfig and be done with it? We would presumably need another controller to terminate any nodes that have been up for e.g. 360 days, assuming their cert is going to run out in 365 days. This controller might be a nice thing to have - I know some people deliberately rotate their nodes every week; it seems like a good idea if your workloads can tolerate it.
An alternative would be to make nodeup kick-in before the year was up, but I don't know that we need a (semi) persistent nodeup for anything else.

johngmyers · 2020-02-17T18:02:53Z

Some people might want to reduce their cert lifetimes to be less than a year. But it is an interesting observation that the node lifetime should probably be less than the cert lifetime.

Any node expiration mechanism should respect the concurrency and cluster validation limitations that rolling update uses. This is the primary motivation for #8272—to coordinate node expiration with rolling updates for other reasons by having the node expiration controller signal to the rolling update controller to remove the nodes.

I would also like to issue kubelet a TLS server certificate. The rotation approver won't approve server certs because it doesn't have the cloud-provider-specific information to validate the domains. kops-controller could reasonably do so.

k8s-ci-robot · 2020-02-17T18:03:01Z

@justinsb: PR needs rebase.

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.

johngmyers · 2020-08-19T03:33:29Z

@justinsb can this be closed?

k8s-ci-robot · 2021-01-11T11:53:47Z

@justinsb: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kops-verify-staticcheck	`37a4960`	link	`/test pull-kops-verify-staticcheck`
pull-kops-e2e-cni-calico	`37a4960`	link	`/test pull-kops-e2e-cni-calico`
pull-kops-e2e-cni-cilium	`37a4960`	link	`/test pull-kops-e2e-cni-cilium`
pull-kops-e2e-cni-amazonvpc	`37a4960`	link	`/test pull-kops-e2e-cni-amazonvpc`
pull-kops-e2e-cni-flannel	`37a4960`	link	`/test pull-kops-e2e-cni-flannel`
pull-kops-e2e-cni-weave	`37a4960`	link	`/test pull-kops-e2e-cni-weave`
pull-kops-e2e-cni-kuberouter	`37a4960`	link	`/test pull-kops-e2e-cni-kuberouter`
pull-kops-verify-cloudformation	`37a4960`	link	`/test pull-kops-verify-cloudformation`
pull-kops-verify-hashes	`37a4960`	link	`/test pull-kops-verify-hashes`
pull-kops-e2e-k8s-containerd	`37a4960`	link	`/test pull-kops-e2e-k8s-containerd`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

olemarkus · 2021-01-18T13:49:33Z

I think we can call a timeout on this one :)

/close

k8s-ci-robot · 2021-01-18T13:49:47Z

@olemarkus: Closed this PR.

In response to this:

I think we can call a timeout on this one :)

/close

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.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Oct 14, 2019

k8s-ci-robot requested review from granular-ryanbonham and robinpercy October 14, 2019 03:36

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2019

justinsb added this to the 1.17 milestone Oct 14, 2019

justinsb force-pushed the authorizer_in_kops_controller branch from 4d941f2 to 7da8906 Compare November 29, 2019 14:55

justinsb force-pushed the authorizer_in_kops_controller branch from 7da8906 to 6eac1ff Compare December 22, 2019 17:39

justinsb force-pushed the authorizer_in_kops_controller branch 2 times, most recently from 8e42c70 to 9476cbf Compare January 26, 2020 16:13

justinsb force-pushed the authorizer_in_kops_controller branch 2 times, most recently from 77b7233 to 88c7f96 Compare February 16, 2020 20:25

justinsb changed the title ~~WIP: Run node authorizer in kops-controller~~ Run node authorizer in kops-controller Feb 16, 2020

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 Feb 16, 2020

justinsb added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 16, 2020

johngmyers reviewed Feb 16, 2020

View reviewed changes

justinsb added 4 commits February 16, 2020 17:24

EnsureFileOwner: cleanup logic

75713f4

If the user/group isn't specified, don't try to change it.

Allow kops-controller to act as node-authorizer for GCE

7bb774b

kops-controller now exposes a GRPC service, which is used to more securely provide nodes their kubelet bootstrap tokens.

Autogen: protobuf, bazel, tests

73359f8

vendor changes

37a4960

justinsb force-pushed the authorizer_in_kops_controller branch from 88c7f96 to 37a4960 Compare February 16, 2020 22:25

johngmyers reviewed Feb 16, 2020

View reviewed changes

justinsb mentioned this pull request Feb 17, 2020

WIP: Bootstrapping using client certificates #8580

Closed

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

justinsb modified the milestones: v1.17, v1.19 Apr 10, 2020

hakman mentioned this pull request Aug 28, 2020

Move from debian-hyperkube-base to debian-base for node-authorizer #9828

Merged

k8s-ci-robot closed this Jan 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run node authorizer in kops-controller #7780

Run node authorizer in kops-controller #7780

justinsb commented Oct 14, 2019

k8s-ci-robot commented Oct 14, 2019

johngmyers commented Feb 2, 2020

justinsb commented Feb 16, 2020

johngmyers left a comment

johngmyers Feb 16, 2020

justinsb Feb 17, 2020 •

edited

Loading

johngmyers Feb 16, 2020

johngmyers Feb 16, 2020

johngmyers Feb 16, 2020

justinsb Feb 17, 2020

johngmyers Feb 16, 2020

justinsb Feb 17, 2020

johngmyers Feb 17, 2020

johngmyers Feb 16, 2020

justinsb Feb 17, 2020

johngmyers Feb 17, 2020

johngmyers Feb 16, 2020

justinsb Feb 17, 2020

johngmyers Feb 17, 2020

johngmyers Feb 16, 2020

justinsb Feb 17, 2020

johngmyers Feb 16, 2020

justinsb Feb 17, 2020

johngmyers Feb 16, 2020

justinsb Feb 17, 2020

johngmyers commented Feb 17, 2020

justinsb commented Feb 17, 2020

justinsb commented Feb 17, 2020

johngmyers commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

johngmyers commented Aug 19, 2020

k8s-ci-robot commented Jan 11, 2021

olemarkus commented Jan 18, 2021

k8s-ci-robot commented Jan 18, 2021

Run node authorizer in kops-controller #7780

Run node authorizer in kops-controller #7780

Conversation

justinsb commented Oct 14, 2019

k8s-ci-robot commented Oct 14, 2019

johngmyers commented Feb 2, 2020

justinsb commented Feb 16, 2020

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justinsb Feb 17, 2020 • 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

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

johngmyers commented Feb 17, 2020

justinsb commented Feb 17, 2020

justinsb commented Feb 17, 2020

johngmyers commented Feb 17, 2020

k8s-ci-robot commented Feb 17, 2020

johngmyers commented Aug 19, 2020

k8s-ci-robot commented Jan 11, 2021

olemarkus commented Jan 18, 2021

k8s-ci-robot commented Jan 18, 2021

justinsb Feb 17, 2020 •

edited

Loading