amadan
console: accept an uppercase listing id, report whether revoke matched
Review round on the mint doors: C40, C41, C42 and two minors. C40 (Important) — listingIDRe accepted lowercase hex only, so an id copied out of PowerShell's Get-FileHash, certutil or a pasted audit extract matched neither arm. The handle addressed nothing, the door answered with the same bytes as a successful revoke, no store call was made and the early return skipped the audit entirely. An owner racing a compromise walked away from a live admin credential believing it dead. The regex now accepts [0-9a-fA-F] and the id arm folds to lower. The NAME arm stays byte-exact: a 64-character segment can never be a name, so folding there is safe, while folding a name would make HUB silently revoke hub. C41 — revoke gains a non-authoritative "matched" boolean beside the unchanged 200 "result":"revoked". Idempotence only requires the second call on a handle to answer like the first, and "matched": false on a second revoke is both true and useful. The likely miss is a typo'd name, and an opaque 200 turned that into a false all-clear at the door used in anger. TestRevokeIsIdempotent's byte-identity assertion relaxes to status + result (controller's call) and gains a per-case matched assertion. C42 — an unaddressable handle is now audited under a subject that cannot be an app name, so the branch stops contradicting the principle its neighbour states. The handle itself is not echoed: it is unbounded caller-controlled text, which is why it never reaches the store either. Minor 5 — serviceView says why it carries no `revoked` field. Minor 6 — the account-scoped audit test now exercises rotate and checks the actor and detail, so serviceAuditKey(name, "") on the rotate path is pinned.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ
2 files changed,
+220
−34
internal/console/apiservices.go+84 −27internal/console/apiservices_test.go+136 −7
diff --git a/internal/console/apiservices.go b/internal/console/apiservices.go| index dc3e09be3..4508c215d 100644 |
| --- a/internal/console/apiservices.go |
| +++ b/internal/console/apiservices.go |
| @@ -71,6 +71,11 @@ type serviceView struct { |
| Rotated string `json:"rotated"` |
| LastUsed string `json:"last_used"` |
| MintedBy string `json:"minted_by"` |
| + // There is deliberately no `revoked` field, though the brief's field |
| + // list names one. ServicePrincipals filters revoked_at = 0 (a revoked |
| + // row is a tombstone, not history — see RevokeServicePrincipal), so |
| + // every row reaching here is live and the field would be a constant |
| + // false. Absent as derived, not forgotten. |
| } |
| // apiOwnerAccount is apiPrincipalAccount plus the owner check, for the doors |
| @@ -279,9 +284,26 @@ func serviceStamp(t time.Time) string { |
| return t.UTC().Format(time.RFC3339) |
| } |
| -// listingIDRe is the shape of a listing id: the lowercase hex sha256 |
| -// hashToken writes, exactly 64 characters. |
| -var listingIDRe = regexp.MustCompile(`^[0-9a-f]{64}$`) |
| +// listingIDRe is the shape of a listing id: the hex sha256 hashToken writes, |
| +// exactly 64 characters. It accepts UPPERCASE hex as well as lower, and the |
| +// id arm below folds what it matches to lower. |
| +// |
| +// That is not politeness, it is the difference between a credential being |
| +// dead and an operator believing it is. hashToken writes lowercase, but the |
| +// person reaching for this door is racing a compromise and is as likely to |
| +// have the id from PowerShell's Get-FileHash, certutil, or a pasted audit |
| +// extract — all of which emit uppercase — as from the listing. Case-strict, |
| +// that paste matches neither arm, and the answer to an unaddressable handle |
| +// is a 200 indistinguishable from a successful revoke. The operator walks |
| +// away from a live admin credential believing they killed it. |
| +// |
| +// Folding is provably safe HERE and nowhere else. A 64-character segment can |
| +// never be a credential name (ValidTunnelName caps a name at 32), so nothing |
| +// this arm folds could have been meant as a name. The NAME arm stays |
| +// byte-exact for exactly the opposite reason: folding there would make the |
| +// segment HUB revoke the credential hub, which is a different credential |
| +// than the one the caller typed, and silently. |
| +var listingIDRe = regexp.MustCompile(`^[0-9a-fA-F]{64}$`) |
| // serviceHandle is the {name} path segment resolved to how it addresses a |
| // row: a NAME (create's grammar) or a listing ID (C36). |
| @@ -289,8 +311,10 @@ var listingIDRe = regexp.MustCompile(`^[0-9a-f]{64}$`) |
| // The two grammars are disjoint — a name is at most 32 characters of |
| // [a-z0-9-] and a listing id is exactly 64 hex characters — so a segment can |
| // never be read as both, and no flag or query parameter is needed to say |
| -// which one the caller meant. A segment that is neither addresses nothing: |
| -// no row this console ever wrote could match it. |
| +// which one the caller meant. That disjointness is also what makes the id |
| +// arm safe to case-fold while the name arm is not (see listingIDRe). A |
| +// segment that is neither addresses nothing: no row this console ever wrote |
| +// could match it. |
| // |
| // Refusing an unrecognisable segment here rather than passing it to the |
| // store also keeps an arbitrary, arbitrarily long path segment out of the |
| @@ -314,7 +338,9 @@ func serviceHandleFrom(r *http.Request) serviceHandle { |
| case registry.ValidTunnelName(seg): |
| return serviceHandle{name: seg} |
| case listingIDRe.MatchString(seg): |
| - return serviceHandle{id: seg} |
| + // Folded to the case hashToken writes. See listingIDRe: safe on this |
| + // arm alone, because a 64-character segment cannot be a name. |
| + return serviceHandle{id: strings.ToLower(seg)} |
| } |
| return serviceHandle{} |
| } |
| @@ -337,6 +363,15 @@ func serviceAuditKey(name, app string) string { |
| return name |
| } |
| +// serviceAuditSubject is the app column an audit row is filed under when |
| +// there is no credential to name — an attempted revoke whose handle |
| +// addresses nothing (C42). |
| +// |
| +// The parentheses and the space are load-bearing: an app name is lowercase |
| +// letters, digits and dashes starting with a letter (handleAPICreateApp), so |
| +// this string cannot collide with a real app's activity feed. |
| +const serviceAuditSubject = "(service credentials)" |
| + |
| // findServiceCredential looks one of this account's LIVE credentials up by |
| // whichever handle addressed it. |
| // |
| @@ -432,19 +467,25 @@ func (s *Server) handleAPIServiceRotate(w http.ResponseWriter, r *http.Request) |
| // without it an owner could see such a credential in the listing and have no |
| // way to kill it from here. serviceHandle tells the two apart by shape. |
| // |
| -// REVOCATION IS IDEMPOTENT, and that is a deliberate refusal to be precise. |
| -// An unknown name, an unknown id, an already-revoked credential and a live |
| -// one all answer the same 200, because the person reaching for this door is |
| -// usually racing a compromise and must not have to work out whether their |
| -// first attempt landed. The one thing they need to know afterwards — that |
| -// the credential is dead — is true in every one of those cases. |
| +// REVOCATION IS IDEMPOTENT. An unknown name, an unknown id, an |
| +// already-revoked credential and a live one all answer 200 |
| +// "result":"revoked", because the person reaching for this door is usually |
| +// racing a compromise and must not have to work out whether their first |
| +// attempt landed. What they need afterwards — that no credential of that |
| +// handle is live — is true in every one of those cases. |
| // |
| -// It costs nothing in disclosure: the caller is the account's owner, so |
| -// "does a credential of this name exist here" is not a fact being kept from |
| -// them. It is available from the listing one door away. An id belonging to |
| -// ANOTHER account is a different matter, and it is refused by construction: |
| -// both store calls below are scoped to acct.ID, so a foreign id matches |
| -// nothing and gets the same 200 as a fabricated one, having changed |
| +// AND IT SAYS WHICH IT WAS, in a non-authoritative "matched" boolean (C41). |
| +// Idempotence demands only that a SECOND call on the same handle answer like |
| +// the first, and "matched": false on a second revoke is both true and |
| +// useful. The likely miss here is not a stolen id but a typo — `revoke hubb` |
| +// — and an opaque 200 turns that into a false all-clear at the one door used |
| +// in anger. It costs nothing in disclosure either: the caller is the |
| +// account's owner, so "is there a live credential of this handle" is not a |
| +// fact being kept from them; it is in the listing one door away. |
| +// |
| +// An id belonging to ANOTHER account is a different matter, and it is |
| +// refused by construction: both store calls below are scoped to acct.ID, so |
| +// a foreign id matches nothing, reports "matched": false, and has changed |
| // nothing. |
| func (s *Server) handleAPIServiceRevoke(w http.ResponseWriter, r *http.Request) { |
| p, acct, ok := s.apiOwnerAccount(w, r) |
| @@ -455,12 +496,27 @@ func (s *Server) handleAPIServiceRevoke(w http.ResponseWriter, r *http.Request) |
| jsonError(w, http.StatusTooManyRequests, "too many requests — wait a minute and try again") |
| return |
| } |
| + actor := auditActor(p, acct) |
| h := serviceHandleFrom(r) |
| if !h.addresses() { |
| // A segment that is neither a name nor a listing id matches no row |
| - // this console ever wrote, so it gets the answer an unknown handle |
| - // gets — which is the same answer every other case gets. |
| - writeJSON(w, http.StatusOK, map[string]any{"result": "revoked"}) |
| + // this console ever wrote, so it reports the same "nothing matched" |
| + // an unknown handle does. |
| + // |
| + // It is AUDITED, under a subject that is not an app (C42). An |
| + // attempted revoke that could not have worked is exactly the event |
| + // an operator goes looking for afterwards — "I definitely revoked |
| + // that" — and the early return used to leave no trace of it at all, |
| + // which contradicted the principle stated at the audit call below. |
| + // |
| + // The handle itself is not echoed into the row. By definition it is |
| + // not something this console wrote, and it is an unbounded |
| + // caller-controlled path segment; keeping it out of the trail is the |
| + // same reason serviceHandle refuses to pass it to the store. Who, |
| + // when, and "the handle was unusable" is what the operator needs. |
| + s.audit(acct, serviceAuditSubject, actor, "service-revoke", |
| + "revoke attempted with a handle that is neither a credential name nor a listing id (cli)") |
| + writeJSON(w, http.StatusOK, map[string]any{"result": "revoked", "matched": false}) |
| return |
| } |
| // Read before the write, for the audit only: a revoked row leaves the |
| @@ -505,15 +561,16 @@ func (s *Server) handleAPIServiceRevoke(w http.ResponseWriter, r *http.Request) |
| } |
| } |
| } |
| - actor := auditActor(p, acct) |
| detail := "revoked " + what + " (cli)" |
| if err != nil { |
| detail = "revoked " + what + " — nothing live matched (cli)" |
| } |
| s.audit(acct, serviceAuditKey(subject, row.App), actor, "service-revoke", detail) |
| - // The body carries no name and nothing else request-derived: it is the |
| - // same bytes for a live credential, an already-revoked one and a name |
| - // that never existed, which is what makes the idempotence above visible |
| - // from the outside rather than merely true inside. |
| - writeJSON(w, http.StatusOK, map[string]any{"result": "revoked"}) |
| + // The body carries no name and nothing else request-derived, so two |
| + // revokes of the same handle are the same bytes — which is what makes |
| + // the idempotence above visible from the outside rather than merely true |
| + // inside. `matched` is the one thing that varies, and it varies with |
| + // what HAPPENED rather than with what was asked: false on the second |
| + // call for the same credential the first call killed. |
| + writeJSON(w, http.StatusOK, map[string]any{"result": "revoked", "matched": err == nil}) |
| } |
diff --git a/internal/console/apiservices_test.go b/internal/console/apiservices_test.go| index d9c4a5b61..873d31fd8 100644 |
| --- a/internal/console/apiservices_test.go |
| +++ b/internal/console/apiservices_test.go |
| @@ -52,6 +52,20 @@ func jsonField(t *testing.T, body, key string) string { |
| return s |
| } |
| +// jsonBool pulls one boolean field out of a raw JSON response body. |
| +func jsonBool(t *testing.T, body, key string) bool { |
| + t.Helper() |
| + var out map[string]any |
| + if err := json.Unmarshal([]byte(body), &out); err != nil { |
| + t.Fatalf("decode %s: %v (body %s)", key, err, body) |
| + } |
| + v, ok := out[key].(bool) |
| + if !ok { |
| + t.Fatalf("%s is not a boolean in %s", key, body) |
| + } |
| + return v |
| +} |
| + |
| // TestServiceMintIsOwnerOnly — minting is an authority grant that outlives |
| // its minter, so it needs the owner gate, not the ordinary member write |
| // door. The two refusals differ deliberately: a member who already proved |
| @@ -454,6 +468,9 @@ func TestRevokeIsIdempotent(t *testing.T) { |
| if first != http.StatusOK { |
| t.Fatalf("revoke = %d %s; want 200", first, firstBody) |
| } |
| + if jsonField(t, firstBody, "result") != "revoked" || !jsonBool(t, firstBody, "matched") { |
| + t.Errorf("revoking a live credential = %s; want result revoked and matched true", firstBody) |
| + } |
| if status, out := getRaw(t, fx.ts.URL+"/api/cli/apps", tok); status != http.StatusUnauthorized { |
| t.Errorf("a revoked credential still works: %d %s", status, out) |
| } |
| @@ -467,9 +484,17 @@ func TestRevokeIsIdempotent(t *testing.T) { |
| t.Errorf("re-revoke = %d, unknown revoke = %d, unmintable revoke = %d; want 200 from all three", |
| second, unknown, junk) |
| } |
| - if secondBody != firstBody || unknownBody != firstBody || junkBody != firstBody { |
| - t.Errorf("revoke answers differ:\nfirst: %s\nsecond: %s\nunknown: %s\njunk: %s", |
| - firstBody, secondBody, unknownBody, junkBody) |
| + // Idempotence is the STATUS and the result: every one of these leaves |
| + // the caller with the same fact, that nothing of that handle is live. |
| + for what, body := range map[string]string{"second": secondBody, "unknown": unknownBody, "junk": junkBody} { |
| + if jsonField(t, body, "result") != "revoked" { |
| + t.Errorf("%s revoke = %s; want result revoked", what, body) |
| + } |
| + // ...and `matched` is the non-authoritative half that says which it |
| + // was, so a typo'd name is not a false all-clear (C41). |
| + if jsonBool(t, body, "matched") { |
| + t.Errorf("%s revoke reports matched true, but nothing was live: %s", what, body) |
| + } |
| } |
| // And the row leaves the listing rather than lingering as a tombstone |
| // somebody has to interpret. |
| @@ -534,6 +559,9 @@ func TestServiceDoorsAuditAgainstTheApp(t *testing.T) { |
| // TestAccountScopedCredentialAuditsUnderItsName: with no app to file |
| // against, the subject IS the credential, so it keeps the shape fleets uses. |
| +// It exercises all three verbs and checks the actor and the detail, the same |
| +// as its app-scoped sibling — rotate especially, because |
| +// serviceAuditKey(name, "") on the rotate path is pinned nowhere else. |
| func TestAccountScopedCredentialAuditsUnderItsName(t *testing.T) { |
| fx := newConsoleFixture(t) |
| owner := cliToken(t, fx, "paul@keymail.dev", "code-svc-audit-acct") |
| @@ -541,6 +569,9 @@ func TestAccountScopedCredentialAuditsUnderItsName(t *testing.T) { |
| if code, body := postRaw(t, base, mintBody("root", "admin", ""), owner); code != http.StatusCreated { |
| t.Fatalf("mint = %d %s", code, body) |
| } |
| + if code, body := postRaw(t, base+"/root/rotate", map[string]string{}, owner); code != http.StatusOK { |
| + t.Fatalf("rotate = %d %s", code, body) |
| + } |
| if code, body := postRaw(t, base+"/root/revoke", map[string]string{}, owner); code != http.StatusOK { |
| t.Fatalf("revoke = %d %s", code, body) |
| } |
| @@ -548,12 +579,104 @@ func TestAccountScopedCredentialAuditsUnderItsName(t *testing.T) { |
| if err != nil { |
| t.Fatal(err) |
| } |
| - seen := map[string]bool{} |
| + want := map[string]bool{"service-create": false, "service-rotate": false, "service-revoke": false} |
| for _, e := range events { |
| - seen[e.Action] = true |
| + if _, ok := want[e.Action]; !ok { |
| + continue |
| + } |
| + want[e.Action] = true |
| + if e.Address != "paul@keymail.dev" { |
| + t.Errorf("%s audited as %q; want the human who pressed it", e.Action, e.Address) |
| + } |
| + if !strings.Contains(e.Detail, "root") { |
| + t.Errorf("%s detail = %q; it must name the credential", e.Action, e.Detail) |
| + } |
| + } |
| + for action, seen := range want { |
| + if !seen { |
| + t.Errorf("no %s audit row under the credential's name", action) |
| + } |
| + } |
| +} |
| + |
| +// TestRevokeAcceptsAnUppercaseListingID (C40) is the one that fires when it |
| +// matters most. hashToken writes lowercase, but an owner racing a compromise |
| +// as often has the id from PowerShell's Get-FileHash, certutil or a pasted |
| +// audit extract — all uppercase. Case-strict, that paste matched neither arm |
| +// and the door answered exactly like a successful revoke, leaving a live |
| +// admin credential behind a false all-clear. |
| +func TestRevokeAcceptsAnUppercaseListingID(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + owner := cliToken(t, fx, "paul@keymail.dev", "code-svc-revoke-upper") |
| + tok := serviceToken(t, fx.st, OpsAccountID, "hub", "", svcrole.Admin) |
| + base := fx.ts.URL + "/api/cli/services/" + fx.sqid |
| + |
| + upper := strings.ToUpper(hashToken(tok)) |
| + code, body := postRaw(t, base+"/"+upper+"/revoke", map[string]string{}, owner) |
| + if code != http.StatusOK { |
| + t.Fatalf("revoke by uppercase id = %d %s; want 200", code, body) |
| + } |
| + if !jsonBool(t, body, "matched") { |
| + t.Errorf("revoke by uppercase id = %s; want matched true — the credential was live", body) |
| + } |
| + if status, out := getRaw(t, fx.ts.URL+"/api/cli/apps", tok); status != http.StatusUnauthorized { |
| + t.Errorf("the credential still works after an uppercase-id revoke: %d %s", status, out) |
| + } |
| +} |
| + |
| +// TestRevokeDoesNotFoldACredentialName is the other half of C40, and the |
| +// reason the folding lives in the id arm alone: a 64-character segment |
| +// cannot be a name, so folding there can misroute nothing — but folding a |
| +// NAME would make the segment HUB revoke the credential hub, which is a |
| +// different credential from the one the caller typed, killed silently. |
| +func TestRevokeDoesNotFoldACredentialName(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + owner := cliToken(t, fx, "paul@keymail.dev", "code-svc-revoke-case") |
| + tok := serviceToken(t, fx.st, OpsAccountID, "hub", "", svcrole.Publish) |
| + base := fx.ts.URL + "/api/cli/services/" + fx.sqid |
| + |
| + code, body := postRaw(t, base+"/HUB/revoke", map[string]string{}, owner) |
| + if code != http.StatusOK { |
| + t.Fatalf("revoke HUB = %d %s; want 200", code, body) |
| + } |
| + if jsonBool(t, body, "matched") { |
| + t.Errorf("revoke HUB reports matched true — it must not reach hub: %s", body) |
| } |
| - if !seen["service-create"] || !seen["service-revoke"] { |
| - t.Errorf("account-scoped credential's rows not filed under its name: %+v", events) |
| + if status, out := getRaw(t, fx.ts.URL+"/api/cli/apps", tok); status != http.StatusOK { |
| + t.Errorf("hub was revoked by a request naming HUB: %d %s", status, out) |
| + } |
| +} |
| + |
| +// TestUnusableRevokeHandleIsAudited (C42): an attempted revoke that could |
| +// not have worked is exactly the event an operator goes looking for |
| +// afterwards. The early return used to leave no row at all, which |
| +// contradicted the principle stated at the other audit call in the same |
| +// handler. Filed under a subject that cannot be an app. |
| +func TestUnusableRevokeHandleIsAudited(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + owner := cliToken(t, fx, "paul@keymail.dev", "code-svc-revoke-audit") |
| + base := fx.ts.URL + "/api/cli/services/" + fx.sqid |
| + if code, body := postRaw(t, base+"/Not%20A%20Handle/revoke", map[string]string{}, owner); code != http.StatusOK { |
| + t.Fatalf("revoke with an unusable handle = %d %s; want 200", code, body) |
| + } |
| + events, err := fx.st.Events(OpsAccountID, serviceAuditSubject, 50) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if len(events) != 1 { |
| + t.Fatalf("audit rows under %q = %d, want 1: %+v", serviceAuditSubject, len(events), events) |
| + } |
| + e := events[0] |
| + if e.Action != "service-revoke" || e.Address != "paul@keymail.dev" { |
| + t.Errorf("row = %+v; want a service-revoke by the human who pressed it", e) |
| + } |
| + if !strings.Contains(e.Detail, "neither a credential name nor a listing id") { |
| + t.Errorf("detail = %q; it must say the handle was unusable", e.Detail) |
| + } |
| + // The handle itself is unbounded caller-controlled text and must not be |
| + // echoed into the trail — the same reason it never reaches the store. |
| + if strings.Contains(e.Detail, "Not A Handle") { |
| + t.Errorf("detail echoes the caller's handle: %q", e.Detail) |
| } |
| } |
| @@ -647,6 +770,12 @@ func TestRevokeIgnoresAnIdFromAnotherAccount(t *testing.T) { |
| t.Errorf("a foreign id is distinguishable from an unknown name:\nunknown: %s\nforeign: %s", |
| mineBody, foreignBody) |
| } |
| + // And it says so: a foreign id must never report a match, or the |
| + // non-authoritative signal would be telling the caller they killed |
| + // something in an account they do not hold. |
| + if jsonBool(t, foreignBody, "matched") { |
| + t.Errorf("a foreign id reports matched true: %s", foreignBody) |
| + } |
| if status, body := getRaw(t, fx.ts.URL+"/api/cli/apps", theirs); status != http.StatusOK { |
| t.Errorf("the other account's credential was revoked through this door: %d %s", status, body) |
| } |