Make login OAuth state single-use #193

4 changed files with 77 additions and 8 deletions
Showing only changes of commit 0f7f8014f8 - Show all commits
+5 -1
View File
@@ -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),
+37
View File
@@ -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)
+32 -6
View File
@@ -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
+3 -1
View File
@@ -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,