From 3b3c099c2d2c1198225502d62f05f63616cc53f6 Mon Sep 17 00:00:00 2001
From: Andrew Harding <azdagron@gmail.com>
Date: Tue, 14 May 2024 07:43:20 -0600
Subject: [PATCH] Tighten up PSAT audience validation (#5142)

Kubernetes docs advise that callers of the TokenReview API should cross
check the audience fields in the spec and status just in case there is
a validator out there that is audience-unaware.

Signed-off-by: Andrew Harding <azdagron@gmail.com>
---
 pkg/common/plugin/k8s/apiserver/client.go     | 18 ++++++++++++
 .../plugin/k8s/apiserver/client_test.go       | 29 +++++++++++++++++--
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/pkg/common/plugin/k8s/apiserver/client.go b/pkg/common/plugin/k8s/apiserver/client.go
index 832d1f5abd..163f74c6c0 100644
--- a/pkg/common/plugin/k8s/apiserver/client.go
+++ b/pkg/common/plugin/k8s/apiserver/client.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"slices"
 
 	authv1 "k8s.io/api/authentication/v1"
 	v1 "k8s.io/api/core/v1"
@@ -126,6 +127,23 @@ func (c *client) ValidateToken(ctx context.Context, token string, audiences []st
 		return nil, fmt.Errorf("token review API response contains an error: %v", resp.Status.Error)
 	}
 
+	// Ensure the audiences returned in the status are compatible with those requested
+	// in the TokenReviewSpec (if any). This is to ensure the validator is
+	// audience aware.
+	// See the documentation on the Status Audiences field.
+	if resp.Status.Authenticated && len(audiences) > 0 {
+		atLeastOnePresent := false
+		for _, audience := range audiences {
+			if slices.Contains(resp.Status.Audiences, audience) {
+				atLeastOnePresent = true
+				break
+			}
+		}
+		if !atLeastOnePresent {
+			return nil, fmt.Errorf("token review API did not validate audience: wanted one of %q but got %q", audiences, resp.Status.Audiences)
+		}
+	}
+
 	return &resp.Status, nil
 }
 
diff --git a/pkg/common/plugin/k8s/apiserver/client_test.go b/pkg/common/plugin/k8s/apiserver/client_test.go
index ef1b1d0ee4..d0f77b4349 100644
--- a/pkg/common/plugin/k8s/apiserver/client_test.go
+++ b/pkg/common/plugin/k8s/apiserver/client_test.go
@@ -111,6 +111,8 @@ CFEm0wKBgQCwARG9qV4sUoBvwLyBHQbPFZi/9PYwvDsnzjmKTUPa+kd4ATrv7gBY
 oN1CqmWqJQYVB6oGxFMaebeijY82beDN3WSBAK2FGvmdi3vZUAHHXyNOBS2Wq6PA
 oIrPuyjOmscrC627wX3LGUHwPKtNArBT8lKFfda1B1BqAk0q1/ui/A==
 -----END RSA PRIVATE KEY-----`)
+
+	wantAudiences = []string{"aud1", "aud2"}
 )
 
 const (
@@ -275,15 +277,38 @@ func (s *ClientSuite) TestValidateTokenFailsIfStatusContainsError() {
 	s.Nil(status)
 }
 
+func (s *ClientSuite) TestValidateTokenFailsDueToAudienceUnawareValidator() {
+	fakeClient := fake.NewSimpleClientset()
+	fakeClient.AuthenticationV1().(*fake_authv1.FakeAuthenticationV1).PrependReactor("create", "tokenreviews",
+		func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
+			return true, &authv1.TokenReview{
+				Status: authv1.TokenReviewStatus{
+					Authenticated: true,
+					Audiences:     []string{"aud3"},
+				},
+			}, nil
+		})
+
+	client := s.createClient(fakeClient)
+	status, err := client.ValidateToken(ctx, testToken, wantAudiences)
+	s.AssertErrorContains(err, `token review API did not validate audience: wanted one of ["aud1" "aud2"] but got ["aud3"]`)
+	s.Nil(status)
+}
+
 func (s *ClientSuite) TestValidateTokenSucceeds() {
 	fakeClient := fake.NewSimpleClientset()
 	fakeClient.AuthenticationV1().(*fake_authv1.FakeAuthenticationV1).PrependReactor("create", "tokenreviews",
 		func(action k8stesting.Action) (handled bool, ret runtime.Object, err error) {
-			return true, &authv1.TokenReview{Status: authv1.TokenReviewStatus{Authenticated: true}}, nil
+			return true, &authv1.TokenReview{
+				Status: authv1.TokenReviewStatus{
+					Authenticated: true,
+					Audiences:     wantAudiences[:1],
+				},
+			}, nil
 		})
 
 	client := s.createClient(fakeClient)
-	status, err := client.ValidateToken(ctx, testToken, []string{"aud1"})
+	status, err := client.ValidateToken(ctx, testToken, wantAudiences)
 	s.NoError(err)
 	s.NotNil(status)
 	s.True(status.Authenticated)