Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix authenticators bug: stop allowing usage when validation fails #2013

Merged
merged 2 commits into from
Jul 17, 2024
Merged

fix authenticators bug: stop allowing usage when validation fails #2013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b5a509f
fix authenticators bug: stop allowing usage when validation fails
cfryanr Jul 12, 2024
a2be4b7
clarify some comments based on PR feedback
cfryanr Jul 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 41 additions & 27 deletions internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,9 @@
}

func (c *cachedJWTAuthenticator) Close() {
if c == nil {
return
}
c.cancel()
}

Expand Down Expand Up @@ -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

Check warning on line 173 in internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

View check run for this annotation

Codecov / codecov/patch

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go#L173 

Added line #L173 was not covered by tests
return fmt.Errorf("failed to get JWTAuthenticator %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err)
}

Expand All @@ -177,35 +180,37 @@
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 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()
// Only revalidate and update the cache if the cached authenticator is different from the desired authenticator.
// There is no need to repeat validations for a spec that was already successfully validated. We are making a
// design decision to avoid repeating the validation which dials the server, even though the server's TLS
// configuration could have changed, because it is also possible that the network could be flaky. We are choosing
// to prefer to keep the authenticator cached (available for end-user auth attempts) during times of network flakes
// rather than trying to show the most up-to-date status possible. These validations 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.
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

Expand All @@ -217,21 +222,30 @@
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. We avoid calling Close() until it is
// removed from the cache, because we do not want any end-user authentications to use a closed authenticator.
jwtAuthenticatorFromCache.Close()
joshuatcasey marked this conversation as resolved.
Show resolved Hide resolved

err = c.updateStatus(ctx.Context, obj, conditions)
errs = append(errs, err)

Expand All @@ -243,7 +257,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>"
Expand All @@ -256,7 +270,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())
Expand Down
57 changes: 56 additions & 1 deletion internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,14 +998,17 @@ func TestController(t *testing.T) {
runTestsOnResultingAuthenticator: false, // skip the tests because the authenticator left in the cache is the mock version that was added above
},
{
name: "Sync: JWTAuthenticator update when cached authenticator is different type: loop will complete successfully and update status conditions.",
name: "Sync: authenticator update when cached authenticator is the wrong data type, which should never really happen: loop will complete successfully and update status conditions.",
cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) {
cache.Store(
authncache.Key{
Name: "test-name",
Kind: "JWTAuthenticator",
APIGroup: authenticationv1alpha1.SchemeGroupVersion.Group,
},
// Only entries of type cachedJWTAuthenticator are ever put into the cache, so this should never really happen.
// This test is to provide coverage on the production code which reads from the cache and casts those entries to
// the appropriate data type.
struct{ authenticator.Token }{},
)
},
Expand Down Expand Up @@ -1143,6 +1146,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{
Expand Down
67 changes: 58 additions & 9 deletions internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"crypto/x509"
"fmt"
"net/url"
"reflect"
"time"

k8sauthv1beta1 "k8s.io/api/authentication/v1beta1"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -103,33 +109,63 @@ 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,
}

// Only revalidate and update the cache if the cached authenticator is different from the desired authenticator.
// There is no need to repeat validations for a spec that was already successfully validated. We are making a
// design decision to avoid repeating the validation which dials the server, even though the server's TLS
// configuration could have changed, because it is also possible that the network could be flaky. We are choosing
// to prefer to keep the authenticator cached (available for end-user auth attempts) during times of network flakes
// rather than trying to show the most up-to-date status possible. These validations 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.
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)
Expand All @@ -143,6 +179,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(
Expand Down
Loading
Loading