Validate agent token for duplicate IP addresses (#3673)

Problem: In some environments, Pods could share an IP address (for example when a Job completes and a new Pod grabs that IP). This would cause problems when the nginx data plane Pod would attempt to connect to the control plane. The control plane would try to validate the token provided by the nginx agent, by using the IP address in the request to lookup the associated Pod. If multiple Pods existed with that IP address, the control plane would error out.

Solution: Fix the control plane logic to use more criteria when getting the proper Pod to verify the provided token.

Author: sjberman

internal/controller/nginx/agent/grpc/interceptor/interceptor.go

@@ -16,6 +16,7 @@ import (
 	authv1 "k8s.io/api/authentication/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/fields"
+	"k8s.io/apimachinery/pkg/labels"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	grpcContext "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/grpc/context"
@@ -160,35 +161,25 @@ func (c ContextSetter) validateToken(ctx context.Context, gi *grpcContext.GrpcIn
 	var podList corev1.PodList
 	opts := &client.ListOptions{
 		FieldSelector: fields.SelectorFromSet(fields.Set{"status.podIP": gi.IPAddress}),
+		Namespace:     usernameItems[2],
+		LabelSelector: labels.Set(map[string]string{
+			controller.AppNameLabel: usernameItems[3],
+		}).AsSelector(),
 	}
 
 	if err := c.k8sClient.List(getCtx, &podList, opts); err != nil {
 		return nil, status.Error(codes.Internal, fmt.Sprintf("error listing pods: %s", err.Error()))
 	}
 
-	if len(podList.Items) != 1 {
-		msg := fmt.Sprintf("expected one Pod to have IP address %s, found %d", gi.IPAddress, len(podList.Items))
-		return nil, status.Error(codes.Internal, msg)
-	}
-
-	podNS := podList.Items[0].GetNamespace()
-	if podNS != usernameItems[2] {
-		msg := fmt.Sprintf(
-			"token user namespace %q does not match namespace of requesting pod %q", usernameItems[2], podNS,
-		)
-		return nil, status.Error(codes.Unauthenticated, msg)
-	}
-
-	scName, ok := podList.Items[0].GetLabels()[controller.AppNameLabel]
-	if !ok {
-		msg := fmt.Sprintf("could not get app name from %q label; unable to authenticate token", controller.AppNameLabel)
-		return nil, status.Error(codes.Unauthenticated, msg)
+	runningCount := 0
+	for _, pod := range podList.Items {
+		if pod.Status.Phase == corev1.PodRunning {
+			runningCount++
+		}
 	}
 
-	if scName != usernameItems[3] {
-		msg := fmt.Sprintf(
-			"token user name %q does not match service account name of requesting pod %q", usernameItems[3], scName,
-		)
+	if runningCount != 1 {
+		msg := fmt.Sprintf("expected a single Running pod with IP address %q, but found %d", gi.IPAddress, runningCount)
 		return nil, status.Error(codes.Unauthenticated, msg)
 	}
 

internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go

@@ -17,7 +17,9 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
+	grpcContext "github.com/nginx/nginx-gateway-fabric/v2/internal/controller/nginx/agent/grpc/context"
 	"github.com/nginx/nginx-gateway-fabric/v2/internal/framework/controller"
 )
 
@@ -67,6 +69,7 @@ func (m *mockClient) List(_ context.Context, obj client.ObjectList, _ ...client.
 				Namespace: m.podNamespace,
 				Labels:    labels,
 			},
+			Status: corev1.PodStatus{Phase: corev1.PodRunning},
 		},
 	}
 
@@ -204,38 +207,6 @@ func TestInterceptor(t *testing.T) {
 			expErrCode:    codes.Unauthenticated,
 			expErrMsg:     "must be of the format",
 		},
-		{
-			name:          "mismatched namespace in username",
-			md:            validMetadata,
-			peer:          validPeerData,
-			username:      "system:serviceaccount:invalid:gateway-nginx",
-			appName:       "gateway-nginx",
-			podNamespace:  "default",
-			authenticated: true,
-			expErrCode:    codes.Unauthenticated,
-			expErrMsg:     "does not match namespace",
-		},
-		{
-			name:          "mismatched name in username",
-			md:            validMetadata,
-			peer:          validPeerData,
-			username:      "system:serviceaccount:default:invalid",
-			appName:       "gateway-nginx",
-			podNamespace:  "default",
-			authenticated: true,
-			expErrCode:    codes.Unauthenticated,
-			expErrMsg:     "does not match service account name",
-		},
-		{
-			name:          "missing app name label",
-			md:            validMetadata,
-			peer:          validPeerData,
-			username:      "system:serviceaccount:default:gateway-nginx",
-			podNamespace:  "default",
-			authenticated: true,
-			expErrCode:    codes.Unauthenticated,
-			expErrMsg:     "could not get app name",
-		},
 	}
 
 	streamHandler := func(_ any, _ grpc.ServerStream) error {
@@ -261,7 +232,7 @@ func TestInterceptor(t *testing.T) {
 			}
 			cs := NewContextSetter(mockK8sClient, "ngf-audience")
 
-			ctx := context.Background()
+			ctx := t.Context()
 			if test.md != nil {
 				peerCtx := context.Background()
 				if test.peer != nil {
@@ -290,3 +261,147 @@ func TestInterceptor(t *testing.T) {
 		})
 	}
 }
+
+type patchClient struct {
+	client.Client
+}
+
+func (p *patchClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error {
+	tr, ok := obj.(*authv1.TokenReview)
+	if ok {
+		tr.Status.Authenticated = true
+		tr.Status.User.Username = "system:serviceaccount:default:gateway-nginx"
+	}
+	return nil
+}
+
+func TestValidateToken_PodListOptions(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		pod       *corev1.Pod
+		gi        *grpcContext.GrpcInfo
+		name      string
+		shouldErr bool
+	}{
+		{
+			name: "all match",
+			pod: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "nginx-pod",
+					Namespace: "default",
+					Labels: map[string]string{
+						controller.AppNameLabel: "gateway-nginx",
+					},
+				},
+				Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning},
+			},
+			gi:        &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"},
+			shouldErr: false,
+		},
+		{
+			name: "ip matches, namespace does not",
+			pod: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "nginx-pod",
+					Namespace: "other-namespace",
+					Labels: map[string]string{
+						controller.AppNameLabel: "gateway-nginx",
+					},
+				},
+				Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning},
+			},
+			gi:        &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"},
+			shouldErr: true,
+		},
+		{
+			name: "ip matches, label value does not match",
+			pod: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "nginx-pod",
+					Namespace: "default",
+					Labels: map[string]string{
+						controller.AppNameLabel: "not-gateway-nginx",
+					},
+				},
+				Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning},
+			},
+			gi:        &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"},
+			shouldErr: true,
+		},
+		{
+			name: "ip matches, label does not exist",
+			pod: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "nginx-pod",
+					Namespace: "default",
+					Labels:    map[string]string{},
+				},
+				Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning},
+			},
+			gi:        &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"},
+			shouldErr: true,
+		},
+		{
+			name: "ip does not match",
+			pod: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "nginx-pod",
+					Namespace: "default",
+					Labels: map[string]string{
+						controller.AppNameLabel: "gateway-nginx",
+					},
+				},
+				Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodRunning},
+			},
+			gi:        &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "9.9.9.9"},
+			shouldErr: true,
+		},
+		{
+			name: "all match but pod not running",
+			pod: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "nginx-pod",
+					Namespace: "default",
+					Labels: map[string]string{
+						controller.AppNameLabel: "gateway-nginx",
+					},
+				},
+				Status: corev1.PodStatus{PodIP: "1.2.3.4", Phase: corev1.PodPending},
+			},
+			gi:        &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "1.2.3.4"},
+			shouldErr: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			g := NewWithT(t)
+
+			fakeClient := fake.NewClientBuilder().
+				WithObjects(tc.pod).
+				WithIndex(&corev1.Pod{}, "status.podIP", func(obj client.Object) []string {
+					pod, ok := obj.(*corev1.Pod)
+					g.Expect(ok).To(BeTrue())
+					if pod.Status.PodIP != "" {
+						return []string{pod.Status.PodIP}
+					}
+					return nil
+				}).
+				Build()
+
+			patchedClient := &patchClient{fakeClient}
+			csPatched := NewContextSetter(patchedClient, "ngf-audience")
+
+			resultCtx, err := csPatched.validateToken(t.Context(), tc.gi)
+			if tc.shouldErr {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(err.Error()).To(ContainSubstring("expected a single Running pod"))
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+				g.Expect(resultCtx).ToNot(BeNil())
+			}
+		})
+	}
+}