From 106a480dad3ab248e279fd23d09f83772b91d3d8 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 4 Nov 2024 12:49:18 -0800 Subject: [PATCH] JWTAuthenticator must reload when spec.audience or spec.claims changes --- .../jwtcachefiller/jwtcachefiller.go | 16 +- .../jwtcachefiller/jwtcachefiller_test.go | 153 +++++++++++++++++- .../webhookcachefiller/webhookcachefiller.go | 4 + 3 files changed, 168 insertions(+), 5 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index a84ad931d..3570b8255 100644 --- 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 diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index 3b0708350..3128501ab 100644 --- 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:$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:$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:$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:$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:$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:$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 diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index 9d4ff80fa..53d90caac 100644 --- 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) {