console: passkey step-up for sensitive actions, and account deletion that sticks (#7)
* console: passkey step-up for sensitive actions — membership says who you are, not that you are here Spec §7a. A session cookie lifted from a laptop is currently enough to approve a `carlos auth login` device grant, remove a member, or revoke somebody's terminals. Membership answers "who is this?"; nothing here answered "are they still at the keyboard?". The route is a fresh Keymail authentication (keymail PR #149: prompt=login, max_age, auth_time), not console-native WebAuthn — identity stays in one place per this platform's design. - stepup.go: requireFreshAuth. Reads database state written only by a verified callback, so the gate is UNSKIPPABLE server-side: a direct POST with a good session and a good CSRF token is refused all the same. The interrupted action is preserved so a human is not dumped back at square one, but it is held SERVER-SIDE, bound to the session that stashed it, single-use by row deletion and short-lived; the browser carries an opaque token in an HttpOnly cookie and nothing else, so it cannot swap the action or replay it. The csrf field is deliberately not stored — sessions are kept hashed here, and a form blob is no exemption; the resume re-injects it from the browser's own cookie. - Freshness window: 5 minutes, measured from Keymail's auth_time (the gesture), never from when the console heard about it. - Gated: approving a device grant, revoking a CLI token, removing a member, transferring ownership, deleting an account. NOT promote/rollback — those already need the release signing key, and a daily passkey touch buys little. Denying a device grant stays ungated: it is the panic button and only ever destroys a pending request. - ownership.go: the three owner actions §7a names but that had no HTTP handler at all. Deletion is narrow on purpose — an account still holding apps is refused rather than cascading into objects, routes and domains nobody can restore. - FAILS CLOSED, every path: Keymail unreachable, a deployment that does not advertise re-authentication, a grant with no auth_time, a stale auth_time, or a gesture from a different Keymail address — all refuse, none downgrade to session-only. - The byte-identical 404 survives: step-up runs AFTER the ownership gate, so a non-member never sees a redirect that would confirm the account exists, and never parks a pending action. CSRF is unchanged and still runs first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * console: deleting an account is a tombstone in the bucket, not rows in a console DeleteAccount emptied this console's tables and nothing else. Account identity lives at accounts/<sqid>/account.json, and every console start materializes from it, so the account came back on the next deploy — with the owner membership the manifest names, which after an ownership transfer is not the person who deleted it. The operator saw "Deleted X" and X returned. The manifest is now tombstoned with deleted_at rather than removed: the id stays spent, because NextAccountID takes one past the highest id any manifest claims and a reused id is a reused sqid — a hostname already published in Certificate Transparency and cached in DNS. owner is cleared so a consumer that has never heard of deleted_at at worst materializes something nobody holds. Every consumer that materializes the manifest skips a tombstone (console.MaterializeAccounts, domains.Reconcile, which also drops the registry row so the deletion reaches every box, not just the console that performed it); NextAccountID still counts it. The bucket write happens first and a failure abandons the deletion — a console with no release store now refuses rather than deleting locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> ---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
17 files changed,
+1635
−26
docs/superpowers/specs/2026-08-02-multi-account-design.md+31 −0internal/console/authcli.go+18 −0internal/console/authcli_test.go+22 −5internal/console/deletion_test.go+131 −0internal/console/keymail.go+78 −6internal/console/keymail_test.go+38 −4internal/console/materialize.go+8 −0internal/console/ownership.go+285 −0internal/console/server.go+13 −0internal/console/stepup.go+434 −0internal/console/stepup_test.go+401 −0internal/console/store.go+5 −1internal/console/templates/apps.html+32 −2internal/domains/mint.go+72 −3internal/domains/reconcile.go+12 −5internal/domains/reconcile_test.go+36 −0internal/registry/registry.go+19 −0
diff --git a/docs/superpowers/specs/2026-08-02-multi-account-design.md b/docs/superpowers/specs/2026-08-02-multi-account-design.md| index 76295cc8d..24b4dade0 100644 |
| --- a/docs/superpowers/specs/2026-08-02-multi-account-design.md |
| +++ b/docs/superpowers/specs/2026-08-02-multi-account-design.md |
| @@ -271,6 +271,37 @@ origin (marginally stronger against relay) and would let the platform |
| stand alone without Keymail — it stays a named alternative if the |
| platform ever needs IdP independence. |
| +## 7b. Deleting an account is a tombstone (approved 2026-08-02) |
| + |
| +An account can be retired once it holds no apps. Deletion is refused while |
| +any app is homed there — an app's objects, registry routes and platform |
| +domains are not one click's to destroy — and the ops account is never |
| +deletable. |
| + |
| +**The deletion happens in the bucket, and only then in the console.** |
| +Account identity lives at `accounts/<sqid>/account.json`, and every console |
| +start materializes from it, so emptying the console's own tables deletes |
| +nothing durable: the next deploy re-creates the account and re-adopts the |
| +owner the manifest names — which, after an ownership transfer, is not even |
| +the person who deleted it. A console that cannot write to the bucket |
| +refuses the deletion outright rather than performing a local one that |
| +undoes itself. |
| + |
| +**The manifest is rewritten, not removed.** `deleted_at` marks it; the id |
| +stays spent, because `NextAccountID` takes one past the highest id any |
| +manifest claims and a reused id is a reused sqid — a hostname already in |
| +Certificate Transparency, in DNS caches and in somebody's bookmarks, handed |
| +to a stranger. `owner` is cleared, so a consumer too old to know about |
| +`deleted_at` at worst materializes an account nobody holds rather than one |
| +it hands back to a former owner. |
| + |
| +The split that governs every consumer of this manifest: anything that |
| +**materializes** it must skip a tombstone (`console.MaterializeAccounts`, |
| +`domains.Reconcile` — which also drops the registry row so the deletion |
| +reaches every box, not just the console that performed it); anything that |
| +**allocates** against it must still count it (`NextAccountID`). Backwards |
| +in either direction resurrects a deleted account or reissues its identity. |
| + |
| ## 8. Future work (named, not built) |
| - **Roles beyond owner/member** — read-only members, deploy-but-not-invite. |
diff --git a/internal/console/authcli.go b/internal/console/authcli.go| index faa76a60c..6fdd51679 100644 |
| --- a/internal/console/authcli.go |
| +++ b/internal/console/authcli.go |
| @@ -509,6 +509,17 @@ func (s *Server) handleCLIApprove(w http.ResponseWriter, r *http.Request) { |
| switch action { |
| case "approve", "deny": |
| + // Passkey step-up on APPROVE only (spec §7a, stepup.go). Approving |
| + // mints a 30-day credential that acts as this identity from a machine |
| + // that has never been seen — the single largest thing a stolen session |
| + // cookie could do here, and the reason presence has to be proved rather |
| + // than inferred. Denying is left ungated on purpose: "that wasn't me" |
| + // is the panic button, it only ever destroys a pending request, and |
| + // putting an authenticator prompt in front of it would be friction on |
| + // exactly the path we want people to take without hesitating. |
| + if action == "approve" && !s.requireFreshAuth(w, r, addr, raw) { |
| + return |
| + } |
| var err error |
| if action == "deny" { |
| err = s.store.DenyCLIAuth(code, now) |
| @@ -579,6 +590,13 @@ func (s *Server) handleCLITokenRevoke(w http.ResponseWriter, r *http.Request) { |
| s.renderMessage(w, r, http.StatusForbidden, "Refused", "Cross-site request blocked.") |
| return |
| } |
| + // Step-up (spec §7a): killing a terminal is a security action, and the |
| + // person doing it should be the person whose terminals they are. An |
| + // attacker with a stolen cookie revoking the real owner's credentials is a |
| + // lockout, and the freshness check is what stops it being free. |
| + if !s.requireFreshAuth(w, r, addr, raw) { |
| + return |
| + } |
| // Scoped to the signed-in address inside the store call: an id belonging |
| // to somebody else simply is not found. |
| if err := s.store.RevokeCLIToken(addr, r.FormValue("id")); err != nil { |
diff --git a/internal/console/authcli_test.go b/internal/console/authcli_test.go| index c79cea0d8..ee59bc508 100644 |
| --- a/internal/console/authcli_test.go |
| +++ b/internal/console/authcli_test.go |
| @@ -122,7 +122,18 @@ func TestCLILoginEndToEnd(t *testing.T) { |
| t.Fatalf("signed-out approve = %d -> %q, want 302 -> /", code, loc) |
| } |
| - code, _, body = postForm(t, browser, fx.ts.URL+"/auth/cli/approve", |
| + // Approving mints a 30-day credential, so it is behind passkey step-up |
| + // (stepup.go): a direct POST is answered with a trip through Keymail, and |
| + // only the confirmed round trip approves. |
| + if code, loc, _ := postForm(t, browser, fx.ts.URL+"/auth/cli/approve", |
| + url.Values{"code": {userCode}, "action": {"approve"}, "csrf": {csrf}}); code != http.StatusSeeOther || |
| + !strings.HasPrefix(loc, fx.fk.srv.URL) { |
| + t.Fatalf("approve without a fresh passkey = %d -> %q, want a redirect into Keymail", code, loc) |
| + } |
| + if _, poll := postJSON(t, fx.ts.URL+"/auth/cli/poll", map[string]string{"device_code": deviceCode}, ""); str(t, poll, "status") == CLIAuthApproved { |
| + t.Fatal("the terminal was approved before the passkey confirmation") |
| + } |
| + code, _, body = postGated(t, fx, browser, fx.ts.URL+"/auth/cli/approve", |
| url.Values{"code": {userCode}, "action": {"approve"}, "csrf": {csrf}}) |
| if code != http.StatusOK || !strings.Contains(body, "Approved") { |
| t.Fatalf("approve = %d, body:\n%s", code, body) |
| @@ -170,7 +181,9 @@ func TestCLILoginEndToEnd(t *testing.T) { |
| url.Values{"id": {id}, "csrf": {"wrong"}}); code != http.StatusForbidden { |
| t.Fatalf("revoke with a bad CSRF token = %d, want 403", code) |
| } |
| - if code, _, _ := postForm(t, browser, fx.ts.URL+"/auth/cli/tokens/revoke", |
| + // Revoking is step-up gated too (the same session that has just confirmed |
| + // a passkey is still inside the window here, so no second trip is made). |
| + if code, _, _ := postGated(t, fx, browser, fx.ts.URL+"/auth/cli/tokens/revoke", |
| url.Values{"id": {id}, "csrf": {csrf}}); code != http.StatusSeeOther { |
| t.Fatalf("revoke = %d, want 303", code) |
| } |
| @@ -234,7 +247,7 @@ func TestCLIIssuanceNotifiesTheUser(t *testing.T) { |
| _, start := postJSON(t, fx.ts.URL+"/auth/cli/start", map[string]string{"label": "paul@laptop"}, "") |
| userCode, deviceCode := str(t, start, "user_code"), str(t, start, "device_code") |
| - postForm(t, browser, fx.ts.URL+"/auth/cli/approve", |
| + postGated(t, fx, browser, fx.ts.URL+"/auth/cli/approve", |
| url.Values{"code": {userCode}, "action": {"approve"}, "csrf": {sessionToken(t, fx, browser)}}) |
| if _, poll := postJSON(t, fx.ts.URL+"/auth/cli/poll", map[string]string{"device_code": deviceCode}, ""); str(t, poll, "token") == "" { |
| t.Fatalf("no token issued: %v", poll) |
| @@ -366,7 +379,11 @@ func TestCLIApproveRateLimitsWrongCodes(t *testing.T) { |
| csrf := sessionToken(t, fx, browser) |
| limited := false |
| for i := 0; i < cliGuessBurst+3; i++ { |
| - code, _, _ := postForm(t, browser, fx.ts.URL+"/auth/cli/approve", |
| + // postGated, because approve is step-up gated: the first |
| + // attempt walks the passkey round trip, the rest are inside |
| + // the window. The limiter still has to bite either way — a |
| + // confirmed human is not licensed to guess codes. |
| + code, _, _ := postGated(t, fx, browser, fx.ts.URL+"/auth/cli/approve", |
| url.Values{"code": {"AAAA-AAAA"}, "action": {action}, "csrf": {csrf}}) |
| if code == http.StatusTooManyRequests { |
| limited = true |
| @@ -451,7 +468,7 @@ func cliToken(t *testing.T, fx *consoleFixture, address, oauthCode string) strin |
| signUp(t, fx, browser, oauthCode) |
| _, start := postJSON(t, fx.ts.URL+"/auth/cli/start", map[string]string{"label": "test@harness"}, "") |
| userCode, deviceCode := str(t, start, "user_code"), str(t, start, "device_code") |
| - if code, _, body := postForm(t, browser, fx.ts.URL+"/auth/cli/approve", |
| + if code, _, body := postGated(t, fx, browser, fx.ts.URL+"/auth/cli/approve", |
| url.Values{"code": {userCode}, "action": {"approve"}, "csrf": {sessionToken(t, fx, browser)}}); code != http.StatusOK { |
| t.Fatalf("approve = %d:\n%s", code, body) |
| } |
diff --git a/internal/console/deletion_test.go b/internal/console/deletion_test.go| new file mode 100644 |
| index 000000000..5fdb21e04 |
| --- /dev/null |
| +++ b/internal/console/deletion_test.go |
| @@ -0,0 +1,131 @@ |
| +package console |
| + |
| +import ( |
| + "context" |
| + "encoding/json" |
| + "net/http" |
| + "net/url" |
| + "testing" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/domains" |
| + "github.com/carlosframework/platform/internal/release" |
| +) |
| + |
| +// deletableFixture signs an owner in and gives them a SECOND account — one that |
| +// is not ops, holds no apps, and therefore can actually be deleted. Minted |
| +// through the bucket and materialized, which is how every real account arrives. |
| +func deletableFixture(t *testing.T) (*consoleFixture, *http.Client, string, string) { |
| + t.Helper() |
| + fx := newConsoleFixture(t) |
| + c := newBrowser(t) |
| + signUp(t, fx, c, "code-delete") |
| + _, sqid, err := domains.MintAccount(context.Background(), fx.rel, "Testco", "paul@keymail.dev", time.Now()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if n, err := MaterializeAccounts(context.Background(), fx.rel, fx.st); err != nil || n != 1 { |
| + t.Fatalf("materialize = %d %v, want 1", n, err) |
| + } |
| + return fx, c, sessionToken(t, fx, c), sqid |
| +} |
| + |
| +// A deleted account stays deleted across a console restart. |
| +// |
| +// This is the failure the tombstone exists for: account identity lives in the |
| +// bucket, and every console start materializes it back, so a deletion that only |
| +// emptied the console's own tables was undone by the next deploy — silently, |
| +// and complete with the owner membership the manifest names. |
| +func TestDeletedAccountDoesNotComeBack(t *testing.T) { |
| + fx, c, csrf, sqid := deletableFixture(t) |
| + ctx := context.Background() |
| + |
| + status, _, body := postGated(t, fx, c, fx.ts.URL+"/a/"+sqid+"/delete", |
| + url.Values{"csrf": {csrf}, "confirm": {"Testco"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("delete = %d:\n%s", status, body) |
| + } |
| + if a, _ := fx.st.AccountBySqid(sqid); a != nil { |
| + t.Fatal("the account survived its own deletion") |
| + } |
| + |
| + // The next console start. Nothing may bring it back. |
| + if n, err := MaterializeAccounts(ctx, fx.rel, fx.st); err != nil || n != 0 { |
| + t.Fatalf("materialize after deletion = %d %v, want 0", n, err) |
| + } |
| + if a, _ := fx.st.AccountBySqid(sqid); a != nil { |
| + t.Fatal("the deleted account was materialized back out of the bucket") |
| + } |
| + // And no membership was resurrected with it — the manifest's owner field is |
| + // cleared by the tombstone precisely so a consumer that has never heard of |
| + // deleted_at cannot hand the account back to a former owner. |
| + m := readManifest(t, fx.rel, sqid) |
| + if !m.Deleted() { |
| + t.Fatal("the manifest was not tombstoned") |
| + } |
| + if m.Owner != "" { |
| + t.Fatalf("tombstone kept owner %q", m.Owner) |
| + } |
| +} |
| + |
| +// The id a deleted account held is never handed to another account. Sqids ride |
| +// in hostnames and in Certificate Transparency; reissuing one would give a new |
| +// account a name the world already associates with somebody else. |
| +func TestTombstonedAccountKeepsItsIDSpent(t *testing.T) { |
| + fx, c, csrf, sqid := deletableFixture(t) |
| + ctx := context.Background() |
| + before, err := domains.NextAccountID(ctx, fx.rel) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if status, _, body := postGated(t, fx, c, fx.ts.URL+"/a/"+sqid+"/delete", |
| + url.Values{"csrf": {csrf}, "confirm": {"Testco"}}); status != http.StatusSeeOther { |
| + t.Fatalf("delete = %d:\n%s", status, body) |
| + } |
| + |
| + after, err := domains.NextAccountID(ctx, fx.rel) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if after != before { |
| + t.Fatalf("next account id moved from %d to %d after a deletion — the retired id is being reused", before, after) |
| + } |
| + _, next, err := domains.MintAccount(ctx, fx.rel, "Nextco", "paul@keymail.dev", time.Now()) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if next == sqid { |
| + t.Fatalf("a new account was minted onto the retired sqid %q", sqid) |
| + } |
| +} |
| + |
| +// A bucket that cannot be written is a deletion that does not happen. Refusing |
| +// is the only honest answer: a console-only deletion looks like it worked and |
| +// then undoes itself. |
| +func TestDeletionRefusedWhenTheBucketIsUnreachable(t *testing.T) { |
| + fx, c, csrf, sqid := deletableFixture(t) |
| + fx.srv.cfg.Releases = nil |
| + |
| + status, _, body := postGated(t, fx, c, fx.ts.URL+"/a/"+sqid+"/delete", |
| + url.Values{"csrf": {csrf}, "confirm": {"Testco"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("delete = %d:\n%s", status, body) |
| + } |
| + if a, _ := fx.st.AccountBySqid(sqid); a == nil { |
| + t.Fatal("the account was deleted locally even though the bucket could not be told") |
| + } |
| +} |
| + |
| +func readManifest(t *testing.T, s release.Store, sqid string) domains.AccountManifest { |
| + t.Helper() |
| + body, _, err := s.Get(context.Background(), AccountManifestKey(sqid)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + var m domains.AccountManifest |
| + if err := json.Unmarshal(body, &m); err != nil { |
| + t.Fatal(err) |
| + } |
| + return m |
| +} |
diff --git a/internal/console/keymail.go b/internal/console/keymail.go| index 5e1812ce3..d149e4b8c 100644 |
| --- a/internal/console/keymail.go |
| +++ b/internal/console/keymail.go |
| @@ -28,6 +28,7 @@ import ( |
| "io" |
| "net/http" |
| "net/url" |
| + "strconv" |
| "strings" |
| "time" |
| ) |
| @@ -85,27 +86,93 @@ func (k *Keymail) scope() string { |
| // redirectURI is fixed by the console's route table. |
| func (k *Keymail) redirectURI() string { return k.origin + "/auth/callback" } |
| +// stepUpRedirectURI is where a re-authentication lands. A separate route from |
| +// the sign-in callback on purpose: a code minted for a step-up must not be |
| +// spendable as a sign-in, and a sign-in must never be able to satisfy a |
| +// step-up — the two flows never share a landing pad or a state cookie. |
| +func (k *Keymail) stepUpRedirectURI() string { return k.origin + "/auth/stepup/callback" } |
| + |
| // AuthorizeURL is where the browser goes to approve the console. |
| func (k *Keymail) AuthorizeURL(state, challenge string) string { |
| + return k.authorizeURL(state, challenge, k.redirectURI(), 0) |
| +} |
| + |
| +// StepUpURL is the same ceremony with one extra demand: a FRESH authenticator |
| +// gesture, however live the user's Keymail session is. `prompt=login` is the |
| +// ask; `max_age` is the same ask spelled for an IdP that only understands |
| +// seconds, and both are sent because the tighter of the two wins at Keymail. |
| +// The redirect lands on the step-up callback, not the sign-in one — a step-up |
| +// must never be mistakable for a sign-in, in either direction. |
| +func (k *Keymail) StepUpURL(state, challenge string, window time.Duration) string { |
| + return k.authorizeURL(state, challenge, k.stepUpRedirectURI(), window) |
| +} |
| + |
| +func (k *Keymail) authorizeURL(state, challenge, redirect string, window time.Duration) string { |
| q := url.Values{ |
| "client_id": {k.origin}, |
| - "redirect_uri": {k.redirectURI()}, |
| + "redirect_uri": {redirect}, |
| "scope": {k.scope()}, |
| "state": {state}, |
| "code_challenge": {challenge}, |
| "code_challenge_method": {"S256"}, |
| } |
| + if window > 0 { |
| + q.Set("prompt", "login") |
| + q.Set("max_age", strconv.Itoa(int(window.Seconds()))) |
| + } |
| return k.base + "/oauth/authorize?" + q.Encode() |
| } |
| +// SupportsStepUp asks the deployment whether it can re-authenticate anyone at |
| +// all: `/.well-known/keymail` advertises `reauth` where step-up exists (only |
| +// the relay shape has an identity service behind it). An error here is NOT a |
| +// "probably yes" — callers must treat both a false and an error as "cannot", |
| +// because the alternative is walking a human through a ceremony that was never |
| +// going to produce proof and then deciding what to do about it afterwards. |
| +func (k *Keymail) SupportsStepUp(ctx context.Context) (bool, error) { |
| + var out struct { |
| + Reauth bool `json:"reauth"` |
| + } |
| + if _, err := k.do(ctx, http.MethodGet, "/.well-known/keymail", "", nil, &out); err != nil { |
| + return false, err |
| + } |
| + return out.Reauth, nil |
| +} |
| + |
| +// Grant is one completed ceremony: who the user is, the console's access |
| +// token, and — for a step-up — when the authenticator gesture behind it |
| +// happened. |
| +type Grant struct { |
| + Address string |
| + Token string |
| + // AuthTime is zero when Keymail proved no gesture. Zero is the only honest |
| + // representation of "it did not say", and the caller MUST treat it as a |
| + // refusal rather than substituting now: a fabricated freshness is worse |
| + // than no freshness, because it looks like proof. |
| + AuthTime time.Time |
| +} |
| + |
| // Exchange trades the callback's code (plus the PKCE verifier) for the |
| // user's address and the console's access token, server-side. |
| func (k *Keymail) Exchange(ctx context.Context, code, verifier string) (address, token string, err error) { |
| + g, err := k.ExchangeGrant(ctx, code, verifier, false) |
| + return g.Address, g.Token, err |
| +} |
| + |
| +// ExchangeGrant is Exchange with the step-up answer attached. stepUp selects |
| +// which redirect_uri is replayed — Keymail re-checks it against the one the |
| +// authorization was minted for, so a code from the sign-in flow cannot be |
| +// spent here and vice versa. |
| +func (k *Keymail) ExchangeGrant(ctx context.Context, code, verifier string, stepUp bool) (Grant, error) { |
| + redirect := k.redirectURI() |
| + if stepUp { |
| + redirect = k.stepUpRedirectURI() |
| + } |
| req := map[string]string{ |
| "code": code, |
| "code_verifier": verifier, |
| "client_id": k.origin, |
| - "redirect_uri": k.redirectURI(), |
| + "redirect_uri": redirect, |
| } |
| if sp := k.SignPub(); sp != "" { |
| req["sign_pub"] = sp |
| @@ -115,17 +182,22 @@ func (k *Keymail) Exchange(ctx context.Context, code, verifier string) (address, |
| TokenType string `json:"token_type"` |
| Scope string `json:"scope"` |
| Address string `json:"address"` |
| + AuthTime int64 `json:"auth_time"` |
| } |
| if _, err := k.do(ctx, http.MethodPost, "/api/oauth/token", "", req, &out); err != nil { |
| - return "", "", err |
| + return Grant{}, err |
| } |
| if !strings.EqualFold(out.TokenType, "bearer") { |
| - return "", "", fmt.Errorf("keymail: token exchange returned token_type %q, want bearer", out.TokenType) |
| + return Grant{}, fmt.Errorf("keymail: token exchange returned token_type %q, want bearer", out.TokenType) |
| } |
| if out.AccessToken == "" || out.Address == "" { |
| - return "", "", errors.New("keymail: token exchange returned an empty token or address") |
| + return Grant{}, errors.New("keymail: token exchange returned an empty token or address") |
| + } |
| + g := Grant{Address: out.Address, Token: out.AccessToken} |
| + if out.AuthTime > 0 { |
| + g.AuthTime = time.Unix(out.AuthTime, 0) |
| } |
| - return out.Address, out.AccessToken, nil |
| + return g, nil |
| } |
| // Revoke retires an access token (RFC 7009 manners server-side: always 200, |
diff --git a/internal/console/keymail_test.go b/internal/console/keymail_test.go| index 12f1e8152..444d2d4cd 100644 |
| --- a/internal/console/keymail_test.go |
| +++ b/internal/console/keymail_test.go |
| @@ -20,6 +20,7 @@ import ( |
| "strings" |
| "sync" |
| "testing" |
| + "time" |
| ) |
| // ---- a fake keymail, faithful to oauth.go / appapi.go on the wires the |
| @@ -45,6 +46,14 @@ type fakeKeymail struct { |
| revoked []string |
| sends []sentMessage |
| exchanges int |
| + |
| + // Step-up (stepup.go). reauth mirrors /.well-known/keymail's advertised |
| + // capability; authTime is what the next exchange reports (zero = "this |
| + // deployment proved no gesture", the fail-closed case); stepUp records |
| + // whether the last exchange replayed the step-up redirect_uri. |
| + reauth bool |
| + authTime time.Time |
| + stepUp bool |
| } |
| // sentBodies is every message body the fake has accepted — how a test proves |
| @@ -65,8 +74,11 @@ func newFakeKeymail(t *testing.T, address string) *fakeKeymail { |
| if err != nil { |
| t.Fatal(err) |
| } |
| - f := &fakeKeymail{t: t, address: address, boxPriv: boxPriv} |
| + // reauth defaults on: the deployments this console talks to are relays, and |
| + // a test that wants the fail-closed path turns it off explicitly. |
| + f := &fakeKeymail{t: t, address: address, boxPriv: boxPriv, reauth: true} |
| mux := http.NewServeMux() |
| + mux.HandleFunc("GET /.well-known/keymail", f.handleWellKnown) |
| mux.HandleFunc("POST /api/oauth/token", f.handleToken) |
| mux.HandleFunc("POST /api/oauth/revoke", f.handleRevoke) |
| mux.HandleFunc("GET /api/federation/lookup", f.handleLookup) |
| @@ -105,7 +117,12 @@ func (f *fakeKeymail) handleToken(w http.ResponseWriter, r *http.Request) { |
| jsonErr(w, http.StatusBadRequest, "PKCE verification failed") |
| return |
| } |
| - if req.RedirectURI != req.ClientID+"/auth/callback" { |
| + switch req.RedirectURI { |
| + case req.ClientID + "/auth/callback": |
| + f.stepUp = false |
| + case req.ClientID + "/auth/stepup/callback": |
| + f.stepUp = true |
| + default: |
| jsonErr(w, http.StatusBadRequest, "client_id or redirect_uri does not match the authorization") |
| return |
| } |
| @@ -120,12 +137,29 @@ func (f *fakeKeymail) handleToken(w http.ResponseWriter, r *http.Request) { |
| f.origin = req.ClientID |
| f.token = "user.tok-" + f.address |
| f.exchanges++ |
| - w.Header().Set("Content-Type", "application/json") |
| - json.NewEncoder(w).Encode(map[string]string{ |
| + out := map[string]any{ |
| "access_token": f.token, |
| "token_type": "bearer", |
| "scope": "identify send", |
| "address": f.address, |
| + } |
| + // auth_time is OMITTED when no gesture was proved — keymail never invents |
| + // one, and the console must treat its absence as a refusal. |
| + if !f.authTime.IsZero() { |
| + out["auth_time"] = f.authTime.Unix() |
| + } |
| + w.Header().Set("Content-Type", "application/json") |
| + json.NewEncoder(w).Encode(out) |
| +} |
| + |
| +// handleWellKnown is the capability probe the step-up gate makes before it |
| +// sends anyone anywhere. |
| +func (f *fakeKeymail) handleWellKnown(w http.ResponseWriter, r *http.Request) { |
| + f.mu.Lock() |
| + defer f.mu.Unlock() |
| + w.Header().Set("Content-Type", "application/json") |
| + json.NewEncoder(w).Encode(map[string]any{ |
| + "version": "keymail1", "host": "keymail.dev", "pow_bits": 8, "reauth": f.reauth, |
| }) |
| } |
diff --git a/internal/console/materialize.go b/internal/console/materialize.go| index ce021aadc..bf5ca33f1 100644 |
| --- a/internal/console/materialize.go |
| +++ b/internal/console/materialize.go |
| @@ -78,6 +78,14 @@ func MaterializeAccounts(ctx context.Context, s release.Store, st *Store) (int, |
| "not a new account", key, sqid, m.Sqid, m.ID, domains.Sqid(m.ID)) |
| continue |
| } |
| + // A retired account stays retired. Without this, deletion would be a |
| + // lie that lasts until the next console start: this pass would find the |
| + // manifest, see no local row, and re-create the account together with |
| + // the owner membership the manifest names — which after an ownership |
| + // transfer is not even the person who deleted it. |
| + if m.Deleted() { |
| + continue |
| + } |
| created := m.CreatedAt |
| if created.IsZero() { |
| created = time.Now() |
diff --git a/internal/console/ownership.go b/internal/console/ownership.go| new file mode 100644 |
| index 000000000..54a46c8b6 |
| --- /dev/null |
| +++ b/internal/console/ownership.go |
| @@ -0,0 +1,285 @@ |
| +package console |
| + |
| +// The three owner-only account-administration actions the multi-account spec |
| +// names (§1) and the step-up spec gates (§7a): remove a member, transfer |
| +// ownership, delete the account. They live together because they share one |
| +// shape — owner gate, CSRF, passkey step-up, then a store call that cannot |
| +// leave an account with nobody able to administer it. |
| +// |
| +// All three were previously reachable only through `carlos members …` or not at |
| +// all. Putting them behind the console's gates is what makes "membership says |
| +// who you are, not whether you are here now" something a browser can act on. |
| + |
| +import ( |
| + "context" |
| + "crypto/subtle" |
| + "database/sql" |
| + "errors" |
| + "fmt" |
| + "log" |
| + "net/http" |
| + "strings" |
| + "time" |
| + |
| + "github.com/carlosframework/platform/internal/domains" |
| +) |
| + |
| +// handleRemoveMember drops someone's membership. Owner-only, CSRF-checked and |
| +// step-up gated, in that order: a non-owner must meet the byte-identical 404 |
| +// before step-up is ever mentioned, or the redirect itself would confirm the |
| +// account exists. |
| +func (s *Server) handleRemoveMember(w http.ResponseWriter, r *http.Request) { |
| + addr, raw, acct, ok := s.owner(w, r) |
| + if !ok { |
| + return |
| + } |
| + if subtle.ConstantTimeCompare([]byte(r.FormValue("csrf")), []byte(raw)) != 1 { |
| + s.renderMessage(w, r, http.StatusForbidden, "Refused", "Cross-site request blocked.") |
| + return |
| + } |
| + if !s.requireFreshAuth(w, r, addr, raw) { |
| + return |
| + } |
| + target := strings.TrimSpace(r.FormValue("address")) |
| + if err := s.store.RemoveMember(acct.ID, target); err != nil { |
| + log.Printf("console: remove member: %v", err) |
| + s.setFlash(w, "err", removalAdvice(err, target)) |
| + } else { |
| + s.setFlash(w, "ok", target+" is no longer a member of this account.") |
| + } |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/", http.StatusSeeOther) |
| +} |
| + |
| +// handleTransferOwnership hands the account to another member and steps the |
| +// current owner down to member — one transaction, so there is no instant with |
| +// two owners by accident or none by failure. |
| +func (s *Server) handleTransferOwnership(w http.ResponseWriter, r *http.Request) { |
| + addr, raw, acct, ok := s.owner(w, r) |
| + if !ok { |
| + return |
| + } |
| + if subtle.ConstantTimeCompare([]byte(r.FormValue("csrf")), []byte(raw)) != 1 { |
| + s.renderMessage(w, r, http.StatusForbidden, "Refused", "Cross-site request blocked.") |
| + return |
| + } |
| + if !s.requireFreshAuth(w, r, addr, raw) { |
| + return |
| + } |
| + target := strings.TrimSpace(r.FormValue("address")) |
| + if err := s.store.TransferOwnership(acct.ID, addr, target, time.Now()); err != nil { |
| + log.Printf("console: transfer ownership: %v", err) |
| + s.setFlash(w, "err", transferAdvice(err, target)) |
| + } else { |
| + s.setFlash(w, "ok", target+" now owns this account. You are a member of it.") |
| + } |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/", http.StatusSeeOther) |
| +} |
| + |
| +// handleDeleteAccount retires an EMPTY account. Deliberately narrow: an account |
| +// that still holds apps is refused rather than cascading, because the objects, |
| +// registry routes and platform domains an app owns are not this handler's to |
| +// destroy and undoing that is not a thing anyone can do. The useful case — a |
| +// mistyped or abandoned account — is fully served, and the dangerous case is |
| +// left to a deliberate migration. |
| +func (s *Server) handleDeleteAccount(w http.ResponseWriter, r *http.Request) { |
| + addr, raw, acct, ok := s.owner(w, r) |
| + if !ok { |
| + return |
| + } |
| + if subtle.ConstantTimeCompare([]byte(r.FormValue("csrf")), []byte(raw)) != 1 { |
| + s.renderMessage(w, r, http.StatusForbidden, "Refused", "Cross-site request blocked.") |
| + return |
| + } |
| + // Typing the name is not a security control — it is a "did you mean this |
| + // account?" control, and it is checked before step-up so a slip does not |
| + // cost a passkey touch. |
| + if strings.TrimSpace(r.FormValue("confirm")) != acct.Name { |
| + s.setFlash(w, "err", "Type the account's name exactly to delete it.") |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/", http.StatusSeeOther) |
| + return |
| + } |
| + if !s.requireFreshAuth(w, r, addr, raw) { |
| + return |
| + } |
| + // Both refusals that DeleteAccount enforces are re-checked HERE, before the |
| + // bucket is touched. They are cheap, and getting them in the wrong order is |
| + // expensive: tombstoning an account the store then refuses to delete would |
| + // leave a live account marked retired, which the next reconcile pass would |
| + // act on by dropping its registry row. |
| + if acct.ID == OpsAccountID { |
| + s.setFlash(w, "err", deletionAdvice(fmt.Errorf("console: ops"))) |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/", http.StatusSeeOther) |
| + return |
| + } |
| + if n, err := s.store.AppCount(acct.ID); err != nil || n > 0 { |
| + if err != nil { |
| + log.Printf("console: count apps for %s: %v", acct.Sqid, err) |
| + s.setFlash(w, "err", "Could not check whether this account still holds apps. Nothing was deleted.") |
| + } else { |
| + s.setFlash(w, "err", deletionAdvice(fmt.Errorf("console: still holds %d app(s)", n))) |
| + } |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/", http.StatusSeeOther) |
| + return |
| + } |
| + // The bucket first, and the deletion is abandoned if it will not take. |
| + // |
| + // This console's rows are not where account identity lives — every console |
| + // start materializes accounts from accounts/<sqid>/account.json, so a |
| + // deletion that only emptied these tables would be undone by the next |
| + // deploy, silently, re-creating the account AND re-adopting the owner the |
| + // manifest names. That is not the person who deleted it if ownership was |
| + // ever transferred. |
| + // |
| + // Ordering is the safe way round. Tombstone-then-rows can fail after the |
| + // tombstone, leaving an account that still works here and is retired in the |
| + // bucket — visibly wrong, recoverable, and harmless while it lasts, because |
| + // an account with apps never reaches this line. Rows-then-tombstone fails |
| + // the other way: the account is gone from under the operator and comes back |
| + // on its own, which is the bug this replaces. |
| + if err := s.tombstoneAccount(r.Context(), acct); err != nil { |
| + log.Printf("console: tombstone account %s: %v", acct.Sqid, err) |
| + s.setFlash(w, "err", "Could not retire this account in the deployment bucket, so nothing "+ |
| + "was deleted — an account deleted only here would come back on the next restart. "+ |
| + "Try again once the bucket is reachable.") |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/", http.StatusSeeOther) |
| + return |
| + } |
| + if err := s.store.DeleteAccount(acct.ID); err != nil { |
| + log.Printf("console: delete account: %v", err) |
| + s.setFlash(w, "err", deletionAdvice(err)) |
| + http.Redirect(w, r, "/a/"+acct.Sqid+"/", http.StatusSeeOther) |
| + return |
| + } |
| + s.setFlash(w, "ok", "Deleted “"+acct.Name+"”.") |
| + s.redirectToCurrentAccount(w, r, addr, http.StatusSeeOther) |
| +} |
| + |
| +// tombstoneAccount retires the account where identity actually lives. A |
| +// deployment with no release store configured cannot delete an account at all, |
| +// and says so: the alternative is a console-only deletion that undoes itself, |
| +// which is worse than a refusal because it looks like it worked. |
| +func (s *Server) tombstoneAccount(ctx context.Context, acct *Account) error { |
| + if s.cfg.Releases == nil { |
| + return errors.New("console: no deployment release store is configured on this box") |
| + } |
| + return domains.TombstoneAccount(ctx, s.cfg.Releases, acct.Sqid, time.Now()) |
| +} |
| + |
| +// AppCount reports how many apps are homed in an account. Read-only pre-flight |
| +// for deletion; DeleteAccount re-checks it inside its own transaction, which is |
| +// the check that actually binds. |
| +func (s *Store) AppCount(accountID int64) (int, error) { |
| + var n int |
| + err := s.db.QueryRow(`SELECT COUNT(*) FROM apps WHERE account_id = ?`, accountID).Scan(&n) |
| + return n, err |
| +} |
| + |
| +// removalAdvice / transferAdvice / deletionAdvice turn a store error into |
| +// something a human can act on. The store's own messages are written for a log; |
| +// these are written for the person who just pressed the button. |
| +func removalAdvice(err error, target string) string { |
| + if strings.Contains(err.Error(), "last owner") { |
| + return target + " is the last owner of this account — transfer ownership first." |
| + } |
| + return "Could not remove " + target + " from this account." |
| +} |
| + |
| +func transferAdvice(err error, target string) string { |
| + if strings.Contains(err.Error(), "not a member") { |
| + return target + " is not a member of this account. Invite them first, then transfer." |
| + } |
| + if strings.Contains(err.Error(), "already") { |
| + return target + " already owns this account." |
| + } |
| + return "Could not transfer ownership to " + target + "." |
| +} |
| + |
| +func deletionAdvice(err error) string { |
| + if strings.Contains(err.Error(), "still holds") { |
| + return "This account still holds apps. Move or retire them first — deleting an account " + |
| + "never deletes an app." |
| + } |
| + if strings.Contains(err.Error(), "ops") { |
| + return "The ops account is the platform's own and cannot be deleted." |
| + } |
| + return "Could not delete this account." |
| +} |
| + |
| +// ---- store ------------------------------------------------------------------ |
| + |
| +// TransferOwnership promotes `to` and demotes `from`, atomically. Both must |
| +// already be members: ownership is handed to someone who is here, never used as |
| +// a back door for adding a person. The order inside the transaction matters — |
| +// promote first, so lastOwnerGuard sees two owners when the demotion runs and |
| +// there is no window in which the account has none. |
| +func (s *Store) TransferOwnership(accountID int64, from, to string, now time.Time) error { |
| + if strings.EqualFold(from, to) { |
| + return fmt.Errorf("console: %s already owns this account", to) |
| + } |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return err |
| + } |
| + defer tx.Rollback() |
| + var role string |
| + err = tx.QueryRow(`SELECT role FROM memberships WHERE account_id = ? AND address = ?`, |
| + accountID, to).Scan(&role) |
| + if err == sql.ErrNoRows { |
| + return fmt.Errorf("console: %s is not a member of this account", to) |
| + } |
| + if err != nil { |
| + return err |
| + } |
| + if _, err := tx.Exec(`UPDATE memberships SET role = ? WHERE account_id = ? AND address = ?`, |
| + RoleOwner, accountID, to); err != nil { |
| + return err |
| + } |
| + if err := lastOwnerGuard(tx, accountID, from); err != nil { |
| + return err |
| + } |
| + if _, err := tx.Exec(`UPDATE memberships SET role = ? WHERE account_id = ? AND address = ?`, |
| + RoleMember, accountID, from); err != nil { |
| + return err |
| + } |
| + return tx.Commit() |
| +} |
| + |
| +// DeleteAccount retires an account that owns nothing. It refuses the ops |
| +// account (id 1 is the platform's own) and refuses while any app is still |
| +// homed here — an account row is cheap to delete and an app's objects, routes |
| +// and domains are not, so the two are never destroyed by one click. |
| +// |
| +// Audit rows are deliberately left behind: they are the record of what was |
| +// done, and a deletion is not a reason to lose it. |
| +func (s *Store) DeleteAccount(accountID int64) error { |
| + if accountID == OpsAccountID { |
| + return fmt.Errorf("console: the ops account cannot be deleted") |
| + } |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return err |
| + } |
| + defer tx.Rollback() |
| + var apps int |
| + if err := tx.QueryRow(`SELECT COUNT(*) FROM apps WHERE account_id = ?`, accountID).Scan(&apps); err != nil { |
| + return err |
| + } |
| + if apps > 0 { |
| + return fmt.Errorf("console: this account still holds %d app(s)", apps) |
| + } |
| + for _, stmt := range []string{ |
| + `DELETE FROM memberships WHERE account_id = ?`, |
| + `DELETE FROM invites WHERE account_id = ?`, |
| + `DELETE FROM accounts WHERE id = ?`, |
| + } { |
| + if _, err := tx.Exec(stmt, accountID); err != nil { |
| + return err |
| + } |
| + } |
| + // Anyone whose landing spot was this account gets sent back to the picker |
| + // rather than to a 404 they cannot explain. |
| + if _, err := tx.Exec(`UPDATE users SET last_account = 0 WHERE last_account = ?`, accountID); err != nil { |
| + return err |
| + } |
| + return tx.Commit() |
| +} |
diff --git a/internal/console/server.go b/internal/console/server.go| index 4e49d6b1a..7c26c3422 100644 |
| --- a/internal/console/server.go |
| +++ b/internal/console/server.go |
| @@ -145,6 +145,9 @@ func NewServer(cfg Config, st *Store) (*Server, error) { |
| s.mux.HandleFunc("POST /auth/continue", s.handleContinue) |
| s.mux.HandleFunc("GET /auth/signin", s.handleSignin) |
| s.mux.HandleFunc("GET /auth/callback", s.handleCallback) |
| + // Passkey step-up (stepup.go): where a re-authentication lands, on its own |
| + // route so a step-up code and a sign-in code are never interchangeable. |
| + s.mux.HandleFunc("GET /auth/stepup/callback", s.handleStepUpCallback) |
| s.mux.HandleFunc("GET /auth/confirm", s.handleConfirm) |
| s.mux.HandleFunc("POST /auth/signout", s.handleSignout) |
| s.mux.HandleFunc("GET /fleet", s.handleFleet) |
| @@ -162,6 +165,11 @@ func NewServer(cfg Config, st *Store) (*Server, error) { |
| s.mux.HandleFunc("POST /a/{sqid}/apps/{app}/canary/retire", s.handleRetireCanary) |
| s.mux.HandleFunc("POST /a/{sqid}/invite", s.handleInvite) |
| s.mux.HandleFunc("POST /a/{sqid}/invite/revoke", s.handleRevokeInvite) |
| + // Owner-only account administration, every one of them behind passkey |
| + // step-up as well as ownership and CSRF (ownership.go, spec §7a). |
| + s.mux.HandleFunc("POST /a/{sqid}/members/remove", s.handleRemoveMember) |
| + s.mux.HandleFunc("POST /a/{sqid}/members/transfer", s.handleTransferOwnership) |
| + s.mux.HandleFunc("POST /a/{sqid}/delete", s.handleDeleteAccount) |
| s.mux.HandleFunc("GET /invite/{token}", s.handleAcceptInvite) |
| s.mux.HandleFunc("GET /accounts/new", s.handleNewAccountForm) |
| s.mux.HandleFunc("POST /accounts/new", s.handleNewAccount) |
| @@ -859,6 +867,11 @@ func (s *Server) handleSignout(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| s.store.DeleteSession(raw) |
| + // Presence dies with the session it was proved for (stepup.go). Belt and |
| + // braces — the step-up row is keyed by the session hash, which no future |
| + // session can reproduce — but leaving proof-of-presence lying around after |
| + // a deliberate sign-out is not a thing to be relaxed about. |
| + s.store.ClearStepUp(raw) |
| s.clearCookie(w, sessionCookie) |
| http.Redirect(w, r, "/", http.StatusSeeOther) |
| } |
diff --git a/internal/console/stepup.go b/internal/console/stepup.go| new file mode 100644 |
| index 000000000..b8869d37e |
| --- /dev/null |
| +++ b/internal/console/stepup.go |
| @@ -0,0 +1,434 @@ |
| +package console |
| + |
| +// Passkey step-up for sensitive actions (spec §7a). |
| +// |
| +// Membership answers *who you are*. It does not answer *are you here now* — and |
| +// for the handful of actions where that is the whole question (approving a |
| +// `carlos auth login` device grant, removing a member, transferring ownership, |
| +// deleting an account, revoking CLI tokens) a session cookie lifted from a |
| +// laptop is currently indistinguishable from the person it belongs to. |
| +// |
| +// The route is a FRESH Keymail authentication, not console-native WebAuthn: |
| +// identity belongs in one place in this platform, and a console holding its own |
| +// credentials would be a second identity system with a second attack surface. |
| +// Keymail already asserts with `UserVerification: VerificationRequired`; what it |
| +// gained for this is `prompt=login` and an `auth_time` claim (keymail |
| +// docs/apps.md), so the console can DEMAND a gesture and then VERIFY one |
| +// happened rather than trusting the redirect it got back. |
| +// |
| +// Three properties this file exists to hold, in order of how easily they are |
| +// lost: |
| +// |
| +// 1. UNSKIPPABLE SERVER-SIDE. The gate is not a UI affordance. requireFreshAuth |
| +// reads database state written only by a verified callback; a direct POST |
| +// with a valid session and a valid CSRF token is refused all the same. |
| +// |
| +// 2. THE PENDING ACTION IS NOT A CAPABILITY. The action is preserved across the |
| +// round trip so a human is not dumped back at square one, but it is stored |
| +// SERVER-SIDE, bound to the session that stashed it, single-use (burned by |
| +// row deletion) and short-lived. The browser carries an opaque token in an |
| +// HttpOnly cookie and nothing else — it cannot swap the action, replay it, |
| +// or hand it to another session. |
| +// |
| +// 3. NO ORACLE. Step-up runs AFTER the membership/ownership gate, never before. |
| +// A non-member probing an owner-only action must still get the |
| +// byte-identical http.NotFound, not a redirect to Keymail that would confirm |
| +// the account exists. |
| +// |
| +// The CSRF check stays exactly where it was. Step-up is in addition to it: CSRF |
| +// answers "did this browser mean to send this?", step-up answers "is the human |
| +// still here?", and neither substitutes for the other. |
| + |
| +import ( |
| + "context" |
| + "crypto/subtle" |
| + "database/sql" |
| + "log" |
| + "net/http" |
| + "net/url" |
| + "strings" |
| + "time" |
| +) |
| + |
| +const ( |
| + // StepUpWindow is how recent the authenticator gesture must be for a |
| + // sensitive action to proceed. |
| + // |
| + // Five minutes. Shorter is not obviously safer here and is definitely |
| + // worse: the gated flows are multi-step (approving a device grant means |
| + // typing a code, reading the terminal's description, then deciding), and a |
| + // window that expires mid-decision trains people to click through |
| + // authenticator prompts without reading them. Longer starts to defeat the |
| + // point — the threat is a session stolen and used soon after, and every |
| + // extra minute is a minute of that theft that lands inside the window. |
| + // Five minutes also covers the round trip itself (redirect, consent card, |
| + // a slow authenticator) with room to spare, so the common case is one |
| + // touch, not two. |
| + // |
| + // It is measured from the GESTURE (Keymail's auth_time), not from when the |
| + // console heard about it — the freshness that matters is the human's, and |
| + // timing the console's own bookkeeping would let a slow network buy time. |
| + StepUpWindow = 5 * time.Minute |
| + |
| + // stepUpFlowTTL bounds the round trip: mint state, go to Keymail, come |
| + // back. Ten minutes is the same order as the sign-in flow's own cookie. |
| + stepUpFlowTTL = 10 * time.Minute |
| + |
| + // stepUpSkew tolerates a small clock difference between this console and |
| + // the Keymail deployment. Deliberately tiny — this is skew tolerance, not |
| + // a second freshness window. |
| + stepUpSkew = 60 * time.Second |
| + |
| + // stepUpCookie carries state + PKCE verifier + the pending-action token |
| + // across the round trip. HttpOnly, like every other cookie here, and |
| + // never a query parameter: a URL ends up in history, referrers and logs, |
| + // and the pending-action token must not. |
| + stepUpCookie = "carlos_console_stepup" |
| +) |
| + |
| +// stepUpUnavailable is what a human sees when the identity provider cannot |
| +// re-authenticate them, or cannot be reached to try. Failing CLOSED is the |
| +// whole design: the alternative — quietly proceeding on session-only |
| +// authority — is precisely the hole this feature exists to close, and it would |
| +// close it only on the days Keymail happens to be up. |
| +const stepUpUnavailable = "This action needs you to confirm it's really you with your passkey, " + |
| + "and Keymail can't be asked right now. Nothing has been changed. Try again in a moment — " + |
| + "if this keeps happening, this deployment's Keymail does not support re-authentication " + |
| + "and these actions stay blocked until it does." |
| + |
| +// ---- storage ---------------------------------------------------------------- |
| + |
| +// migrateStepUp adds this feature's two tables. Additive and idempotent, and |
| +// kept here rather than in store.go's list so the feature is one file to read |
| +// and one file to remove (the authstore.go precedent). |
| +func (s *Store) migrateStepUp() error { |
| + for _, stmt := range []string{ |
| + // One row per session that has stepped up. Keyed by the session hash, |
| + // so it dies with the session and cannot be moved to another one. |
| + // address is stored alongside for a belt-and-braces check: a session |
| + // row could in principle be re-pointed, and a step-up recorded for one |
| + // identity must never satisfy another. |
| + `CREATE TABLE IF NOT EXISTS step_ups ( |
| + session_hash TEXT PRIMARY KEY, |
| + address TEXT NOT NULL, |
| + auth_time INTEGER NOT NULL, |
| + recorded_at INTEGER NOT NULL |
| +);`, |
| + // The action a human was part-way through when the gate stopped them. |
| + // Stored server-side precisely so it cannot be edited in flight: the |
| + // browser holds an opaque token, never the method, path or fields. |
| + `CREATE TABLE IF NOT EXISTS pending_actions ( |
| + token_hash TEXT PRIMARY KEY, |
| + session_hash TEXT NOT NULL, |
| + address TEXT NOT NULL, |
| + method TEXT NOT NULL, |
| + path TEXT NOT NULL, |
| + form TEXT NOT NULL, |
| + created_at INTEGER NOT NULL, |
| + expires_at INTEGER NOT NULL |
| +);`, |
| + } { |
| + if _, err := s.db.Exec(stmt); err != nil { |
| + return err |
| + } |
| + } |
| + return nil |
| +} |
| + |
| +// RecordStepUp remembers that this session's owner proved presence at |
| +// authTime. Upsert: a newer gesture replaces an older one. |
| +func (s *Store) RecordStepUp(rawSession, address string, authTime, now time.Time) error { |
| + _, err := s.db.Exec(`INSERT INTO step_ups (session_hash, address, auth_time, recorded_at) |
| +VALUES (?, ?, ?, ?) |
| +ON CONFLICT(session_hash) DO UPDATE SET |
| + address = excluded.address, auth_time = excluded.auth_time, recorded_at = excluded.recorded_at`, |
| + hashToken(rawSession), address, authTime.Unix(), now.Unix()) |
| + return err |
| +} |
| + |
| +// FreshStepUp is the gate's one question. Both the session AND the address must |
| +// match, and the gesture must be inside the window; anything else — no row, a |
| +// stale row, a row belonging to another identity — is false. |
| +func (s *Store) FreshStepUp(rawSession, address string, now time.Time, window time.Duration) bool { |
| + var storedAddr string |
| + var authTime int64 |
| + err := s.db.QueryRow(`SELECT address, auth_time FROM step_ups WHERE session_hash = ?`, |
| + hashToken(rawSession)).Scan(&storedAddr, &authTime) |
| + if err != nil || storedAddr != address { |
| + return false |
| + } |
| + age := now.Sub(time.Unix(authTime, 0)) |
| + return age >= -stepUpSkew && age <= window |
| +} |
| + |
| +// ClearStepUp drops a session's proof of presence. Called on sign-out: the |
| +// session is gone, and so is any authority it had accrued. |
| +func (s *Store) ClearStepUp(rawSession string) error { |
| + _, err := s.db.Exec(`DELETE FROM step_ups WHERE session_hash = ?`, hashToken(rawSession)) |
| + return err |
| +} |
| + |
| +// pendingAction is a POST held mid-flight. |
| +type pendingAction struct { |
| + Method string |
| + Path string |
| + Form string |
| +} |
| + |
| +// StashPendingAction parks the request the gate interrupted and returns the |
| +// opaque token that redeems it. Housekeeping on the write path: expired stashes |
| +// have nothing left to resume. |
| +func (s *Store) StashPendingAction(rawSession, address, method, path, form string, now time.Time) (string, error) { |
| + s.db.Exec(`DELETE FROM pending_actions WHERE expires_at < ?`, now.Unix()) |
| + raw, err := NewVerifier() |
| + if err != nil { |
| + return "", err |
| + } |
| + _, err = s.db.Exec(`INSERT INTO pending_actions |
| +(token_hash, session_hash, address, method, path, form, created_at, expires_at) |
| +VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, |
| + hashToken(raw), hashToken(rawSession), address, method, path, form, |
| + now.Unix(), now.Add(stepUpFlowTTL).Unix()) |
| + if err != nil { |
| + return "", err |
| + } |
| + return raw, nil |
| +} |
| + |
| +// TakePendingAction redeems a stash exactly once, and only for the session that |
| +// created it. Burn-then-check, like the OAuth code and the confirmation link: the |
| +// row is deleted before any verdict is returned, so a wrong-session or expired |
| +// attempt cannot be retried into a right one. |
| +func (s *Store) TakePendingAction(rawToken, rawSession string, now time.Time) (*pendingAction, bool) { |
| + if rawToken == "" { |
| + return nil, false |
| + } |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return nil, false |
| + } |
| + defer tx.Rollback() |
| + var sessionHash string |
| + var expires int64 |
| + var p pendingAction |
| + err = tx.QueryRow(`SELECT session_hash, method, path, form, expires_at |
| +FROM pending_actions WHERE token_hash = ?`, hashToken(rawToken)). |
| + Scan(&sessionHash, &p.Method, &p.Path, &p.Form, &expires) |
| + if err == sql.ErrNoRows { |
| + return nil, false |
| + } |
| + if err != nil { |
| + return nil, false |
| + } |
| + if _, err := tx.Exec(`DELETE FROM pending_actions WHERE token_hash = ?`, hashToken(rawToken)); err != nil { |
| + return nil, false |
| + } |
| + if err := tx.Commit(); err != nil { |
| + return nil, false |
| + } |
| + if subtle.ConstantTimeCompare([]byte(sessionHash), []byte(hashToken(rawSession))) != 1 { |
| + return nil, false |
| + } |
| + if now.Unix() > expires { |
| + return nil, false |
| + } |
| + return &p, true |
| +} |
| + |
| +// ---- the gate --------------------------------------------------------------- |
| + |
| +// requireFreshAuth is the step-up gate. It returns true only when this session |
| +// has proved presence inside StepUpWindow; otherwise it has ALREADY written the |
| +// response (a redirect into Keymail's re-authentication, or a fail-closed |
| +// message) and the caller must simply return. |
| +// |
| +// Call it LAST — after the membership/ownership gate and after the CSRF check. |
| +// Order is load-bearing twice over: a non-member must get the byte-identical |
| +// 404 rather than a redirect that confirms the account exists, and a |
| +// cross-site POST must be refused before it can park a pending action. |
| +func (s *Server) requireFreshAuth(w http.ResponseWriter, r *http.Request, addr, raw string) bool { |
| + now := time.Now() |
| + if s.store.FreshStepUp(raw, addr, now, StepUpWindow) { |
| + return true |
| + } |
| + // Ask the identity provider whether it can do this at all before sending a |
| + // human off to try. Unreachable and "no" are the same answer here — see |
| + // stepUpUnavailable. |
| + ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second) |
| + defer cancel() |
| + switch supported, err := s.km.SupportsStepUp(ctx); { |
| + case err != nil: |
| + log.Printf("console: step-up capability check: %v", err) |
| + s.renderMessage(w, r, http.StatusServiceUnavailable, "Can't confirm it's you", stepUpUnavailable) |
| + return false |
| + case !supported: |
| + log.Printf("console: step-up refused: %s does not advertise re-authentication", s.cfg.Keymail) |
| + s.renderMessage(w, r, http.StatusServiceUnavailable, "Can't confirm it's you", stepUpUnavailable) |
| + return false |
| + } |
| + state, err := NewVerifier() |
| + if err != nil { |
| + s.renderMessage(w, r, http.StatusInternalServerError, "Error", "Could not start the confirmation. Please try again.") |
| + return false |
| + } |
| + verifier, err := NewVerifier() |
| + if err != nil { |
| + s.renderMessage(w, r, http.StatusInternalServerError, "Error", "Could not start the confirmation. Please try again.") |
| + return false |
| + } |
| + // The handler has already read fields off this request, so PostForm is |
| + // parsed and cached; Encode gives back exactly what it will be re-fed. |
| + if err := r.ParseForm(); err != nil { |
| + s.renderMessage(w, r, http.StatusBadRequest, "Refused", "That request could not be read.") |
| + return false |
| + } |
| + // The csrf field is deliberately NOT stored. It is the raw session token, |
| + // and sessions are kept hashed here precisely so a database read is not a |
| + // session; parking a live one in a form blob would quietly undo that. The |
| + // resume re-injects it from the cookie the resuming request carries — which |
| + // costs nothing in safety, because the cross-site question was already |
| + // answered above, before this stash was ever created. |
| + form := url.Values{} |
| + for k, v := range r.PostForm { |
| + if k != "csrf" { |
| + form[k] = v |
| + } |
| + } |
| + pending, err := s.store.StashPendingAction(raw, addr, r.Method, r.URL.Path, form.Encode(), now) |
| + if err != nil { |
| + log.Printf("console: stash pending action: %v", err) |
| + s.renderMessage(w, r, http.StatusInternalServerError, "Error", "Could not hold on to that action. Please try again.") |
| + return false |
| + } |
| + s.setCookie(w, stepUpCookie, state+"."+verifier+"."+pending, int(stepUpFlowTTL.Seconds())) |
| + http.Redirect(w, r, s.km.StepUpURL(state, ChallengeS256(verifier), StepUpWindow), http.StatusSeeOther) |
| + return false |
| +} |
| + |
| +// handleStepUpCallback is where a re-authentication lands. It trusts nothing |
| +// about the redirect itself: the code is exchanged server-side, and the grant |
| +// that comes back must name the SAME address as the session and carry an |
| +// auth_time inside the window. Only then is presence recorded and the held |
| +// action resumed. |
| +func (s *Server) handleStepUpCallback(w http.ResponseWriter, r *http.Request) { |
| + addr, raw, ok := s.session(r) |
| + if !ok { |
| + http.Redirect(w, r, "/", http.StatusFound) |
| + return |
| + } |
| + c, err := r.Cookie(stepUpCookie) |
| + if err != nil || c.Value == "" { |
| + s.renderMessage(w, r, http.StatusBadRequest, "Confirmation expired", |
| + "That confirmation is stale or was started in another browser. Nothing has been changed — please try the action again.") |
| + return |
| + } |
| + s.clearCookie(w, stepUpCookie) |
| + parts := strings.SplitN(c.Value, ".", 3) |
| + if len(parts) != 3 || parts[0] == "" || |
| + subtle.ConstantTimeCompare([]byte(parts[0]), []byte(r.URL.Query().Get("state"))) != 1 { |
| + s.renderMessage(w, r, http.StatusBadRequest, "Confirmation mismatch", |
| + "That response didn't match this browser's confirmation attempt. Nothing has been changed — please try the action again.") |
| + return |
| + } |
| + verifier, pendingToken := parts[1], parts[2] |
| + if e := r.URL.Query().Get("error"); e != "" { |
| + // Cancelled at Keymail, or refused there. Not an error state — but |
| + // emphatically not a pass either. |
| + s.store.TakePendingAction(pendingToken, raw, time.Now()) // burn the stash |
| + s.renderMessage(w, r, http.StatusOK, "Not confirmed", |
| + "You didn't confirm it was you, so nothing has been changed.") |
| + return |
| + } |
| + code := r.URL.Query().Get("code") |
| + if code == "" { |
| + s.renderMessage(w, r, http.StatusBadRequest, "Not confirmed", |
| + "Keymail sent no authorization code. Nothing has been changed — please try the action again.") |
| + return |
| + } |
| + ctx, cancel := context.WithTimeout(r.Context(), 15*time.Second) |
| + defer cancel() |
| + grant, err := s.km.ExchangeGrant(ctx, code, verifier, true) |
| + if err != nil { |
| + log.Printf("console: step-up exchange: %v", err) |
| + s.renderMessage(w, r, http.StatusBadGateway, "Can't confirm it's you", stepUpUnavailable) |
| + return |
| + } |
| + now := time.Now() |
| + // The identity check. Keymail's identity service is deliberately anonymous — |
| + // a fresh assertion there proves SOME registered passkey was verified, not |
| + // whose. This comparison is what turns it into proof about this session's |
| + // owner: step up with somebody else's passkey and the address won't match. |
| + if !strings.EqualFold(grant.Address, addr) { |
| + log.Printf("console: step-up address mismatch for session of %s", addr) |
| + s.renderMessage(w, r, http.StatusForbidden, "Not confirmed", |
| + "That passkey belongs to a different Keymail address than the one you're signed in as. Nothing has been changed.") |
| + return |
| + } |
| + // A re-authorization rotates the console's grant at Keymail, so the stored |
| + // token would otherwise be dead by the next invite or confirmation send. |
| + if err := s.store.SetGrantToken(grant.Address, grant.Token, now); err != nil { |
| + log.Printf("console: step-up grant refresh: %v", err) |
| + } |
| + // Fail closed on a missing or stale auth_time. An IdP that did not report |
| + // when the gesture happened has not proved that one did. |
| + age := now.Sub(grant.AuthTime) |
| + if grant.AuthTime.IsZero() || age > StepUpWindow || age < -stepUpSkew { |
| + log.Printf("console: step-up produced no usable auth_time (zero=%v age=%s)", grant.AuthTime.IsZero(), age) |
| + s.renderMessage(w, r, http.StatusForbidden, "Can't confirm it's you", stepUpUnavailable) |
| + return |
| + } |
| + if err := s.store.RecordStepUp(raw, addr, grant.AuthTime, now); err != nil { |
| + log.Printf("console: record step-up: %v", err) |
| + s.renderMessage(w, r, http.StatusInternalServerError, "Error", "Could not record your confirmation. Please try the action again.") |
| + return |
| + } |
| + pending, ok := s.store.TakePendingAction(pendingToken, raw, now) |
| + if !ok { |
| + // Confirmed, but the held action is gone (expired, already resumed, or |
| + // belonged to another session). Say so plainly rather than guessing. |
| + s.renderMessage(w, r, http.StatusOK, "Confirmed", |
| + "Thanks — that's you. The action you were doing has expired, so nothing was changed. Please try it again.") |
| + return |
| + } |
| + s.resume(w, r, pending) |
| +} |
| + |
| +// resume re-runs the held POST through the console's own router, so it meets |
| +// every gate it would have met the first time — session, CSRF, membership, |
| +// ownership, and the step-up gate that now passes. Re-dispatching rather than |
| +// calling a handler directly is the point: there is no bypass path, because |
| +// there is no second door. |
| +func (s *Server) resume(w http.ResponseWriter, r *http.Request, p *pendingAction) { |
| + _, raw, ok := s.session(r) |
| + if !ok { |
| + http.Redirect(w, r, "/", http.StatusFound) |
| + return |
| + } |
| + form, err := url.ParseQuery(p.Form) |
| + if err != nil { |
| + log.Printf("console: resume %s %s: %v", p.Method, p.Path, err) |
| + s.renderMessage(w, r, http.StatusInternalServerError, "Error", "Could not resume that action. Please try it again.") |
| + return |
| + } |
| + // Put back the one field the stash refuses to hold (see requireFreshAuth): |
| + // the resumed request must satisfy the same CSRF check as any other, and |
| + // the token it satisfies it with is this browser's own, read from its |
| + // cookie, not something recovered from storage. |
| + form.Set("csrf", raw) |
| + req, err := http.NewRequestWithContext(r.Context(), p.Method, p.Path, strings.NewReader(form.Encode())) |
| + if err != nil { |
| + log.Printf("console: resume %s %s: %v", p.Method, p.Path, err) |
| + s.renderMessage(w, r, http.StatusInternalServerError, "Error", "Could not resume that action. Please try it again.") |
| + return |
| + } |
| + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| + // The session cookie rides along — the resumed request is the same browser, |
| + // and every gate it passes it passes on its own credentials, not on ours. |
| + req.Header["Cookie"] = r.Header["Cookie"] |
| + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { |
| + req.Header.Set("X-Forwarded-For", xff) |
| + } |
| + req.Host = r.Host |
| + req.RemoteAddr = r.RemoteAddr |
| + s.mux.ServeHTTP(w, req) |
| +} |
diff --git a/internal/console/stepup_test.go b/internal/console/stepup_test.go| new file mode 100644 |
| index 000000000..aa7ace9c9 |
| --- /dev/null |
| +++ b/internal/console/stepup_test.go |
| @@ -0,0 +1,401 @@ |
| +package console |
| + |
| +// Passkey step-up (stepup.go, spec §7a). The properties under test are the |
| +// ones that are easy to lose and expensive to lose: the gate cannot be skipped |
| +// server-side, the held action cannot be swapped or replayed, the whole thing |
| +// fails CLOSED when Keymail can't or won't re-authenticate, and it never |
| +// becomes an oracle telling a stranger which accounts exist. |
| + |
| +import ( |
| + "net/http" |
| + "net/url" |
| + "strconv" |
| + "strings" |
| + "testing" |
| + "time" |
| +) |
| + |
| +// completeStepUp plays the browser's half of a re-authentication: it takes the |
| +// redirect the gate produced, checks the console really asked Keymail for a |
| +// fresh gesture, and lands the callback. What comes back is the RESUMED |
| +// action's own response — the point of the whole round trip. |
| +func completeStepUp(t *testing.T, fx *consoleFixture, c *http.Client, loc string) (status int, location, body string) { |
| + t.Helper() |
| + u, err := url.Parse(loc) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + km, _ := url.Parse(fx.fk.srv.URL) |
| + if u.Host != km.Host || u.Path != "/oauth/authorize" { |
| + t.Fatalf("step-up redirect went to %v, want %s/oauth/authorize", u, km.Host) |
| + } |
| + q := u.Query() |
| + if q.Get("prompt") != "login" { |
| + t.Fatalf("step-up authorize did not ask for a fresh gesture: %v", q) |
| + } |
| + if q.Get("max_age") != strconv.Itoa(int(StepUpWindow.Seconds())) { |
| + t.Fatalf("step-up max_age = %q, want %d", q.Get("max_age"), int(StepUpWindow.Seconds())) |
| + } |
| + if q.Get("redirect_uri") != fx.ts.URL+"/auth/stepup/callback" { |
| + t.Fatalf("step-up redirect_uri = %q, want the step-up callback", q.Get("redirect_uri")) |
| + } |
| + code := "stepup-" + q.Get("state")[:8] |
| + fx.fk.mu.Lock() |
| + fx.fk.challenge = q.Get("code_challenge") |
| + fx.fk.expectCode = code |
| + if fx.fk.authTime.IsZero() { |
| + fx.fk.authTime = time.Now() |
| + } |
| + fx.fk.mu.Unlock() |
| + return get(t, c, fx.ts.URL+"/auth/stepup/callback?code="+url.QueryEscape(code)+ |
| + "&state="+url.QueryEscape(q.Get("state"))) |
| +} |
| + |
| +// postGated is postForm for an action behind the step-up gate: it transparently |
| +// walks the passkey round trip when one is demanded, and is otherwise postForm. |
| +func postGated(t *testing.T, fx *consoleFixture, c *http.Client, u string, form url.Values) (int, string, string) { |
| + t.Helper() |
| + status, loc, body := postForm(t, c, u, form) |
| + if status == http.StatusSeeOther && strings.HasPrefix(loc, fx.fk.srv.URL) { |
| + return completeStepUp(t, fx, c, loc) |
| + } |
| + return status, loc, body |
| +} |
| + |
| +// memberFixture signs in an owner and adds a second member, which is the |
| +// smallest world in which removal, transfer and deletion all mean something. |
| +func memberFixture(t *testing.T) (*consoleFixture, *http.Client, string) { |
| + t.Helper() |
| + fx := newConsoleFixture(t) |
| + c := newBrowser(t) |
| + signUp(t, fx, c, "code-stepup") |
| + if err := fx.st.AddMember(OpsAccountID, "kim@keymail.dev", RoleMember, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + return fx, c, sessionToken(t, fx, c) |
| +} |
| + |
| +// TestStepUpIsUnskippable is the whole feature in one test: a signed-in owner |
| +// with a perfectly good CSRF token cannot remove a member by POSTing. The gate |
| +// is server-side state, not a button that renders differently. |
| +func TestStepUpIsUnskippable(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) |
| + |
| + status, loc, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusSeeOther || !strings.HasPrefix(loc, fx.fk.srv.URL) { |
| + t.Fatalf("un-stepped-up removal = %d -> %q, want a redirect into Keymail", status, loc) |
| + } |
| + // Nothing happened yet — the redirect is a question, not a deferral. |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != RoleMember { |
| + t.Fatalf("membership changed before the passkey confirmation (role %q)", role) |
| + } |
| + // The held action is parked, and it did NOT park a live session token with |
| + // it: sessions are stored hashed here, and a form blob is no exemption. |
| + var stashed, leaked int |
| + fx.st.db.QueryRow(`SELECT COUNT(*) FROM pending_actions`).Scan(&stashed) |
| + fx.st.db.QueryRow(`SELECT COUNT(*) FROM pending_actions WHERE form LIKE ?`, "%"+csrf+"%").Scan(&leaked) |
| + if stashed != 1 { |
| + t.Fatalf("%d pending actions parked, want exactly 1", stashed) |
| + } |
| + if leaked != 0 { |
| + t.Fatal("a pending action stored the raw session token") |
| + } |
| + |
| + // Complete the gesture: the held action resumes and lands. |
| + status, loc, body := completeStepUp(t, fx, c, loc) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("resumed removal = %d -> %q:\n%s", status, loc, body) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != "" { |
| + t.Fatalf("kim is still a %q after a confirmed removal", role) |
| + } |
| + |
| + // And presence persists for the window, so a second action in the same |
| + // few minutes does not demand a second touch. |
| + if err := fx.st.AddMember(OpsAccountID, "kim@keymail.dev", RoleMember, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if status, _, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}); status != http.StatusSeeOther { |
| + t.Fatalf("second removal inside the window = %d, want 303", status) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != "" { |
| + t.Fatalf("second removal did not take (role %q)", role) |
| + } |
| +} |
| + |
| +// TestStepUpFailsClosed covers every way the identity provider can decline to |
| +// prove presence. Not one of them may end with the action going through. |
| +func TestStepUpFailsClosed(t *testing.T) { |
| + t.Run("keymail cannot re-authenticate", func(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) |
| + fx.fk.mu.Lock() |
| + fx.fk.reauth = false // a deployment whose IdP lacks prompt=login |
| + fx.fk.mu.Unlock() |
| + status, _, body := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusServiceUnavailable { |
| + t.Fatalf("removal against a step-up-less Keymail = %d, want 503", status) |
| + } |
| + if !strings.Contains(body, "passkey") { |
| + t.Fatalf("the refusal does not say why:\n%s", body) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != RoleMember { |
| + t.Fatal("membership changed despite a fail-closed refusal") |
| + } |
| + }) |
| + |
| + t.Run("keymail unreachable", func(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) |
| + fx.fk.srv.Close() // the IdP is simply not there |
| + status, _, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusServiceUnavailable { |
| + t.Fatalf("removal with Keymail down = %d, want 503", status) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != RoleMember { |
| + t.Fatal("membership changed despite Keymail being unreachable") |
| + } |
| + }) |
| + |
| + t.Run("no auth_time in the grant", func(t *testing.T) { |
| + // The subtle one: Keymail says it supports step-up, the ceremony |
| + // completes, and the token response carries no auth_time. That is not |
| + // proof of anything and must not be treated as any. |
| + fx, c, csrf := memberFixture(t) |
| + status, loc, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("gate = %d, want 303", status) |
| + } |
| + u, _ := url.Parse(loc) |
| + q := u.Query() |
| + fx.fk.mu.Lock() |
| + fx.fk.challenge, fx.fk.expectCode = q.Get("code_challenge"), "nostamp" |
| + fx.fk.authTime = time.Time{} // reports nothing |
| + fx.fk.mu.Unlock() |
| + status, _, _ = get(t, c, fx.ts.URL+"/auth/stepup/callback?code=nostamp&state="+url.QueryEscape(q.Get("state"))) |
| + if status != http.StatusForbidden { |
| + t.Fatalf("callback with no auth_time = %d, want 403", status) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != RoleMember { |
| + t.Fatal("membership changed on an auth_time-less grant") |
| + } |
| + }) |
| + |
| + t.Run("stale auth_time", func(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) |
| + fx.fk.mu.Lock() |
| + fx.fk.authTime = time.Now().Add(-2 * StepUpWindow) // yesterday's gesture |
| + fx.fk.mu.Unlock() |
| + status, loc, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("gate = %d, want 303", status) |
| + } |
| + status, _, _ = completeStepUp(t, fx, c, loc) |
| + if status != http.StatusForbidden { |
| + t.Fatalf("callback with a stale auth_time = %d, want 403", status) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != RoleMember { |
| + t.Fatal("membership changed on a stale gesture") |
| + } |
| + }) |
| + |
| + t.Run("somebody else's passkey", func(t *testing.T) { |
| + // Keymail's identity service is anonymous: a fresh assertion proves |
| + // SOME passkey was verified. The address comparison is what makes it |
| + // proof about THIS session's owner. |
| + fx, c, csrf := memberFixture(t) |
| + status, loc, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("gate = %d, want 303", status) |
| + } |
| + fx.setUser("attacker@keymail.dev") // their own passkey, their own address |
| + status, _, body := completeStepUp(t, fx, c, loc) |
| + if status != http.StatusForbidden { |
| + t.Fatalf("step-up with another identity = %d, want 403:\n%s", status, body) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != RoleMember { |
| + t.Fatal("membership changed on somebody else's gesture") |
| + } |
| + }) |
| +} |
| + |
| +// TestPendingActionIsNotACapability pins the held action's three limits: it |
| +// cannot be replayed, it cannot be redeemed by another session, and the |
| +// browser cannot change what it says. |
| +func TestPendingActionIsNotACapability(t *testing.T) { |
| + fx, _, csrf := memberFixture(t) |
| + now := time.Now() |
| + |
| + raw, err := fx.st.StashPendingAction(csrf, "paul@keymail.dev", "POST", |
| + fx.path("/members/remove"), "address=kim%40keymail.dev", now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + // Another session cannot spend it, even knowing the token. |
| + other := newBrowser(t) |
| + fx.setUser("kim@keymail.dev") |
| + signUp(t, fx, other, "code-other") |
| + if _, ok := fx.st.TakePendingAction(raw, sessionToken(t, fx, other), now); ok { |
| + t.Fatal("a pending action was redeemed by a different session") |
| + } |
| + // ...and that failed attempt burned it, so the rightful session cannot |
| + // either — burn-then-check, exactly like the OAuth code. |
| + if _, ok := fx.st.TakePendingAction(raw, csrf, now); ok { |
| + t.Fatal("a pending action survived a wrong-session attempt") |
| + } |
| + |
| + raw, err = fx.st.StashPendingAction(csrf, "paul@keymail.dev", "POST", fx.path("/members/remove"), "", now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, ok := fx.st.TakePendingAction(raw, csrf, now); !ok { |
| + t.Fatal("the rightful session could not redeem its own pending action") |
| + } |
| + if _, ok := fx.st.TakePendingAction(raw, csrf, now); ok { |
| + t.Fatal("a pending action was redeemed twice") |
| + } |
| + |
| + raw, err = fx.st.StashPendingAction(csrf, "paul@keymail.dev", "POST", fx.path("/members/remove"), "", now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, ok := fx.st.TakePendingAction(raw, csrf, now.Add(stepUpFlowTTL+time.Second)); ok { |
| + t.Fatal("an expired pending action was redeemed") |
| + } |
| +} |
| + |
| +// TestStepUpIsNotAnOracle is the property the account gate exists for: a |
| +// signed-in stranger POSTing at an owner-only action must get the same |
| +// byte-identical 404 an unrouted URL gets — never a redirect into Keymail, |
| +// which would confirm the account is real. |
| +func TestStepUpIsNotAnOracle(t *testing.T) { |
| + fx, _, _ := memberFixture(t) |
| + stranger := newBrowser(t) |
| + fx.setUser("stranger@keymail.dev") |
| + signUp(t, fx, stranger, "code-stranger") |
| + csrf := sessionToken(t, fx, stranger) |
| + |
| + _, _, missing := postForm(t, stranger, fx.ts.URL+"/a/zzzzz/members/remove", |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + |
| + for _, path := range []string{"/members/remove", "/members/transfer", "/delete"} { |
| + status, loc, body := postForm(t, stranger, fx.u(path), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}, "confirm": {"ops"}}) |
| + if status != http.StatusNotFound { |
| + t.Fatalf("%s as a non-member = %d -> %q, want 404", path, status, loc) |
| + } |
| + if body != missing { |
| + t.Fatalf("%s as a non-member answers differently from a nonexistent account:\n%q\nvs\n%q", |
| + path, body, missing) |
| + } |
| + } |
| + // A member who is not an owner gets the same silence. |
| + member := newBrowser(t) |
| + fx.setUser("kim@keymail.dev") |
| + signUp(t, fx, member, "code-kim") |
| + mcsrf := sessionToken(t, fx, member) |
| + status, _, body := postForm(t, member, fx.u("/members/remove"), |
| + url.Values{"csrf": {mcsrf}, "address": {"paul@keymail.dev"}}) |
| + if status != http.StatusNotFound || body != missing { |
| + t.Fatalf("a member probing an owner action = %d, body %q", status, body) |
| + } |
| + // And no pending action was parked for any of them: the gate never ran. |
| + var stashed int |
| + fx.st.db.QueryRow(`SELECT COUNT(*) FROM pending_actions`).Scan(&stashed) |
| + if stashed != 0 { |
| + t.Fatalf("%d pending actions parked by requests that should never have reached the gate", stashed) |
| + } |
| +} |
| + |
| +// TestCSRFStillOutranksStepUp: the two checks are independent, and the older |
| +// one runs first. A cross-site POST must be refused outright, not parked as a |
| +// pending action for the victim to unknowingly confirm. |
| +func TestCSRFStillOutranksStepUp(t *testing.T) { |
| + fx, c, _ := memberFixture(t) |
| + for _, path := range []string{"/members/remove", "/members/transfer", "/delete"} { |
| + status, _, _ := postForm(t, c, fx.u(path), |
| + url.Values{"csrf": {"wrong"}, "address": {"kim@keymail.dev"}, "confirm": {"ops"}}) |
| + if status != http.StatusForbidden { |
| + t.Fatalf("%s with a bad CSRF token = %d, want 403", path, status) |
| + } |
| + } |
| + var stashed int |
| + fx.st.db.QueryRow(`SELECT COUNT(*) FROM pending_actions`).Scan(&stashed) |
| + if stashed != 0 { |
| + t.Fatal("a cross-site POST parked a pending action") |
| + } |
| +} |
| + |
| +// TestOwnershipActionsBehindStepUp walks transfer and deletion the way a |
| +// browser does, and pins the guards that keep an account administrable. |
| +func TestOwnershipActionsBehindStepUp(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) |
| + |
| + // Ownership goes to an existing member, and the old owner steps down. |
| + status, loc, body := postGated(t, fx, c, fx.u("/members/transfer"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("transfer = %d -> %q:\n%s", status, loc, body) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "kim@keymail.dev"); role != RoleOwner { |
| + t.Fatalf("kim's role after transfer = %q, want owner", role) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "paul@keymail.dev"); role != RoleMember { |
| + t.Fatalf("paul's role after transfer = %q, want member", role) |
| + } |
| + // And paul, now a member, meets the same 404 every non-owner does. |
| + if status, _, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}); status != http.StatusNotFound { |
| + t.Fatalf("the former owner can still act = %d, want 404", status) |
| + } |
| + |
| + // Deletion: an account holding apps is refused even with a confirmed |
| + // passkey, and ops is refused outright. |
| + fx2, c2, csrf2 := memberFixture(t) |
| + status, _, _ = postGated(t, fx2, c2, fx2.u("/delete"), url.Values{"csrf": {csrf2}, "confirm": {"ops"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("delete = %d, want a 303 back to the account", status) |
| + } |
| + if a, _ := fx2.st.AccountBySqid(fx2.sqid); a == nil { |
| + t.Fatal("the ops account was deleted") |
| + } |
| +} |
| + |
| +// TestLastOwnerStillProtected: step-up does not become a way around the |
| +// invariant that every account keeps somebody who can administer it. |
| +func TestLastOwnerStillProtected(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) |
| + status, _, _ := postGated(t, fx, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"paul@keymail.dev"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("self-removal = %d, want a 303 with a refusal flash", status) |
| + } |
| + if role, _ := fx.st.Role(OpsAccountID, "paul@keymail.dev"); role != RoleOwner { |
| + t.Fatalf("the last owner removed themselves (role now %q)", role) |
| + } |
| +} |
| + |
| +// TestSignOutClearsPresence: proof of presence belongs to a session and dies |
| +// with it. |
| +func TestSignOutClearsPresence(t *testing.T) { |
| + fx, c, csrf := memberFixture(t) |
| + status, loc, _ := postForm(t, c, fx.u("/members/remove"), |
| + url.Values{"csrf": {csrf}, "address": {"kim@keymail.dev"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("gate = %d", status) |
| + } |
| + if _, _, _ = completeStepUp(t, fx, c, loc); !fx.st.FreshStepUp(csrf, "paul@keymail.dev", time.Now(), StepUpWindow) { |
| + t.Fatal("no presence recorded after a completed step-up") |
| + } |
| + if status, _, _ := postForm(t, c, fx.ts.URL+"/auth/signout", url.Values{"csrf": {csrf}}); status != http.StatusSeeOther { |
| + t.Fatalf("signout = %d", status) |
| + } |
| + if fx.st.FreshStepUp(csrf, "paul@keymail.dev", time.Now(), StepUpWindow) { |
| + t.Fatal("presence outlived the session it was proved for") |
| + } |
| +} |
diff --git a/internal/console/store.go b/internal/console/store.go| index 8b07369be..1271001eb 100644 |
| --- a/internal/console/store.go |
| +++ b/internal/console/store.go |
| @@ -191,7 +191,11 @@ func (s *Store) migrate() error { |
| } |
| // CLI identity's own tables (authstore.go), kept out of the list above so |
| // the feature is one file to read and one file to remove. |
| - return s.migrateCLIAuth() |
| + if err := s.migrateCLIAuth(); err != nil { |
| + return err |
| + } |
| + // Passkey step-up's own tables (stepup.go), for the same reason. |
| + return s.migrateStepUp() |
| } |
| // migrateAppsKey moves the apps table from PRIMARY KEY(app) — a GLOBAL name |
diff --git a/internal/console/templates/apps.html b/internal/console/templates/apps.html| index 2482956dd..ba7f6fd52 100644 |
| --- a/internal/console/templates/apps.html |
| +++ b/internal/console/templates/apps.html |
| @@ -34,11 +34,30 @@ |
| <h2>Members</h2> |
| <div class="tablewrap"> |
| <table> |
| - <tr><th>Address</th><th>Role</th></tr> |
| - {{range .Members}}<tr><td>{{.Name}}</td><td>{{.Role}}</td></tr>{{end}} |
| + <tr><th>Address</th><th>Role</th>{{if .IsOwner}}<th></th>{{end}}</tr> |
| + {{range .Members}}<tr> |
| + <td>{{.Name}}</td> |
| + <td>{{.Role}}</td> |
| + {{if $.IsOwner}}<td> |
| + {{if ne .Name $.Address}} |
| + <form method="post" action="{{$.Base}}/members/transfer"> |
| + <input type="hidden" name="csrf" value="{{$.CSRF}}"> |
| + <input type="hidden" name="address" value="{{.Name}}"> |
| + <button class="linkish" type="submit">Hand over ownership</button> |
| + </form> |
| + <form method="post" action="{{$.Base}}/members/remove"> |
| + <input type="hidden" name="csrf" value="{{$.CSRF}}"> |
| + <input type="hidden" name="address" value="{{.Name}}"> |
| + <button class="linkish" type="submit">Remove</button> |
| + </form> |
| + {{end}} |
| + </td>{{end}} |
| + </tr>{{end}} |
| </table> |
| </div> |
| {{if .IsOwner}} |
| +<p class="small">Removing a member, handing over ownership and deleting the account each ask |
| + you to confirm with your passkey — a signed-in browser on its own isn't enough.</p> |
| <h2>Invite a teammate</h2> |
| <form class="signin" method="post" action="{{.Base}}/invite"> |
| <input type="hidden" name="csrf" value="{{.CSRF}}"> |
| @@ -67,5 +86,16 @@ |
| </table> |
| </div> |
| {{end}} |
| +{{if not .Apps}} |
| +<h2>Delete this account</h2> |
| +<p class="small">An account with no apps can be retired. Type <strong>{{.Account.Name}}</strong> to |
| + confirm, then confirm with your passkey. Members and pending invitations go with it; nothing |
| + outside this account is touched.</p> |
| +<form class="signin" method="post" action="{{.Base}}/delete"> |
| + <input type="hidden" name="csrf" value="{{.CSRF}}"> |
| + <input type="text" name="confirm" placeholder="{{.Account.Name}}"> |
| + <button type="submit">Delete account</button> |
| +</form> |
| +{{end}} |
| {{end}} |
| {{template "bottom" .}}{{end}} |
diff --git a/internal/domains/mint.go b/internal/domains/mint.go| index 7d827e023..be905f63e 100644 |
| --- a/internal/domains/mint.go |
| +++ b/internal/domains/mint.go |
| @@ -23,9 +23,9 @@ import ( |
| // without opening every object; the two must always agree, and both must equal |
| // Sqid(id). |
| type AccountManifest struct { |
| - ID int64 `json:"id"` |
| - Sqid string `json:"sqid"` |
| - Name string `json:"name"` |
| + ID int64 `json:"id"` |
| + Sqid string `json:"sqid"` |
| + Name string `json:"name"` |
| CreatedAt time.Time `json:"created_at"` |
| // Owner is the keymail address that should hold the first owner membership |
| // of this account. ADDITIVE and omitempty: every manifest written before |
| @@ -51,8 +51,26 @@ type AccountManifest struct { |
| // memberships primary key is literal text, so an unnormalized address here |
| // would materialize a membership the real session can never match. |
| Owner string `json:"owner,omitempty"` |
| + |
| + // DeletedAt marks a retired account. The manifest is TOMBSTONED rather than |
| + // removed, and the difference is the whole point: deleting the object would |
| + // free the id for reuse (NextAccountID takes one past the highest id any |
| + // manifest claims), and a reused id is a reused sqid — a hostname that used |
| + // to be somebody's, handed to somebody else, with the old name still in |
| + // Certificate Transparency and in whatever DNS caches and bookmarks the |
| + // world kept. An account id is spent once. |
| + // |
| + // Every consumer that MATERIALIZES this manifest must skip a tombstoned one |
| + // (console.MaterializeAccounts, domains.Reconcile); every consumer that |
| + // ALLOCATES against it must still count it (NextAccountID). That split is |
| + // the invariant — get it backwards in either direction and you either |
| + // resurrect a deleted account or hand its identity to a new one. |
| + DeletedAt time.Time `json:"deleted_at,omitempty"` |
| } |
| +// Deleted reports whether this manifest is a tombstone. |
| +func (m AccountManifest) Deleted() bool { return !m.DeletedAt.IsZero() } |
| + |
| // NormalizeAddress is the one normalization every keymail address gets before |
| // it is stored or compared: trimmed and lowercased. Defined here, next to the |
| // manifest field that carries one, so the CLI can apply the console's rule |
| @@ -192,3 +210,54 @@ func MintAccount(ctx context.Context, s release.Store, name, owner string, creat |
| "another minter keeps taking the next free id, or the deployment bucket is rejecting "+ |
| "create-only writes", mintAttempts) |
| } |
| + |
| +// TombstoneAccount retires an account in the bucket — the control plane, and |
| +// therefore the only place a deletion is real. |
| +// |
| +// A console that deleted only its own rows would watch the account come back: |
| +// every console start materializes from these manifests, so the next deploy |
| +// would re-create the account AND re-adopt the manifest's owner, handing it |
| +// back to whoever held it at creation even if ownership had since been |
| +// transferred away. Deleting has to happen where identity lives. |
| +// |
| +// The manifest is REWRITTEN, not removed — see AccountManifest.DeletedAt for |
| +// why the id must stay spent. Name and CreatedAt are kept so the record still |
| +// says what this account was; Owner is cleared, because a tombstone is not a |
| +// grant and a consumer too old to know about DeletedAt should at worst |
| +// materialize something nobody owns rather than something it hands to a |
| +// former owner. |
| +// |
| +// CAS overwrite with one retry, the same shape as WriteAccountManifest. |
| +func TombstoneAccount(ctx context.Context, s release.Store, sqid string, now time.Time) error { |
| + if sqid == Sqid(ReservedAccountID) { |
| + return fmt.Errorf("domains: the ops account cannot be deleted") |
| + } |
| + key := AccountManifestKey(sqid) |
| + tombstone := func() error { |
| + body, etag, err := s.Get(ctx, key) |
| + if err != nil { |
| + return fmt.Errorf("domains: read %s: %w", key, err) |
| + } |
| + var m AccountManifest |
| + if err := json.Unmarshal(body, &m); err != nil { |
| + return fmt.Errorf("domains: parse %s: %w", key, err) |
| + } |
| + if m.Deleted() { |
| + return nil // already retired; nothing to do and nothing to undo |
| + } |
| + m.Owner = "" |
| + m.DeletedAt = now.UTC() |
| + out, err := json.Marshal(m) |
| + if err != nil { |
| + return err |
| + } |
| + _, err = s.Put(ctx, key, out, etag) |
| + return err |
| + } |
| + if err := tombstone(); errors.Is(err, release.ErrCASConflict) { |
| + return tombstone() // one retry on a lost race; a second loss surfaces |
| + } else if err != nil { |
| + return err |
| + } |
| + return nil |
| +} |
diff --git a/internal/domains/reconcile.go b/internal/domains/reconcile.go| index 2d0845f46..b41aadae8 100644 |
| --- a/internal/domains/reconcile.go |
| +++ b/internal/domains/reconcile.go |
| @@ -93,11 +93,7 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| log.Printf("domains: skipping %s: read failed: %v", key, err) |
| continue |
| } |
| - var m struct { |
| - ID int64 `json:"id"` |
| - Sqid string `json:"sqid"` |
| - Name string `json:"name"` |
| - } |
| + var m AccountManifest |
| if err := json.Unmarshal(body, &m); err != nil || m.ID <= 0 || m.Sqid == "" { |
| log.Printf("domains: skipping %s: malformed account manifest: %v", key, err) |
| continue |
| @@ -111,6 +107,17 @@ func Reconcile(ctx context.Context, s release.Store, reg *registry.Registry, cfg |
| log.Printf("domains: skipping %s: sqid %q/%q is not Sqid(%d)=%q", key, sqid, m.Sqid, m.ID, Sqid(m.ID)) |
| continue |
| } |
| + // A tombstoned manifest is materialized nowhere and, if this registry |
| + // still carries the account from before it was retired, un-materialized |
| + // here. Leaving the row would keep the account listed by `carlos |
| + // accounts list` on every box forever — a deletion that only ever took |
| + // effect in the console that performed it. |
| + if m.Deleted() { |
| + if err := reg.DeleteAccount(m.Sqid); err != nil { |
| + log.Printf("domains: retire account %s: %v", sqid, err) |
| + } |
| + continue |
| + } |
| if err := reg.PutAccount(m.ID, m.Sqid, m.Name); err != nil { |
| log.Printf("domains: materialize account %s: %v", sqid, err) |
| continue |
diff --git a/internal/domains/reconcile_test.go b/internal/domains/reconcile_test.go| index 515737ad0..6f9475d7e 100644 |
| --- a/internal/domains/reconcile_test.go |
| +++ b/internal/domains/reconcile_test.go |
| @@ -145,6 +145,42 @@ func TestReconcileMaterializesAccountsFromManifests(t *testing.T) { |
| } |
| } |
| +// A tombstoned manifest is materialized nowhere, and un-materializes an account |
| +// this registry already carried. Without the second half, a deletion would take |
| +// effect only in the console that performed it: every other box on the |
| +// deployment would keep listing the account forever. |
| +func TestReconcileRetiresTombstonedAccounts(t *testing.T) { |
| + s, reg := dirStore(t), regWith(t) |
| + sqid := writeAccountManifest(t, s, 793481920157, "Kass") |
| + if err := domains.Reconcile(ctx, s, reg, cfg(t)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if id, _, _ := reg.AccountBySqid(sqid); id == 0 { |
| + t.Fatal("the account did not materialize in the first place") |
| + } |
| + if err := domains.TombstoneAccount(ctx, s, sqid, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := domains.Reconcile(ctx, s, reg, cfg(t)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if id, name, _ := reg.AccountBySqid(sqid); id != 0 { |
| + t.Errorf("retired account still in the registry as id=%d name=%q", id, name) |
| + } |
| + // A registry built from scratch never learns about it at all. |
| + reg2 := regWith(t) |
| + if err := domains.Reconcile(ctx, s, reg2, cfg(t)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if id, _, _ := reg2.AccountBySqid(sqid); id != 0 { |
| + t.Errorf("a rebuilt registry materialized the retired account as id=%d", id) |
| + } |
| + // Ops is never deletable, however the manifest is written. |
| + if err := domains.TombstoneAccount(ctx, s, domains.Sqid(domains.ReservedAccountID), time.Now()); err == nil { |
| + t.Error("tombstoning the ops account was allowed") |
| + } |
| +} |
| + |
| // The app→account linkage IS the bucket key path: an app under |
| // apps/<sqid>/<app>/… belongs to that account; a legacy unqualified |
| // apps/<app>/… belongs to ops until it is migrated. |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index 1a022aae0..3ac50d731 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -263,6 +263,25 @@ ON CONFLICT(id) DO UPDATE SET sqid = excluded.sqid, name = excluded.name`, id, s |
| return err |
| } |
| +// DeleteAccount drops a retired account's row. Called only by the reconcile |
| +// pass, for a manifest the bucket has tombstoned — the bucket decides, this |
| +// follows. Idempotent: an account already gone is a success, because the |
| +// caller's goal is "not here", not "I removed it". |
| +// |
| +// Routes are deliberately untouched. An account cannot be deleted while it |
| +// holds apps, so there should be none; if some route still names this sqid, |
| +// silently unhooking it would take a live site down to tidy up a listing. |
| +// The ops account is pinned and never deletable. The id is duplicated here |
| +// rather than imported from domains.ReservedAccountID because the dependency |
| +// runs the other way — domains imports registry — and a constant is a cheaper |
| +// thing to keep in step than a cycle is to break. |
| +func (r *Registry) DeleteAccount(sqid string) error { |
| + _, err := r.sql.Exec(`DELETE FROM accounts WHERE sqid = ? AND id != ?`, sqid, opsAccountID) |
| + return err |
| +} |
| + |
| +const opsAccountID = 1 |
| + |
| // AccountBySqid resolves an account's public identity to its id and name, or |
| // (0, "", nil) when absent — "no such account" is an answer, not an error. |
| func (r *Registry) AccountBySqid(sqid string) (int64, string, error) { |