fix authenticators bug: stop allowing usage when validation fails

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,19 +109,38 @@ 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,
@@ -123,13 +148,18 @@ func (c *webhookCacheFillerController) Sync(ctx controllerlib.Context) error {
 	)
 	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(

internal/controller/authenticator/webhookcachefiller/webhookcachefiller_test.go

@@ -24,6 +24,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apiserver/pkg/authentication/authenticator"
 	coretesting "k8s.io/client-go/testing"
 	clocktesting "k8s.io/utils/clock/testing"
 	"k8s.io/utils/ptr"
@@ -360,6 +361,7 @@ func TestController(t *testing.T) {
 
 	tests := []struct {
 		name     string
+		cache    func(*testing.T, *authncache.Cache)
 		syncKey  controllerlib.Key
 		webhooks []runtime.Object
 		// for modifying the clients to hack in arbitrary api responses
@@ -424,7 +426,122 @@ func TestController(t *testing.T) {
 			wantCacheEntries: 1,
 		},
 		{
-			name:    "Sync: changed WebhookAuthenticator: loop will update timestamps only on relevant statuses",
+			name: "Sync: valid and unchanged WebhookAuthenticator which was already cached: skips any updates to status or cache",
+			cache: func(t *testing.T, cache *authncache.Cache) {
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "WebhookAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					&cachedWebhookAuthenticator{spec: &goodWebhookAuthenticatorSpecWithCA},
+				)
+			},
+			syncKey: controllerlib.Key{Name: "test-name"},
+			webhooks: []runtime.Object{
+				&authenticationv1alpha1.WebhookAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: goodWebhookAuthenticatorSpecWithCA,
+					Status: authenticationv1alpha1.WebhookAuthenticatorStatus{
+						Conditions: allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 0),
+						Phase:      "Ready",
+					},
+				},
+			},
+			wantLogs: []map[string]any{
+				{
+					"level":     "info",
+					"timestamp": "2099-08-08T13:57:36.123456Z",
+					"logger":    "webhookcachefiller-controller",
+					"message":   "actual webhook authenticator and desired webhook authenticator are the same",
+					"endpoint":  goodWebhookDefaultServingCertEndpoint,
+					"webhook": map[string]any{
+						"name": "test-name",
+					},
+				},
+			},
+			wantActions: func() []coretesting.Action {
+				return []coretesting.Action{
+					coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}),
+				}
+			},
+			wantCacheEntries: 1,
+		},
+		{
+			name: "Sync: authenticator update when cached authenticator is different type: loop will complete successfully and update status conditions.",
+			cache: func(t *testing.T, cache *authncache.Cache) {
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "WebhookAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					struct{ authenticator.Token }{},
+				)
+			},
+			syncKey: controllerlib.Key{Name: "test-name"},
+			webhooks: []runtime.Object{
+				&authenticationv1alpha1.WebhookAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: goodWebhookAuthenticatorSpecWithCA,
+				},
+			},
+			wantLogs: []map[string]any{
+				{
+					"level":      "info",
+					"timestamp":  "2099-08-08T13:57:36.123456Z",
+					"logger":     "webhookcachefiller-controller",
+					"message":    "wrong webhook authenticator type in cache",
+					"actualType": "struct { authenticator.Token }",
+				},
+				{
+					"level":     "info",
+					"timestamp": "2099-08-08T13:57:36.123456Z",
+					"logger":    "webhookcachefiller-controller",
+					"message":   "added new webhook authenticator",
+					"endpoint":  goodWebhookDefaultServingCertEndpoint,
+					"webhook": map[string]any{
+						"name": "test-name",
+					},
+				},
+			},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &authenticationv1alpha1.WebhookAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: goodWebhookAuthenticatorSpecWithCA,
+					Status: authenticationv1alpha1.WebhookAuthenticatorStatus{
+						Conditions: allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 0),
+						Phase:      "Ready",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			wantCacheEntries: 1,
+		},
+		{
+			name: "Sync: changed WebhookAuthenticator: loop will update timestamps only on relevant statuses",
+			cache: func(t *testing.T, cache *authncache.Cache) {
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "WebhookAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					&cachedWebhookAuthenticator{spec: &goodWebhookAuthenticatorSpecWith404Endpoint},
+				)
+			},
 			syncKey: controllerlib.Key{Name: "test-name"},
 			webhooks: []runtime.Object{
 				&authenticationv1alpha1.WebhookAuthenticator{
@@ -662,6 +779,60 @@ 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) {
+				cache.Store(
+					authncache.Key{
+						Name:     "test-name",
+						Kind:     "WebhookAuthenticator",
+						APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
+					},
+					&cachedWebhookAuthenticator{spec: &goodWebhookAuthenticatorSpecWithCA},
+				)
+			},
+			syncKey: controllerlib.Key{Name: "test-name"},
+			webhooks: []runtime.Object{
+				&authenticationv1alpha1.WebhookAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: authenticationv1alpha1.WebhookAuthenticatorSpec{
+						Endpoint: badEndpointInvalidURL,
+					},
+				},
+			},
+			wantActions: func() []coretesting.Action {
+				updateStatusAction := coretesting.NewUpdateAction(webhookAuthenticatorGVR, "", &authenticationv1alpha1.WebhookAuthenticator{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-name",
+					},
+					Spec: authenticationv1alpha1.WebhookAuthenticatorSpec{
+						Endpoint: badEndpointInvalidURL,
+					},
+					Status: authenticationv1alpha1.WebhookAuthenticatorStatus{
+						Conditions: conditionstestutil.Replace(
+							allHappyConditionsSuccess(goodWebhookDefaultServingCertEndpoint, frozenMetav1Now, 0),
+							[]metav1.Condition{
+								happyTLSConfigurationValidNoCA(frozenMetav1Now, 0),
+								sadEndpointURLValid("https://.café   .com/café/café/café/coffee", frozenMetav1Now, 0),
+								unknownWebhookConnectionValid(frozenMetav1Now, 0),
+								unknownAuthenticatorValid(frozenMetav1Now, 0),
+								sadReadyCondition(frozenMetav1Now, 0),
+							},
+						),
+						Phase: "Error",
+					},
+				})
+				updateStatusAction.Subresource = "status"
+				return []coretesting.Action{
+					coretesting.NewListAction(webhookAuthenticatorGVR, webhookAuthenticatorGVK, "", metav1.ListOptions{}),
+					coretesting.NewWatchAction(webhookAuthenticatorGVR, "", metav1.ListOptions{}),
+					updateStatusAction,
+				}
+			},
+			wantCacheEntries: 0, // removed from cache
+		},
 		{
 			name:    "validateEndpoint: parsing error (spec.endpoint URL is invalid) will fail sync loop and will report failed and unknown conditions and Error phase, but will not enqueue a resync due to user config error",
 			syncKey: controllerlib.Key{Name: "test-name"},
@@ -1319,6 +1490,10 @@ func TestController(t *testing.T) {
 			var log bytes.Buffer
 			logger := plog.TestLogger(t, &log)
 
+			if tt.cache != nil {
+				tt.cache(t, cache)
+			}
+
 			controller := New(
 				cache,
 				pinnipedAPIClient,