Validate agent token for duplicate IP addresses

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 handle multiple Pods with the same IP address, and look through all of them to find a proper match for the 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,34 +161,23 @@ 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)
-	}
-
-	if scName != usernameItems[3] {
+	if len(podList.Items) == 0 {
 		msg := fmt.Sprintf(
-			"token user name %q does not match service account name of requesting pod %q", usernameItems[3], scName,
+			"did not find any Pods with IP address %q, namespace %q, and %q label value %q",
+			gi.IPAddress,
+			usernameItems[2],
+			controller.AppNameLabel,
+			usernameItems[3],
 		)
 		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"
 )
 
@@ -204,38 +206,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 +231,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 +260,132 @@ 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"},
+			},
+			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"},
+			},
+			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"},
+			},
+			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"},
+			},
+			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"},
+			},
+			gi:        &grpcContext.GrpcInfo{Token: "dummy-token", IPAddress: "9.9.9.9"},
+			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("did not find any Pods"))
+			} else {
+				g.Expect(err).ToNot(HaveOccurred())
+				g.Expect(resultCtx).ToNot(BeNil())
+			}
+		})
+	}
+}