From 044347d190903d8f2d3441e12316c90b3cf525fb Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 25 Sep 2026 07:07:42 -0500 Subject: [PATCH 1/2] Remember the token endpoint's refusal of an agent's secret An agent credential renews by minting with its client secret, and every process mints for itself. Nothing remembered a refusal, so once a secret was rotated server-side, anything that polled through the CLI presented the dead secret on every poll, indefinitely: in production, one agent's rotated secret was sent about 350 times an hour for more than twelve hours, nearly all of it answered 429 by the abuse tracker the first 401s had tripped. The verdict is now kept on the stored credential (MintHold) and answered locally, before any network I/O, for the client credentials it was about: - invalid_client: held until a login stores a different secret. bc3's rotate! and disconnect! replace or clear the digest and a quarantine is one-way, so nothing makes that secret good again. - invalid_grant (bc3's "Agent is no longer active") and a bare 401/403: held an hour, then tried once, so a reactivated account recovers on its own. - 429: held until Retry-After (60s default, capped), answered as a retryable rate limit. - 5xx, network: nothing held, retried as before. A successful mint clears the hold, and the hold names a fingerprint of the client id and secret, so a new secret is never held to the old one's refusal. The write happens under the credential key's lock, against a fresh read, and only while that read still holds the refused secret. auth status reports the remembered refusal and when it lifts. --- internal/auth/agent.go | 112 +++++-- internal/auth/agent_hold.go | 224 +++++++++++++ internal/auth/agent_hold_test.go | 343 ++++++++++++++++++++ internal/auth/agent_test.go | 3 + internal/auth/auth.go | 4 + internal/auth/keyring.go | 7 + internal/commands/auth.go | 13 + internal/commands/auth_status_agent_test.go | 66 ++++ 8 files changed, 741 insertions(+), 31 deletions(-) create mode 100644 internal/auth/agent_hold.go create mode 100644 internal/auth/agent_hold_test.go diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 60f14e171..07f66bf33 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -31,6 +31,13 @@ import ( // produce, in the OS keyring wherever one is available — the same place // every other profile's refresh token lives. // +// Because every renewal presents the secret again, a secret the server has +// stopped accepting would be presented again by every command after it — +// and by every poll of anything automated that runs one. The token +// endpoint's refusals are therefore remembered with the credential and +// answered locally until the secret changes, or for as long as the server +// asked; see mintAgentCredential and agent_hold.go. +// // The tokens come back bound to the agent with an RFC 8707 resource // indicator of the form urn:bc:agent:. Nothing here parses or requires // that shape: whatever the server binds the token to is stored and echoed @@ -103,6 +110,13 @@ func (m *Manager) resolveAgentMint(creds *Credentials) (*agentMint, error) { return nil, output.ErrAuth("Agent credentials are missing their token endpoint and cannot mint a token") } + // A verdict the token endpoint already gave on these client + // credentials is answered here, before anything is resolved or sent — + // which is also what lets a report say the next command will not ask. + if err := m.heldMint(creds); err != nil { + return nil, err + } + // The token endpoint is a persisted value and receives the client // secret, so it passes the same strict check every other stored OAuth // endpoint does before a byte goes out. @@ -131,24 +145,48 @@ func (m *Manager) resolveAgentMint(creds *Credentials) (*agentMint, error) { // mintAgentCredential replaces creds' access token with a freshly minted // one and stores the result. // -// Nothing is written unless the mint succeeded, and nothing is ever -// deleted: a mint that fails — the network is down, the server is having a -// bad minute, the secret was rotated out from under us — leaves the stored -// credential exactly as it was, so the next command tries again with the -// same client credentials rather than finding an empty store and an -// instruction to log in. There is no invalid_grant equivalent to forget -// here: a refused client_credentials request says the CLIENT is wrong, and -// the operator's remedy is to re-run the login with a good secret, which -// overwrites the credential anyway. +// Nothing is ever deleted: a mint that fails leaves the client credentials +// where they are, rather than an empty store and an instruction to log in. +// There is no invalid_grant equivalent to forget here — a refused +// client_credentials request says the CLIENT is wrong, and the operator's +// remedy is to re-run the login with a good secret, which overwrites the +// credential anyway. +// +// But a refusal is not forgotten either. The token endpoint's verdict is +// remembered on the stored credential (MintHold, agent_hold.go), and the +// next mint answers it locally, before any network I/O: // -// The caller holds m.mu and the credential key's cross-process lock. +// - invalid_client refuses the secret, and no later state of the server +// makes that secret good again. Held until a login stores another. +// - invalid_grant, or a 401/403 that names no reason, can reverse. Held +// an hour, then tried once more. +// - 429 is held until its Retry-After (a minute when it gives none, at +// most an hour), and answered in the meantime as a rate limit. +// - Anything else — a 5xx, the network, a response this cannot read — +// holds nothing, and the next command tries again as it always has. +// +// It is remembered because nothing else would stop the asking. Every +// process mints for itself, so an automated caller — a connector polling +// through the CLI, or `basecamp connect` — would present a dead secret to +// the token endpoint on every poll, indefinitely, each attempt charged to +// the abuse tracker that then rate-limits the address. +// +// A successful mint clears the hold along with everything else it +// replaces, and a login writes a credential that never had one. +// +// The caller holds m.mu and the credential key's cross-process lock, which +// is what makes the hold's write safe against a concurrent login (see +// rememberMintHold). func (m *Manager) mintAgentCredential(ctx context.Context, origin string, creds *Credentials) error { mint, err := m.prepareAgentMint(creds) if err != nil { return err } - token, err := m.mintAgentToken(ctx, mint) + token, hold, err := m.mintAgentToken(ctx, mint) if err != nil { + if hold != nil { + m.rememberMintHold(origin, hold) + } return err } applyAgentToken(creds, token) @@ -165,6 +203,8 @@ func (m *Manager) mintAgentCredential(ctx context.Context, origin string, creds func applyAgentToken(creds *Credentials, token *oauth.Token) { creds.AccessToken = token.AccessToken creds.RefreshToken = "" + // The client was just accepted, so whatever was held against it is over. + creds.MintHold = nil if token.Resource != "" { creds.Resource = token.Resource } @@ -254,14 +294,16 @@ func agentTokenExpiry(token *oauth.Token) time.Time { } // mintAgentToken POSTs one client_credentials grant and returns the token -// it was answered with. +// it was answered with — or, on a refusal worth remembering, the hold it +// leaves (see mintHoldFor), which is the caller's to store or not: a login +// proving a secret it has not stored yet has nothing to store it on. // // The SDK's Exchanger has no client_credentials form, so the request is // made here — but to the same rules its token requests follow, because the // body carries a client secret: a bounded response read, a refusal to treat // a redirect as a hop, and RFC 6749 §5.2 error rendering in the shape the // rest of this package already matches on ("token error: - "). -func (m *Manager) mintAgentToken(ctx context.Context, mint *agentMint) (*oauth.Token, error) { +func (m *Manager) mintAgentToken(ctx context.Context, mint *agentMint) (*oauth.Token, *MintHold, error) { form := url.Values{ "grant_type": {"client_credentials"}, "client_id": {mint.clientID}, @@ -281,14 +323,14 @@ func (m *Manager) mintAgentToken(ctx context.Context, mint *agentMint) (*oauth.T req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, mint.tokenEndpoint, strings.NewReader(form.Encode())) if err != nil { - return nil, wrapOAuthError("minting an agent token", err) + return nil, nil, wrapOAuthError("minting an agent token", err) } req.Header.Set("Content-Type", "application/x-www-form-urlencoded") req.Header.Set("Accept", "application/json") resp, err := mint.client.Do(req) if err != nil { - return nil, wrapOAuthError("minting an agent token", err) + return nil, nil, wrapOAuthError("minting an agent token", err) } defer func() { _ = resp.Body.Close() }() @@ -297,30 +339,31 @@ func (m *Manager) mintAgentToken(ctx context.Context, mint *agentMint) (*oauth.T // body read, as the SDK does: a 3xx that never finishes streaming must // surface as this, not as a timeout halfway through a read. if isRedirect(resp.StatusCode) { - return nil, output.ErrAPI(resp.StatusCode, + return nil, nil, output.ErrAPI(resp.StatusCode, fmt.Sprintf("minting an agent token: redirect %d on the token endpoint is not followed", resp.StatusCode)) } body, err := io.ReadAll(io.LimitReader(resp.Body, maxAgentTokenBytes+1)) if err != nil { - return nil, wrapOAuthError("minting an agent token", err) + return nil, nil, wrapOAuthError("minting an agent token", err) } if int64(len(body)) > maxAgentTokenBytes { - return nil, output.ErrAPI(resp.StatusCode, + return nil, nil, output.ErrAPI(resp.StatusCode, fmt.Sprintf("minting an agent token: response body exceeds %d bytes", maxAgentTokenBytes)) } if resp.StatusCode != http.StatusOK { - return nil, m.agentMintRefusal(resp, body, mint) + hold, err := m.agentMintRefusal(resp, body, mint) + return nil, hold, err } var token oauth.Token if err := json.Unmarshal(body, &token); err != nil { // Not even the parser's complaint: it quotes a byte of the body. - return nil, output.ErrAPI(resp.StatusCode, "minting an agent token: the token response could not be parsed") + return nil, nil, output.ErrAPI(resp.StatusCode, "minting an agent token: the token response could not be parsed") } if token.AccessToken == "" { - return nil, output.ErrAPI(resp.StatusCode, "minting an agent token: the token response carries no access_token") + return nil, nil, output.ErrAPI(resp.StatusCode, "minting an agent token: the token response carries no access_token") } // The CLI can only represent read and full, and this value is stored // with the credential, written into the profile entry, and printed. @@ -330,7 +373,7 @@ func (m *Manager) mintAgentToken(ctx context.Context, mint *agentMint) (*oauth.T // The value is not repeated, for the reason oauthErrorCodes gives: // it is another field the server chose on a request that carried // the secret. - return nil, output.ErrAPI(resp.StatusCode, + return nil, nil, output.ErrAPI(resp.StatusCode, "minting an agent token: the server reported a scope other than read or full, and only those can be stored") } // Every request this CLI makes sends the token as a Bearer credential, @@ -346,11 +389,11 @@ func (m *Manager) mintAgentToken(ctx context.Context, mint *agentMint) (*oauth.T // approved scope is the operator's decision on a connection, and a // server contradicting it is not a token to spend. if widensScope(mint.scope, token.Scope) { - return nil, output.ErrAPI(resp.StatusCode, + return nil, nil, output.ErrAPI(resp.StatusCode, "minting an agent token: the server issued a token wider than the scope the credential was approved for, and a credential is not widened past what was approved") } if token.TokenType != "" && !strings.EqualFold(token.TokenType, "bearer") { - return nil, output.ErrAPI(resp.StatusCode, + return nil, nil, output.ErrAPI(resp.StatusCode, "minting an agent token: the server issued a token of a type this CLI cannot send; it only sends Bearer credentials") } if token.RefreshToken != "" { @@ -360,9 +403,9 @@ func (m *Manager) mintAgentToken(ctx context.Context, mint *agentMint) (*oauth.T m.warnf("warning: the agent token response carried a refresh token; agent credentials re-mint instead and it will not be stored") } if err := applyTokenLifetime(&token, body); err != nil { - return nil, output.ErrAPI(resp.StatusCode, "minting an agent token: "+err.Error()) + return nil, nil, output.ErrAPI(resp.StatusCode, "minting an agent token: "+err.Error()) } - return &token, nil + return &token, nil, nil } // oauthErrorCodes are the RFC 6749 §5.2 token-endpoint error codes (and @@ -396,7 +439,11 @@ var oauthErrorCodes = map[string]bool{ // agentMintRefusal renders a non-200 token response: the RFC 6749 §5.2 // error code when the body carries one this package knows, and the HTTP // status otherwise. See oauthErrorCodes for why nothing else is repeated. -func (m *Manager) agentMintRefusal(resp *http.Response, body []byte, mint *agentMint) error { +// +// It also returns the hold the refusal leaves, if it leaves one — the +// verdict the next mint will answer locally rather than ask for again (see +// mintHoldFor, and mintAgentCredential for what is held and for how long). +func (m *Manager) agentMintRefusal(resp *http.Response, body []byte, mint *agentMint) (*MintHold, error) { detail := fmt.Sprintf("the server answered HTTP %d", resp.StatusCode) var errResp struct { Error string `json:"error"` @@ -414,7 +461,7 @@ func (m *Manager) agentMintRefusal(resp *http.Response, body []byte, mint *agent // makes it a server saying two things at once, of which the status is // the one that says what to do next. if resp.StatusCode < 400 || resp.StatusCode >= 500 || resp.StatusCode == http.StatusTooManyRequests { - return statusFailure("minting an agent token: "+detail, resp) + return mintHoldFor(mint, resp, detail, code, false, m.now()), statusFailure("minting an agent token: "+detail, resp) } // Among the remaining 4xx: when the server NAMED its reason, that name @@ -430,9 +477,10 @@ func (m *Manager) agentMintRefusal(resp *http.Response, body []byte, mint *agent // fetch its secret again for one is advice that cannot help. bareUnauthorized := code == "" && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) if clientRefusalCodes[code] || bareUnauthorized { - return m.agentRemedy(output.ErrAuth("Minting an agent token was refused ("+detail+")"), mint.clientID, mint.scope) + return mintHoldFor(mint, resp, detail, code, true, m.now()), + m.agentRemedy(output.ErrAuth("Minting an agent token was refused ("+detail+")"), mint.clientID, mint.scope) } - return statusFailure("minting an agent token: "+detail, resp) + return nil, statusFailure("minting an agent token: "+detail, resp) } // clientRefusalCodes are the RFC 6749 §5.2 codes that say THE CREDENTIALS @@ -638,7 +686,9 @@ func (m *Manager) adoptAgentGrant(ctx context.Context, disc *discovery, opts Cli if err != nil { return nil, err } - token, err := m.mintAgentToken(ctx, mint) + // A refusal here leaves no hold: nothing is stored yet to hold it on, + // and a login is a person's one attempt, not a loop. + token, _, err := m.mintAgentToken(ctx, mint) if err != nil { return nil, err } diff --git a/internal/auth/agent_hold.go b/internal/auth/agent_hold.go new file mode 100644 index 000000000..662de7e55 --- /dev/null +++ b/internal/auth/agent_hold.go @@ -0,0 +1,224 @@ +package auth + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "math" + "net/http" + "time" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// Remembered mint verdicts. +// +// An agent credential renews by minting, and every process that finds its +// token inside the renewal window mints. For a person at a terminal that is +// one request per command. For an automated caller it is one request per +// poll, forever: a connector that shells out to `basecamp` on a timer, or +// the in-process `basecamp connect`, will present a dead secret to the +// token endpoint as often as it polls, and nothing stops it but a person +// noticing. +// +// That is not hypothetical. An agent whose secret was rotated server-side +// kept presenting the old one about six times a minute for more than twelve +// hours — some 350 requests an hour, nearly all of them answered 429 by the +// abuse tracker the first few 401s had tripped, and all of them for a +// secret that would never work again. +// +// So the token endpoint's verdict on a mint is kept with the credential, and +// the next mint reads it before it sends anything. It is kept for exactly +// the client credentials it was given about (see agentClientFingerprint): a +// login with a new secret writes a new credential, which carries no hold, +// and a hold that names other client credentials than the ones stored is +// ignored however it got there. + +// Mint hold kinds. Anything else read back from a store — a newer CLI's +// kind, a damaged record — holds nothing, so a hold this version cannot +// read degrades to the old behavior of trying again, never to a refusal +// nobody can explain. +const ( + // mintHoldRefused is the token endpoint refusing the client + // credentials themselves (clientRefusalCodes, or a bare 401/403). + mintHoldRefused = "refused" + + // mintHoldRateLimited is a 429, held until its Retry-After. + mintHoldRateLimited = "rate_limited" +) + +// agentRefusalRecheck is how long a refusal that could reverse is held +// before the client credentials are presented once more. +// +// What bc3 says, and why, decides which refusals get one +// (app/controllers/oauth/tokens_controller.rb, +// handle_client_credentials_grant): +// +// - invalid_client is the secret. authenticate_client! refuses a secret +// that does not match the client's digest, and the mint refuses one +// that stopped being the live digest while it was in flight or whose +// client is quarantined. Oauth::AgentClients.rotate! replaces the +// digest, disconnect! clears it, and a quarantine is one-way +// (Oauth::Client::Disabling), so no later state of the server makes the +// same secret good again. It is held with no recheck at all: asking +// again cannot succeed, and each ask is charged to the abuse tracker. +// +// - invalid_grant is "Agent is no longer active" — the agent's account is +// not active. That is a fact about the account, not the secret, and an +// account that is reactivated makes the same secret good again. Held +// for an hour, then tried once, so a reactivated agent recovers on its +// own without anyone having to find its secret again. +// +// - A bare 401 or 403 names nothing. bc3 always names its refusals, so +// one of these came from something else — a proxy, a WAF challenge — +// and is no evidence the secret is dead. Held as invalid_grant is: +// one try an hour costs nothing, and "never" would be a guess. +// +// An hour against the incident's 350: the abuse tracker blocks after ten +// invalid_client answers from one address, and one attempt an hour never +// gets there. +const agentRefusalRecheck = time.Hour + +// defaultAgentRateLimitHold is how long a 429 with no readable Retry-After +// is held — the fallback the rest of the CLI takes for one (see +// resilience.GatingHooks.OnRequestEnd). +const defaultAgentRateLimitHold = 60 * time.Second + +// maxAgentMintHold bounds any hold that expires. A Retry-After past it is +// not honored in full, and a stored expiry further out than this from now — +// a clock that stepped back, a damaged record — is not believed at all: +// that hold is ignored and the mint goes out, which is the old behavior and +// the safe direction to fail in. +const maxAgentMintHold = time.Hour + +// MintHold is the token endpoint's last refusal of an agent credential's +// client, remembered so the next mint can answer it without asking again. +// +// Nothing in it came from the server as free text: Detail is the same fixed +// vocabulary agentMintRefusal renders (see oauthErrorCodes). +type MintHold struct { + // Kind is mintHoldRefused or mintHoldRateLimited. + Kind string `json:"kind"` + + // Detail is what the server said, as agentMintRefusal rendered it: + // "token error: invalid_client", or "the server answered HTTP 401". + Detail string `json:"detail"` + + // Client is agentClientFingerprint of the client credentials the + // verdict was about. A hold for any others holds nothing. + Client string `json:"client"` + + // At is when the verdict was given, in Unix seconds. + At int64 `json:"at"` + + // Until is when the next mint may be sent, in Unix seconds. Zero on a + // refusal means never with these client credentials. + Until int64 `json:"until,omitempty"` +} + +// agentClientFingerprint names a client id and secret without carrying the +// secret: a truncated SHA-256 under a label of its own. The secret is a +// long random string, so the digest cannot be walked back to it, and it is +// stored beside the secret itself in any case — the point is only that +// nothing printed or compared needs the secret in hand. +func agentClientFingerprint(clientID, clientSecret string) string { + sum := sha256.Sum256([]byte("basecamp-cli agent mint hold\x00" + clientID + "\x00" + clientSecret)) + return hex.EncodeToString(sum[:16]) +} + +// mintHoldFor is the hold a refused mint leaves, or nil for a failure the +// next command should simply retry — a 5xx, a redirect, a response that +// does not name the client. rateLimited is a 429; refused is a verdict on +// the client credentials, and code the RFC 6749 §5.2 code the server named +// with it, if any. +func mintHoldFor(mint *agentMint, resp *http.Response, detail, code string, refused bool, now time.Time) *MintHold { + hold := &MintHold{ + Detail: detail, + Client: agentClientFingerprint(mint.clientID, mint.clientSecret), + At: now.Unix(), + } + switch { + case resp.StatusCode == http.StatusTooManyRequests: + wait := retryAfter(resp.Header, now) + if wait <= 0 { + wait = defaultAgentRateLimitHold + } + hold.Kind = mintHoldRateLimited + hold.Until = now.Add(min(wait, maxAgentMintHold)).Unix() + case refused && code == "invalid_client": + // Permanent for this secret; see agentRefusalRecheck. + hold.Kind = mintHoldRefused + case refused: + hold.Kind = mintHoldRefused + hold.Until = now.Add(agentRefusalRecheck).Unix() + default: + return nil + } + return hold +} + +// heldMint is the error a remembered verdict answers a mint with, or nil +// when the mint may be sent. It reads nothing but creds, so a report can +// ask it too. +func (m *Manager) heldMint(creds *Credentials) error { + hold := creds.MintHold + if hold == nil || hold.Client != agentClientFingerprint(creds.ClientID, creds.ClientSecret) { + return nil + } + now := m.now() + if hold.Until != 0 { + until := time.Unix(hold.Until, 0) + if !now.Before(until) || until.Sub(now) > maxAgentMintHold { + return nil + } + } + when := time.Unix(hold.Until, 0).UTC().Format(time.RFC3339) + + switch hold.Kind { + case mintHoldRateLimited: + e := output.ErrRateLimit(int(math.Ceil(time.Unix(hold.Until, 0).Sub(now).Seconds()))) + e.Message = fmt.Sprintf("Minting an agent token is held until %s: the token endpoint rate-limited the last attempt (%s)", when, hold.Detail) + return e + case mintHoldRefused: + msg := "Minting an agent token was refused (" + hold.Detail + ")" + if hold.Until == 0 { + msg += "; the refusal is remembered, and this client secret will not be sent again — a login with a new secret replaces it" + } else { + msg += "; the refusal is remembered until " + when + ", when this client secret will be tried once more — a login with a new secret replaces it sooner" + } + return output.ErrAuth(msg) + } + return nil +} + +// rememberMintHold records hold on the stored credential — if the stored +// credential is still the one the verdict was about. +// +// The caller holds the credential key's cross-process lock, so no login can +// land between the refusal and this write. The fingerprint is compared +// anyway, against a fresh read rather than the copy the mint was made from: +// on a host where the lock could not be taken at all, a login that stored a +// new secret while the old one was being refused must not have that +// refusal written over it. A failure to write is reported and otherwise +// changes nothing — the refusal is what the caller sees either way. +func (m *Manager) rememberMintHold(origin string, hold *MintHold) { + current, err := m.store.Load(origin) + if err != nil { + return + } + if current.OAuthType != oauthTypeAgent || agentClientFingerprint(current.ClientID, current.ClientSecret) != hold.Client { + return + } + current.MintHold = hold + if err := m.store.Save(origin, current); err != nil { + m.warnf("warning: could not remember the token endpoint's refusal for %s, so the next command will ask again: %v", origin, err) + } +} + +// now is the Manager's clock: the clock field when a test set one. +func (m *Manager) now() time.Time { + if m.clock != nil { + return m.clock() + } + return time.Now() +} diff --git a/internal/auth/agent_hold_test.go b/internal/auth/agent_hold_test.go new file mode 100644 index 000000000..5ac476a95 --- /dev/null +++ b/internal/auth/agent_hold_test.go @@ -0,0 +1,343 @@ +package auth + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/output" +) + +// mintEndpoint is a token endpoint that counts what reaches it and answers +// each request with whatever answer returns for it. +type mintEndpoint struct { + srv *httptest.Server + calls atomic.Int32 + answer func(call int) (status int, header http.Header, body string) +} + +func startMintEndpoint(t *testing.T) *mintEndpoint { + t.Helper() + e := &mintEndpoint{} + e.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + call := int(e.calls.Add(1)) - 1 + status, header, body := e.answer(call) + for k, v := range header { + w.Header()[k] = v + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + fmt.Fprint(w, body) + })) + t.Cleanup(e.srv.Close) + return e +} + +func (e *mintEndpoint) respond(status int, body string) { + e.answer = func(int) (int, http.Header, string) { return status, nil, body } +} + +func (e *mintEndpoint) url() string { return e.srv.URL + "/oauth/tokens" } + +const mintedToken = `{"access_token":"minted","token_type":"bearer","expires_in":3600}` + +// heldManager is a manager holding an agent credential due for renewal, +// with a clock the test moves by hand. +func heldManager(t *testing.T, e *mintEndpoint) (*Manager, string, *time.Time) { + t.Helper() + m := newDeviceTestManager(t, e.srv.URL) + now := time.Now() + m.clock = func() time.Time { return now } + key := storeAgent(t, m, agentCredential(e.url(), time.Now().Add(-time.Minute))) + return m, key, &now +} + +// TestARefusedSecretIsNeverSentAgain is the production incident: a secret +// rotated server-side, and a caller that polls. After the first +// invalid_client, the next mint — in this process or any later one, however +// much later — answers from the store and sends nothing. +func TestARefusedSecretIsNeverSentAgain(t *testing.T) { + e := startMintEndpoint(t) + e.respond(http.StatusUnauthorized, `{"error":"invalid_client"}`) + m, key, now := heldManager(t, e) + + _, err := m.AccessToken(context.Background()) + require.Error(t, err) + require.EqualValues(t, 1, e.calls.Load()) + + stored, loadErr := m.store.Load(key) + require.NoError(t, loadErr) + require.NotNil(t, stored.MintHold) + assert.Zero(t, stored.MintHold.Until, "invalid_client is permanent for the secret it refused") + assert.NotContains(t, stored.MintHold.Client, "agent-secret") + + *now = now.Add(30 * 24 * time.Hour) + for range 3 { + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + } + assert.EqualValues(t, 1, e.calls.Load(), "a remembered refusal sent the dead secret again") + + e2 := output.AsError(err) + assert.Equal(t, output.CodeAuth, e2.Code) + assert.Contains(t, e2.Message, "invalid_client") + assert.Contains(t, e2.Message, "remembered") + assert.Contains(t, e2.Hint, "--with-client-credentials", "the remedy is the one the refusal itself carried") + assert.NotContains(t, err.Error(), "agent-secret") + + // A later process reads the same verdict. + fresh := newDeviceTestManager(t, e.srv.URL) + fresh.store = m.store + _, err = fresh.AccessToken(context.Background()) + require.Error(t, err) + assert.EqualValues(t, 1, e.calls.Load()) + + // And the report says so before anyone asks. + refusal := m.RefreshRefusal(stored) + require.Error(t, refusal) + assert.Contains(t, output.AsError(refusal).Message, "remembered") +} + +// TestANewSecretIsNotHeldToTheOldOnesRefusal: the hold names the client +// credentials it was about. A credential carrying a hold for a different +// secret — however it came to — mints as if it had none, and the success +// clears it. +func TestANewSecretIsNotHeldToTheOldOnesRefusal(t *testing.T) { + e := startMintEndpoint(t) + e.respond(http.StatusUnauthorized, `{"error":"invalid_client"}`) + m, key, _ := heldManager(t, e) + + _, err := m.AccessToken(context.Background()) + require.Error(t, err) + + stored, err := m.store.Load(key) + require.NoError(t, err) + require.NotNil(t, stored.MintHold) + stored.ClientSecret = "a-new-secret" + require.NoError(t, m.store.Save(key, stored)) + + e.respond(http.StatusOK, mintedToken) + token, err := m.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "minted", token) + assert.EqualValues(t, 2, e.calls.Load()) + + stored, err = m.store.Load(key) + require.NoError(t, err) + assert.Nil(t, stored.MintHold, "a successful mint left the old refusal behind") +} + +// TestARateLimitIsHeldUntilItsRetryAfter: nothing goes out until the +// server's deadline, and the caller hears a retryable rate limit saying +// when. After it, one request goes out, and its success clears the hold. +func TestARateLimitIsHeldUntilItsRetryAfter(t *testing.T) { + e := startMintEndpoint(t) + e.answer = func(call int) (int, http.Header, string) { + if call == 0 { + return http.StatusTooManyRequests, http.Header{"Retry-After": {"300"}}, `{"error":"invalid_client"}` + } + return http.StatusOK, nil, mintedToken + } + m, key, now := heldManager(t, e) + + _, err := m.AccessToken(context.Background()) + require.Error(t, err) + assert.Equal(t, output.CodeRateLimit, output.AsError(err).Code) + + *now = now.Add(299 * time.Second) + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + assert.EqualValues(t, 1, e.calls.Load(), "a mint went out inside the Retry-After") + held := output.AsError(err) + assert.Equal(t, output.CodeRateLimit, held.Code) + assert.True(t, held.Retryable) + assert.Contains(t, held.Message, "held until") + assert.Contains(t, held.Hint, "1 seconds") + assert.NotContains(t, held.Hint, "--with-client-credentials", "a rate limit is not a refused secret") + + *now = now.Add(time.Second) + token, err := m.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "minted", token) + assert.EqualValues(t, 2, e.calls.Load()) + + stored, err := m.store.Load(key) + require.NoError(t, err) + assert.Nil(t, stored.MintHold) +} + +// TestARateLimitHoldIsBounded: a 429 with no Retry-After is held for the +// default minute, and one asking for a day is not believed past the cap. +func TestARateLimitHoldIsBounded(t *testing.T) { + for name, c := range map[string]struct { + header http.Header + want time.Duration + }{ + "no Retry-After": {nil, defaultAgentRateLimitHold}, + "an absurd one": {http.Header{"Retry-After": {"86400"}}, maxAgentConnectLifetime}, + "an HTTP-date": {http.Header{"Retry-After": {time.Now().Add(10 * time.Minute).UTC().Format(http.TimeFormat)}}, 10 * time.Minute}, + "one that means now": {http.Header{"Retry-After": {"0"}}, defaultAgentRateLimitHold}, + } { + t.Run(name, func(t *testing.T) { + e := startMintEndpoint(t) + e.answer = func(int) (int, http.Header, string) { return http.StatusTooManyRequests, c.header, `{}` } + m, key, now := heldManager(t, e) + + _, err := m.AccessToken(context.Background()) + require.Error(t, err) + + stored, err := m.store.Load(key) + require.NoError(t, err) + require.NotNil(t, stored.MintHold) + assert.Equal(t, mintHoldRateLimited, stored.MintHold.Kind) + assert.InDelta(t, now.Add(c.want).Unix(), stored.MintHold.Until, 2) + assert.LessOrEqual(t, stored.MintHold.Until, now.Add(maxAgentMintHold).Unix()) + }) + } +} + +// TestAServerFaultHoldsNothing: a 5xx is the server's bad minute, not a +// verdict, and the next command asks again exactly as it always has. +func TestAServerFaultHoldsNothing(t *testing.T) { + for name, status := range map[string]int{ + "internal error": http.StatusInternalServerError, + "bad gateway": http.StatusBadGateway, + "one naming client": http.StatusServiceUnavailable, + "a proxy's bare 400": http.StatusBadRequest, + } { + t.Run(name, func(t *testing.T) { + e := startMintEndpoint(t) + e.respond(status, `{"error":"invalid_client"}`) + if status == http.StatusBadRequest { + e.respond(status, `Bad Request`) + } + m, key, _ := heldManager(t, e) + + for range 2 { + _, err := m.AccessToken(context.Background()) + require.Error(t, err) + } + assert.EqualValues(t, 2, e.calls.Load(), "a server fault was held") + + stored, err := m.store.Load(key) + require.NoError(t, err) + assert.Nil(t, stored.MintHold) + }) + } +} + +// TestARefusalThatCanReverseIsRecheckedHourly: invalid_grant is bc3's +// "Agent is no longer active", which reactivating the account undoes, and a +// bare 401 names nothing at all. Each is held an hour, then tried once: a +// second refusal holds another hour, and a success clears it without +// anyone finding the secret again. +func TestARefusalThatCanReverseIsRecheckedHourly(t *testing.T) { + for name, refusal := range map[string]struct { + status int + body string + }{ + "invalid_grant": {http.StatusBadRequest, `{"error":"invalid_grant"}`}, + "a bare 401": {http.StatusUnauthorized, `nothing useful`}, + "a bare 403": {http.StatusForbidden, `nothing useful`}, + } { + t.Run(name, func(t *testing.T) { + e := startMintEndpoint(t) + e.answer = func(call int) (int, http.Header, string) { + if call < 2 { + return refusal.status, nil, refusal.body + } + return http.StatusOK, nil, mintedToken + } + m, key, now := heldManager(t, e) + + _, err := m.AccessToken(context.Background()) + require.Error(t, err) + + *now = now.Add(agentRefusalRecheck - time.Second) + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + assert.EqualValues(t, 1, e.calls.Load(), "a held refusal was rechecked early") + assert.Equal(t, output.CodeAuth, output.AsError(err).Code) + assert.Contains(t, output.AsError(err).Message, "tried once more") + + *now = now.Add(time.Second) + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + assert.EqualValues(t, 2, e.calls.Load(), "the hourly recheck did not go out") + + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + assert.EqualValues(t, 2, e.calls.Load(), "a second refusal was not held again") + + *now = now.Add(agentRefusalRecheck) + token, err := m.AccessToken(context.Background()) + require.NoError(t, err) + assert.Equal(t, "minted", token) + + stored, err := m.store.Load(key) + require.NoError(t, err) + assert.Nil(t, stored.MintHold) + }) + } +} + +// TestAStaleRefusalNeverLandsOnANewSecret: the hold is written against a +// fresh read, and only while that read still holds the client credentials +// the refusal was about. A login that stored a new secret between the +// refusal and the write keeps its credential clean. +func TestAStaleRefusalNeverLandsOnANewSecret(t *testing.T) { + e := startMintEndpoint(t) + m, key, now := heldManager(t, e) + + stale := &MintHold{ + Kind: mintHoldRefused, + Detail: "token error: invalid_client", + Client: agentClientFingerprint("agent-client", "agent-secret"), + At: now.Unix(), + } + + relogged := agentCredential(e.url(), time.Now().Add(time.Hour)) + relogged.ClientSecret = "a-new-secret" + require.NoError(t, m.store.Save(key, relogged)) + + m.rememberMintHold(key, stale) + + stored, err := m.store.Load(key) + require.NoError(t, err) + assert.Nil(t, stored.MintHold, "a refusal of the old secret was written over the new one") + assert.Equal(t, "a-new-secret", stored.ClientSecret) +} + +// TestAHoldTooFarOutIsNotBelieved: a clock that stepped back, or a damaged +// record, must not be able to hold a mint for longer than any hold is +// allowed to last — nor can a kind this version does not know hold one at +// all. Either is ignored, which is the old behavior: ask again. +func TestAHoldTooFarOutIsNotBelieved(t *testing.T) { + for name, hold := range map[string]MintHold{ + "a day out": {Kind: mintHoldRateLimited, Until: time.Now().Add(24 * time.Hour).Unix()}, + "a kind unknown": {Kind: "something_newer"}, + } { + t.Run(name, func(t *testing.T) { + e := startMintEndpoint(t) + e.respond(http.StatusOK, mintedToken) + m, key, _ := heldManager(t, e) + + stored, err := m.store.Load(key) + require.NoError(t, err) + hold.Client = agentClientFingerprint(stored.ClientID, stored.ClientSecret) + stored.MintHold = &hold + require.NoError(t, m.store.Save(key, stored)) + + _, err = m.AccessToken(context.Background()) + require.NoError(t, err) + assert.EqualValues(t, 1, e.calls.Load()) + }) + } +} diff --git a/internal/auth/agent_test.go b/internal/auth/agent_test.go index 3ba9d839f..7094671fd 100644 --- a/internal/auth/agent_test.go +++ b/internal/auth/agent_test.go @@ -188,6 +188,9 @@ func TestRefusedMintIsAnAuthErrorAndAServerFaultIsNot(t *testing.T) { assert.Equal(t, output.CodeAuth, output.AsError(err).Code) assert.Contains(t, err.Error(), "invalid_client") + // The refusal is remembered now, so the server fault needs a + // credential the server has not already refused. + storeAgent(t, m, agentCredential(as.srv.URL+"/oauth/token", time.Now().Add(-time.Minute))) as.token = func(int) (int, string) { return http.StatusBadGateway, `no json here` } _, err = m.AccessToken(context.Background()) require.Error(t, err) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index c5a7b56aa..6967f975f 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -103,6 +103,10 @@ type Manager struct { // traffic, a malformed opt-out value). Test seam; nil means stderr. Warnf func(format string, args ...any) + // clock is the time a remembered mint verdict is read and written + // against (agent_hold.go). Test seam; nil means time.Now. + clock func() time.Time + mu sync.Mutex // kindMu guards kind, which is read while building an error's remedy diff --git a/internal/auth/keyring.go b/internal/auth/keyring.go index d041c3796..55df2b15e 100644 --- a/internal/auth/keyring.go +++ b/internal/auth/keyring.go @@ -41,6 +41,13 @@ type Credentials struct { // today except a minted agent self-token — means the default. RenewAfter int64 `json:"renew_after,omitempty"` + // MintHold is the token endpoint's last refusal of an agent + // credential's client — a refused secret, or a rate limit — kept so + // the next mint answers it locally instead of asking again (see + // agent_hold.go). Nil for every other kind of credential, and for an + // agent's whose last mint succeeded. + MintHold *MintHold `json:"mint_hold,omitempty"` + // Issuer is the RFC 8414 issuer of the authorization server that minted // a BC5 credential — where its metadata, and so its revocation endpoint, // is found at logout. Credentials stored before it was recorded derive diff --git a/internal/commands/auth.go b/internal/commands/auth.go index fef3eedbb..759fcdfe0 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -383,6 +383,12 @@ func authStatusReport(ctx context.Context, app *appctx.App) (*authStatus, error) report.data["source"] = source report.data["oauth_type"] = creds.OAuthType report.data["refreshable"] = refreshable + if refusal != nil && creds.OAuthType == "agent" { + // Why an agent cannot renew — above all a refusal the token + // endpoint gave that is being remembered, and until when — for a + // caller reading the JSON rather than the token line. + report.data["renewal_refused"] = output.AsError(refusal).Message + } report.data["storage"] = storage if scope != "" { report.data["scope"] = scope @@ -436,6 +442,13 @@ func authStatusReport(ctx context.Context, app *appctx.App) (*authStatus, error) case expiresIn >= 0: expiry = "expired (" + coarseDuration(expiresIn) + " left, inside the " + coarseDuration(auth.RefreshWindow) + " the CLI keeps clear of expiry, and the refresh would be refused: " + output.AsError(refusal).Message + ")" report.hint = remedyFor(app, refusal) + case creds.OAuthType == "agent": + // An agent's renewal can be refused by a verdict the token + // endpoint already gave — a secret it refused, a rate limit + // still running — and that says why, and until when, where + // "expired" alone would not. + expiry = "expired, and the renewal would be refused: " + output.AsError(refusal).Message + report.hint = remedyFor(app, refusal) default: expiry = "expired" report.hint = app.Auth.LoginHint() diff --git a/internal/commands/auth_status_agent_test.go b/internal/commands/auth_status_agent_test.go index 9b33e3a86..6aa8d6a74 100644 --- a/internal/commands/auth_status_agent_test.go +++ b/internal/commands/auth_status_agent_test.go @@ -79,6 +79,72 @@ func TestAuthStatusOnABrokenAgentOffersTheAgentLogin(t *testing.T) { assert.NotContains(t, envelope.Notice, "Run: basecamp auth login -P") } +// TestAuthStatusSaysARefusalIsRemembered: an agent whose secret the token +// endpoint refused holds that verdict, and the next command will not ask +// again. The report is where someone looks to find out why the agent went +// quiet, so it says so — and what replaces the secret. +func TestAuthStatusSaysARefusalIsRemembered(t *testing.T) { + t.Setenv("BASECAMP_NO_KEYRING", "1") + t.Setenv("BASECAMP_TOKEN", "") + dir := t.TempDir() + t.Setenv("XDG_CONFIG_HOME", dir) + + var calls int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + calls++ + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"error":"invalid_client"}`)) + })) + defer srv.Close() + + cfg := &config.Config{BaseURL: srv.URL, ActiveProfile: "clawdito", Sources: map[string]string{}} + authMgr := auth.NewManager(cfg, srv.Client()) + store := auth.NewStore(config.GlobalConfigDir()) + authMgr.SetStore(store) + require.NoError(t, store.Save("profile:clawdito", &auth.Credentials{ + AccessToken: "spent", + OAuthType: "agent", + ClientID: "agent-client", + ClientSecret: "rotated-away", + TokenEndpoint: srv.URL + "/oauth/tokens", + ExpiresAt: time.Now().Add(-time.Minute).Unix(), + })) + + // The refusal that is remembered. + _, err := authMgr.AccessToken(context.Background()) + require.Error(t, err) + require.Equal(t, 1, calls) + + buf := &bytes.Buffer{} + app := &appctx.App{ + Config: cfg, + Auth: authMgr, + Output: output.New(output.Options{Format: output.FormatJSON, Writer: buf}), + } + app.Flags.JSON = true + + cmd := NewAuthCmd() + cmd.SetArgs([]string{"status"}) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + require.NoError(t, cmd.Execute()) + + assert.Equal(t, 1, calls, "the report asked the token endpoint") + assert.Contains(t, buf.String(), "the refusal is remembered") + assert.Contains(t, buf.String(), "invalid_client") + + var envelope struct { + Notice string `json:"notice"` + } + require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Contains(t, envelope.Notice, "--with-client-credentials") + assert.NotContains(t, buf.String(), "rotated-away") +} + // TestDoctorOffersTheAgentLoginForABrokenAgent: doctor is the diagnostic // people (and agents) read for exactly the command to run, in its check // hints and its breadcrumbs. Naming the interactive login there would have From 05ede0fc42ae59e1f217025625f7448232882b94 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Tue, 29 Sep 2026 20:53:23 -0700 Subject: [PATCH 2/2] Believe a stored hold only when it is well formed; round its deadline up A rate_limited hold with no deadline was answered as a rate limit ending in 1970, on every mint, until a login replaced the credential. Holds are now checked when they're read, in one place (MintHold.holds), and anything malformed is ignored. The mint goes out, as it did before holds. A Retry-After counted from partway through a second is stored rounded up, so the hold doesn't end up to a second early. Docs now match the code. A 429 is held for at most retryAfter's 30-minute cap. On a host that cannot take the key lock, the fresh read in rememberMintHold narrows the race with a concurrent login but doesn't close it. The auth status test asserts data.renewal_refused directly, not text the token line already carries. --- internal/auth/agent.go | 5 +- internal/auth/agent_hold.go | 65 +++++++++++++++------ internal/auth/agent_hold_test.go | 37 ++++++++++-- internal/commands/auth_status_agent_test.go | 7 ++- 4 files changed, 89 insertions(+), 25 deletions(-) diff --git a/internal/auth/agent.go b/internal/auth/agent.go index 07f66bf33..81b60ebb5 100644 --- a/internal/auth/agent.go +++ b/internal/auth/agent.go @@ -161,7 +161,10 @@ func (m *Manager) resolveAgentMint(creds *Credentials) (*agentMint, error) { // - invalid_grant, or a 401/403 that names no reason, can reverse. Held // an hour, then tried once more. // - 429 is held until its Retry-After (a minute when it gives none, at -// most an hour), and answered in the meantime as a rate limit. +// most retryAfter's cap of maxAgentConnectLifetime), and answered in +// the meantime as a rate limit. bc3's abuse blocks run longer (up to a +// day), and it answers a request inside one with another 429 without +// charging it, so the cap costs one request per cap, not an escalation. // - Anything else — a 5xx, the network, a response this cannot read — // holds nothing, and the next command tries again as it always has. // diff --git a/internal/auth/agent_hold.go b/internal/auth/agent_hold.go index 662de7e55..cd1202b96 100644 --- a/internal/auth/agent_hold.go +++ b/internal/auth/agent_hold.go @@ -84,8 +84,9 @@ const agentRefusalRecheck = time.Hour // resilience.GatingHooks.OnRequestEnd). const defaultAgentRateLimitHold = 60 * time.Second -// maxAgentMintHold bounds any hold that expires. A Retry-After past it is -// not honored in full, and a stored expiry further out than this from now — +// maxAgentMintHold bounds any hold that expires. No hold is written past it +// (retryAfter already caps a Retry-After at maxAgentConnectLifetime, below +// it), and a stored expiry further out than this from now — // a clock that stepped back, a damaged record — is not believed at all: // that hold is ignored and the mint goes out, which is the old behavior and // the safe direction to fail in. @@ -111,8 +112,9 @@ type MintHold struct { // At is when the verdict was given, in Unix seconds. At int64 `json:"at"` - // Until is when the next mint may be sent, in Unix seconds. Zero on a - // refusal means never with these client credentials. + // Until is when the next mint may be sent, in Unix seconds, rounded + // up. Zero on a refusal means never with these client credentials; a + // rate limit without one is damaged, and holds nothing. Until int64 `json:"until,omitempty"` } @@ -144,7 +146,7 @@ func mintHoldFor(mint *agentMint, resp *http.Response, detail, code string, refu wait = defaultAgentRateLimitHold } hold.Kind = mintHoldRateLimited - hold.Until = now.Add(min(wait, maxAgentMintHold)).Unix() + hold.Until = ceilUnix(now.Add(min(wait, maxAgentMintHold))) case refused && code == "invalid_client": // Permanent for this secret; see agentRefusalRecheck. hold.Kind = mintHoldRefused @@ -157,20 +159,20 @@ func mintHoldFor(mint *agentMint, resp *http.Response, detail, code string, refu return hold } +// ceilUnix is t in Unix seconds, rounded up: a deadline stored in whole +// seconds must not end before the one it was given. +func ceilUnix(t time.Time) int64 { + return t.Add(time.Second - time.Nanosecond).Unix() +} + // heldMint is the error a remembered verdict answers a mint with, or nil // when the mint may be sent. It reads nothing but creds, so a report can // ask it too. func (m *Manager) heldMint(creds *Credentials) error { hold := creds.MintHold - if hold == nil || hold.Client != agentClientFingerprint(creds.ClientID, creds.ClientSecret) { - return nil - } now := m.now() - if hold.Until != 0 { - until := time.Unix(hold.Until, 0) - if !now.Before(until) || until.Sub(now) > maxAgentMintHold { - return nil - } + if !hold.holds(creds, now) { + return nil } when := time.Unix(hold.Until, 0).UTC().Format(time.RFC3339) @@ -191,16 +193,43 @@ func (m *Manager) heldMint(creds *Credentials) error { return nil } +// holds reports whether a stored hold is one this version believes: about +// these client credentials, of a kind it knows, and — where it expires — +// expiring in the future and within maxAgentMintHold. A rate limit always +// expires. Anything else is ignored, and the mint goes out. +func (hold *MintHold) holds(creds *Credentials, now time.Time) bool { + if hold == nil || hold.Client != agentClientFingerprint(creds.ClientID, creds.ClientSecret) { + return false + } + switch { + case hold.Kind == mintHoldRefused && hold.Until == 0: + return true + case hold.Kind == mintHoldRefused, hold.Kind == mintHoldRateLimited: + until := time.Unix(hold.Until, 0) + return hold.Until > 0 && now.Before(until) && until.Sub(now) <= maxAgentMintHold + default: + return false + } +} + // rememberMintHold records hold on the stored credential — if the stored // credential is still the one the verdict was about. // // The caller holds the credential key's cross-process lock, so no login can // land between the refusal and this write. The fingerprint is compared -// anyway, against a fresh read rather than the copy the mint was made from: -// on a host where the lock could not be taken at all, a login that stored a -// new secret while the old one was being refused must not have that -// refusal written over it. A failure to write is reported and otherwise -// changes nothing — the refusal is what the caller sees either way. +// anyway, against a fresh read rather than the copy the mint was made from, +// for a host where that lock could not be taken at all (lock.go runs such a +// host unsynchronized, with a warning). There it narrows the window a +// concurrent login can be written over from the token round trip to this +// load and save; it does not close it, and nothing without a lock can — the +// store has no compare-and-swap, and the whole-store lock lives in the same +// directory the key lock could not be made in. A successful mint on such a +// host saves the copy it loaded before its round trip, a wider window than +// this one, and has since before holds existed. Skipping the write there +// instead would leave exactly those hosts minting on every poll. +// +// A failure to write is reported and otherwise changes nothing — the +// refusal is what the caller sees either way. func (m *Manager) rememberMintHold(origin string, hold *MintHold) { current, err := m.store.Load(origin) if err != nil { diff --git a/internal/auth/agent_hold_test.go b/internal/auth/agent_hold_test.go index 5ac476a95..e9d7da54f 100644 --- a/internal/auth/agent_hold_test.go +++ b/internal/auth/agent_hold_test.go @@ -49,11 +49,11 @@ func (e *mintEndpoint) url() string { return e.srv.URL + "/oauth/tokens" } const mintedToken = `{"access_token":"minted","token_type":"bearer","expires_in":3600}` // heldManager is a manager holding an agent credential due for renewal, -// with a clock the test moves by hand. +// with a clock the test moves by hand, starting on a whole second. func heldManager(t *testing.T, e *mintEndpoint) (*Manager, string, *time.Time) { t.Helper() m := newDeviceTestManager(t, e.srv.URL) - now := time.Now() + now := time.Now().Truncate(time.Second) m.clock = func() time.Time { return now } key := storeAgent(t, m, agentCredential(e.url(), time.Now().Add(-time.Minute))) return m, key, &now @@ -173,6 +173,34 @@ func TestARateLimitIsHeldUntilItsRetryAfter(t *testing.T) { assert.Nil(t, stored.MintHold) } +// TestARateLimitHoldRoundsItsDeadlineUp: the hold is stored in whole +// seconds, and a Retry-After counted from partway through a second must not +// end early — bc3 answers a request inside its block with another 429. +func TestARateLimitHoldRoundsItsDeadlineUp(t *testing.T) { + e := startMintEndpoint(t) + e.answer = func(call int) (int, http.Header, string) { + if call == 0 { + return http.StatusTooManyRequests, http.Header{"Retry-After": {"300"}}, `{}` + } + return http.StatusOK, nil, mintedToken + } + m, _, now := heldManager(t, e) + *now = now.Truncate(time.Second).Add(999 * time.Millisecond) + + _, err := m.AccessToken(context.Background()) + require.Error(t, err) + + *now = now.Add(299*time.Second + 500*time.Millisecond) + _, err = m.AccessToken(context.Background()) + require.Error(t, err) + assert.EqualValues(t, 1, e.calls.Load(), "a mint went out before the Retry-After had elapsed") + + *now = now.Add(501 * time.Millisecond) + _, err = m.AccessToken(context.Background()) + require.NoError(t, err) + assert.EqualValues(t, 2, e.calls.Load()) +} + // TestARateLimitHoldIsBounded: a 429 with no Retry-After is held for the // default minute, and one asking for a day is not believed past the cap. func TestARateLimitHoldIsBounded(t *testing.T) { @@ -321,8 +349,9 @@ func TestAStaleRefusalNeverLandsOnANewSecret(t *testing.T) { // all. Either is ignored, which is the old behavior: ask again. func TestAHoldTooFarOutIsNotBelieved(t *testing.T) { for name, hold := range map[string]MintHold{ - "a day out": {Kind: mintHoldRateLimited, Until: time.Now().Add(24 * time.Hour).Unix()}, - "a kind unknown": {Kind: "something_newer"}, + "a day out": {Kind: mintHoldRateLimited, Until: time.Now().Add(24 * time.Hour).Unix()}, + "a kind unknown": {Kind: "something_newer"}, + "a rate limit with no deadline": {Kind: mintHoldRateLimited}, } { t.Run(name, func(t *testing.T) { e := startMintEndpoint(t) diff --git a/internal/commands/auth_status_agent_test.go b/internal/commands/auth_status_agent_test.go index 6aa8d6a74..a1c82a2f7 100644 --- a/internal/commands/auth_status_agent_test.go +++ b/internal/commands/auth_status_agent_test.go @@ -134,13 +134,16 @@ func TestAuthStatusSaysARefusalIsRemembered(t *testing.T) { require.NoError(t, cmd.Execute()) assert.Equal(t, 1, calls, "the report asked the token endpoint") - assert.Contains(t, buf.String(), "the refusal is remembered") - assert.Contains(t, buf.String(), "invalid_client") var envelope struct { Notice string `json:"notice"` + Data struct { + RenewalRefused string `json:"renewal_refused"` + } `json:"data"` } require.NoError(t, json.Unmarshal(buf.Bytes(), &envelope), buf.String()) + assert.Contains(t, envelope.Data.RenewalRefused, "the refusal is remembered") + assert.Contains(t, envelope.Data.RenewalRefused, "invalid_client") assert.Contains(t, envelope.Notice, "--with-client-credentials") assert.NotContains(t, buf.String(), "rotated-away") }