Bound OIDC discovery with a client timeout and failure backoff

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 <jwt>` 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"}`))

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) {

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)
 	}