From bb073eec1b9e801515801bc1e5bef3d928502426 Mon Sep 17 00:00:00 2001 From: ZverGuy Date: Thu, 5 Nov 2026 00:24:30 +0700 Subject: [PATCH] fix(observability-proxy): do not cache reviews that failed to decide A TokenReview whose authenticator failed comes back unauthenticated with status.error set, and a SubjectAccessReview whose authorizer failed comes back with an evaluation error and no decision. Both were cached as refusals, so a valid caller stayed locked out for the cache TTL after the outage ended. Such answers are now refused for the current request only. A review that failed to decide still answers 401 or 403 rather than 503: RBAC sets an evaluation error for any binding to a missing role, which a retryable status would turn into a permanent 503 for that user. Assisted-by: LLM Signed-off-by: ZverGuy --- internal/observabilityproxy/auth.go | 26 ++++++++----- internal/observabilityproxy/auth_test.go | 49 +++++++++++++++++++++++- 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/internal/observabilityproxy/auth.go b/internal/observabilityproxy/auth.go index 90756bdad..0baac558a 100644 --- a/internal/observabilityproxy/auth.go +++ b/internal/observabilityproxy/auth.go @@ -37,8 +37,9 @@ var ErrUnauthenticated = errors.New("unauthenticated") // Authorizer asks kube-apiserver who a token belongs to and whether that // identity may read a scope. Answers, including negative ones, are cached -// for the TTL; transport errors are not, so an apiserver blip is retried on -// the next request instead of locking a user out for the TTL. +// for the TTL; transport errors and reviews that failed to decide are not, +// so an outage is retried on the next request instead of locking a user +// out for the TTL. type Authorizer struct { client kubernetes.Interface now func() time.Time @@ -72,14 +73,16 @@ func (a *Authorizer) Authenticate(ctx context.Context, token string) (*authentic if err != nil { return nil, fmt.Errorf("token review: %w", err) } - var u *authenticationv1.UserInfo - if tr.Status.Authenticated && tr.Status.User.Username != "" { - u = tr.Status.User.DeepCopy() - } - a.tokens.set(key, u, a.now()) - if u == nil { + if !tr.Status.Authenticated || tr.Status.User.Username == "" { + // status.error also marks an authenticator that failed to decide; + // a cached refusal would outlast its recovery. + if tr.Status.Error == "" { + a.tokens.set(key, nil, a.now()) + } return nil, ErrUnauthenticated } + u := tr.Status.User.DeepCopy() + a.tokens.set(key, u, a.now()) return u, nil } @@ -109,7 +112,12 @@ func (a *Authorizer) Authorize(ctx context.Context, user *authenticationv1.UserI return false, fmt.Errorf("subject access review: %w", err) } allowed := sar.Status.Allowed && !sar.Status.Denied - a.decisions.set(key, allowed, a.now()) + // An evaluation error without a decision denies this request only: it + // may be a webhook authorizer that is down, and RBAC reports one for + // any binding to a missing role, so it cannot be told from a refusal. + if allowed || sar.Status.Denied || sar.Status.EvaluationError == "" { + a.decisions.set(key, allowed, a.now()) + } return allowed, nil } diff --git a/internal/observabilityproxy/auth_test.go b/internal/observabilityproxy/auth_test.go index b57b80cf1..fe940ce04 100644 --- a/internal/observabilityproxy/auth_test.go +++ b/internal/observabilityproxy/auth_test.go @@ -38,6 +38,10 @@ type fakeKube struct { sars atomic.Int32 lastSAR atomic.Pointer[authorizationv1.SubjectAccessReviewSpec] reviewFn func() error + // reviewError and evaluationError, when set, are what the + // authenticator or the authorizer report as failing to decide. + reviewError atomic.Pointer[string] + evaluationError atomic.Pointer[string] } func newFakeKube(users map[string]string, allow func(user string, attrs *authorizationv1.ResourceAttributes) bool) *fakeKube { @@ -50,7 +54,9 @@ func newFakeKube(users map[string]string, allow func(user string, attrs *authori } } tr := action.(k8stesting.CreateAction).GetObject().(*authenticationv1.TokenReview).DeepCopy() - if name, ok := users[tr.Spec.Token]; ok { + if e := f.reviewError.Load(); e != nil { + tr.Status = authenticationv1.TokenReviewStatus{Error: *e} + } else if name, ok := users[tr.Spec.Token]; ok { tr.Status = authenticationv1.TokenReviewStatus{Authenticated: true, User: authenticationv1.UserInfo{Username: name, Groups: []string{"tenant-a-view"}}} } return true, tr, nil @@ -60,6 +66,10 @@ func newFakeKube(users map[string]string, allow func(user string, attrs *authori sar := action.(k8stesting.CreateAction).GetObject().(*authorizationv1.SubjectAccessReview).DeepCopy() spec := sar.Spec f.lastSAR.Store(&spec) + if e := f.evaluationError.Load(); e != nil { + sar.Status.EvaluationError = *e + return true, sar, nil + } sar.Status.Allowed = allow(sar.Spec.User, sar.Spec.ResourceAttributes) return true, sar, nil }) @@ -171,3 +181,40 @@ func TestAuthorize(t *testing.T) { t.Fatalf("a different group set must not hit another identity's cache entry, SARs = %d", n) } } + +// TestAuthenticateFailedReviewNotCached: a TokenReview the authenticator +// could not decide carries status.error; caching it would refuse a valid +// token for the TTL after the authenticator recovers. +func TestAuthenticateFailedReviewNotCached(t *testing.T) { + kube := newFakeKube(map[string]string{"good": "alice"}, allowNamespace("tenant-a")) + outage := "webhook authenticator unreachable" + kube.reviewError.Store(&outage) + a := NewAuthorizer(kube.client, 10*time.Second) + ctx := context.Background() + if _, err := a.Authenticate(ctx, "good"); !errors.Is(err, ErrUnauthenticated) { + t.Fatalf("err = %v, want ErrUnauthenticated", err) + } + kube.reviewError.Store(nil) + if u, err := a.Authenticate(ctx, "good"); err != nil || u.Username != "alice" { + t.Fatalf("after recovery: %+v, %v", u, err) + } +} + +// TestAuthorizeEvaluationErrorNotCached: a SubjectAccessReview with an +// evaluation error and no decision is a denial for this request only. +func TestAuthorizeEvaluationErrorNotCached(t *testing.T) { + kube := newFakeKube(map[string]string{"good": "alice"}, allowNamespace("tenant-a")) + failure := "webhook authorizer unreachable" + kube.evaluationError.Store(&failure) + a := NewAuthorizer(kube.client, 10*time.Second) + ctx := context.Background() + user := &authenticationv1.UserInfo{Username: "alice"} + attrs := Route{Kind: NamespaceLogs, Namespace: "tenant-a"}.ResourceAttributes() + if ok, err := a.Authorize(ctx, user, attrs); err != nil || ok { + t.Fatalf("undecided review = %v, %v; want a denial", ok, err) + } + kube.evaluationError.Store(nil) + if ok, err := a.Authorize(ctx, user, attrs); err != nil || !ok { + t.Fatalf("after recovery: %v, %v", ok, err) + } +}