Skip to content

Validate agent token for duplicate IP addresses #3673

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+160 −54
Open

Validate agent token for duplicate IP addresses #3673

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 12 additions & 21 deletions internal/controller/nginx/agent/grpc/interceptor/interceptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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(),
Comment on lines +165 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

if, in this case, the job has the same labels, does that mean it would still error? is that just an edge case which we shouldn't need to worry about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We check the Pod phase to ensure that only one pod is running, even if the Job fits this same selector

}

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)
}

Expand Down
181 changes: 148 additions & 33 deletions internal/controller/nginx/agent/grpc/interceptor/interceptor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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},
},
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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())
}
})
}
}
Loading