amadan
console: fix rate-limit review findings C32/C33/C34 and Risk A/minors
C32: principalKey mirrors auditActor's legacy-app-token branch — a kind='app' row (Name=="") now keys on App, not on the empty name, so titogo's several per-app routers no longer share one wildFail/write bucket per account. C33: rateLimiter gains tripped(key, now), a read-only peek. The wildcard-enable path now peeks wildFailTripped before the live DNS round trip and only consumes (allowWildFail) after an actual failure, so a tripped principal stops costing the console outbound DNS instead of merely getting a different status code for it. C34: domainsGate keeps a convergence-rate ceiling (domainsCeilingBurst, 120/min, keyed on the principal, via allowDomains) even though attach/ detach are exempt from the write/instance workload budgets — the exemption proves the legitimate workload needs no budget, not that no budget is safe, and every effective attach/detach bumps control/serial which every box reacts to within 2s. Risk A: comment at the first call site explaining why discarding principalFrom's ok is safe (cliRoute's resolve-before-handler ordering) and what breaks if it ever isn't (one global bucket keyed on ). Minors: each service-budget constant now argues its own number instead of sharing prose that didn't reach svcWriteBurst and conflicted with svcShipBurst; wildFail gets its own allowWildFail/wildFailTripped pickers so no call site reaches into s.cli by hand. New tests: TestDomainsCeilingBoundsConvergenceRate, TestLegacyAppTokensHaveSeparateWildFailBudgets, TestTwoNamedServicesHaveSeparateBudgets. TestWildcardEnableIsNotRateLimited's volume drops to domainsCeilingBurst/2 so it demonstrates the write/instance exemption without also exercising the new ceiling it doesn't test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ
6 files changed,
+311
−43
internal/console/apicli.go+11 −0internal/console/apidomains.go+51 −19internal/console/apidomains_test.go+87 −3internal/console/authcli.go+87 −17internal/console/authcli_test.go+27 −0internal/console/svcprincipal.go+48 −4
diff --git a/internal/console/apicli.go b/internal/console/apicli.go| index 900d5cde0..8c29fe390 100644 |
| --- a/internal/console/apicli.go |
| +++ b/internal/console/apicli.go |
| @@ -570,6 +570,17 @@ func (s *Server) handleAPIPromote(w http.ResponseWriter, r *http.Request) { |
| if !ok { |
| return |
| } |
| + // ok discarded here, and at every other write/create/ship/domains call |
| + // site in this package that reads a principal back off the context: |
| + // apiAccountApp/apiAccount/apiFleet above already called |
| + // apiPrincipalAccount, principalFrom's only writer, so ok is already |
| + // true by the time this line runs — cliRoute stashed the principal on |
| + // the context before the handler ever started (Task 5 review, Risk A). |
| + // If that ordering were ever broken, the zero Principal reads as |
| + // PrincipalUser with Address "", which keys the PERSON limiter on the |
| + // empty string for every caller alike — turning, say, the create |
| + // door's 10-a-day-per-address budget into ONE shared bucket of 10 |
| + // creates a day for the entire deployment, not a per-caller refusal. |
| p, _ := principalFrom(r.Context()) |
| // allowWrite picks the person or the service write budget from the |
| // principal (svcprincipal.go). A person is keyed on the ADDRESS, not the |
diff --git a/internal/console/apidomains.go b/internal/console/apidomains.go| index 7e06e2642..df0236c1e 100644 |
| --- a/internal/console/apidomains.go |
| +++ b/internal/console/apidomains.go |
| @@ -136,22 +136,37 @@ func (s *Server) resolveAliasRoute(ctx context.Context, w http.ResponseWriter, a |
| } |
| // domainsGate is instanceGate's principal, binding and readiness checks with |
| -// the instance-verb BUDGET step removed (spec §4a). Wildcard DNS changes the |
| -// budget question: a Pages hub with wildcard DNS makes NO per-repo hostname |
| -// call at all — one claim on the hub host with both flags, and every repo's |
| -// subdomain serves off the catchall — so gating attach and detach on the |
| -// instance family's per-minute request budget would uplift a call that |
| -// makes no per-repo call at all. The one thing on this path that IS bounded |
| -// is the FAILING half of a wildcard enable, and it is bounded separately, |
| -// inside setDomainCapabilities (wildFailBurst) — a live, sequential DNS |
| -// round trip on the request path is a hazard whether or not the ordinary |
| -// write budget would have caught it. |
| +// the instance-verb WORKLOAD budget step replaced by a CONVERGENCE-RATE |
| +// ceiling (spec §4a, sharpened by Task 5 review C34). Wildcard DNS changes |
| +// the WORKLOAD question: a Pages hub with wildcard DNS makes NO per-repo |
| +// hostname call at all — one claim on the hub host with both flags, and |
| +// every repo's subdomain serves off the catchall — so gating attach and |
| +// detach on the instance family's per-minute request budget would uplift a |
| +// call that makes no per-repo call at all. That is the whole of what the |
| +// exemption establishes: the legitimate workload does not NEED a budget. It |
| +// does not establish that no budget is safe — every EFFECTIVE attach or |
| +// detach audits and bumps control/serial, which every box polls at 2s and |
| +// reacts to with a full pass, so an unlimited exempt door would let one |
| +// authenticated credential drive fleet-wide convergence at request rate. |
| +// allowDomains (domainsCeilingBurst, svcprincipal.go) is that ceiling: high |
| +// enough that no real caller — including the exempt wildcard workload |
| +// itself — ever notices it, low enough that it still caps how fast one |
| +// credential can make every box look at the world again. |
| +// |
| +// The other thing bounded on this path is the FAILING half of a wildcard |
| +// enable, bounded separately, inside setDomainCapabilities (wildFailBurst) |
| +// — a live, sequential DNS round trip on the request path is a hazard |
| +// whether or not this ceiling would eventually have caught it too. |
| func (s *Server) domainsGate(w http.ResponseWriter, r *http.Request) (Principal, *Account, string, bool) { |
| _, acct, app, ok := s.apiAccountApp(w, r) |
| if !ok { |
| return Principal{}, nil, "", false |
| } |
| p, _ := principalFrom(r.Context()) |
| + if !s.allowDomains(p, time.Now()) { |
| + jsonError(w, http.StatusTooManyRequests, "too many requests — wait a minute and try again") |
| + return Principal{}, nil, "", false |
| + } |
| // A console with no deployment bucket configured can answer none of |
| // these — instanceGate's own check, kept here since this gate stops |
| // short of calling it. |
| @@ -304,6 +319,12 @@ func (s *Server) handleAPIDomainAttach(w http.ResponseWriter, r *http.Request) { |
| writeJSON(w, http.StatusOK, map[string]any{"host": host, "route": route, "created": created}) |
| } |
| +// wildFailMsg is the ONE 429 body both the peek (wildFailTripped) and the |
| +// consume (allowWildFail) refusals in setDomainCapabilities answer with — a |
| +// caller must not be able to tell which of the two turned it away, since |
| +// both mean exactly the same thing to them: stop retrying for a minute. |
| +const wildFailMsg = "too many wildcard attempts with no _acme-challenge CNAME in place — wait a minute and retry" |
| + |
| // setDomainCapabilities applies an attach body's optional wildcard/catchall |
| // flip AFTER the underlying claim attach above has already succeeded and |
| // established ownership. ok=false means a response has already been |
| @@ -354,16 +375,27 @@ func (s *Server) setDomainCapabilities(w http.ResponseWriter, r *http.Request, c |
| jsonError(w, http.StatusUnprocessableEntity, "deployment has no delegation zone configured") |
| return false |
| } |
| + // PEEKED before the DNS round trip (C33 review): once a principal |
| + // has already tripped wildFail, running the lookup again buys the |
| + // caller nothing but costs the console another live, sequential |
| + // outbound DNS call. A caller who has never failed never trips |
| + // this — wildFailTripped only ever reads TRUE from a count that |
| + // CONSUMED (below) put there — so this can never turn away a |
| + // legitimate, exempt caller. |
| + if s.wildFailTripped(p, time.Now()) { |
| + jsonError(w, http.StatusTooManyRequests, wildFailMsg) |
| + return false |
| + } |
| if !s.delegationCNAMEDelegated(ctx, host) { |
| - // Bounded ONLY on the failing path (spec §4a). A succeeding |
| - // enable is once-per-domain and exempt (domainsGate skips the |
| - // write budget entirely); a failure is a live, sequential DNS |
| - // round trip a caller can repeat forever. Counted AFTER the |
| - // lookup, so the bound is on FAILURES rather than on attempts — |
| - // a caller whose CNAME is correct never meets it at all. |
| - if !s.cli.wildFail.allow(principalKey(p), time.Now()) { |
| - jsonError(w, http.StatusTooManyRequests, |
| - "too many wildcard attempts with no _acme-challenge CNAME in place — wait a minute and retry") |
| + // CONSUMED only here, on an ACTUAL failure (spec §4a). A |
| + // succeeding enable is once-per-domain and exempt (domainsGate |
| + // carries only the convergence ceiling, not a workload budget); |
| + // a failure is what the peek above exists to eventually stop |
| + // paying for. Counted AFTER the lookup, so the bound is on |
| + // FAILURES rather than on attempts — a caller whose CNAME is |
| + // correct never meets it at all. |
| + if !s.allowWildFail(p, time.Now()) { |
| + jsonError(w, http.StatusTooManyRequests, wildFailMsg) |
| return false |
| } |
| jsonError(w, http.StatusUnprocessableEntity, |
diff --git a/internal/console/apidomains_test.go b/internal/console/apidomains_test.go| index b21875fa9..a7a4fa59b 100644 |
| --- a/internal/console/apidomains_test.go |
| +++ b/internal/console/apidomains_test.go |
| @@ -1096,17 +1096,21 @@ func TestDelegationZoneInvalidAtConstruction(t *testing.T) { |
| // TestWildcardEnableIsNotRateLimited: a Pages hub with wildcard DNS makes no |
| // per-repo hostname call at all — one claim on the hub host with both flags, |
| // every repo's subdomain serves off the catchall. So the domains doors are |
| -// EXEMPT from the write budget rather than uplifted: far more than |
| -// svcWriteBurst SUCCEEDING wildcard enables in the window must never 429. |
| +// EXEMPT from the write/instance WORKLOAD budgets rather than uplifted — |
| +// this loop stays well under domainsCeilingBurst (the separate CONVERGENCE- |
| +// RATE ceiling that does still apply, see TestDomainsCeilingBoundsConvergence |
| +// below) precisely to isolate that claim: every one of these must succeed |
| +// for a reason that has nothing to do with hitting any door's cap. |
| func TestWildcardEnableIsNotRateLimited(t *testing.T) { |
| const zone = "acme.example" |
| + const attempts = domainsCeilingBurst / 2 |
| fx, _ := instanceFixture(t, "code-wc-exempt") |
| svc := serviceToken(t, fx.st, OpsAccountID, "hub", "titogo", svcrole.Publish) |
| fx.srv.delegationZone = zone |
| fx.srv.lookupCNAME = func(_ context.Context, name string) (string, error) { |
| return name + "." + zone, nil |
| } |
| - for i := 0; i < svcWriteBurst*3; i++ { |
| + for i := 0; i < attempts; i++ { |
| host := fmt.Sprintf("r%d.pages.example", i) |
| code, _, out := rawReq(t, http.MethodPost, domainsURL(fx, "bab", "titogo"), svc, |
| `{"host":"`+host+`","route":"hub.pages.example","wildcard":true,"catchall":true}`) |
| @@ -1119,6 +1123,35 @@ func TestWildcardEnableIsNotRateLimited(t *testing.T) { |
| } |
| } |
| +// TestDomainsCeilingBoundsConvergenceRate is C34's correction: the |
| +// exemption above establishes that the legitimate wildcard workload does |
| +// not NEED a budget, not that no budget is safe. Every effective attach |
| +// audits and bumps control/serial, which every box polls at 2s and reacts |
| +// to with a full pass, so domainsGate keeps a convergence-rate ceiling |
| +// (domainsCeilingBurst, keyed on the principal) even though it dropped the |
| +// write/instance budgets. Plain, non-wildcard attaches are enough to show |
| +// it — the ceiling applies to the door, not to the wildcard flags. |
| +func TestDomainsCeilingBoundsConvergenceRate(t *testing.T) { |
| + fx, _ := instanceFixture(t, "code-wc-ceiling") |
| + svc := serviceToken(t, fx.st, OpsAccountID, "hub", "titogo", svcrole.Publish) |
| + var saw429 bool |
| + for i := 0; i < domainsCeilingBurst+5; i++ { |
| + host := fmt.Sprintf("c%d.pages.example", i) |
| + code, _, out := rawReq(t, http.MethodPost, domainsURL(fx, "bab", "titogo"), svc, |
| + `{"host":"`+host+`","route":"hub.pages.example"}`) |
| + if code == http.StatusTooManyRequests { |
| + saw429 = true |
| + break |
| + } |
| + if code != http.StatusOK { |
| + t.Fatalf("attach %d = %d: %s", i, code, out) |
| + } |
| + } |
| + if !saw429 { |
| + t.Error("unbounded attaches from one credential; want a 429 after domainsCeilingBurst") |
| + } |
| +} |
| + |
| // TestFailedWildcardEnableIsBounded is the hazard the exemption has to |
| // survive: the CNAME check is a live, sequential DNS round trip on the |
| // request path, and a caller whose CNAME is not there yet can retry |
| @@ -1180,3 +1213,54 @@ func TestIdempotentReattachTouchesNeitherBudget(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// TestLegacyAppTokensHaveSeparateWildFailBudgets is C32's fix, made |
| +// concrete: titogo's routers each hold their OWN app token (kind='app', |
| +// Name==""), often bound to different apps in the same account. Before the |
| +// fix, principalKey collapsed every one of them onto "service:<id>/" — |
| +// whichever app it was bound to — so one router's mistyped CNAME could 429 |
| +// every other router's wildcard enable in the account. auditActor already |
| +// keys this exact row on App for the same reason; principalKey must agree. |
| +func TestLegacyAppTokensHaveSeparateWildFailBudgets(t *testing.T) { |
| + const zone = "acme.example" |
| + fx, _ := instanceFixture(t, "code-wc-legacy-sep") |
| + now := time.Now() |
| + if _, err := fx.st.CreateAppIn(OpsAccountID, "widgets", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + tokTitogo, err := fx.st.NewAppToken(OpsAccountID, "titogo", "paul@keymail.dev", "router-titogo", now) |
| + if err != nil { |
| + t.Fatalf("NewAppToken(titogo): %v", err) |
| + } |
| + tokWidgets, err := fx.st.NewAppToken(OpsAccountID, "widgets", "paul@keymail.dev", "router-widgets", now) |
| + if err != nil { |
| + t.Fatalf("NewAppToken(widgets): %v", err) |
| + } |
| + fx.srv.delegationZone = zone |
| + fx.srv.lookupCNAME = func(context.Context, string) (string, error) { |
| + return "", errors.New("not found") // no _acme-challenge CNAME anywhere |
| + } |
| + // Exhaust titogo's app token's wildFail budget. |
| + var saw429 bool |
| + for i := 0; i < wildFailBurst+5; i++ { |
| + code, _, _ := rawReq(t, http.MethodPost, domainsURL(fx, "bab", "titogo"), tokTitogo, |
| + `{"host":"retry.pages.example","route":"hub.pages.example","wildcard":true}`) |
| + if code == http.StatusTooManyRequests { |
| + saw429 = true |
| + break |
| + } |
| + if code != http.StatusUnprocessableEntity { |
| + t.Fatalf("titogo failing attach %d = %d, want 422 (until the bound trips)", i, code) |
| + } |
| + } |
| + if !saw429 { |
| + t.Fatal("titogo's app token never tripped wildFail — the setup is broken, not the fix") |
| + } |
| + // widgets' app token — a DIFFERENT app, same account — must be |
| + // untouched: a single failure answers 422, never 429. |
| + code, _, out := rawReq(t, http.MethodPost, domainsURL(fx, "bab", "widgets"), tokWidgets, |
| + `{"host":"retry2.pages.example","route":"hub.pages.example","wildcard":true}`) |
| + if code != http.StatusUnprocessableEntity { |
| + t.Fatalf("widgets' app token after titogo's exhausted its budget = %d: %s, want 422 (separate budget)", code, out) |
| + } |
| +} |
diff --git a/internal/console/authcli.go b/internal/console/authcli.go| index f367b2fb2..141ef2f45 100644 |
| --- a/internal/console/authcli.go |
| +++ b/internal/console/authcli.go |
| @@ -109,27 +109,58 @@ const ( |
| cliCreateBurst = 10 |
| // Service-principal budgets (Task 5, issue #220 consequence 1). A hub is |
| - // not a person, and the numbers above are calibrated for one. These are |
| - // a judgement, not a derivation: 100 repos enabling Pages in a day, and |
| - // a push every 30 seconds sustained. |
| + // not a person, and each number below is its own judgement calibrated |
| + // for what a legitimate AUTOMATED caller does — not a derivation from |
| + // the person numbers above, and not one shared story: read each |
| + // comment on its own rather than assuming the others generalize it. |
| // |
| - // They are keyed on the principal NAME (principalKey), not the token |
| - // hash: rotating a credential must not hand it a fresh budget, or a |
| - // rotation mid-flood would be the limit doing nothing. |
| + // All three are keyed on the principal NAME (principalKey), not the |
| + // token hash: rotating a credential must not hand it a fresh budget, or |
| + // a rotation mid-flood would be the limit doing nothing. |
| + |
| + // svcCreateBurst: 100 repos enabling Pages (or otherwise claiming an |
| + // app) in one day — ten times cliCreateBurst's per-person ceiling, |
| + // because one hub credential legitimately claims on behalf of many |
| + // repos where one person claims only for themselves. |
| svcCreateBurst = 100 // per day, the cliCreateWindow |
| - svcWriteBurst = 120 // per minute, the cliRateWindow |
| - svcShipBurst = 60 // per hour, the cliShipWindow |
| + // svcWriteBurst: a mutation roughly every half second sustained for a |
| + // full minute — four times cliWriteBurst's per-person ceiling, roomy |
| + // for a CI fleet's shared credential running many promotes, rollbacks |
| + // and restarts back to back. |
| + svcWriteBurst = 120 // per minute, the cliRateWindow |
| + // svcShipBurst: one ship roughly every minute sustained for a full |
| + // hour — six times cliShipBurst's per-token ceiling, for a busy CI |
| + // fleet cutting many releases in a burst rather than one human's |
| + // occasional deploy. |
| + svcShipBurst = 60 // per hour, the cliShipWindow |
| - // wildFailBurst bounds FAILED wildcard enables per principal per minute. |
| - // The successful path is exempt (spec §4a): with wildcard DNS a Pages |
| - // hub makes no per-repo hostname call at all, and an enable is |
| - // genuinely once-per-domain. But the enabling gate does a LIVE, |
| - // SEQUENTIAL DNS round trip on the request path, and a caller whose |
| - // _acme-challenge CNAME is not there yet can retry forever — so the |
| - // failures are bounded even though the successes are not. Roomy enough |
| - // for a resolver blip, closed enough that an exempt door is not a way to |
| - // make the console do unbounded outbound DNS. |
| + // wildFailBurst bounds FAILED wildcard enables per principal per |
| + // minute — PEEKED before the DNS round trip and CONSUMED only after it |
| + // fails (rateLimiter.tripped / .allow, C33 review), so a caller whose |
| + // CNAME is correct never accumulates a failure and never meets it, |
| + // while a retrying caller stops costing the console a live outbound |
| + // DNS lookup once it has tripped. The successful path is exempt |
| + // entirely (spec §4a): with wildcard DNS a Pages hub makes no per-repo |
| + // hostname call at all, and an enable is genuinely once-per-domain. |
| + // Roomy enough for a resolver blip, closed enough that an exempt door |
| + // is not a way to make the console do unbounded outbound DNS. |
| wildFailBurst = 20 |
| + |
| + // domainsCeilingBurst is NOT a workload budget — attach/detach are |
| + // exempt from the write/instance budgets by design (spec §4a: a |
| + // wildcard hub makes near-zero per-repo calls, so there is no |
| + // legitimate workload to bound). It is a CONVERGENCE-RATE governor |
| + // instead: every EFFECTIVE attach or detach audits and bumps |
| + // control/serial, and boxes poll that serial at 2s and run a full pass |
| + // on every move, so an unlimited exempt door would let one |
| + // authenticated credential drive fleet-wide convergence at request |
| + // rate — an alternating attach/detach loop on one host, for instance. |
| + // 120/min is svcWriteBurst's own scale and 2x |
| + // TestWildcardEnableIsNotRateLimited's per-minute volume, so it stays |
| + // "not rate limited" in every sense a real caller experiences; it |
| + // exists purely to cap how fast one credential can make every box look |
| + // at the world again, not to throttle the exempt workload. |
| + domainsCeilingBurst = 120 // per minute, the cliRateWindow, keyed on the principal |
| ) |
| // rateLimiter is a fixed-window counter, keyed by whatever the caller thinks |
| @@ -191,6 +222,37 @@ func (l *rateLimiter) allow(key string, now time.Time) bool { |
| return w.n <= l.max |
| } |
| +// tripped is allow's READ-ONLY half: it reports whether key is ALREADY over |
| +// budget, in the current window, without recording a hit. It exists for a |
| +// caller that wants to skip an expensive operation ENTIRELY once the budget |
| +// is spent, rather than pay for the operation and only then be refused |
| +// (Task 5 review, C33) — a live outbound DNS round trip is the case that |
| +// motivated it, but the shape is general. |
| +// |
| +// Same sweep, same reason as allow: an expired window must read as fresh |
| +// here too, or a peek and the allow that follows it a moment later could |
| +// disagree about whether the caller is still bounded. It does NOT create an |
| +// entry for a key it has never seen and does NOT run allow's over-the-hard- |
| +// bound wholesale drop — a peek must never be the thing that evicts every |
| +// OTHER key's count, which a bare read has no business doing. |
| +func (l *rateLimiter) tripped(key string, now time.Time) bool { |
| + l.mu.Lock() |
| + defer l.mu.Unlock() |
| + if now.Sub(l.lastSweep) >= l.window { |
| + for k, w := range l.hits { |
| + if now.Sub(w.start) > l.window { |
| + delete(l.hits, k) |
| + } |
| + } |
| + l.lastSweep = now |
| + } |
| + w := l.hits[key] |
| + if w == nil || now.Sub(w.start) > l.window { |
| + return false |
| + } |
| + return w.n >= l.max |
| +} |
| + |
| // cliLimits is the set of limiters the CLI endpoints share, hung off the |
| // Server so a test gets a fresh set per fixture. |
| type cliLimits struct { |
| @@ -206,6 +268,12 @@ type cliLimits struct { |
| // wildFailBurst. The domains doors are otherwise exempt from write/ |
| // instances entirely (spec §4a). |
| wildFail *rateLimiter |
| + // domainsCeiling is the convergence-rate governor attach/detach keep |
| + // even though they are exempt from write/instances — see |
| + // domainsCeilingBurst. One limiter, one key shape (principalKey), |
| + // regardless of person or service: the exemption never depended on |
| + // which kind of caller it was, so neither does this ceiling. |
| + domainsCeiling *rateLimiter |
| } |
| func newCLILimits() *cliLimits { |
| @@ -236,6 +304,8 @@ func newCLILimits() *cliLimits { |
| svcWrite: newRateLimiter(svcWriteBurst, cliRateWindow), |
| svcShip: newRateLimiter(svcShipBurst, cliShipWindow), |
| wildFail: newRateLimiter(wildFailBurst, cliRateWindow), |
| + |
| + domainsCeiling: newRateLimiter(domainsCeilingBurst, cliRateWindow), |
| } |
| } |
diff --git a/internal/console/authcli_test.go b/internal/console/authcli_test.go| index f83d5701e..9a46329f5 100644 |
| --- a/internal/console/authcli_test.go |
| +++ b/internal/console/authcli_test.go |
| @@ -625,6 +625,33 @@ func TestPersonBudgetsAreUnchanged(t *testing.T) { |
| } |
| } |
| +// TestTwoNamedServicesHaveSeparateBudgets is Minor 5 (Task 5 review): two |
| +// DIFFERENT named services in one account must not share a budget just |
| +// because they share an account — closing this alongside C32's empty-name |
| +// collapse, since a naive fix for one (keying on account id alone) could |
| +// still fail the other. Exhausting "hub-a"'s create budget must leave |
| +// "hub-b"'s untouched. |
| +func TestTwoNamedServicesHaveSeparateBudgets(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + svcA := serviceToken(t, fx.st, OpsAccountID, "hub-a", "", svcrole.Publish) |
| + svcB := serviceToken(t, fx.st, OpsAccountID, "hub-b", "", svcrole.Publish) |
| + for i := 0; i < svcCreateBurst; i++ { |
| + name := fmt.Sprintf("hub-a-%d", i) |
| + if status, out := postJSON(t, fx.ts.URL+"/api/cli/apps/bab/new", map[string]string{"name": name}, svcA); status != http.StatusOK { |
| + t.Fatalf("hub-a create %d = %d %v", i, status, out) |
| + } |
| + } |
| + status, out := postJSON(t, fx.ts.URL+"/api/cli/apps/bab/new", map[string]string{"name": "hub-a-over"}, svcA) |
| + if status != http.StatusTooManyRequests { |
| + t.Fatalf("hub-a over budget = %d %v, want 429", status, out) |
| + } |
| + // hub-b, a DIFFERENT named service in the SAME account, is untouched. |
| + status, out = postJSON(t, fx.ts.URL+"/api/cli/apps/bab/new", map[string]string{"name": "hub-b-fresh"}, svcB) |
| + if status != http.StatusOK { |
| + t.Fatalf("hub-b create after hub-a exhausted its own budget = %d %v, want 200", status, out) |
| + } |
| +} |
| + |
| // TestServiceWriteBudgetIsOwnAndSeparateFromHuman: the write budget's own |
| // service picker (svcWriteBurst), exercised through a real write door |
| // (restart) rather than the picker in isolation — the call site is the part |
diff --git a/internal/console/svcprincipal.go b/internal/console/svcprincipal.go| index 8bd3d0212..43dc06f1f 100644 |
| --- a/internal/console/svcprincipal.go |
| +++ b/internal/console/svcprincipal.go |
| @@ -331,10 +331,21 @@ WHERE token_hash = ? AND account_id = ? AND revoked_at = 0`, now.Unix(), id, acc |
| // principalKey is what a rate limiter counts against. |
| // |
| // A person keys on their ADDRESS, byte for byte as before — no human's |
| -// limits move in this change. A service keys on its NAME, not on its token |
| -// hash, so that rotating a credential does not hand it a fresh budget: the |
| -// name is stable across a secret change precisely so budgets and audit rows |
| -// stay continuous. |
| +// limits move in this change. A NAMED service keys on its NAME, not on its |
| +// token hash, so that rotating a credential does not hand it a fresh |
| +// budget: the name is stable across a secret change precisely so budgets |
| +// and audit rows stay continuous. |
| +// |
| +// A legacy kind='app' row carries no name (Name == ""), and mirrors |
| +// auditActor's own branch for exactly the same row: without this branch, |
| +// EVERY app token in an account — whichever app it is bound to — would |
| +// collapse onto the single key "service:<id>/", sharing one budget between |
| +// credentials that have nothing to do with each other. Concretely: titogo's |
| +// routers each hold their own app token, several bound to different apps in |
| +// the same account; a mistyped CNAME on one router's wildFail budget must |
| +// not 429 every other router's wildcard enable. Keying on App instead of |
| +// Name gives each of them back its own bucket, the same scope App already |
| +// carries for authorization. |
| // |
| // The account id rather than the sqid because this string is never shown to |
| // anyone — it is a map key, and the id is what the resolver already has on |
| @@ -343,6 +354,9 @@ func principalKey(p Principal) string { |
| if p.Kind == PrincipalUser { |
| return p.Address |
| } |
| + if p.Name == "" { |
| + return "app:" + strconv.FormatInt(p.AccountID, 10) + "/" + p.App |
| + } |
| return "service:" + strconv.FormatInt(p.AccountID, 10) + "/" + p.Name |
| } |
| @@ -377,3 +391,33 @@ func (s *Server) allowShip(p Principal, r *http.Request, now time.Time) bool { |
| } |
| return s.cli.svcShip.allow(principalKey(p), now) |
| } |
| + |
| +// allowWildFail and wildFailTripped are wildFail's own picker pair — the |
| +// task's premise is that no call site reaches into s.cli by hand to choose |
| +// a limiter, and wildFail is no exception even though it is a single |
| +// counter shared by every principal kind rather than a person/service pair |
| +// like the three above. |
| +// |
| +// wildFail's caller needs BOTH a peek and a consume (C33 review): a live, |
| +// sequential DNS round trip on the request path must not run at all once a |
| +// principal has already tripped the bound, but only a FAILED lookup may |
| +// ever add to the count — a caller whose CNAME is correct must never |
| +// accumulate toward it no matter how many times it (harmlessly) succeeds. |
| +func (s *Server) allowWildFail(p Principal, now time.Time) bool { |
| + return s.cli.wildFail.allow(principalKey(p), now) |
| +} |
| + |
| +func (s *Server) wildFailTripped(p Principal, now time.Time) bool { |
| + return s.cli.wildFail.tripped(principalKey(p), now) |
| +} |
| + |
| +// allowDomains is the attach/detach convergence-rate ceiling |
| +// (domainsCeilingBurst) — not a workload budget (domains doors are exempt |
| +// from write/instances, spec §4a), a cap on how fast one credential can |
| +// make every box look at control/serial again. One limiter for every |
| +// principal kind, the same shape as allowWildFail and for the same reason: |
| +// the exemption this ceiling sits behind never depended on person vs. |
| +// service, so the ceiling behind it does not either. |
| +func (s *Server) allowDomains(p Principal, now time.Time) bool { |
| + return s.cli.domainsCeiling.allow(principalKey(p), now) |
| +} |