Skip to content

Commit 5f3dfda

Browse files
committed
Keep the longer hold when a session mint's verdict lands second
A session mint holds no credential lock across its round trip, so another launch's verdict on the same secret can be written while it is in flight. The last write won: an invalid_client refusal, followed by the 429 the abuse tracker answers once those refusals trip it, left a one-minute hold in place of a permanent one, and the dead secret went out again after it. A shorter 429 could likewise cut short a longer one. rememberRenewalHold now keeps a stored hold that still holds and outlasts the new one, and returns it, so the session mint answers with the verdict that stood and tells its caller the wait the next mint will meet.
1 parent 16d1351 commit 5f3dfda

3 files changed

Lines changed: 160 additions & 11 deletions

File tree

‎internal/auth/agent_launch.go‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -204,19 +204,40 @@ func (m *Manager) SessionAccessToken(ctx context.Context, id, label string) (str
204204
token, hold, err := m.mintAgentToken(ctx, mint)
205205
if err != nil {
206206
if hold != nil {
207-
lockErr := m.store.withKeyLock(ctx, credKey, func() error {
208-
m.rememberRenewalHold(credKey, hold)
209-
return nil
210-
})
211-
if lockErr != nil {
212-
m.warnf("warning: could not lock the credential for %s to remember the token endpoint's refusal, so the next command will ask again: %v", credKey, lockErr)
213-
}
207+
err = m.rememberSessionHold(ctx, credKey, hold, err)
214208
}
215209
return "", forSession(launch, m.hintLogin(err))
216210
}
217211
return token.AccessToken, nil
218212
}
219213

214+
// rememberSessionHold writes the hold a session mint's refusal leaves, under
215+
// the credential lock, and returns what the mint answers with: its own
216+
// refusal, or the verdict that stood instead.
217+
//
218+
// With no lock across the round trip, another launch's verdict on the same
219+
// secret can land while this mint is in flight, and one that outlasts this
220+
// mint's stands (rememberRenewalHold). Its answer is then this mint's too,
221+
// so the wait a caller is told is the one the next mint will meet.
222+
func (m *Manager) rememberSessionHold(ctx context.Context, credKey string, hold *RenewalHold, refusal error) error {
223+
var stood *Credentials
224+
lockErr := m.store.withKeyLock(ctx, credKey, func() error {
225+
stood = m.rememberRenewalHold(credKey, hold)
226+
return nil
227+
})
228+
if lockErr != nil {
229+
m.warnf("warning: could not lock the credential for %s to remember the token endpoint's refusal, so the next command will ask again: %v", credKey, lockErr)
230+
}
231+
if stood == nil {
232+
return refusal
233+
}
234+
held := m.heldRenewal(stood)
235+
if held == nil {
236+
return refusal
237+
}
238+
return m.agentRemedy(held, stood.ClientID, stood.Scope)
239+
}
240+
220241
// forSession names the session in a failed session mint's message, keeping
221242
// its class, hint and cause.
222243
func forSession(l *agentLaunch, err error) error {

‎internal/auth/agent_launch_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package auth
22

33
import (
44
"context"
5+
"errors"
56
"net/http"
67
"strings"
78
"testing"
@@ -180,6 +181,100 @@ func TestSessionTokenRefusalOfTheCredentialIsHeld(t *testing.T) {
180181
assert.Len(t, as.tokenCalls(), 1, "a held credential was presented again")
181182
}
182183

184+
// TestConcurrentVerdictsOnTheSecretKeepTheLongerHold: a session mint holds
185+
// no lock across its round trip, so another mint's verdict on the same
186+
// secret can land while it is in flight. Whichever is written last, the
187+
// hold that stands is the one that holds longer, and the session is told
188+
// that one: a dead secret is not sent again a minute later because a 429
189+
// from the abuse tracker it tripped landed after its invalid_client.
190+
func TestConcurrentVerdictsOnTheSecretKeepTheLongerHold(t *testing.T) {
191+
type answer struct {
192+
status int
193+
header http.Header
194+
body string
195+
}
196+
invalidClient := answer{http.StatusUnauthorized, nil, `{"error":"invalid_client"}`}
197+
invalidGrant := answer{http.StatusBadRequest, nil, `{"error":"invalid_grant"}`}
198+
rateLimited := func(seconds string) answer {
199+
return answer{http.StatusTooManyRequests, http.Header{"Retry-After": {seconds}}, `{}`}
200+
}
201+
202+
for name, tc := range map[string]struct {
203+
// early is the verdict that lands while the session's mint is in
204+
// flight, from another launch, or from the shared path when
205+
// shared; late is the session's own.
206+
early, late answer
207+
shared bool
208+
209+
kind string
210+
until time.Duration // from now; zero is never
211+
// stood is whether the session is answered with the earlier
212+
// verdict rather than its own; retryAfter is the wait it is told.
213+
stood bool
214+
retryAfter int
215+
}{
216+
"a 429 after invalid_client": {early: invalidClient, late: rateLimited("60"), kind: renewalHoldRefused, stood: true},
217+
"a 429 after the shared path's refusal": {early: invalidClient, late: rateLimited("60"), shared: true, kind: renewalHoldRefused, stood: true},
218+
"a 429 after invalid_grant": {early: invalidGrant, late: rateLimited("60"), kind: renewalHoldRefused, until: time.Hour, stood: true},
219+
"a shorter 429 after a longer one": {early: rateLimited("1800"), late: rateLimited("60"), kind: renewalHoldRateLimited, until: 30 * time.Minute, stood: true, retryAfter: 1800},
220+
"invalid_client after a 429": {early: rateLimited("60"), late: invalidClient, kind: renewalHoldRefused},
221+
"a longer 429 after a shorter one": {early: rateLimited("60"), late: rateLimited("1800"), kind: renewalHoldRateLimited, until: 30 * time.Minute, retryAfter: 1800},
222+
"invalid_client after the shared path 429": {early: rateLimited("60"), late: invalidClient, shared: true, kind: renewalHoldRefused},
223+
} {
224+
t.Run(name, func(t *testing.T) {
225+
e := startMintEndpoint(t)
226+
first, key, now := heldManager(t, e)
227+
second := newDeviceTestManager(t, e.srv.URL)
228+
second.store, second.clock = first.store, first.clock
229+
start := *now
230+
231+
e.answer = func(call int) (int, http.Header, string) {
232+
a := tc.early
233+
if call == 0 {
234+
// The session's mint is in flight: the other verdict
235+
// lands, and is written, before it is answered.
236+
var err error
237+
if tc.shared {
238+
_, err = first.AccessToken(context.Background())
239+
} else {
240+
_, err = first.SessionAccessToken(context.Background(), "fedcba9876543210fedcba9876543210", "")
241+
}
242+
assert.Error(t, err)
243+
a = tc.late
244+
}
245+
return a.status, a.header, a.body
246+
}
247+
248+
_, err := second.SessionAccessToken(context.Background(), testSessionID, "coworker@box")
249+
require.Error(t, err)
250+
require.EqualValues(t, 2, e.calls.Load())
251+
assert.Contains(t, err.Error(), testSessionID)
252+
assert.Equal(t, tc.stood, errors.Is(err, errRenewalHeld), "whether the session was answered by the hold that stood")
253+
assert.Equal(t, tc.retryAfter, RetryAfter(err), "the wait the session is told")
254+
if tc.kind == renewalHoldRefused {
255+
assert.ErrorIs(t, err, ErrAgentCredentialRefused)
256+
}
257+
258+
stored, loadErr := second.store.Load(key)
259+
require.NoError(t, loadErr)
260+
require.NotNil(t, stored.RenewalHold)
261+
assert.Equal(t, tc.kind, stored.RenewalHold.Kind)
262+
var until int64
263+
if tc.until != 0 {
264+
until = start.Add(tc.until).Unix()
265+
}
266+
assert.Equal(t, until, stored.RenewalHold.Until)
267+
268+
// Past the shorter hold, nothing goes out from either launch.
269+
*now = now.Add(2 * time.Minute)
270+
_, err = second.SessionAccessToken(context.Background(), testSessionID, "coworker@box")
271+
require.Error(t, err)
272+
assert.ErrorIs(t, err, errRenewalHeld)
273+
assert.EqualValues(t, 2, e.calls.Load(), "the shorter hold was written over the longer one")
274+
})
275+
}
276+
}
277+
183278
// TestSessionTokenNeedsAnAgentProfile: only an agent credential mints for
184279
// itself; a person's login has no session to attribute and is not handed
185280
// out in place of one.

‎internal/auth/renewal_hold.go‎

Lines changed: 37 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -393,8 +393,37 @@ func (hold *RenewalHold) holds(creds *Credentials, now time.Time) bool {
393393
}
394394
}
395395

396+
// outlasts reports whether hold, stored on creds, answers mints for at
397+
// least as long as next would: it holds now, and never ends or ends after
398+
// next does.
399+
//
400+
// Two mints of one secret can be in flight at once, and their verdicts land
401+
// in either order, so the later write is not the later word. An
402+
// invalid_client refusal says the secret is dead for good, and a 429 that
403+
// lands after it — the abuse tracker the first refusals tripped answers
404+
// every request after them with one, whatever the secret — must not cut
405+
// that to a minute, after which the dead secret goes out again. Nor may a
406+
// shorter rate limit cut short a longer one the server asked for.
407+
func (hold *RenewalHold) outlasts(next *RenewalHold, creds *Credentials, now time.Time) bool {
408+
switch {
409+
case !hold.holds(creds, now):
410+
return false
411+
case hold.Until == 0:
412+
return true
413+
default:
414+
return next.Until != 0 && hold.Until > next.Until
415+
}
416+
}
417+
396418
// rememberRenewalHold records hold on the stored credential — if the stored
397-
// credential is still the one the verdict was about.
419+
// credential is still the one the verdict was about, and no verdict it
420+
// already carries on it outlasts this one.
421+
//
422+
// It returns the stored credential when such a verdict stands and nothing
423+
// was written, so the caller can answer with it; nil otherwise. Only a mint
424+
// that held no lock across its round trip meets one: a session mint
425+
// (agent_launch.go), or any mint on a host without the lock. Under the lock,
426+
// a standing hold answers the mint before anything is sent.
398427
//
399428
// The caller holds the credential key's cross-process lock, so no login can
400429
// land between the refusal and this write. The fingerprint is compared
@@ -411,23 +440,27 @@ func (hold *RenewalHold) holds(creds *Credentials, now time.Time) bool {
411440
//
412441
// A failure to write is reported and otherwise changes nothing — the
413442
// refusal is what the caller sees either way.
414-
func (m *Manager) rememberRenewalHold(origin string, hold *RenewalHold) {
443+
func (m *Manager) rememberRenewalHold(origin string, hold *RenewalHold) (stood *Credentials) {
415444
current, err := m.store.Load(origin)
416445
if err != nil {
417446
// A credential removed since the mint (a logout) has nothing to
418447
// remember a refusal on; any other failure is worth saying.
419448
if !errors.Is(err, ErrNoCredential) {
420449
m.warnf("warning: could not read the credential for %s to remember the token endpoint's refusal, so the next command will ask again: %v", origin, err)
421450
}
422-
return
451+
return nil
423452
}
424453
if renewalSubject(current) != hold.Client {
425-
return
454+
return nil
455+
}
456+
if current.RenewalHold.outlasts(hold, current, m.now()) {
457+
return current
426458
}
427459
current.RenewalHold = hold
428460
if err := m.store.Save(origin, current); err != nil {
429461
m.warnf("warning: could not remember the token endpoint's refusal for %s, so the next command will ask again: %v", origin, err)
430462
}
463+
return nil
431464
}
432465

433466
// now is the Manager's clock: the clock field when a test set one.

0 commit comments

Comments
 (0)