Node authorizer #46076

liggitt · 2017-05-19T05:09:31Z

This PR implements the authorization portion of https://github.com/kubernetes/community/blob/master/contributors/design-proposals/kubelet-authorizer.md and kubernetes/enhancements#279:

Adds a new authorization mode (Node) that authorizes requests from nodes based on a graph of related pods,secrets,configmaps,pvcs, and pvs:
- Watches pods, adds edges (secret -> pod, configmap -> pod, pvc -> pod, pod -> node)
- Watches pvs, adds edges (secret -> pv, pv -> pvc)
- When both Node and RBAC authorization modes are enabled, the default RBAC binding that grants the system:node role to the system:nodes group is not automatically created.
Tightens the NodeRestriction admission plugin to require identifiable nodes for requests from users in the system:nodes group.

This authorization mode is intended to be used in combination with the NodeRestriction admission plugin, which limits the pods and nodes a node may modify. To enable in combination with RBAC authorization and the NodeRestriction admission plugin:

start the API server with --authorization-mode=Node,RBAC --admission-control=...,NodeRestriction,...
start kubelets with TLS boostrapping or with client credentials that place them in the system:nodes group with a username of system:node:<nodeName>

kube-apiserver: a new authorization mode (`--authorization-mode=Node`) authorizes nodes to access secrets, configmaps, persistent volume claims and persistent volumes related to their pods.
* Nodes must use client credentials that place them in the `system:nodes` group with a username of `system:node:<nodeName>` in order to be authorized by the node authorizer (the credentials obtained by the kubelet via TLS bootstrapping satisfy these requirements)
* When used in combination with the `RBAC` authorization mode (`--authorization-mode=Node,RBAC`), the `system:node` role is no longer automatically granted to the `system:nodes` group.

RBAC: the automatic binding of the `system:node` role to the `system:nodes` group is deprecated and will not be created in future releases. It is recommended that nodes be authorized using the new `Node` authorization mode instead. Installations that wish to continue giving all members of the `system:nodes` group the `system:node` role (which grants broad read access, including all secrets and configmaps) must create an installation-specific ClusterRoleBinding.

Follow-up:

~~enable e2e CI environment with admission and authorizer enabled~~ enable kubelet csr bootstrap in GCE/GKE #40760 enable Node authorizer and NodeRestriction admission controller #46796
~~enable this authorizer and admission plugin in kubeadm~~ kubeadm: Enable the Node Authorizer/Admission plugin in v1.7 #46879
~~enable this authorizer and admission plugin in kube-up~~ enable Node authorizer and NodeRestriction admission controller #46796
~~scale-test in kubemark~~ Enable Node authorizer and NodeRestriction admission in kubemark #46921

ncdc · 2017-05-19T13:08:39Z

plugin/pkg/auth/authorizer/node/graph.go

+
+const (
+	ConfigMapNode nodeType = iota
+	NodeNode      nodeType = iota


iota copypasta?

ncdc · 2017-05-19T13:12:17Z

plugin/pkg/auth/authorizer/node/graph_populator.go

+type GraphPopulator struct {
+	graph *Graph
+
+	podInformer              cache.SharedIndexInformer


We generally haven't been storing refs to the underlying cache.SharedIndexInformer objects. If you're using a shared informer, it's generally safe to assume that something else (e.g. kube-controller-manager) will start it.

ncdc · 2017-05-19T13:13:30Z

plugin/pkg/auth/authorizer/node/graph_populator.go

+}
+
+func (g *GraphPopulator) Run(stopCh <-chan struct{}) {
+	g.podInformer.GetController().Run(stopCh)


These 2 lines are likely no-ops (see my comment above,

kubernetes/staging/src/k8s.io/client-go/tools/cache/shared_informer.go

Lines 261 to 263 in 44127c5

func (s *sharedIndexInformer) GetController() Controller {

return &dummyController{informer: s}

}

, and

kubernetes/staging/src/k8s.io/client-go/tools/cache/shared_informer.go

Line 161 in 44127c5

func (v *dummyController) Run(stopCh <-chan struct{}) {

).

ncdc · 2017-05-19T13:14:04Z

plugin/pkg/auth/authorizer/node/graph_populator_pod.go

+	}
+	if oldObj != nil && pod.Spec.NodeName == oldObj.(*api.Pod).Spec.NodeName {
+		// Node is unchanged
+		// TODO: do we need to check for uid changes?


I would say yes

liggitt · 2017-05-20T03:38:11Z

Comments addressed. Still working on tests, and I'll have some follow-ups for performance/memory related improvements to the graph, but the authorizer and graph population code is ready for review

0xmichalis · 2017-05-20T16:58:30Z

third_party/forked/gonum/graph/README.md

@@ -0,0 +1 @@
+Forked from gonum/graph@50b27dea7ebbfb052dfaf91681afc6fde28d8796 to support memory-use improvements to the simple graph


Any plans to upstream the changes?

Absolutely, I just don't want to block on the upstream PRs being accepted and vendorable prior to code freeze

liggitt · 2017-05-30T19:36:34Z

rebased, comments addressed. will look more into graph space optimizations in 1.8 timeframe.

smarterclayton · 2017-05-30T19:55:29Z

No other comments from me

/approve

Pending other reviewers sign off

timstclair · 2017-05-30T20:05:20Z

LGTM, but I'll avoid adding the label to let other reviewers respond. Feel free to self apply once they have.

ericchiang · 2017-05-30T20:07:24Z

/lgtm

k8s-github-robot · 2017-05-30T20:07:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ericchiang, liggitt, smarterclayton

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

~~OWNERS~~ [smarterclayton]

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

liggitt · 2017-05-30T20:53:32Z

fixed lint issue, retagging

liggitt · 2017-05-30T22:02:00Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

liggitt · 2017-05-31T05:23:52Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-github-robot · 2017-05-31T05:42:54Z

Automatic merge from submit-queue

k8s-ci-robot · 2017-05-31T05:48:11Z

@liggitt: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`fc8e915`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

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.

Automatic merge from submit-queue (batch tested with PRs 49619, 49598, 47267, 49597, 49638) Remove default binding of system:node role to system:nodes group part of kubernetes/enhancements#279 deprecation of this automatic binding announced in 1.7 in #46076 ```release-note RBAC: the `system:node` role is no longer automatically granted to the `system:nodes` group in new clusters. It is recommended that nodes be authorized using the `Node` authorization mode instead. Installations that wish to continue giving all members of the `system:nodes` group the `system:node` role (which grants broad read access, including all secrets and configmaps) must create an installation-specific `ClusterRoleBinding`. ```

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

k8s-github-robot assigned erictune and dchen1107 May 19, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 19, 2017

ncdc reviewed May 19, 2017

View reviewed changes

plugin/pkg/auth/authorizer/node/graph.go Outdated

const (

ConfigMapNode nodeType = iota

NodeNode nodeType = iota

Copy link

Member

ncdc May 19, 2017

Choose a reason for hiding this comment

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

iota copypasta?

ncdc reviewed May 19, 2017

View reviewed changes

liggitt force-pushed the node-authorizer branch 3 times, most recently from 7d81814 to 9f152be Compare May 20, 2017 03:28

liggitt changed the title ~~WIP - Node authorizer~~ Node authorizer May 20, 2017

liggitt force-pushed the node-authorizer branch from 9f152be to 9c1d319 Compare May 20, 2017 03:51

0xmichalis reviewed May 20, 2017

View reviewed changes

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

liggitt force-pushed the node-authorizer branch from 9c1d319 to 915d215 Compare May 22, 2017 00:54

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

liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 23, 2017

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

liggitt force-pushed the node-authorizer branch from 915d215 to dc890a3 Compare May 23, 2017 04:22

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

liggitt force-pushed the node-authorizer branch 2 times, most recently from fc2aa23 to 90da41d Compare May 23, 2017 05:17

liggitt assigned ericchiang, timstclair and deads2k and unassigned dchen1107 May 24, 2017

Add PV util for extracting referenced secrets

48a9357

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

liggitt added 2 commits May 30, 2017 15:14

Separate system:node rules

d278a80

Make NodeRestriction admission require identifiable nodes

fed3004

liggitt force-pushed the node-authorizer branch from 245e2a4 to 49ed2d1 Compare May 30, 2017 19:28

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

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2017

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

liggitt force-pushed the node-authorizer branch from 49ed2d1 to fc8e915 Compare May 30, 2017 20:52

Add Node authorization mode based on graph of node-related objects

fc8e915

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2017

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

liggitt added this to the v1.7 milestone May 31, 2017

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

saad-ali mentioned this pull request Jun 1, 2017

StorageOS Volume Plugin #42156

Merged

liggitt deleted the node-authorizer branch June 2, 2017 02:34

liggitt mentioned this pull request Jul 26, 2017

Remove default binding of system:node role to system:nodes group #49638

Merged

scfast mentioned this pull request Apr 30, 2023

1.2.7 Ensure that the --authorization-mode argument includes Node sandevistan-server-hardening/CIS_Kubernetes_Benchmark_v1.7.0#24

Open

carlory mentioned this pull request Apr 30, 2024

remove resourceclaims resource from the system:node clusterrole #124472

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node authorizer #46076

Node authorizer #46076

liggitt commented May 19, 2017 •

edited

ncdc May 19, 2017

ncdc May 19, 2017

ncdc May 19, 2017

ncdc May 19, 2017

liggitt commented May 20, 2017

0xmichalis May 20, 2017

liggitt May 20, 2017

liggitt commented May 30, 2017

smarterclayton commented May 30, 2017

timstclair commented May 30, 2017

ericchiang commented May 30, 2017

k8s-github-robot commented May 30, 2017

liggitt commented May 30, 2017

liggitt commented May 30, 2017

liggitt commented May 31, 2017

k8s-github-robot commented May 31, 2017

k8s-ci-robot commented May 31, 2017

	func (s *sharedIndexInformer) GetController() Controller {
	return &dummyController{informer: s}
	}

		@@ -0,0 +1 @@
		Forked from gonum/graph@50b27dea7ebbfb052dfaf91681afc6fde28d8796 to support memory-use improvements to the simple graph

Node authorizer #46076

Node authorizer #46076

Conversation

liggitt commented May 19, 2017 • edited

ncdc May 19, 2017

Choose a reason for hiding this comment

ncdc May 19, 2017

Choose a reason for hiding this comment

ncdc May 19, 2017

Choose a reason for hiding this comment

ncdc May 19, 2017

Choose a reason for hiding this comment

liggitt commented May 20, 2017

0xmichalis May 20, 2017

Choose a reason for hiding this comment

liggitt May 20, 2017

Choose a reason for hiding this comment

liggitt commented May 30, 2017

smarterclayton commented May 30, 2017

timstclair commented May 30, 2017

ericchiang commented May 30, 2017

k8s-github-robot commented May 30, 2017

liggitt commented May 30, 2017

liggitt commented May 30, 2017

liggitt commented May 31, 2017

k8s-github-robot commented May 31, 2017

k8s-ci-robot commented May 31, 2017

liggitt commented May 19, 2017 •

edited