diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 1928451..fca62b3 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -253,12 +253,16 @@ func (a *Auth) login(w http.ResponseWriter, r *http.Request) { Return: returnTo, } now := time.Now() + if err := a.pending.Add(state.State, now); err != nil { + a.logger.WarnContext(r.Context(), "login refused", "error", err) + http.Error(w, "too many sign-ins in progress; try again later", http.StatusServiceUnavailable) + return + } if err := a.cookie.set(w, state, now); err != nil { a.logger.ErrorContext(r.Context(), "set login cookie", "error", err) http.Error(w, "internal error", http.StatusInternalServerError) return } - a.pending.Add(state.State, now) http.Redirect(w, r, a.oauth.AuthCodeURL(state.State, oidc.Nonce(state.Nonce), oauth2.S256ChallengeOption(state.Verifier), diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 2cacb86..9ac6fb9 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -872,6 +872,43 @@ func TestLoginCookieExpires(t *testing.T) { } } +func TestPendingStates(t *testing.T) { + t.Parallel() + p := NewPendingStates(time.Minute) + now := time.Now() + if err := p.Add("first", now); err != nil { + t.Fatal(err) + } + if !p.Consume("first") { + t.Error("Consume of a pending state = false") + } + if p.Consume("first") { + t.Error("second Consume of a state = true") + } + if p.Consume("never-added") { + t.Error("Consume of an unknown state = true") + } + + for i := range maxPendingStates { + if err := p.Add(fmt.Sprint("s", i), now); err != nil { + t.Fatalf("Add %d: %v", i, err) + } + } + if err := p.Add("over", now); !errors.Is(err, ErrTooManyPending) { + t.Errorf("Add past the bound = %v, want ErrTooManyPending", err) + } + // Once the earlier states expire, they are forgotten and a new one fits. + if err := p.Add("later", now.Add(time.Minute)); err != nil { + t.Errorf("Add after expiry: %v", err) + } + if p.Consume("s0") { + t.Error("an expired state was still pending") + } + if !p.Consume("later") { + t.Error("the state added after expiry is not pending") + } +} + func TestLoginRejectsOffOriginReturn(t *testing.T) { t.Parallel() h := newHarness(t) diff --git a/internal/auth/cookie.go b/internal/auth/cookie.go index 32a5473..df00244 100644 --- a/internal/auth/cookie.go +++ b/internal/auth/cookie.go @@ -154,6 +154,15 @@ func (c FlowCookie) Read(r *http.Request, v flowValue, now time.Time) error { return nil } +// maxPendingStates bounds each flow's pending states. The login route is +// unauthenticated, so without a bound anyone could grow the set without +// limit; one household never has close to this many flows in progress. +const maxPendingStates = 10000 + +// ErrTooManyPending means a flow's pending states are at maxPendingStates, +// so a new flow is refused until older ones expire. +var ErrTooManyPending = errors.New("too many sign-in flows in progress") + // PendingStates holds the state of every redirect flow started and not yet // completed, until its flow cookie expires. A copy of a flow cookie verifies // until it expires, so a flow is single-use only because its callback @@ -163,6 +172,15 @@ type PendingStates struct { ttl time.Duration mu sync.Mutex states map[string]time.Time + // order is every added state in the order added. Every state is pending + // for the same ttl, so it is also expiry order and Add forgets expired + // states from its front. + order []pendingState +} + +type pendingState struct { + state string + expires time.Time } // NewPendingStates returns an empty set whose states are pending for ttl, @@ -172,15 +190,23 @@ func NewPendingStates(ttl time.Duration) *PendingStates { } // Add records state, started at now, as pending and forgets expired ones. -func (p *PendingStates) Add(state string, now time.Time) { +// It fails with ErrTooManyPending when maxPendingStates are pending. +func (p *PendingStates) Add(state string, now time.Time) error { p.mu.Lock() defer p.mu.Unlock() - for k, expires := range p.states { - if !now.Before(expires) { - delete(p.states, k) - } + expired := 0 + for expired < len(p.order) && !now.Before(p.order[expired].expires) { + delete(p.states, p.order[expired].state) + expired++ } - p.states[state] = now.Add(p.ttl) + p.order = p.order[expired:] + if len(p.states) >= maxPendingStates { + return ErrTooManyPending + } + expires := now.Add(p.ttl) + p.states[state] = expires + p.order = append(p.order, pendingState{state: state, expires: expires}) + return nil } // Consume removes state from the pending states, reporting whether it was diff --git a/internal/gcal/connect.go b/internal/gcal/connect.go index f7962a8..b97f042 100644 --- a/internal/gcal/connect.go +++ b/internal/gcal/connect.go @@ -209,10 +209,12 @@ func (s *Service) Connect(w http.ResponseWriter, session auth.Session) (string, Session: sessionBinding(session), } now := s.now() + if err := s.pending.Add(state.State, now); err != nil { + return "", fmt.Errorf("gcal: %w", err) + } if err := s.cookie.Set(w, &state, now); err != nil { return "", fmt.Errorf("gcal: set connect cookie: %w", err) } - s.pending.Add(state.State, now) return s.oauth.AuthCodeURL(state.State, oauth2.AccessTypeOffline, oauth2.ApprovalForce,