From 60bcd0333530b7fa57c3fafd146bf13629f978c3 Mon Sep 17 00:00:00 2001 From: Saylor Berman Date: Tue, 29 Jul 2025 11:36:55 -0600 Subject: [PATCH] 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 use more criteria when getting the proper Pod to verify the provided token. --- .../agent/grpc/interceptor/interceptor.go | 33 ++-- .../grpc/interceptor/interceptor_test.go | 181 ++++++++++++++---- 2 files changed, 160 insertions(+), 54 deletions(-) diff --git a/internal/controller/nginx/agent/grpc/interceptor/interceptor.go b/internal/controller/nginx/agent/grpc/interceptor/interceptor.go index 0a32234400..7732adbe6e 100644 --- a/internal/controller/nginx/agent/grpc/interceptor/interceptor.go +++ b/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) } diff --git a/internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go b/internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go index d334c9a465..c31a56c241 100644 --- a/internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go +++ b/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()) + } + }) + } +}