diff --git a/internal/auth/oidc.go b/internal/auth/oidc.go index 47cafea..86c63d9 100644 --- a/internal/auth/oidc.go +++ b/internal/auth/oidc.go @@ -22,10 +22,10 @@ type BearerOIDC struct { timeout time.Duration backoff time.Duration - mu sync.Mutex - verifier *oidc.IDTokenVerifier - discovering bool - failedAt time.Time + mu sync.Mutex + verifier *oidc.IDTokenVerifier + inflight chan struct{} + failedAt time.Time } var errIssuerUnavailable = errors.New("oidc issuer unavailable") @@ -39,18 +39,34 @@ func NewBearerOIDC(issuer string, audiences []string) *BearerOIDC { } // 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) { +// across network I/O; concurrent callers wait for it, and only callers within +// the post-failure backoff fail fast. +func (b *BearerOIDC) getVerifier(ctx context.Context) (*oidc.IDTokenVerifier, error) { b.mu.Lock() - if b.verifier != nil || b.discovering || time.Since(b.failedAt) < b.backoff { - v := b.verifier + if v := b.verifier; v != nil { b.mu.Unlock() - if v == nil { - return nil, errIssuerUnavailable - } return v, nil } - b.discovering = true + if done := b.inflight; done != nil { + b.mu.Unlock() + select { + case <-done: + case <-ctx.Done(): + return nil, ctx.Err() + } + b.mu.Lock() + defer b.mu.Unlock() + if b.verifier == nil { + return nil, errIssuerUnavailable + } + return b.verifier, nil + } + if time.Since(b.failedAt) < b.backoff { + b.mu.Unlock() + return nil, errIssuerUnavailable + } + done := make(chan struct{}) + b.inflight = done b.mu.Unlock() // The provider keeps this context and client for JWKS refreshes, so the @@ -60,7 +76,8 @@ func (b *BearerOIDC) getVerifier() (*oidc.IDTokenVerifier, error) { b.mu.Lock() defer b.mu.Unlock() - b.discovering = false + b.inflight = nil + defer close(done) if err != nil { b.failedAt = time.Now() slog.Error("oidc discovery failed", "issuer", b.issuer, "error", err) @@ -83,7 +100,7 @@ func (b *BearerOIDC) Require(next http.Handler) http.Handler { unauthorized(w, "missing bearer token") return } - v, err := b.getVerifier() + v, err := b.getVerifier(r.Context()) if err != nil { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusServiceUnavailable) diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index 38ea85a..8c552b6 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "strconv" + "sync" "sync/atomic" "testing" "time" @@ -145,6 +146,41 @@ func TestBearerOIDCHangingIssuerBacksOff(t *testing.T) { } } +func TestBearerOIDCConcurrentFirstRequestsWaitForDiscovery(t *testing.T) { + iss := testsupport.NewOIDCIssuer() + defer iss.Close() + var hits atomic.Int32 + inner := iss.Config.Handler + iss.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/.well-known/openid-configuration" { + hits.Add(1) + time.Sleep(200 * time.Millisecond) + } + inner.ServeHTTP(w, r) + }) + + h := NewBearerOIDC(iss.URL, nil).Require(ok) + tok := iss.Token(nil, "artifactapi", "ci", time.Hour) + var wg sync.WaitGroup + codes := make([]int, 20) + for i := range codes { + wg.Add(1) + go func() { + defer wg.Done() + codes[i] = serve(h, http.MethodPost, tok) + }() + } + wg.Wait() + for i, c := range codes { + if c != http.StatusNoContent { + t.Errorf("request %d: status %d, want 204", i, c) + } + } + if n := hits.Load(); n != 1 { + t.Fatalf("discovery hit %d times, want 1", n) + } +} + func TestBearerOIDCRetriesAfterBackoff(t *testing.T) { iss := testsupport.NewOIDCIssuer() defer iss.Close()