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 bcf38e7e0..6ff52d9fd 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -369,6 +369,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 @@ -422,6 +428,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