Merge pull request #143445 from natasha41575/node_authorizer_graph

auth/node: update NodeAuthorizer graph edges when pod volumes change

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

@@ -351,7 +351,7 @@ func (g *Graph) recomputeDestinationIndexLocked(n graph.Node) {
 }
 
 // AddPod should only be called once spec.NodeName is populated.
-// It sets up edges for the following relationships (which are immutable for a pod once bound to a node):
+// It sets up edges for the following relationships:
 //
 //	pod       -> node
 //	secret    -> pod

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

@@ -25,6 +25,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	resourceapi "k8s.io/api/resource/v1"
 	storagev1 "k8s.io/api/storage/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -170,13 +171,11 @@ func (g *graphPopulator) updatePod(oldObj, obj interface{}) {
 		return
 	}
 	if oldPod, ok := oldObj.(*corev1.Pod); ok && oldPod != nil {
-		// Ephemeral containers can add new secret or config map references to the pod.
-		hasNewEphemeralContainers := len(pod.Spec.EphemeralContainers) > len(oldPod.Spec.EphemeralContainers)
 		if (pod.Spec.NodeName == oldPod.Spec.NodeName) && (pod.UID == oldPod.UID) &&
-			!hasNewEphemeralContainers &&
+			!podSpecChanged(oldPod, pod) &&
 			resourceclaim.PodStatusEqual(oldPod.Status.ResourceClaimStatuses, pod.Status.ResourceClaimStatuses) &&
 			resourceclaim.PodExtendedStatusEqual(oldPod.Status.ExtendedResourceClaimStatus, pod.Status.ExtendedResourceClaimStatus) {
-			// Node and uid are unchanged, all object references in the pod spec are immutable respectively unmodified (claim statuses).
+			// Node and uid are unchanged, and all object references in the pod spec and status are unmodified.
 			klog.V(5).Infof("updatePod %s/%s, node unchanged", pod.Namespace, pod.Name)
 			return
 		}
@@ -185,6 +184,22 @@ func (g *graphPopulator) updatePod(oldObj, obj interface{}) {
 	g.podQueue.Add(types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name})
 }
 
+// podSpecChanged returns true if the pod spec changed in a way that can affect
+// object references in the authorization graph.
+func podSpecChanged(oldPod, newPod *corev1.Pod) bool {
+	if newPod.Generation == oldPod.Generation {
+		return false
+	}
+	// Ephemeral containers can add new secret or config map references to the pod.
+	if len(newPod.Spec.EphemeralContainers) > len(oldPod.Spec.EphemeralContainers) {
+		return true
+	}
+	if !apiequality.Semantic.DeepEqual(oldPod.Spec.Volumes, newPod.Spec.Volumes) {
+		return true
+	}
+	return false
+}
+
 func (g *graphPopulator) deletePod(obj interface{}) {
 	if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok {
 		obj = tombstone.Obj

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

@@ -152,3 +152,220 @@ func TestGraphPopulator_CoalescedDeleteAndRecreate(t *testing.T) {
 		}
 	}
 }
+
+func TestGraphPopulator_UpdatePodVolumes(t *testing.T) {
+	secretVolume := func(volName, secretName string) corev1.Volume {
+		return corev1.Volume{
+			Name: volName,
+			VolumeSource: corev1.VolumeSource{
+				Secret: &corev1.SecretVolumeSource{SecretName: secretName},
+			},
+		}
+	}
+	configMapVolume := func(volName, cmName string) corev1.Volume {
+		return corev1.Volume{
+			Name: volName,
+			VolumeSource: corev1.VolumeSource{
+				ConfigMap: &corev1.ConfigMapVolumeSource{
+					LocalObjectReference: corev1.LocalObjectReference{Name: cmName},
+				},
+			},
+		}
+	}
+	projectedVolume := func(volName, secretName, cmName string) corev1.Volume {
+		return corev1.Volume{
+			Name: volName,
+			VolumeSource: corev1.VolumeSource{
+				Projected: &corev1.ProjectedVolumeSource{
+					Sources: []corev1.VolumeProjection{
+						{
+							Secret: &corev1.SecretProjection{
+								LocalObjectReference: corev1.LocalObjectReference{Name: secretName},
+							},
+						},
+						{
+							ConfigMap: &corev1.ConfigMapProjection{
+								LocalObjectReference: corev1.LocalObjectReference{Name: cmName},
+							},
+						},
+					},
+				},
+			},
+		}
+	}
+
+	testCases := []struct {
+		name                   string
+		initialVolumes         []corev1.Volume
+		updatedVolumes         []corev1.Volume
+		incrementGeneration    bool
+		expectEnqueued         bool
+		expectedSecretEdges    []string
+		absentSecrets          []string
+		expectedConfigMapEdges []string
+		absentConfigMaps       []string
+	}{
+		{
+			name: "unchanged generation does not enqueue pod",
+			initialVolumes: []corev1.Volume{
+				secretVolume("secret-vol-1", "secret1"),
+				configMapVolume("cm-vol-1", "cm1"),
+			},
+			updatedVolumes: []corev1.Volume{
+				secretVolume("secret-vol-1", "secret1"),
+				configMapVolume("cm-vol-1", "cm1"),
+			},
+			incrementGeneration:    false,
+			expectEnqueued:         false,
+			expectedSecretEdges:    []string{"secret1"},
+			expectedConfigMapEdges: []string{"cm1"},
+		},
+		{
+			name: "incremented generation with unchanged volumes does not enqueue pod",
+			initialVolumes: []corev1.Volume{
+				secretVolume("secret-vol-1", "secret1"),
+				configMapVolume("cm-vol-1", "cm1"),
+			},
+			updatedVolumes: []corev1.Volume{
+				secretVolume("secret-vol-1", "secret1"),
+				configMapVolume("cm-vol-1", "cm1"),
+			},
+			incrementGeneration:    true,
+			expectEnqueued:         false,
+			expectedSecretEdges:    []string{"secret1"},
+			expectedConfigMapEdges: []string{"cm1"},
+		},
+		{
+			name: "adding secret and projected volumes adds new graph edges",
+			initialVolumes: []corev1.Volume{
+				secretVolume("secret-vol-1", "secret1"),
+				configMapVolume("cm-vol-1", "cm1"),
+			},
+			updatedVolumes: []corev1.Volume{
+				secretVolume("secret-vol-1", "secret1"),
+				configMapVolume("cm-vol-1", "cm1"),
+				secretVolume("secret-vol-2", "secret2"),
+				projectedVolume("projected-vol", "projected-secret", "projected-cm"),
+			},
+			incrementGeneration:    true,
+			expectEnqueued:         true,
+			expectedSecretEdges:    []string{"secret1", "secret2", "projected-secret"},
+			expectedConfigMapEdges: []string{"cm1", "projected-cm"},
+		},
+		{
+			name: "removing secret and projected volumes removes their graph edges",
+			initialVolumes: []corev1.Volume{
+				secretVolume("secret-vol-1", "secret1"),
+				configMapVolume("cm-vol-1", "cm1"),
+				secretVolume("secret-vol-2", "secret2"),
+				projectedVolume("projected-vol", "projected-secret", "projected-cm"),
+			},
+			updatedVolumes: []corev1.Volume{
+				configMapVolume("cm-vol-1", "cm1"),
+				secretVolume("secret-vol-2", "secret2"),
+			},
+			incrementGeneration:    true,
+			expectEnqueued:         true,
+			expectedSecretEdges:    []string{"secret2"},
+			absentSecrets:          []string{"secret1", "projected-secret"},
+			expectedConfigMapEdges: []string{"cm1"},
+			absentConfigMaps:       []string{"projected-cm"},
+		},
+		{
+			name: "replacing volume with same volume count updates graph edges",
+			initialVolumes: []corev1.Volume{
+				configMapVolume("cm-vol-1", "cm1"),
+				secretVolume("secret-vol-2", "secret2"),
+			},
+			updatedVolumes: []corev1.Volume{
+				configMapVolume("cm-vol-1", "cm1"),
+				secretVolume("secret-vol-3", "secret3"),
+			},
+			incrementGeneration:    true,
+			expectEnqueued:         true,
+			expectedSecretEdges:    []string{"secret3"},
+			absentSecrets:          []string{"secret2"},
+			expectedConfigMapEdges: []string{"cm1"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})
+			graph := NewGraph()
+			g := &graphPopulator{
+				graph:     graph,
+				podQueue:  newRateLimitingQueue("test_node_authorizer_pod_volumes"),
+				podLister: corev1listers.NewPodLister(indexer),
+			}
+
+			oldPod := &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:       "pod-dynamic-vols",
+					Namespace:  "default",
+					UID:        types.UID("uid-1"),
+					Generation: 1,
+				},
+				Spec: corev1.PodSpec{
+					NodeName: "node1",
+					Volumes:  tc.initialVolumes,
+				},
+			}
+			if err := indexer.Add(oldPod); err != nil {
+				t.Fatalf("failed to add oldPod to indexer: %v", err)
+			}
+			g.addPod(oldPod)
+			if !processNextWorkItem(g.podQueue, g.processPodKey) {
+				t.Fatalf("expected initial pod add to be processed")
+			}
+
+			newPod := oldPod.DeepCopy()
+			if tc.incrementGeneration {
+				newPod.Generation++
+			}
+			newPod.Spec.Volumes = tc.updatedVolumes
+			if err := indexer.Update(newPod); err != nil {
+				t.Fatalf("failed to update newPod in indexer: %v", err)
+			}
+			g.updatePod(oldPod, newPod)
+
+			if tc.expectEnqueued {
+				if g.podQueue.Len() != 1 {
+					t.Fatalf("expected queue length 1, got %d", g.podQueue.Len())
+				}
+				if !processNextWorkItem(g.podQueue, g.processPodKey) {
+					t.Fatalf("expected processNextWorkItem to return true")
+				}
+			} else if g.podQueue.Len() != 0 {
+				t.Fatalf("expected queue length 0, got %d", g.podQueue.Len())
+			}
+
+			podVert, ok := graph.getVertexRLocked(podVertexType, "default", "pod-dynamic-vols")
+			if !ok {
+				t.Fatalf("expected pod vertex")
+			}
+			for _, secretName := range tc.expectedSecretEdges {
+				vert, exists := graph.getVertexRLocked(secretVertexType, "default", secretName)
+				if !exists || !graph.graph.HasEdgeFromTo(vert, podVert) {
+					t.Errorf("expected edge from secret %q to pod", secretName)
+				}
+			}
+			for _, secretName := range tc.absentSecrets {
+				if _, exists := graph.getVertexRLocked(secretVertexType, "default", secretName); exists {
+					t.Errorf("expected secret %q vertex to be removed from graph", secretName)
+				}
+			}
+			for _, cmName := range tc.expectedConfigMapEdges {
+				vert, exists := graph.getVertexRLocked(configMapVertexType, "default", cmName)
+				if !exists || !graph.graph.HasEdgeFromTo(vert, podVert) {
+					t.Errorf("expected edge from configmap %q to pod", cmName)
+				}
+			}
+			for _, cmName := range tc.absentConfigMaps {
+				if _, exists := graph.getVertexRLocked(configMapVertexType, "default", cmName); exists {
+					t.Errorf("expected configmap %q vertex to be removed from graph", cmName)
+				}
+			}
+		})
+	}
+}

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

@@ -1155,6 +1155,7 @@ func TestNodeAuthorizerAddEphemeralContainers(t *testing.T) {
 		if tc.EphCont != nil {
 			newPod := &corev1.Pod{}
 			pod.DeepCopyInto(newPod)
+			newPod.Generation++
 			newPod.Spec.EphemeralContainers = append(newPod.Spec.EphemeralContainers, *tc.EphCont)
 			p.updatePod(pod, newPod)
 		}
@@ -1294,6 +1295,115 @@ func TestNodeAuthorizerUpdateExtendedResourceClaim(t *testing.T) {
 	}
 }
 
+func TestNodeAuthorizerUpdatePodVolumes(t *testing.T) {
+	g := NewGraph()
+	g.destinationEdgeThreshold = 1
+	identifier := nodeidentifier.NewDefaultNodeIdentifier()
+	authz := NewAuthorizer(g, identifier, bootstrappolicy.NodeRules())
+
+	node1 := &user.DefaultInfo{Name: "system:node:node1", Groups: []string{"system:nodes"}}
+	volInitialSecret := corev1.Volume{
+		Name:         "secret-vol-1",
+		VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "initial-secret"}},
+	}
+	volDynamicSecret := corev1.Volume{
+		Name:         "secret-vol-2",
+		VolumeSource: corev1.VolumeSource{Secret: &corev1.SecretVolumeSource{SecretName: "dynamic-secret"}},
+	}
+	volDynamicCM := corev1.Volume{
+		Name: "cm-vol-1",
+		VolumeSource: corev1.VolumeSource{
+			ConfigMap: &corev1.ConfigMapVolumeSource{
+				LocalObjectReference: corev1.LocalObjectReference{Name: "dynamic-cm"},
+			},
+		},
+	}
+
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "pod1-node1",
+			Namespace: "ns1",
+			UID:       "pod1uid",
+		},
+		Spec: corev1.PodSpec{
+			NodeName: "node1",
+			Volumes:  []corev1.Volume{volInitialSecret},
+		},
+	}
+
+	p := newTestGraphPopulator(g)
+	p.addPod(pod)
+
+	type authzCheck struct {
+		resource string
+		name     string
+		decision authorizer.Decision
+	}
+
+	testCases := []struct {
+		name           string
+		updatedVolumes []corev1.Volume
+		checks         []authzCheck
+	}{
+		{
+			name:           "initial pod volumes authorize only initial-secret",
+			updatedVolumes: nil,
+			checks: []authzCheck{
+				{resource: "secrets", name: "initial-secret", decision: authorizer.DecisionAllow},
+				{resource: "secrets", name: "dynamic-secret", decision: authorizer.DecisionNoOpinion},
+				{resource: "configmaps", name: "dynamic-cm", decision: authorizer.DecisionNoOpinion},
+			},
+		},
+		{
+			name:           "adding secret and configmap volumes grants node access",
+			updatedVolumes: []corev1.Volume{volInitialSecret, volDynamicSecret, volDynamicCM},
+			checks: []authzCheck{
+				{resource: "secrets", name: "initial-secret", decision: authorizer.DecisionAllow},
+				{resource: "secrets", name: "dynamic-secret", decision: authorizer.DecisionAllow},
+				{resource: "configmaps", name: "dynamic-cm", decision: authorizer.DecisionAllow},
+			},
+		},
+		{
+			name:           "removing initial-secret volume revokes node access",
+			updatedVolumes: []corev1.Volume{volDynamicSecret, volDynamicCM},
+			checks: []authzCheck{
+				{resource: "secrets", name: "initial-secret", decision: authorizer.DecisionNoOpinion},
+				{resource: "secrets", name: "dynamic-secret", decision: authorizer.DecisionAllow},
+				{resource: "configmaps", name: "dynamic-cm", decision: authorizer.DecisionAllow},
+			},
+		},
+	}
+
+	currentPod := pod
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.updatedVolumes != nil {
+				nextPod := currentPod.DeepCopy()
+				nextPod.Generation++
+				nextPod.Spec.Volumes = tc.updatedVolumes
+				p.updatePod(currentPod, nextPod)
+				currentPod = nextPod
+			}
+			for _, check := range tc.checks {
+				decision, _, err := authz.Authorize(context.Background(), authorizer.AttributesRecord{
+					User:            node1,
+					ResourceRequest: true,
+					Verb:            "get",
+					Resource:        check.resource,
+					Namespace:       "ns1",
+					Name:            check.name,
+				})
+				if err != nil {
+					t.Fatalf("unexpected error authorizing %s/%s: %v", check.resource, check.name, err)
+				}
+				if decision != check.decision {
+					t.Errorf("%s/%s: expected %v, got %v", check.resource, check.name, check.decision, decision)
+				}
+			}
+		})
+	}
+}
+
 type sampleDataOpts struct {
 	nodes       int
 	namespaces  int