fix authenticators bug: stop allowing usage when validation fails

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

@@ -119,6 +119,9 @@
 }
 
 func (c *cachedJWTAuthenticator) Close() {
+	if c == nil {
+		return
+	}
 	c.cancel()
 }
 
@@ -162,12 +165,12 @@
 // Sync implements controllerlib.Syncer.
 func (c *jwtCacheFillerController) Sync(ctx controllerlib.Context) error {
 	obj, err := c.jwtAuthenticators.Lister().Get(ctx.Key.Name)
-
 	if err != nil && apierrors.IsNotFound(err) {
 		c.log.Info("Sync() found that the JWTAuthenticator does not exist yet or was deleted")
 		return nil
 	}
 	if err != nil {
+		// no unit test for this failure
 		return fmt.Errorf("failed to get JWTAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err)
 	}
 
@@ -179,33 +182,29 @@
 
 	// If this authenticator already exists, then only recreate it if is different from the desired
 	// authenticator. We don't want to be creating a new authenticator for every resync period.
-	//
-	// If we do need to recreate the authenticator, then make sure cancel the old one to avoid
-	// goroutine leaks.
-	if value := c.cache.Get(cacheKey); value != nil {
-		jwtAuthenticator := c.extractValueAsJWTAuthenticator(value)
-		if jwtAuthenticator != nil {
-			if reflect.DeepEqual(jwtAuthenticator.spec, &obj.Spec) {
-				c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer).Info("actual jwt authenticator and desired jwt authenticator are the same")
-				return nil
-			}
-			jwtAuthenticator.Close()
+	var jwtAuthenticatorFromCache *cachedJWTAuthenticator
+	if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil {
+		jwtAuthenticatorFromCache = c.cacheValueAsJWTAuthenticator(valueFromCache)
+		if jwtAuthenticatorFromCache != nil && reflect.DeepEqual(jwtAuthenticatorFromCache.spec, &obj.Spec) {
+			c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer).
+				Info("actual jwt authenticator and desired jwt authenticator are the same")
+			// Stop, no more work to be done. This authenticator is already validated and cached.
+			return nil
 		}
 	}
 
 	conditions := make([]*metav1.Condition, 0)
-	specCopy := obj.Spec.DeepCopy()
 	var errs []error
 
-	rootCAs, conditions, tlsOk := c.validateTLS(specCopy.TLS, conditions)
-	_, conditions, issuerOk := c.validateIssuer(specCopy.Issuer, conditions)
+	rootCAs, conditions, tlsOk := c.validateTLSBundle(obj.Spec.TLS, conditions)
+	_, conditions, issuerOk := c.validateIssuer(obj.Spec.Issuer, conditions)
 	okSoFar := tlsOk && issuerOk
 
 	client := phttp.Default(rootCAs)
 	client.Timeout = 30 * time.Second // copied from Kube OIDC code
 	coreOSCtx := coreosoidc.ClientContext(context.Background(), client)
 
-	pJSON, provider, conditions, providerErr := c.validateProviderDiscovery(coreOSCtx, specCopy.Issuer, conditions, okSoFar)
+	pJSON, provider, conditions, providerErr := c.validateProviderDiscovery(coreOSCtx, obj.Spec.Issuer, conditions, okSoFar)
 	errs = append(errs, providerErr)
 	okSoFar = okSoFar && providerErr == nil
 
@@ -217,21 +216,29 @@
 	errs = append(errs, jwksFetchErr)
 	okSoFar = okSoFar && jwksFetchErr == nil
 
-	// Make a deep copy of the spec so we aren't storing pointers to something that the informer cache
-	// may mutate! We don't store status as status is derived from spec.
-	cachedAuthenticator, conditions, err := c.newCachedJWTAuthenticator(
+	newJWTAuthenticatorForCache, conditions, err := c.newCachedJWTAuthenticator(
 		client,
-		obj.Spec.DeepCopy(),
+		obj.Spec.DeepCopy(), // deep copy to avoid caching original object
 		keySet,
 		conditions,
 		okSoFar)
 	errs = append(errs, err)
 
-	if !conditionsutil.HadErrorCondition(conditions) {
-		c.cache.Store(cacheKey, cachedAuthenticator)
-		c.log.Info("added new jwt authenticator", "jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer)
+	if conditionsutil.HadErrorCondition(conditions) {
+		// The authenticator was determined to be invalid. Remove it from the cache, in case it was previously
+		// validated and cached. Do not allow an old, previously validated spec of the authenticator to continue
+		// being used for authentication.
+		c.cache.Delete(cacheKey)
+	} else {
+		c.cache.Store(cacheKey, newJWTAuthenticatorForCache)
+		c.log.WithValues("jwtAuthenticator", klog.KObj(obj), "issuer", obj.Spec.Issuer).
+			Info("added new jwt authenticator")
 	}
 
+	// In case we just overwrote or deleted the authenticator from the cache, clean up the old instance
+	// to avoid leaking goroutines. It's safe to call Close() on nil.
+	jwtAuthenticatorFromCache.Close()
+
 	err = c.updateStatus(ctx.Context, obj, conditions)
 	errs = append(errs, err)
 
@@ -243,7 +250,7 @@
 	return utilerrors.NewAggregate(errs)
 }
 
-func (c *jwtCacheFillerController) extractValueAsJWTAuthenticator(value authncache.Value) *cachedJWTAuthenticator {
+func (c *jwtCacheFillerController) cacheValueAsJWTAuthenticator(value authncache.Value) *cachedJWTAuthenticator {
 	jwtAuthenticator, ok := value.(*cachedJWTAuthenticator)
 	if !ok {
 		actualType := "<nil>"
@@ -256,7 +263,7 @@
 	return jwtAuthenticator
 }
 
-func (c *jwtCacheFillerController) validateTLS(tlsSpec *authenticationv1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []*metav1.Condition, bool) {
+func (c *jwtCacheFillerController) validateTLSBundle(tlsSpec *authenticationv1alpha1.TLSSpec, conditions []*metav1.Condition) (*x509.CertPool, []*metav1.Condition, bool) {
 	rootCAs, _, err := pinnipedcontroller.BuildCertPoolAuth(tlsSpec)
 	if err != nil {
 		msg := fmt.Sprintf("%s: %s", "invalid TLS configuration", err.Error())

internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go

@@ -1143,6 +1143,58 @@ func TestController(t *testing.T) {
 			},
 			wantCacheEntries: 0,
 		},
+		{
+			name: "previously valid cached authenticator's spec changes and becomes invalid (e.g. spec.issuer URL is invalid): loop will fail sync, will write failed and unknown status conditions, and will remove authenticator from cache",
+			cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "JWTAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					newCacheValue(t, *someJWTAuthenticatorSpec, wantClose),
+				)
+			},
+			jwtAuthenticators: []runtime.Object{
+				&authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *invalidIssuerJWTAuthenticatorSpec,
+				},
+			},
+			syncKey: controllerlib.Key{Name: "test-name"},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(jwtAuthenticatorsGVR, "", &authenticationv1alpha1.JWTAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: *invalidIssuerJWTAuthenticatorSpec,
+					Status: authenticationv1alpha1.JWTAuthenticatorStatus{
+						Conditions: conditionstestutil.Replace(
+							allHappyConditionsSuccess(goodIssuer, frozenMetav1Now, 0),
+							[]metav1.Condition{
+								sadReadyCondition(frozenMetav1Now, 0),
+								sadIssuerURLValidInvalid("https://.café   .com/café/café/café/coffee", frozenMetav1Now, 0),
+								unknownDiscoveryURLValid(frozenMetav1Now, 0),
+								unknownAuthenticatorValid(frozenMetav1Now, 0),
+								unknownJWKSURLValid(frozenMetav1Now, 0),
+								unknownJWKSFetch(frozenMetav1Now, 0),
+							},
+						),
+						Phase: "Error",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(jwtAuthenticatorsGVR, jwtAUthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(jwtAuthenticatorsGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			wantCacheEntries: 0,    // removed from cache
+			wantClose:        true, // the removed cache entry was also closed
+		},
 		{
 			name: "validateIssuer: parsing error (spec.issuer URL is invalid): loop will fail sync, will write failed and unknown status conditions, but will not enqueue a resync due to user config error",
 			jwtAuthenticators: []runtime.Object{

internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go

@@ -10,6 +10,7 @@ import (
 	"crypto/x509"
 	"fmt"
 	"net/url"
+	"reflect"
 	"time"
 
 	k8sauthv1beta1 "k8s.io/api/authentication/v1beta1"
@@ -56,6 +57,11 @@ const (
 	msgUnableToValidate              = "unable to validate; see other conditions for details"
 )
 
+type cachedWebhookAuthenticator struct {
+	authenticator.Token
+	spec *authenticationv1alpha1.WebhookAuthenticatorSpec
+}
+
 // New instantiates a new controllerlib.Controller which will populate the provided authncache.Cache.
 func New(
 	cache *authncache.Cache,
@@ -103,33 +109,57 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error {
 		return fmt.Errorf("failed to get WebhookAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err)
 	}
 
+	cacheKey := authncache.Key{
+		APIGroup: authenticationv1alpha1.GroupName,
+		Kind:     "WebhookAuthenticator",
+		Name:     ctx.Key.Name,
+	}
+
+	// If this authenticator already exists, then only recreate it if is different from the desired
+	// authenticator. We don't want to be creating a new authenticator for every resync period.
+	if valueFromCache := c.cache.Get(cacheKey); valueFromCache != nil {
+		webhookAuthenticatorFromCache := c.cacheValueAsWebhookAuthenticator(valueFromCache)
+		if webhookAuthenticatorFromCache != nil && reflect.DeepEqual(webhookAuthenticatorFromCache.spec, &obj.Spec) {
+			c.log.WithValues("webhookAuthenticator", klog.KObj(obj), "endpoint", obj.Spec.Endpoint).
+				Info("actual webhook authenticator and desired webhook authenticator are the same")
+			// Stop, no more work to be done. This authenticator is already validated and cached.
+			return nil
+		}
+	}
+
 	conditions := make([]*metav1.Condition, 0)
 	var errs []error
 
 	certPool, pemBytes, conditions, tlsBundleOk := c.validateTLSBundle(obj.Spec.TLS, conditions)
 	endpointHostPort, conditions, endpointOk := c.validateEndpoint(obj.Spec.Endpoint, conditions)
 	okSoFar := tlsBundleOk && endpointOk
+
 	conditions, tlsNegotiateErr := c.validateConnection(certPool, endpointHostPort, conditions, okSoFar)
 	errs = append(errs, tlsNegotiateErr)
 	okSoFar = okSoFar && tlsNegotiateErr == nil
 
-	webhookAuthenticator, conditions, err := newWebhookAuthenticator(
+	newWebhookAuthenticatorForCache, conditions, err := newWebhookAuthenticator(
 		// Note that we use the whole URL when constructing the webhook client,
-		// not just the host and port that ew validated above. We need the path, etc.
+		// not just the host and port that we validated above. We need the path, etc.
 		obj.Spec.Endpoint,
 		pemBytes,
 		conditions,
 		okSoFar,
 	)
 	errs = append(errs, err)
 
-	if !conditionsutil.HadErrorCondition(conditions) {
-		c.cache.Store(authncache.Key{
-			APIGroup: authenticationv1alpha1.GroupName,
-			Kind:     "WebhookAuthenticator",
-			Name:     ctx.Key.Name,
-		}, webhookAuthenticator)
-		c.log.WithValues("webhook", klog.KObj(obj), "endpoint", obj.Spec.Endpoint).Info("added new webhook authenticator")
+	if conditionsutil.HadErrorCondition(conditions) {
+		// The authenticator was determined to be invalid. Remove it from the cache, in case it was previously
+		// validated and cached. Do not allow an old, previously validated spec of the authenticator to continue
+		// being used for authentication.
+		c.cache.Delete(cacheKey)
+	} else {
+		c.cache.Store(cacheKey, &cachedWebhookAuthenticator{
+			Token: newWebhookAuthenticatorForCache,
+			spec:  obj.Spec.DeepCopy(), // deep copy to avoid caching original object
+		})
+		c.log.WithValues("webhook", klog.KObj(obj), "endpoint", obj.Spec.Endpoint).
+			Info("added new webhook authenticator")
 	}
 
 	err = c.updateStatus(ctx.Context, obj, conditions)
@@ -143,6 +173,19 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error {
 	return utilerrors.NewAggregate(errs)
 }
 
+func (c *webhookCacheFillerController) cacheValueAsWebhookAuthenticator(value authncache.Value) *cachedWebhookAuthenticator {
+	webhookAuthenticator, ok := value.(*cachedWebhookAuthenticator)
+	if !ok {
+		actualType := "<nil>"
+		if t := reflect.TypeOf(value); t != nil {
+			actualType = t.String()
+		}
+		c.log.WithValues("actualType", actualType).Info("wrong webhook authenticator type in cache")
+		return nil
+	}
+	return webhookAuthenticator
+}
+
 // newWebhookAuthenticator creates a webhook from the provided API server url and caBundle
 // used to validate TLS connections.
 func newWebhookAuthenticator(