diff --git a/internal/web/oidc/discovery_retry_test.go b/internal/web/oidc/discovery_retry_test.go new file mode 100644 index 0000000..4c2a646 --- /dev/null +++ b/internal/web/oidc/discovery_retry_test.go @@ -0,0 +1,61 @@ +package oidc_test + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/stretchr/testify/require" + + "git.d-ma.be/mathias/tapir/internal/web/oidc" +) + +func testConfig(issuer string) oidc.Config { + return oidc.Config{ + Issuer: issuer, + ClientID: testClientID, + ClientSecret: "test-client-secret", + RedirectURL: "http://tapir.test/auth/callback", + SessionSecret: "test-session-secret-please-change", + } +} + +// #59: a discovery 503 at start-up is retried instead of failing New. +func TestNewRetriesDiscoveryAfter503UntilIssuerAnswers(t *testing.T) { + f := newFakeIssuer(t) + f.unavailableFor = 2 + + auth, err := oidc.New(context.Background(), testConfig(f.server.URL), + oidc.WithDiscoveryRetry(5, time.Millisecond)) + + require.NoError(t, err) + require.NotNil(t, auth) + require.Equal(t, int32(3), f.discoveryHits.Load()) +} + +func TestNewReturnsDiscoveryErrorAfterAttemptsExhausted(t *testing.T) { + f := newFakeIssuer(t) + f.unavailableFor = 1000 + + _, err := oidc.New(context.Background(), testConfig(f.server.URL), + oidc.WithDiscoveryRetry(3, time.Millisecond)) + + require.ErrorContains(t, err, "503") + require.Equal(t, int32(3), f.discoveryHits.Load()) +} + +func TestNewStopsDiscoveryBackoffWhenContextCancelled(t *testing.T) { + f := newFakeIssuer(t) + f.unavailableFor = 1000 + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + start := time.Now() + _, err := oidc.New(ctx, testConfig(f.server.URL), + oidc.WithDiscoveryRetry(5, time.Hour)) + + require.True(t, errors.Is(err, context.DeadlineExceeded), "err = %v", err) + require.Less(t, time.Since(start), 5*time.Second) + require.Equal(t, int32(1), f.discoveryHits.Load()) +} diff --git a/internal/web/oidc/oidc.go b/internal/web/oidc/oidc.go index 45b1eaa..97bfb79 100644 --- a/internal/web/oidc/oidc.go +++ b/internal/web/oidc/oidc.go @@ -57,6 +57,10 @@ const ( pendingTTL = 10 * time.Minute sessionCookie = "tapir_session" loginPath = "/auth/login" + + // Discovery retry (#59): 5 tries, 2+4+8+16 s back-off ≈ 30 s, then fail. + defaultDiscoveryAttempts = 5 + defaultDiscoveryDelay = 2 * time.Second ) // DexAuth implements web.Auth against Dex. @@ -70,6 +74,9 @@ type DexAuth struct { sessionTTL time.Duration now func() time.Time insecure bool // tests only: drop the Secure attribute so cookies flow over http + + discoveryAttempts int + discoveryDelay time.Duration } var _ web.Auth = (*DexAuth)(nil) @@ -96,14 +103,55 @@ func WithSessionTTL(ttl time.Duration) Option { } } +// WithDiscoveryRetry overrides the discovery attempt count and the initial +// back-off delay (doubled after each failed attempt). +func WithDiscoveryRetry(attempts int, delay time.Duration) Option { + return func(d *DexAuth) { + if attempts > 0 { + d.discoveryAttempts = attempts + } + if delay > 0 { + d.discoveryDelay = delay + } + } +} + // WithInsecureCookies drops the Secure cookie attribute. TEST ONLY — it lets // the session cookie travel over plain http; never use it in production. func WithInsecureCookies() Option { return func(d *DexAuth) { d.insecure = true } } -// New discovers the issuer and returns a configured DexAuth. ctx bounds the -// discovery request only. +// discover runs OIDC discovery up to attempts times, doubling delay between +// tries (#59: an issuer briefly unavailable behind its proxy answered 503 and +// serve exited). A cancelled ctx ends the back-off immediately with ctx's error. +func discover(ctx context.Context, issuer string, attempts int, delay time.Duration) (*oidc.Provider, error) { + var lastErr error + for i := range attempts { + if i > 0 { + t := time.NewTimer(delay) + select { + case <-ctx.Done(): + t.Stop() + return nil, fmt.Errorf("after %d attempts (last: %v): %w", i, lastErr, ctx.Err()) + case <-t.C: + } + delay *= 2 + } + p, err := oidc.NewProvider(ctx, issuer) + if err == nil { + return p, nil + } + lastErr = err + if ctx.Err() != nil { + return nil, fmt.Errorf("after %d attempts: %w", i+1, ctx.Err()) + } + } + return nil, fmt.Errorf("after %d attempts: %w", attempts, lastErr) +} + +// New discovers the issuer and returns a configured DexAuth. ctx bounds discovery, +// including its retry back-off. func New(ctx context.Context, cfg Config, opts ...Option) (*DexAuth, error) { for name, val := range map[string]string{ "issuer": cfg.Issuer, @@ -117,29 +165,31 @@ func New(ctx context.Context, cfg Config, opts ...Option) (*DexAuth, error) { } } - provider, err := oidc.NewProvider(ctx, cfg.Issuer) - if err != nil { - return nil, fmt.Errorf("oidc: discover issuer: %w", err) - } - d := &DexAuth{ - cfg: cfg, - verifier: provider.Verifier(&oidc.Config{ClientID: cfg.ClientID}), - oauth: &oauth2.Config{ - ClientID: cfg.ClientID, - ClientSecret: cfg.ClientSecret, - Endpoint: provider.Endpoint(), - RedirectURL: cfg.RedirectURL, - Scopes: []string{oidc.ScopeOpenID, "profile", "email"}, - }, - pending: newPendingStore(), - secret: []byte(cfg.SessionSecret), - sessionTTL: defaultSessionTTL, - now: time.Now, + cfg: cfg, + pending: newPendingStore(), + secret: []byte(cfg.SessionSecret), + sessionTTL: defaultSessionTTL, + now: time.Now, + discoveryAttempts: defaultDiscoveryAttempts, + discoveryDelay: defaultDiscoveryDelay, } for _, o := range opts { o(d) } + + provider, err := discover(ctx, cfg.Issuer, d.discoveryAttempts, d.discoveryDelay) + if err != nil { + return nil, fmt.Errorf("oidc: discover issuer: %w", err) + } + d.verifier = provider.Verifier(&oidc.Config{ClientID: cfg.ClientID}) + d.oauth = &oauth2.Config{ + ClientID: cfg.ClientID, + ClientSecret: cfg.ClientSecret, + Endpoint: provider.Endpoint(), + RedirectURL: cfg.RedirectURL, + Scopes: []string{oidc.ScopeOpenID, "profile", "email"}, + } return d, nil } diff --git a/internal/web/oidc/oidc_test.go b/internal/web/oidc/oidc_test.go index 44a38c6..5c90139 100644 --- a/internal/web/oidc/oidc_test.go +++ b/internal/web/oidc/oidc_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "sync/atomic" "testing" "time" @@ -34,6 +35,11 @@ type fakeIssuer struct { sub string email string nonce string + + // discoveryHits counts requests to the discovery document; the first + // unavailableFor of them answer 503 (issuer briefly down behind its proxy). + discoveryHits atomic.Int32 + unavailableFor int32 } const testKID = "test-key" @@ -47,6 +53,10 @@ func newFakeIssuer(t *testing.T) *fakeIssuer { mux := http.NewServeMux() mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, _ *http.Request) { + if f.discoveryHits.Add(1) <= f.unavailableFor { + http.Error(w, "no available server", http.StatusServiceUnavailable) + return + } writeJSON(t, w, map[string]any{ "issuer": f.server.URL, "authorization_endpoint": f.server.URL + "/authorize",