diff --git a/README.md b/README.md index 50f465f..6e51b53 100644 --- a/README.md +++ b/README.md @@ -296,6 +296,25 @@ The API for this provider: The provider acts like any other, i.e. will be registered as `/auth/email/login`. +#### Confirmation token replay protection + +Confirmation tokens are one-shot: a token redeemed once cannot be redeemed again within its TTL. This stops anyone with read access to the email link (forwarded mail, mail-gateway logs, mailbox archive) from independently consuming it after the user has. + +By default the library installs an in-memory store on first call to `AddVerifProvider`. This is correct for **single-instance** deployments. + +**Multi-instance deployments behind a load balancer MUST supply a shared backend.** With the default in-memory store, replay protection works only on the instance that originally consumed the token; an attacker who hits any other instance can still replay within the TTL. **This is silent: the request succeeds and the auth flow completes normally, with no log indicating the protection was bypassed.** Plug in Redis or any shared KV by setting `Opts.VerifConfirmationStore` to a value implementing `provider.VerifConfirmationStore`: + +```go +type VerifConfirmationStore interface { + // MarkUsed records key as consumed and returns alreadyUsed=true if it was + // already recorded. err signals a backend failure -- callers fail-closed + // (reject the redemption) on non-nil err to avoid replay during outages. + MarkUsed(key string, ttl time.Duration) (alreadyUsed bool, err error) +} +``` + +The store key is the SHA-256 of the raw confirmation token, so existing tokens issued before this protection landed are de-dup'd correctly without changing the wire format. Consumption is final: a transient downstream failure after the mark burns the token; the user must request a new confirmation email rather than retry the same link. + #### Email-as-identity caveat The verify provider returns a local user id of the form `_`. The confirmation round-trip proves current control of the address at the moment of login; it does **not** prove stable+unique identity over time. The owner of an address can change without the address changing: diff --git a/auth.go b/auth.go index 8ad2813..17faba7 100644 --- a/auth.go +++ b/auth.go @@ -7,6 +7,7 @@ import ( "net/url" "regexp" "strings" + "sync" "time" "github.com/go-pkgz/rest" @@ -26,14 +27,16 @@ type Client struct { // Service provides higher level wrapper allowing to construct everything and get back token middleware type Service struct { - logger logger.L - opts Opts - jwtService *token.Service - providers []provider.Service - authMiddleware middleware.Authenticator - avatarProxy *avatar.Proxy - issuer string - useGravatar bool + logger logger.L + opts Opts + jwtService *token.Service + providers []provider.Service + authMiddleware middleware.Authenticator + avatarProxy *avatar.Proxy + issuer string + useGravatar bool + verifConfirmStore provider.VerifConfirmationStore + verifConfirmStoreOnce sync.Once } // Opts is a full set of all parameters to initialize Service @@ -83,6 +86,15 @@ type Opts struct { AudSecrets bool // allow multiple secrets (secret per aud) Logger logger.L // logger interface, default is no logging at all RefreshCache middleware.RefreshCache // optional cache to keep refreshed tokens + + // VerifConfirmationStore enforces one-shot consumption of email + // confirmation tokens issued by the verify provider. The default + // (nil) installs an in-memory store on first use of AddVerifProvider — + // fine for single-instance deployments. Multi-instance deployments + // MUST supply a shared backend (e.g. Redis) implementing + // provider.VerifConfirmationStore, otherwise replay rejection works + // only on the instance that consumed the token. + VerifConfirmationStore provider.VerifConfirmationStore } // NewService initializes everything @@ -434,6 +446,13 @@ func (s *Service) AddDirectProviderWithUserIDFunc(name string, credChecker provi // AddVerifProvider adds provider user's verification sent by sender func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) { + s.verifConfirmStoreOnce.Do(func() { + if s.opts.VerifConfirmationStore != nil { + s.verifConfirmStore = s.opts.VerifConfirmationStore + return + } + s.verifConfirmStore = provider.NewInMemoryVerifStore() + }) dh := provider.VerifyHandler{ L: s.logger, ProviderName: name, @@ -445,6 +464,7 @@ func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) UseGravatar: s.useGravatar, URL: s.opts.URL, AllowedRedirectHosts: s.opts.AllowedRedirectHosts, + ConfirmationStore: s.verifConfirmStore, } s.addProvider(dh) } diff --git a/auth_test.go b/auth_test.go index 184afa7..7ea8335 100644 --- a/auth_test.go +++ b/auth_test.go @@ -117,6 +117,39 @@ func TestService_AddMicrosoftProvider(t *testing.T) { }) } +func TestService_AddVerifProvider_UsesCustomConfirmationStore(t *testing.T) { + custom := &countingVerifStore{} + svc := NewService(Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + URL: "http://127.0.0.1", + Logger: logger.Std, + VerifConfirmationStore: custom, + }) + svc.AddVerifProvider("email", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + assert.Same(t, custom, svc.verifConfirmStore, "custom store from Opts must be used, not the in-memory default") +} + +func TestService_AddVerifProvider_DefaultsToInMemory(t *testing.T) { + svc := NewService(Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + URL: "http://127.0.0.1", + Logger: logger.Std, + }) + svc.AddVerifProvider("email", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + require.NotNil(t, svc.verifConfirmStore, "default store must be installed when Opts.VerifConfirmationStore is nil") + // second call must reuse the same store (sync.Once guards against re-init) + first := svc.verifConfirmStore + svc.AddVerifProvider("email2", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + assert.Same(t, first, svc.verifConfirmStore, "subsequent AddVerifProvider calls must reuse the same store") +} + +type countingVerifStore struct{ calls int } + +func (s *countingVerifStore) MarkUsed(string, time.Duration) (bool, error) { + s.calls++ + return false, nil +} + func TestService_AddAppleProvider(t *testing.T) { options := Opts{ diff --git a/provider/verify.go b/provider/verify.go index f947e38..1801db0 100644 --- a/provider/verify.go +++ b/provider/verify.go @@ -3,10 +3,13 @@ package provider import ( "bytes" "crypto/sha1" + "crypto/sha256" + "encoding/hex" "fmt" "html/template" "net/http" "strings" + "sync" "time" "github.com/go-pkgz/rest" @@ -50,6 +53,105 @@ type VerifyHandler struct { // here. Nil disables validation and preserves legacy permissive // behavior — any non-empty "from" value is honored. AllowedRedirectHosts token.AllowedHosts + + // ConfirmationStore enforces one-shot consumption of confirmation tokens. + // When non-nil, a token cannot be redeemed twice within its TTL window. + // Leave nil to keep the legacy behavior (token replayable until expiry). + ConfirmationStore VerifConfirmationStore +} + +// VerifConfirmationStore tracks consumed confirmation tokens to prevent replay. +// Implementations must be safe for concurrent use. +type VerifConfirmationStore interface { + // MarkUsed records key as consumed and returns alreadyUsed=true if it was + // already recorded. The implementation MUST retain the marker for at + // least the supplied ttl, or return a non-nil err if it cannot -- + // dropping a marker before its ttl while the underlying JWT is still + // valid reopens the replay window the store is meant to close. err + // signals a backend failure (network, disk, capacity, etc.); callers + // MUST treat a non-nil err as fail-closed (reject the redemption). + MarkUsed(key string, ttl time.Duration) (alreadyUsed bool, err error) +} + +// VerifConfirmationStoreFunc is an adapter to use ordinary functions as +// VerifConfirmationStore, mirroring the SenderFunc / token.AllowedHostsFunc +// house pattern for closure-based config. +type VerifConfirmationStoreFunc func(key string, ttl time.Duration) (alreadyUsed bool, err error) + +// MarkUsed calls f(key, ttl) to implement VerifConfirmationStore. +func (f VerifConfirmationStoreFunc) MarkUsed(key string, ttl time.Duration) (bool, error) { + return f(key, ttl) +} + +// NewInMemoryVerifStore returns a process-local default VerifConfirmationStore. +// Suitable for single-instance deployments. Multi-instance deployments behind +// a load balancer MUST supply a shared backend (e.g. Redis) -- otherwise an +// attacker who lands on a different instance from the legitimate user can +// replay the token there. The default's failure is silent: the request +// completes normally and no log indicates the protection was bypassed. +func NewInMemoryVerifStore() VerifConfirmationStore { + return &inMemoryVerifStore{used: make(map[string]time.Time)} +} + +type inMemoryVerifStore struct { + mu sync.Mutex + used map[string]time.Time // key -> expiry + insertCount int +} + +// inMemoryVerifStoreSweepEvery is the in-memory store's amortization cadence. +// Walking the whole map on every redemption is O(n) under a single mutex, +// which serializes the hot path. Sweeping every N inserts keeps the map size +// bounded by ~N + (concurrent redemptions during the gap) without holding +// the lock through a full walk on most calls. Declared as a var rather than +// a const so tests can lower it to exercise the sweep branch. +var inMemoryVerifStoreSweepEvery = 256 + +func (s *inMemoryVerifStore) MarkUsed(key string, ttl time.Duration) (bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + now := time.Now() + if exp, ok := s.used[key]; ok && exp.After(now) { + return true, nil + } + // amortized eviction: walk the map only every Nth insert, not on every + // hot-path call. The lookup above already rejects unexpired duplicates, + // so worst-case staleness is bounded by N inserts between sweeps. + s.insertCount++ + if s.insertCount >= inMemoryVerifStoreSweepEvery { + s.insertCount = 0 + for k, exp := range s.used { + if !exp.After(now) { + delete(s.used, k) + } + } + } + s.used[key] = now.Add(ttl) + return false, nil +} + +// confirmationKey hashes the raw token so the store key length is bounded +// regardless of token size, and so the in-memory map doesn't retain the +// signed token itself. +func confirmationKey(rawToken string) string { + sum := sha256.Sum256([]byte(rawToken)) + return hex.EncodeToString(sum[:]) +} + +// scrubTokenFromRequest returns a shallow clone of r with the "token" query +// parameter replaced by "". rest.SendErrorJSON logs r.URL, and the +// fail-closed branches in LoginHandler fire while the confirmation JWT is +// still live (store didn't record consumption) -- a single log line equals +// an unredeemed magic link without this scrub. +func scrubTokenFromRequest(r *http.Request) *http.Request { + if r == nil || r.URL == nil || r.URL.Query().Get("token") == "" { + return r + } + rc := r.Clone(r.Context()) + q := rc.URL.Query() + q.Set("token", "") + rc.URL.RawQuery = q.Encode() + return rc } // Sender defines interface to send emails @@ -78,7 +180,13 @@ type VerifTokenService interface { func (e VerifyHandler) Name() string { return e.ProviderName } // LoginHandler gets name and address from query, makes confirmation token and sends it to user. -// In case if confirmation token presented in the query uses it to create auth token +// In case if confirmation token presented in the query uses it to create auth token. +// +// Consumption is final when ConfirmationStore is configured: the token is +// marked used before any further side effects (avatar fetch, token issuance), +// so a transient downstream failure burns the token and the user must request +// a new confirmation email rather than retry the same link. This trade-off +// keeps the replay check atomic with the security boundary. func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) { // GET /login?site=site&user=name&address=someone@example.com @@ -101,6 +209,34 @@ func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) { return } + store := e.ConfirmationStore + // guard against a typed-nil VerifConfirmationStoreFunc: a non-nil + // interface wrapping a nil func survives the != nil check above and + // would panic at MarkUsed. Treat it as no store configured. Mirrors + // the AllowedHostsFunc nil-guard in token/jwt.go. + if fn, ok := store.(VerifConfirmationStoreFunc); ok && fn == nil { + store = nil + } + if store != nil { + ttl := time.Until(time.Unix(confClaims.ExpiresAt, 0)) + if ttl <= 0 { + ttl = time.Minute + } + alreadyUsed, markErr := store.MarkUsed(confirmationKey(tkn), ttl) + if markErr != nil { + // fail-closed: a backend outage must not let attackers replay + // tokens. Reject with the token scrubbed from the logged URL, + // since on this branch the store did NOT record consumption so + // the JWT in the URL is still live. + rest.SendErrorJSON(w, scrubTokenFromRequest(r), e.L, http.StatusForbidden, markErr, "confirmation token store unavailable") + return + } + if alreadyUsed { + rest.SendErrorJSON(w, scrubTokenFromRequest(r), e.L, http.StatusForbidden, fmt.Errorf("token already used"), "confirmation token already consumed") + return + } + } + elems := strings.Split(confClaims.Handshake.ID, "::") if len(elems) != 2 { rest.SendErrorJSON(w, r, e.L, http.StatusBadRequest, fmt.Errorf("%s", confClaims.Handshake.ID), "invalid handshake token") diff --git a/provider/verify_test.go b/provider/verify_test.go index 43c9c71..05aa253 100644 --- a/provider/verify_test.go +++ b/provider/verify_test.go @@ -6,6 +6,8 @@ import ( "net/http/httptest" "net/url" "strings" + "sync" + "sync/atomic" "testing" "time" @@ -259,6 +261,187 @@ func TestVerifyHandler_LoginAcceptConfirmFromAllowsAllowlistedHost(t *testing.T) assert.Equal(t, "https://trusted.example.com/back", rr.Header().Get("Location")) } +func TestInMemoryVerifStore(t *testing.T) { + mark := func(s VerifConfirmationStore, k string, ttl time.Duration) bool { + used, err := s.MarkUsed(k, ttl) + require.NoError(t, err) + return used + } + + t.Run("first MarkUsed returns false, second returns true", func(t *testing.T) { + s := NewInMemoryVerifStore() + assert.False(t, mark(s, "k1", time.Hour), "first call must mark and return not-used") + assert.True(t, mark(s, "k1", time.Hour), "second call must report already-used") + }) + t.Run("expired entry is not considered used", func(t *testing.T) { + s := NewInMemoryVerifStore() + assert.False(t, mark(s, "k1", time.Nanosecond)) + time.Sleep(2 * time.Millisecond) + assert.False(t, mark(s, "k1", time.Hour), "expired entry should be reusable") + }) + t.Run("distinct keys are independent", func(t *testing.T) { + s := NewInMemoryVerifStore() + assert.False(t, mark(s, "k1", time.Hour)) + assert.False(t, mark(s, "k2", time.Hour)) + assert.True(t, mark(s, "k1", time.Hour)) + assert.True(t, mark(s, "k2", time.Hour)) + }) + t.Run("concurrent same-key redemption: exactly one succeeds", func(t *testing.T) { + s := NewInMemoryVerifStore() + const goroutines = 50 + var wg sync.WaitGroup + successes := int32(0) + errs := int32(0) + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + used, err := s.MarkUsed("hot-key", time.Hour) + if err != nil { + atomic.AddInt32(&errs, 1) + return + } + if !used { + atomic.AddInt32(&successes, 1) + } + }() + } + wg.Wait() + assert.EqualValues(t, 0, errs) + assert.EqualValues(t, 1, successes, "exactly one redemption must observe alreadyUsed=false") + }) + t.Run("amortized sweep evicts expired entries", func(t *testing.T) { + orig := inMemoryVerifStoreSweepEvery + inMemoryVerifStoreSweepEvery = 4 + defer func() { inMemoryVerifStoreSweepEvery = orig }() + + s := NewInMemoryVerifStore().(*inMemoryVerifStore) + // 3 inserts with nanosecond TTL — quickly expire, no sweep yet + // (insertCount=3 < 4) + for i := 0; i < 3; i++ { + used, err := s.MarkUsed(fmt.Sprintf("expired-%d", i), time.Nanosecond) + require.NoError(t, err) + require.False(t, used) + } + time.Sleep(5 * time.Millisecond) // let them expire + s.mu.Lock() + require.Equal(t, 3, len(s.used), "sanity: pre-sweep map holds the 3 expired entries") + s.mu.Unlock() + + // 4th insert hits the sweep threshold, should evict the 3 expired + _, err := s.MarkUsed("fresh", time.Hour) + require.NoError(t, err) + + s.mu.Lock() + assert.Equal(t, 1, len(s.used), "sweep evicted the expired entries; only fresh remains") + s.mu.Unlock() + }) +} + +func TestVerifConfirmationStoreFunc(t *testing.T) { + calls := 0 + var lastKey string + var lastTTL time.Duration + var s VerifConfirmationStore = VerifConfirmationStoreFunc(func(key string, ttl time.Duration) (bool, error) { + calls++ + lastKey, lastTTL = key, ttl + return calls > 1, nil + }) + + used, err := s.MarkUsed("k1", 5*time.Minute) + require.NoError(t, err) + assert.False(t, used) + assert.Equal(t, "k1", lastKey) + assert.Equal(t, 5*time.Minute, lastTTL) + + used, err = s.MarkUsed("k1", 5*time.Minute) + require.NoError(t, err) + assert.True(t, used) + assert.Equal(t, 2, calls) +} + +func TestVerifyHandler_LoginAcceptConfirm_TypedNilStoreFuncDoesNotPanic(t *testing.T) { + // Opts.VerifConfirmationStore can be set to a typed-nil + // VerifConfirmationStoreFunc, which is a non-nil interface wrapping a + // nil func. The handler must treat that as "no store configured" and + // fall back to legacy replayable behavior, not panic. + var nilFn VerifConfirmationStoreFunc + e := VerifyHandler{ + ProviderName: "test", + TokenService: token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }), + Issuer: "iss-test", + L: logger.Std, + ConfirmationStore: nilFn, + } + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + require.NotPanics(t, func() { + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + }) + assert.Equal(t, 200, rr.Code, "typed-nil store func must behave like no store") +} + +func TestVerifyHandler_LoginAcceptConfirm_FailClosedOnStoreError(t *testing.T) { + // MarkUsed returning a non-nil err is the security-critical fail-closed + // branch: a backend (e.g. Redis) outage MUST reject the redemption to + // avoid replay during the outage window. + e := VerifyHandler{ + ProviderName: "test", + TokenService: token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }), + Issuer: "iss-test", + L: logger.Std, + ConfirmationStore: VerifConfirmationStoreFunc(func(string, time.Duration) (bool, error) { + return false, fmt.Errorf("redis down") + }), + } + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + assert.Equal(t, 403, rr.Code, "non-nil markErr must fail closed") + assert.Contains(t, rr.Body.String(), "store unavailable") +} + +func TestVerifyHandler_LoginAcceptConfirm_RejectsReplay(t *testing.T) { + e := VerifyHandler{ + ProviderName: "test", + TokenService: token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }), + Issuer: "iss-test", + L: logger.Std, + ConfirmationStore: NewInMemoryVerifStore(), + } + + handler := http.HandlerFunc(e.LoginHandler) + + // first use: success + rr1 := httptest.NewRecorder() + req1, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + handler.ServeHTTP(rr1, req1) + require.Equal(t, 200, rr1.Code, "first consumption must succeed") + + // second use: must be rejected + rr2 := httptest.NewRecorder() + req2, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + handler.ServeHTTP(rr2, req2) + assert.Equal(t, 403, rr2.Code, "replay must be rejected") + assert.Contains(t, rr2.Body.String(), "already") +} + func TestVerifyHandler_LoginAcceptConfirmWithAvatar(t *testing.T) { e := VerifyHandler{ ProviderName: "test", diff --git a/v2/auth.go b/v2/auth.go index a9302d2..0449915 100644 --- a/v2/auth.go +++ b/v2/auth.go @@ -7,6 +7,7 @@ import ( "net/url" "regexp" "strings" + "sync" "time" "github.com/go-pkgz/rest" @@ -26,14 +27,16 @@ type Client struct { // Service provides higher level wrapper allowing to construct everything and get back token middleware type Service struct { - logger logger.L - opts Opts - jwtService *token.Service - providers []provider.Service - authMiddleware middleware.Authenticator - avatarProxy *avatar.Proxy - issuer string - useGravatar bool + logger logger.L + opts Opts + jwtService *token.Service + providers []provider.Service + authMiddleware middleware.Authenticator + avatarProxy *avatar.Proxy + issuer string + useGravatar bool + verifConfirmStore provider.VerifConfirmationStore + verifConfirmStoreOnce sync.Once } // Opts is a full set of all parameters to initialize Service @@ -84,6 +87,15 @@ type Opts struct { Logger logger.L // logger interface, default is no logging at all RefreshCache middleware.RefreshCache // optional cache to keep refreshed tokens ErrorHandler middleware.ErrorHandlerFunc // custom error handler for auth failures + + // VerifConfirmationStore enforces one-shot consumption of email + // confirmation tokens issued by the verify provider. The default + // (nil) installs an in-memory store on first use of AddVerifProvider — + // fine for single-instance deployments. Multi-instance deployments + // MUST supply a shared backend (e.g. Redis) implementing + // provider.VerifConfirmationStore, otherwise replay rejection works + // only on the instance that consumed the token. + VerifConfirmationStore provider.VerifConfirmationStore } // NewService initializes everything @@ -435,6 +447,13 @@ func (s *Service) AddDirectProviderWithUserIDFunc(name string, credChecker provi // AddVerifProvider adds provider user's verification sent by sender func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) { + s.verifConfirmStoreOnce.Do(func() { + if s.opts.VerifConfirmationStore != nil { + s.verifConfirmStore = s.opts.VerifConfirmationStore + return + } + s.verifConfirmStore = provider.NewInMemoryVerifStore() + }) dh := provider.VerifyHandler{ L: s.logger, ProviderName: name, @@ -446,6 +465,7 @@ func (s *Service) AddVerifProvider(name, msgTmpl string, sender provider.Sender) UseGravatar: s.useGravatar, URL: s.opts.URL, AllowedRedirectHosts: s.opts.AllowedRedirectHosts, + ConfirmationStore: s.verifConfirmStore, } s.addProvider(dh) } diff --git a/v2/auth_test.go b/v2/auth_test.go index 00f3e2d..946fc42 100644 --- a/v2/auth_test.go +++ b/v2/auth_test.go @@ -117,6 +117,38 @@ func TestService_AddMicrosoftProvider(t *testing.T) { }) } +func TestService_AddVerifProvider_UsesCustomConfirmationStore(t *testing.T) { + custom := &countingVerifStore{} + svc := NewService(Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + URL: "http://127.0.0.1", + Logger: logger.Std, + VerifConfirmationStore: custom, + }) + svc.AddVerifProvider("email", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + assert.Same(t, custom, svc.verifConfirmStore, "custom store from Opts must be used, not the in-memory default") +} + +func TestService_AddVerifProvider_DefaultsToInMemory(t *testing.T) { + svc := NewService(Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + URL: "http://127.0.0.1", + Logger: logger.Std, + }) + svc.AddVerifProvider("email", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + require.NotNil(t, svc.verifConfirmStore, "default store must be installed when Opts.VerifConfirmationStore is nil") + first := svc.verifConfirmStore + svc.AddVerifProvider("email2", "{{.Token}}", provider.SenderFunc(func(string, string) error { return nil })) + assert.Same(t, first, svc.verifConfirmStore, "subsequent AddVerifProvider calls must reuse the same store") +} + +type countingVerifStore struct{ calls int } + +func (s *countingVerifStore) MarkUsed(string, time.Duration) (bool, error) { + s.calls++ + return false, nil +} + func TestService_AddAppleProvider(t *testing.T) { options := Opts{ diff --git a/v2/provider/verify.go b/v2/provider/verify.go index 737c60b..9b945bc 100644 --- a/v2/provider/verify.go +++ b/v2/provider/verify.go @@ -3,10 +3,13 @@ package provider import ( "bytes" "crypto/sha1" + "crypto/sha256" + "encoding/hex" "fmt" "html/template" "net/http" "strings" + "sync" "time" "github.com/go-pkgz/rest" @@ -50,6 +53,105 @@ type VerifyHandler struct { // here. Nil disables validation and preserves legacy permissive // behavior — any non-empty "from" value is honored. AllowedRedirectHosts token.AllowedHosts + + // ConfirmationStore enforces one-shot consumption of confirmation tokens. + // When non-nil, a token cannot be redeemed twice within its TTL window. + // Leave nil to keep the legacy behavior (token replayable until expiry). + ConfirmationStore VerifConfirmationStore +} + +// VerifConfirmationStore tracks consumed confirmation tokens to prevent replay. +// Implementations must be safe for concurrent use. +type VerifConfirmationStore interface { + // MarkUsed records key as consumed and returns alreadyUsed=true if it was + // already recorded. The implementation MUST retain the marker for at + // least the supplied ttl, or return a non-nil err if it cannot -- + // dropping a marker before its ttl while the underlying JWT is still + // valid reopens the replay window the store is meant to close. err + // signals a backend failure (network, disk, capacity, etc.); callers + // MUST treat a non-nil err as fail-closed (reject the redemption). + MarkUsed(key string, ttl time.Duration) (alreadyUsed bool, err error) +} + +// VerifConfirmationStoreFunc is an adapter to use ordinary functions as +// VerifConfirmationStore, mirroring the SenderFunc / token.AllowedHostsFunc +// house pattern for closure-based config. +type VerifConfirmationStoreFunc func(key string, ttl time.Duration) (alreadyUsed bool, err error) + +// MarkUsed calls f(key, ttl) to implement VerifConfirmationStore. +func (f VerifConfirmationStoreFunc) MarkUsed(key string, ttl time.Duration) (bool, error) { + return f(key, ttl) +} + +// NewInMemoryVerifStore returns a process-local default VerifConfirmationStore. +// Suitable for single-instance deployments. Multi-instance deployments behind +// a load balancer MUST supply a shared backend (e.g. Redis) -- otherwise an +// attacker who lands on a different instance from the legitimate user can +// replay the token there. The default's failure is silent: the request +// completes normally and no log indicates the protection was bypassed. +func NewInMemoryVerifStore() VerifConfirmationStore { + return &inMemoryVerifStore{used: make(map[string]time.Time)} +} + +type inMemoryVerifStore struct { + mu sync.Mutex + used map[string]time.Time // key -> expiry + insertCount int +} + +// inMemoryVerifStoreSweepEvery is the in-memory store's amortization cadence. +// Walking the whole map on every redemption is O(n) under a single mutex, +// which serializes the hot path. Sweeping every N inserts keeps the map size +// bounded by ~N + (concurrent redemptions during the gap) without holding +// the lock through a full walk on most calls. Declared as a var rather than +// a const so tests can lower it to exercise the sweep branch. +var inMemoryVerifStoreSweepEvery = 256 + +func (s *inMemoryVerifStore) MarkUsed(key string, ttl time.Duration) (bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + now := time.Now() + if exp, ok := s.used[key]; ok && exp.After(now) { + return true, nil + } + // amortized eviction: walk the map only every Nth insert, not on every + // hot-path call. The lookup above already rejects unexpired duplicates, + // so worst-case staleness is bounded by N inserts between sweeps. + s.insertCount++ + if s.insertCount >= inMemoryVerifStoreSweepEvery { + s.insertCount = 0 + for k, exp := range s.used { + if !exp.After(now) { + delete(s.used, k) + } + } + } + s.used[key] = now.Add(ttl) + return false, nil +} + +// confirmationKey hashes the raw token so the store key length is bounded +// regardless of token size, and so the in-memory map doesn't retain the +// signed token itself. +func confirmationKey(rawToken string) string { + sum := sha256.Sum256([]byte(rawToken)) + return hex.EncodeToString(sum[:]) +} + +// scrubTokenFromRequest returns a shallow clone of r with the "token" query +// parameter replaced by "". rest.SendErrorJSON logs r.URL, and the +// fail-closed branches in LoginHandler fire while the confirmation JWT is +// still live (store didn't record consumption) -- a single log line equals +// an unredeemed magic link without this scrub. +func scrubTokenFromRequest(r *http.Request) *http.Request { + if r == nil || r.URL == nil || r.URL.Query().Get("token") == "" { + return r + } + rc := r.Clone(r.Context()) + q := rc.URL.Query() + q.Set("token", "") + rc.URL.RawQuery = q.Encode() + return rc } // Sender defines interface to send emails @@ -78,7 +180,13 @@ type VerifTokenService interface { func (e VerifyHandler) Name() string { return e.ProviderName } // LoginHandler gets name and address from query, makes confirmation token and sends it to user. -// In case if confirmation token presented in the query uses it to create auth token +// In case if confirmation token presented in the query uses it to create auth token. +// +// Consumption is final when ConfirmationStore is configured: the token is +// marked used before any further side effects (avatar fetch, token issuance), +// so a transient downstream failure burns the token and the user must request +// a new confirmation email rather than retry the same link. This trade-off +// keeps the replay check atomic with the security boundary. func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) { // GET /login?site=site&user=name&address=someone@example.com @@ -101,6 +209,36 @@ func (e VerifyHandler) LoginHandler(w http.ResponseWriter, r *http.Request) { return } + store := e.ConfirmationStore + // guard against a typed-nil VerifConfirmationStoreFunc: a non-nil + // interface wrapping a nil func survives the != nil check above and + // would panic at MarkUsed. Treat it as no store configured. Mirrors + // the AllowedHostsFunc nil-guard in token/jwt.go. + if fn, ok := store.(VerifConfirmationStoreFunc); ok && fn == nil { + store = nil + } + if store != nil { + ttl := time.Minute + if confClaims.ExpiresAt != nil { + if remaining := time.Until(confClaims.ExpiresAt.Time); remaining > 0 { + ttl = remaining + } + } + alreadyUsed, markErr := store.MarkUsed(confirmationKey(tkn), ttl) + if markErr != nil { + // fail-closed: a backend outage must not let attackers replay + // tokens. Reject with the token scrubbed from the logged URL, + // since on this branch the store did NOT record consumption so + // the JWT in the URL is still live. + rest.SendErrorJSON(w, scrubTokenFromRequest(r), e.L, http.StatusForbidden, markErr, "confirmation token store unavailable") + return + } + if alreadyUsed { + rest.SendErrorJSON(w, scrubTokenFromRequest(r), e.L, http.StatusForbidden, fmt.Errorf("token already used"), "confirmation token already consumed") + return + } + } + elems := strings.Split(confClaims.Handshake.ID, "::") if len(elems) != 2 { rest.SendErrorJSON(w, r, e.L, http.StatusBadRequest, fmt.Errorf("%s", confClaims.Handshake.ID), "invalid handshake token") diff --git a/v2/provider/verify_test.go b/v2/provider/verify_test.go index b9a4edd..e4dab09 100644 --- a/v2/provider/verify_test.go +++ b/v2/provider/verify_test.go @@ -6,6 +6,8 @@ import ( "net/http/httptest" "net/url" "strings" + "sync" + "sync/atomic" "testing" "time" @@ -180,6 +182,187 @@ func TestVerifyHandler_PublicFromFlow_RejectsExternalHost(t *testing.T) { assert.NotContains(t, rr2.Body.String(), "evil.example.com") } +func TestInMemoryVerifStore(t *testing.T) { + mark := func(s VerifConfirmationStore, k string, ttl time.Duration) bool { + used, err := s.MarkUsed(k, ttl) + require.NoError(t, err) + return used + } + + t.Run("first MarkUsed returns false, second returns true", func(t *testing.T) { + s := NewInMemoryVerifStore() + assert.False(t, mark(s, "k1", time.Hour), "first call must mark and return not-used") + assert.True(t, mark(s, "k1", time.Hour), "second call must report already-used") + }) + t.Run("expired entry is not considered used", func(t *testing.T) { + s := NewInMemoryVerifStore() + assert.False(t, mark(s, "k1", time.Nanosecond)) + time.Sleep(2 * time.Millisecond) + assert.False(t, mark(s, "k1", time.Hour), "expired entry should be reusable") + }) + t.Run("distinct keys are independent", func(t *testing.T) { + s := NewInMemoryVerifStore() + assert.False(t, mark(s, "k1", time.Hour)) + assert.False(t, mark(s, "k2", time.Hour)) + assert.True(t, mark(s, "k1", time.Hour)) + assert.True(t, mark(s, "k2", time.Hour)) + }) + t.Run("concurrent same-key redemption: exactly one succeeds", func(t *testing.T) { + s := NewInMemoryVerifStore() + const goroutines = 50 + var wg sync.WaitGroup + successes := int32(0) + errs := int32(0) + wg.Add(goroutines) + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + used, err := s.MarkUsed("hot-key", time.Hour) + if err != nil { + atomic.AddInt32(&errs, 1) + return + } + if !used { + atomic.AddInt32(&successes, 1) + } + }() + } + wg.Wait() + assert.EqualValues(t, 0, errs) + assert.EqualValues(t, 1, successes, "exactly one redemption must observe alreadyUsed=false") + }) + t.Run("amortized sweep evicts expired entries", func(t *testing.T) { + orig := inMemoryVerifStoreSweepEvery + inMemoryVerifStoreSweepEvery = 4 + defer func() { inMemoryVerifStoreSweepEvery = orig }() + + s := NewInMemoryVerifStore().(*inMemoryVerifStore) + // 3 inserts with nanosecond TTL — quickly expire, no sweep yet + // (insertCount=3 < 4) + for i := 0; i < 3; i++ { + used, err := s.MarkUsed(fmt.Sprintf("expired-%d", i), time.Nanosecond) + require.NoError(t, err) + require.False(t, used) + } + time.Sleep(5 * time.Millisecond) // let them expire + s.mu.Lock() + require.Equal(t, 3, len(s.used), "sanity: pre-sweep map holds the 3 expired entries") + s.mu.Unlock() + + // 4th insert hits the sweep threshold, should evict the 3 expired + _, err := s.MarkUsed("fresh", time.Hour) + require.NoError(t, err) + + s.mu.Lock() + assert.Equal(t, 1, len(s.used), "sweep evicted the expired entries; only fresh remains") + s.mu.Unlock() + }) +} + +func TestVerifConfirmationStoreFunc(t *testing.T) { + calls := 0 + var lastKey string + var lastTTL time.Duration + var s VerifConfirmationStore = VerifConfirmationStoreFunc(func(key string, ttl time.Duration) (bool, error) { + calls++ + lastKey, lastTTL = key, ttl + return calls > 1, nil + }) + + used, err := s.MarkUsed("k1", 5*time.Minute) + require.NoError(t, err) + assert.False(t, used) + assert.Equal(t, "k1", lastKey) + assert.Equal(t, 5*time.Minute, lastTTL) + + used, err = s.MarkUsed("k1", 5*time.Minute) + require.NoError(t, err) + assert.True(t, used) + assert.Equal(t, 2, calls) +} + +func TestVerifyHandler_LoginAcceptConfirm_TypedNilStoreFuncDoesNotPanic(t *testing.T) { + // Opts.VerifConfirmationStore can be set to a typed-nil + // VerifConfirmationStoreFunc, which is a non-nil interface wrapping a + // nil func. The handler must treat that as "no store configured" and + // fall back to legacy replayable behavior, not panic. + var nilFn VerifConfirmationStoreFunc + e := VerifyHandler{ + ProviderName: "test", + TokenService: token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }), + Issuer: "iss-test", + L: logger.Std, + ConfirmationStore: nilFn, + } + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + require.NotPanics(t, func() { + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + }) + assert.Equal(t, 200, rr.Code, "typed-nil store func must behave like no store") +} + +func TestVerifyHandler_LoginAcceptConfirm_FailClosedOnStoreError(t *testing.T) { + // MarkUsed returning a non-nil err is the security-critical fail-closed + // branch: a backend (e.g. Redis) outage MUST reject the redemption to + // avoid replay during the outage window. + e := VerifyHandler{ + ProviderName: "test", + TokenService: token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }), + Issuer: "iss-test", + L: logger.Std, + ConfirmationStore: VerifConfirmationStoreFunc(func(string, time.Duration) (bool, error) { + return false, fmt.Errorf("redis down") + }), + } + rr := httptest.NewRecorder() + req, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + http.HandlerFunc(e.LoginHandler).ServeHTTP(rr, req) + assert.Equal(t, 403, rr.Code, "non-nil markErr must fail closed") + assert.Contains(t, rr.Body.String(), "store unavailable") +} + +func TestVerifyHandler_LoginAcceptConfirm_RejectsReplay(t *testing.T) { + e := VerifyHandler{ + ProviderName: "test", + TokenService: token.NewService(token.Opts{ + SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }), + TokenDuration: time.Hour, + CookieDuration: time.Hour * 24 * 31, + }), + Issuer: "iss-test", + L: logger.Std, + ConfirmationStore: NewInMemoryVerifStore(), + } + + handler := http.HandlerFunc(e.LoginHandler) + + // first use: success + rr1 := httptest.NewRecorder() + req1, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + handler.ServeHTTP(rr1, req1) + require.Equal(t, 200, rr1.Code, "first consumption must succeed") + + // second use: must be rejected + rr2 := httptest.NewRecorder() + req2, err := http.NewRequest("GET", "/login?token="+testConfirmedToken, http.NoBody) + require.NoError(t, err) + handler.ServeHTTP(rr2, req2) + assert.Equal(t, 403, rr2.Code, "replay must be rejected") + assert.Contains(t, rr2.Body.String(), "already") +} + func TestVerifyHandler_LoginAcceptConfirmFromRejectsExternalHost(t *testing.T) { jwtSvc := token.NewService(token.Opts{ SecretReader: token.SecretFunc(func(string) (string, error) { return "secret", nil }),