From 51a77e3446742d5f1514156606dc1e771bf6f900 Mon Sep 17 00:00:00 2001 From: unkin-agent Date: Wed, 14 Oct 2026 22:27:20 +1100 Subject: [PATCH] Bound OIDC discovery with a client timeout and failure backoff --- internal/auth/oidc.go | 56 ++++++++++++----- internal/auth/oidc_test.go | 117 ++++++++++++++++++++++++++++++++++- internal/testsupport/oidc.go | 21 ++++--- 3 files changed, 167 insertions(+), 27 deletions(-) diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index a663a8e..47cafea 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -2,6 +2,7 @@ package auth import ( "context" + "errors" "log/slog" "net/http" "slices" @@ -13,35 +14,56 @@ import ( ) // BearerOIDC verifies `Authorization: Bearer ` against an OIDC issuer's -// JWKS. Discovery is lazy and retried per request so startup never depends on -// the issuer being reachable. +// JWKS. Discovery is lazy so startup never depends on the issuer being +// reachable; a failed discovery is not retried until the backoff elapses. type BearerOIDC struct { issuer string audiences []string + timeout time.Duration + backoff time.Duration - mu sync.Mutex - verifier *oidc.IDTokenVerifier + mu sync.Mutex + verifier *oidc.IDTokenVerifier + discovering bool + failedAt time.Time } +var errIssuerUnavailable = errors.New("oidc issuer unavailable") + // NewBearerOIDC returns nil when issuer is empty, meaning auth is disabled. func NewBearerOIDC(issuer string, audiences []string) *BearerOIDC { if issuer == "" { return nil } - return &BearerOIDC{issuer: issuer, audiences: audiences} + return &BearerOIDC{issuer: issuer, audiences: audiences, timeout: 10 * time.Second, backoff: 15 * time.Second} } -func (b *BearerOIDC) getVerifier(ctx context.Context) (*oidc.IDTokenVerifier, error) { +// getVerifier runs at most one discovery at a time without holding the lock +// across network I/O; callers arriving mid-discovery or within the backoff fail fast. +func (b *BearerOIDC) getVerifier() (*oidc.IDTokenVerifier, error) { + b.mu.Lock() + if b.verifier != nil || b.discovering || time.Since(b.failedAt) < b.backoff { + v := b.verifier + b.mu.Unlock() + if v == nil { + return nil, errIssuerUnavailable + } + return v, nil + } + b.discovering = true + b.mu.Unlock() + + // The provider keeps this context and client for JWKS refreshes, so the + // deadline lives on the client rather than the context. + client := &http.Client{Timeout: b.timeout} + p, err := oidc.NewProvider(oidc.ClientContext(context.Background(), client), b.issuer) + b.mu.Lock() defer b.mu.Unlock() - if b.verifier != nil { - return b.verifier, nil - } - ctx, cancel := context.WithTimeout(ctx, 10*time.Second) - defer cancel() - // The provider keeps this context for JWKS refreshes; it must outlive the request. - p, err := oidc.NewProvider(oidc.ClientContext(context.WithoutCancel(ctx), http.DefaultClient), b.issuer) + b.discovering = false if err != nil { + b.failedAt = time.Now() + slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) return nil, err } b.verifier = p.Verifier(&oidc.Config{SkipClientIDCheck: true}) @@ -55,14 +77,14 @@ func (b *BearerOIDC) Require(next http.Handler) http.Handler { return next } return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - raw, ok := strings.CutPrefix(r.Header.Get("Authorization"), "Bearer ") - if !ok || raw == "" { + scheme, raw, _ := strings.Cut(strings.TrimSpace(r.Header.Get("Authorization")), " ") + raw = strings.TrimSpace(raw) + if !strings.EqualFold(scheme, "Bearer") || raw == "" { unauthorized(w, "missing bearer token") return } - v, err := b.getVerifier(r.Context()) + v, err := b.getVerifier() if err != nil { - slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusServiceUnavailable) _, _ = w.Write([]byte(`{"error":"token issuer unavailable"}`)) diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 8fff37b..38ea85a 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -3,24 +3,67 @@ package auth import ( "crypto/rand" "crypto/rsa" + "encoding/base64" "net/http" "net/http/httptest" + "strconv" + "sync/atomic" "testing" "time" + "github.com/go-jose/go-jose/v4" + "github.com/go-jose/go-jose/v4/jwt" + "git.unkin.net/unkin/artifactapi/internal/testsupport" ) func serve(h http.Handler, method, token string) int { + if token == "" { + return serveAuth(h, method, "") + } + return serveAuth(h, method, "Bearer "+token) +} + +func serveAuth(h http.Handler, method, authz string) int { r := httptest.NewRequest(method, "/x", nil) - if token != "" { - r.Header.Set("Authorization", "Bearer "+token) + if authz != "" { + r.Header.Set("Authorization", authz) } w := httptest.NewRecorder() h.ServeHTTP(w, r) return w.Code } +func claims(iss string) jwt.Claims { + now := time.Now() + return jwt.Claims{ + Issuer: iss, + Subject: "ci", + Audience: jwt.Audience{"artifactapi"}, + IssuedAt: jwt.NewNumericDate(now.Add(-time.Minute)), + Expiry: jwt.NewNumericDate(now.Add(time.Hour)), + } +} + +func hs256Token(t *testing.T, c jwt.Claims) string { + sig, err := jose.NewSigner(jose.SigningKey{Algorithm: jose.HS256, Key: []byte("0123456789abcdef0123456789abcdef")}, + (&jose.SignerOptions{}).WithType("JWT").WithHeader("kid", "k1")) + if err != nil { + t.Fatal(err) + } + tok, err := jwt.Signed(sig).Claims(c).Serialize() + if err != nil { + t.Fatal(err) + } + return tok +} + +func noneToken(iss string) string { + enc := base64.RawURLEncoding.EncodeToString + return enc([]byte(`{"alg":"none","typ":"JWT","kid":"k1"}`)) + "." + + enc([]byte(`{"iss":"`+iss+`","sub":"ci","aud":"artifactapi","exp":`+strconv.FormatInt(time.Now().Add(time.Hour).Unix(), 10)+`}`)) + "." +} + var ok = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusNoContent) }) func TestBearerOIDC(t *testing.T) { @@ -40,12 +83,82 @@ func TestBearerOIDC(t *testing.T) { {"bad signature", iss.Token(otherKey, "artifactapi", "ci", time.Hour), http.StatusUnauthorized}, {"wrong aud", iss.Token(nil, "nope", "ci", time.Hour), http.StatusUnauthorized}, {"expired", iss.Token(nil, "artifactapi", "ci", -time.Hour), http.StatusUnauthorized}, + {"wrong iss", iss.Sign(nil, claims("https://evil.example")), http.StatusUnauthorized}, + {"hs256", hs256Token(t, claims(iss.URL)), http.StatusUnauthorized}, + {"alg none", noneToken(iss.URL), http.StatusUnauthorized}, } for _, c := range cases { if got := serve(h, http.MethodPost, c.token); got != c.want { t.Errorf("%s: status %d, want %d", c.name, got, c.want) } } + + valid := iss.Token(nil, "artifactapi", "ci", time.Hour) + headers := []struct { + name string + authz string + want int + }{ + {"lowercase scheme", "bearer " + valid, http.StatusNoContent}, + {"extra whitespace", " Bearer " + valid + " ", http.StatusNoContent}, + {"empty value", "Bearer ", http.StatusUnauthorized}, + {"scheme only", "Bearer", http.StatusUnauthorized}, + {"basic scheme", "Basic " + valid, http.StatusUnauthorized}, + } + for _, c := range headers { + if got := serveAuth(h, http.MethodPost, c.authz); got != c.want { + t.Errorf("%s: status %d, want %d", c.name, got, c.want) + } + } +} + +func TestBearerOIDCHangingIssuerBacksOff(t *testing.T) { + var hits atomic.Int32 + release := make(chan struct{}) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + hits.Add(1) + select { + case <-r.Context().Done(): + case <-release: + } + })) + defer srv.Close() + defer close(release) + + b := NewBearerOIDC(srv.URL, nil) + b.timeout = 100 * time.Millisecond + b.backoff = time.Hour + h := b.Require(ok) + + start := time.Now() + if got := serve(h, http.MethodPost, "x.y.z"); got != http.StatusServiceUnavailable { + t.Fatalf("hanging issuer: status %d, want 503", got) + } + if d := time.Since(start); d > 2*time.Second { + t.Fatalf("hanging issuer: took %s, timeout not applied", d) + } + if got := serve(h, http.MethodPost, "x.y.z"); got != http.StatusServiceUnavailable { + t.Fatalf("during backoff: status %d, want 503", got) + } + if n := hits.Load(); n != 1 { + t.Fatalf("issuer hit %d times, want 1", n) + } +} + +func TestBearerOIDCRetriesAfterBackoff(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + b := NewBearerOIDC(iss.URL, nil) + b.failedAt = time.Now() + b.backoff = 50 * time.Millisecond + tok := iss.Token(nil, "artifactapi", "ci", time.Hour) + if got := serve(b.Require(ok), http.MethodPost, tok); got != http.StatusServiceUnavailable { + t.Fatalf("within backoff: status %d, want 503", got) + } + time.Sleep(60 * time.Millisecond) + if got := serve(b.Require(ok), http.MethodPost, tok); got != http.StatusNoContent { + t.Fatalf("after backoff: status %d, want 204", got) + } } func TestBearerOIDCIssuerDown(t *testing.T) { diff --git a/internal/testsupport/oidc.go b/internal/testsupport/oidc.go index 0ea124a..0acf546 100644 --- a/internal/testsupport/oidc.go +++ b/internal/testsupport/oidc.go @@ -45,6 +45,18 @@ func NewOIDCIssuer() *OIDCIssuer { // Token signs a token for aud and sub. Pass a nil key to use the issuer's own; // expiry is relative to now. func (o *OIDCIssuer) Token(key *rsa.PrivateKey, aud, sub string, expiry time.Duration) string { + now := time.Now() + return o.Sign(key, jwt.Claims{ + Issuer: o.URL, + Subject: sub, + Audience: jwt.Audience{aud}, + IssuedAt: jwt.NewNumericDate(now.Add(-time.Minute)), + Expiry: jwt.NewNumericDate(now.Add(expiry)), + }) +} + +// Sign signs arbitrary claims with RS256. Pass a nil key to use the issuer's own. +func (o *OIDCIssuer) Sign(key *rsa.PrivateKey, claims jwt.Claims) string { if key == nil { key = o.key } @@ -53,14 +65,7 @@ func (o *OIDCIssuer) Token(key *rsa.PrivateKey, aud, sub string, expiry time.Dur if err != nil { panic(err) } - now := time.Now() - tok, err := jwt.Signed(sig).Claims(jwt.Claims{ - Issuer: o.URL, - Subject: sub, - Audience: jwt.Audience{aud}, - IssuedAt: jwt.NewNumericDate(now.Add(-time.Minute)), - Expiry: jwt.NewNumericDate(now.Add(expiry)), - }).Serialize() + tok, err := jwt.Signed(sig).Claims(claims).Serialize() if err != nil { panic(err) }