JWTAuthenticator must reload when spec.audience or spec.claims changes

2026-02-14 18:10:17 +00:00 · 2024-11-04 12:49:18 -08:00
parent 587e6fbd8a
commit 106a480dad
3 changed files with 168 additions and 5 deletions
--- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go
+++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go
@@ -109,6 +109,8 @@ type tokenAuthenticatorCloser interface {
 type cachedJWTAuthenticator struct {
 	authenticator.Token
 	issuer       string
+	audience     string
+	claims       authenticationv1alpha1.JWTTokenClaims
 	caBundleHash tlsconfigutil.CABundleHash
 	cancel       context.CancelFunc
 }
@@ -239,7 +241,7 @@ func (c *jwtCacheFillerController) syncIndividualJWTAuthenticator(ctx context.Co
 	// are for administrator convenience at the time of a configuration change, to catch typos and blatant
 	// misconfigurations, rather than to constantly monitor for external issues.
 	foundAuthenticatorInCache, previouslyValidatedWithSameEndpointAndBundle := c.havePreviouslyValidated(
-		cacheKey, jwtAuthenticator.Spec.Issuer, tlsBundleOk, caBundle.Hash(), logger)
+		cacheKey, jwtAuthenticator.Spec, tlsBundleOk, caBundle.Hash(), logger)
 	if previouslyValidatedWithSameEndpointAndBundle {
 		// Because the authenticator was previously cached, that implies that the following conditions were
 		// previously validated. These are the expensive validations to repeat, so skip them this time.
@@ -331,7 +333,7 @@ func (c *jwtCacheFillerController) doExpensiveValidations(

 func (c *jwtCacheFillerController) havePreviouslyValidated(
 	cacheKey authncache.Key,
-	issuer string,
+	spec authenticationv1alpha1.JWTAuthenticatorSpec,
 	tlsBundleOk bool,
 	caBundleHash tlsconfigutil.CABundleHash,
 	logger plog.Logger,
@@ -345,7 +347,13 @@ func (c *jwtCacheFillerController) havePreviouslyValidated(
 	if authenticatorFromCache == nil {
 		return false, false
 	}
-	if authenticatorFromCache.issuer == issuer &&
+	// Compare all spec fields to check if they have changed since we cached the authenticator.
+	// Instead of directly comparing spec.TLS, compare the effective result of spec.TLS,
+	// which is the CA bundle that was dynamically loaded.
+	// If any spec field has changed, then we need a new in-memory authenticator.
+	if authenticatorFromCache.issuer == spec.Issuer &&
+		authenticatorFromCache.audience == spec.Audience &&
+		authenticatorFromCache.claims == spec.Claims &&
 		tlsBundleOk && // if there was any error while validating the latest CA bundle, then do not consider it previously validated
 		authenticatorFromCache.caBundleHash.Equal(caBundleHash) {
 		return true, true
@@ -702,6 +710,8 @@ func (c *jwtCacheFillerController) newCachedJWTAuthenticator(
 	return &cachedJWTAuthenticator{
 		Token:        oidcAuthenticator,
 		issuer:       spec.Issuer,
+		audience:     spec.Audience,
+		claims:       spec.Claims,
 		caBundleHash: caBundleHash,
 		cancel:       cancel,
 	}, conditions, nil
--- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go
+++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go
@@ -1111,7 +1111,7 @@ func TestController(t *testing.T) {
 			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
 		},
 		{
-			name: "Sync: JWTAuthenticator with new spec fields: loop will close previous instance of JWTAuthenticator and complete successfully and update status conditions",
+			name: "Sync: JWTAuthenticator with new spec.tls fields: loop will close previous instance of JWTAuthenticator and complete successfully and update status conditions",
 			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
 				oldCA, err := base64.StdEncoding.DecodeString(otherJWTAuthenticatorSpec.TLS.CertificateAuthorityData)
 				require.NoError(t, err)
@@ -1157,6 +1157,153 @@ func TestController(t *testing.T) {
 			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
 			wantClose:                           true,
 		},
+		{
+			name: "Sync: JWTAuthenticator with new spec.audience field: loop will close previous instance of JWTAuthenticator and complete successfully and update status conditions",
+			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
+				oldCA, err := base64.StdEncoding.DecodeString(someJWTAuthenticatorSpec.TLS.CertificateAuthorityData)
+				require.NoError(t, err)
+				cacheValue := newCacheValue(t, *someJWTAuthenticatorSpec, string(oldCA), wantClose)
+				cacheValue.audience = "some-old-audience-value"
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "JWTAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					cacheValue,
+				)
+			},
+			jwtAuthenticators: []runtime.Object{
+				&authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+				},
+			},
+			wantLogLines: []string{
+				fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"jwtcachefiller-controller","caller":"jwtcachefiller/jwtcachefiller.go:<line>$jwtcachefiller.(*jwtCacheFillerController).updateStatus","message":"jwtauthenticator status successfully updated","jwtAuthenticator":"test-name","issuer":"%s","phase":"Ready"}`, goodIssuer),
+				fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"jwtcachefiller-controller","caller":"jwtcachefiller/jwtcachefiller.go:<line>$jwtcachefiller.(*jwtCacheFillerController).syncIndividualJWTAuthenticator","message":"added or updated jwt authenticator in cache","jwtAuthenticator":"test-name","issuer":"%s","isOverwrite":true}`, goodIssuer),
+			},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+					Status: authenticationv1alpha1.JWTAuthenticatorStatus{
+						Conditions: allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
+						Phase:      "Ready",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
+			wantClose:                           true,
+		},
+		{
+			name: "Sync: JWTAuthenticator with new spec.claims.username field: loop will close previous instance of JWTAuthenticator and complete successfully and update status conditions",
+			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
+				oldCA, err := base64.StdEncoding.DecodeString(someJWTAuthenticatorSpec.TLS.CertificateAuthorityData)
+				require.NoError(t, err)
+				cacheValue := newCacheValue(t, *someJWTAuthenticatorSpec, string(oldCA), wantClose)
+				cacheValue.claims.Username = "some-old-username-value"
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "JWTAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					cacheValue,
+				)
+			},
+			jwtAuthenticators: []runtime.Object{
+				&authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+				},
+			},
+			wantLogLines: []string{
+				fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"jwtcachefiller-controller","caller":"jwtcachefiller/jwtcachefiller.go:<line>$jwtcachefiller.(*jwtCacheFillerController).updateStatus","message":"jwtauthenticator status successfully updated","jwtAuthenticator":"test-name","issuer":"%s","phase":"Ready"}`, goodIssuer),
+				fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"jwtcachefiller-controller","caller":"jwtcachefiller/jwtcachefiller.go:<line>$jwtcachefiller.(*jwtCacheFillerController).syncIndividualJWTAuthenticator","message":"added or updated jwt authenticator in cache","jwtAuthenticator":"test-name","issuer":"%s","isOverwrite":true}`, goodIssuer),
+			},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+					Status: authenticationv1alpha1.JWTAuthenticatorStatus{
+						Conditions: allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
+						Phase:      "Ready",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
+			wantClose:                           true,
+		},
+		{
+			name: "Sync: JWTAuthenticator with new spec.claims.groups field: loop will close previous instance of JWTAuthenticator and complete successfully and update status conditions",
+			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
+				oldCA, err := base64.StdEncoding.DecodeString(someJWTAuthenticatorSpec.TLS.CertificateAuthorityData)
+				require.NoError(t, err)
+				cacheValue := newCacheValue(t, *someJWTAuthenticatorSpec, string(oldCA), wantClose)
+				cacheValue.claims.Groups = "some-old-groups-value"
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "JWTAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					cacheValue,
+				)
+			},
+			jwtAuthenticators: []runtime.Object{
+				&authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+				},
+			},
+			wantLogLines: []string{
+				fmt.Sprintf(`{"level":"debug","timestamp":"2099-08-08T13:57:36.123456Z","logger":"jwtcachefiller-controller","caller":"jwtcachefiller/jwtcachefiller.go:<line>$jwtcachefiller.(*jwtCacheFillerController).updateStatus","message":"jwtauthenticator status successfully updated","jwtAuthenticator":"test-name","issuer":"%s","phase":"Ready"}`, goodIssuer),
+				fmt.Sprintf(`{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"jwtcachefiller-controller","caller":"jwtcachefiller/jwtcachefiller.go:<line>$jwtcachefiller.(*jwtCacheFillerController).syncIndividualJWTAuthenticator","message":"added or updated jwt authenticator in cache","jwtAuthenticator":"test-name","issuer":"%s","isOverwrite":true}`, goodIssuer),
+			},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *someJWTAuthenticatorSpec,
+					Status: authenticationv1alpha1.JWTAuthenticatorStatus{
+						Conditions: allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
+						Phase:      "Ready",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			wantNamesOfJWTAuthenticatorsInCache: []string{"test-name"},
+			wantClose:                           true,
+		},
 		{
 			name: "Sync: previously cached authenticator gets new spec fields, but status update fails: loop will leave it in the cache and avoid calling close",
 			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
@@ -2675,7 +2822,7 @@ func createJWT(
 	return jwt
 }

-func newCacheValue(t *testing.T, spec authenticationv1alpha1.JWTAuthenticatorSpec, caBundle string, wantClose bool) authncache.Value {
+func newCacheValue(t *testing.T, spec authenticationv1alpha1.JWTAuthenticatorSpec, caBundle string, wantClose bool) *cachedJWTAuthenticator {
 	t.Helper()
 	wasClosed := false

@@ -2685,6 +2832,8 @@ func newCacheValue(t *testing.T, spec authenticationv1alpha1.JWTAuthenticatorSpe

 	return &cachedJWTAuthenticator{
 		issuer:       spec.Issuer,
+		audience:     spec.Audience,
+		claims:       spec.Claims,
 		caBundleHash: tlsconfigutil.NewCABundleHash([]byte(caBundle)),
 		cancel: func() {
 			wasClosed = true
--- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go
+++ b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go
@@ -298,6 +298,10 @@ func (c *webhookCacheFillerController) havePreviouslyValidated(
 	if authenticatorFromCache == nil {
 		return false, false
 	}
+	// Compare all spec fields to check if they have changed since we cached the authenticator.
+	// Instead of directly comparing spec.TLS, compare the effective result of spec.TLS,
+	// which is the CA bundle that was dynamically loaded.
+	// If any spec field has changed, then we need a new in-memory authenticator.
 	if authenticatorFromCache.endpoint == endpoint &&
 		tlsBundleOk && // if there was any error while validating the latest CA bundle, then do not consider it previously validated
 		authenticatorFromCache.caBundleHash.Equal(caBundleHash) {