carlos / platform Public

console: an app-scoped credential cannot claim a new app, and closed doors are pinned by intent

Review fixes C29-C31 and four minors.

C29. handleAPICreateApp resolves through apiPrincipalAccount, which enforces
only the account half — and POST /api/cli/apps/{account}/new has no {app}
segment, so apiAccountAppBy's binding check never ran for it. A credential
minted for one app and handed to that app's CI could claim any other name in
the account: squatting, quota and billing surface, a row the operator never
authorised. The door is for an account-scoped hub, so the fix is a guard and
not a role change, and it refuses with the byte-identical 404 every other
binding mismatch gives — this is scope, not role. Deleting the guard makes
the new test fail twice, once on the refusal shape and once because the squat
actually happens.

C30. instances-manifest was protected only by closedDoors, which is
bookkeeping: a commit that reopened the door and edited that list in the same
breath would have stayed green. It now sits in policyDoors beside mintDoors
and fleetDoors, which say which doors may never open whatever the
registration currently says, and a new end-to-end test dials every closedDoors
pattern with a real admin credential so a ninth door is covered the moment it
is registered. Both catch that commit; so does the enable test, which shows
apiPerson answering 401 rather than letting the enable through. apiPerson
stays at two doors and not eight, recorded as a choice: the six fleets doors
rely on the registration plus the end-to-end pin, and these two carry a second
wall because their handler semantics are person-only in a way an editor must
not miss.

C31. The kind dispatch in apiPrincipalAccount and handleAPIApps is a closed
switch. An else would send a future third kind down the person path, where it
would read as a member with an empty address.

Three stale comments corrected: apiAccountApp is no longer the inner wall at
the mint door, the instance family is no longer the only one a non-person may
speak to, and sameStrings now sorts both sides instead of relying on every
caller passing pre-sorted input.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016UimU7fAfeAoBMm6HKCPwQ

Paul Campbell 2 months ago 9ebbe881e839bafd0adbf204d1451cae53186be9 parent ccdefa2

6 files changed, +224 −34

internal/console/apicli.go +58 −10
internal/console/apiinstances.go +1 −1
internal/console/apiinstances_test.go +5 −3
internal/console/authcli.go +5 −4
internal/console/svcroutes_test.go +87 −13
internal/console/svcscope_test.go +68 −3

diff --git a/internal/console/apicli.go b/internal/console/apicli.go

index e652d3bac..096d0ef1a 100644

--- a/internal/console/apicli.go

+++ b/internal/console/apicli.go

@@ -132,10 +132,16 @@ func (s *Server) serviceBinding(p Principal) (*Account, bool) {

return acct, true

}

-// apiPrincipalAccount is the account half of apiAccountAppBy, in its typed

-// form: the principal cliRoute already resolved, and the account the

-// {account} segment names for THAT principal. Callers that need only the

-// audit actor use apiAccount below.

+// apiPrincipalAccount is the account half of apiAccountAppBy: the principal

+// cliRoute already resolved, and the account the {account} segment names for

+// THAT principal.

+//

+// It is the SHARED BODY, not a convenience for some future constituency.

+// Three callers today: apiAccountAppBy and handleAPICreateApp, which both

+// need the typed principal to check an app binding, and apiAccount, which

+// renders the audit actor for everyone else. Spec §4's principalKey moves

+// the rate limiters onto it in a later task; that is a consequence of this

+// shape, not the reason for it.

// A SERVICE's scope is a BINDING, not a membership set, so the segment is

// matched as a sqid ONLY. It holds no memberships, so there is no set within

@@ -177,16 +183,25 @@ func (s *Server) apiPrincipalAccount(w http.ResponseWriter, r *http.Request) (Pr

}

seg := r.PathValue("account")

- if p.Kind == PrincipalService {

+ // Dispatched as a closed switch rather than an if/else, so a kind added

+ // later fails shut. An `else` here would send any third kind down the

+ // person path, where it would read as a member with an empty address —

+ // the one shape of bug this package's whole thesis is against.

+ var addr string

+ switch p.Kind {

+ case PrincipalService:

acct, ok := s.serviceBinding(p)

if !ok || seg != acct.Sqid {

http.NotFound(w, r)

return Principal{}, nil, false

}

return p, acct, true

+ case PrincipalUser:

+ addr = p.Address

+ default:

+ http.NotFound(w, r)

+ return Principal{}, nil, false

}

- addr := p.Address

accts, err := s.store.AccountsFor(addr)

if err != nil {

jsonError(w, http.StatusInternalServerError, "could not load accounts")

@@ -262,6 +277,17 @@ func (s *Server) apiAccount(w http.ResponseWriter, r *http.Request) (string, *Ac

// person-only semantics are stated where the handler is, not only where it

// is registered.

+// TWO DOORS AND NOT EIGHT, deliberately. The six fleets/* doors are just as

+// person-only and just as closed, and they do NOT call this: adding a second

+// wall to a door no request can reach is ceremony, and ceremony is what gets

+// pattern-matched into the next handler that should not have it. They rely

+// on the registration plus the end-to-end pin

+// (TestEveryClosedDoorRefusesAServiceCredential). These two get the extra

+// line because their handler semantics are person-only in a way a reader

+// editing them must not miss — one mints a credential, the other rewrites

+// the bounds an app runs inside. Recorded as a choice, not left as an

+// asymmetry to be read as an oversight.

+//

// It reads the principal cliRoute already resolved, so it costs no second

// lookup. Pinned directly by TestApiPersonIsTheInnerWall, because the closed

// registrations mean no request can ever deliver a service principal to it

@@ -299,10 +325,25 @@ func (s *Server) apiPerson(w http.ResponseWriter, r *http.Request) bool {

// invalid place must not be a weaker gate than `carlos apps place` run

// straight after create — this is what keeps the two equal.

func (s *Server) handleAPICreateApp(w http.ResponseWriter, r *http.Request) {

- addr, acct, ok := s.apiAccount(w, r)

+ p, acct, ok := s.apiPrincipalAccount(w, r)

if !ok {

return

}

+ // An app-scoped credential's subject is its app. Claiming a NEW one is

+ // ACCOUNT authority it was deliberately not given — name-squatting

+ // inside the account, quota and billing surface, and a row the operator

+ // never authorised. This door has no {app} segment, so apiAccountAppBy's

+ // binding check never runs for it and the guard has to be here.

+ //

+ // The byte-identical 404, not a 403: this is a scope refusal like every

+ // other binding mismatch, not a role refusal. The door is Allow(publish)

+ // and stays that way — it is designed for an ACCOUNT-scoped hub (amadan

+ // Pages), which is exactly the credential it still serves.

+ if p.Kind == PrincipalService && p.App != "" {

+ http.NotFound(w, r)

+ return

+ }

+ addr := auditActor(p, acct)

// Keyed on the ADDRESS, like every write budget here: a token-keyed

// counter is one the holder of several tokens resets at will.

if !s.cli.create.allow(addr, time.Now()) {

@@ -404,8 +445,12 @@ func (s *Server) handleAPIApps(w http.ResponseWriter, r *http.Request) {

http.NotFound(w, r)

return

}

+ // A closed switch, for apiPrincipalAccount's reason: a kind added later

+ // must not fall through to AccountsFor and enumerate a person's whole

+ // membership set on behalf of something nobody has classified yet.

var accts []Account

- if p.Kind == PrincipalService {

+ switch p.Kind {

+ case PrincipalService:

// A binding this console cannot honour enumerates NOTHING. It is not

// an error — the credential's scope is simply empty — and above all

// it is not a fall-through to every account, which is the failure

@@ -413,7 +458,7 @@ func (s *Server) handleAPIApps(w http.ResponseWriter, r *http.Request) {

if acct, ok := s.serviceBinding(p); ok {

accts = []Account{*acct}

}

- } else {

+ case PrincipalUser:

var err error

accts, err = s.store.AccountsFor(p.Address)

if err != nil {

@@ -421,6 +466,9 @@ func (s *Server) handleAPIApps(w http.ResponseWriter, r *http.Request) {

jsonError(w, http.StatusInternalServerError, "could not load accounts")

return

}

+ default:

+ http.NotFound(w, r)

+ return

}

out := make([]map[string]any, 0, len(accts))

for _, a := range accts {

diff --git a/internal/console/apiinstances.go b/internal/console/apiinstances.go

index 8812a5a79..e4e63eabe 100644

--- a/internal/console/apiinstances.go

+++ b/internal/console/apiinstances.go

@@ -842,7 +842,7 @@ func (s *Server) handleAPIInstanceDelete(w http.ResponseWriter, r *http.Request)

// That 401 is the INNER of two walls, and it is the one nothing can reach:

// the outer is cliRoute, where this door's zero Set refuses every role,

-// admin included (TestNoRoleReachesAMintOrFleetDoor,

+// admin included (TestNoRoleReachesAMintFleetOrPolicyDoor,

// TestAdminIsRefusedAtAClosedDoor). Both are kept, because the inner one

// used to be a side effect of resolving through cliUser and would otherwise

// have vanished the day that resolution became principal-aware.

diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go

index c6454f39d..63a8b0939 100644

--- a/internal/console/apiinstances_test.go

+++ b/internal/console/apiinstances_test.go

@@ -241,9 +241,11 @@ func TestAPIInstanceAppTokenContainment(t *testing.T) {

}

// Minting is a human act: the mint door is registered with the zero

// Set, so no role reaches it and admin is no escape hatch either

- // (TestNoRoleReachesAMintOrFleetDoor). Behind it, apiAccountApp still

- // cannot see an app token at all — the gate is the outer of two walls,

- // not a replacement for the inner one.

+ // (TestNoRoleReachesAMintFleetOrPolicyDoor). Behind it, apiPerson still

+ // refuses a non-person outright — the gate is the outer of two walls,

+ // not a replacement for the inner one. (apiAccountApp used to be that

+ // inner wall by accident, because it resolved through cliUser; since it

+ // became principal-aware the refusal is stated rather than inherited.)

if status, _, out := rawReq(t, http.MethodPost,

fx.ts.URL+"/api/cli/apps/bab/titogo/instance-token", appTok, `{}`); status != http.StatusForbidden {

t.Fatalf("app token minting a token = %d %s, want 403", status, out)

diff --git a/internal/console/authcli.go b/internal/console/authcli.go

index 2a231b7d2..9b2180fc8 100644

--- a/internal/console/authcli.go

+++ b/internal/console/authcli.go

@@ -339,8 +339,9 @@ func (s *Server) registerCLIRoutes() {

s.cliRoute("POST /api/cli/apps/{account}/{app}/ledgers/{ledger}/append", svcrole.Allow(svcrole.Admin), s.handleAPILedgerAppend)

s.cliRoute("POST /api/cli/apps/{account}/{app}/ledgers/{ledger}/blobs", svcrole.Allow(svcrole.Admin), s.handleAPILedgerBlob)

// The instance provisioning API (apiinstances.go) — the only door onto

- // instances/ and domains/ from outside a box, and the only family an

- // APP's own token may speak to. `instance-token` is a literal segment beside the

+ // instances/ and domains/ from outside a box. It was once the only

+ // family a non-person could speak to; since the role gate landed that is

+ // a property of each door's Allow set, not of this block. `instance-token` is a literal segment beside the

// {host} wildcards; the mux prefers a literal regardless of

// registration order, and it deliberately sits under a DIFFERENT

// prefix from instances/{host} so a host can never be mistaken for the

@@ -378,7 +379,7 @@ func (s *Server) registerCLIRoutes() {

// has never been able to see a non-person at all — it moves the refusal

// out to the wrapper, where it is structural, and changes the ANSWER a

// credential gets here from 401 to 403. Pinned by

- // TestNoRoleReachesAMintOrFleetDoor and TestAdminIsRefusedAtAClosedDoor.

+ // TestNoRoleReachesAMintFleetOrPolicyDoor and TestAdminIsRefusedAtAClosedDoor.

s.cliRoute("POST /api/cli/apps/{account}/{app}/instance-token", svcrole.Set{}, s.handleAPIInstanceToken)

// The extra-hostname API (apidomains.go, issue #151) — the only door onto

// domains/ from outside a box, sharing the instance family's gate exactly

@@ -424,7 +425,7 @@ func (s *Server) registerCLIRoutes() {

// fleet, and rotating or detaching one, hands out a MACHINE's credentials

// and changes where a whole account's apps can be placed. That is a human

// act, and no service role reaches it — the same escalation cut as the

- // mint door above. Pinned by TestNoRoleReachesAMintOrFleetDoor.

+ // mint door above. Pinned by TestNoRoleReachesAMintFleetOrPolicyDoor.

s.cliRoute("POST /api/cli/fleets/{account}/new", svcrole.Set{}, s.handleAPIFleetNew)

s.cliRoute("GET /api/cli/fleets/{account}", svcrole.Set{}, s.handleAPIFleetList)

s.cliRoute("POST /api/cli/fleets/{account}/{fleet}/boxes", svcrole.Set{}, s.handleAPIFleetAddBox)

diff --git a/internal/console/svcroutes_test.go b/internal/console/svcroutes_test.go

index 6256f8b12..5ab5356c9 100644

--- a/internal/console/svcroutes_test.go

+++ b/internal/console/svcroutes_test.go

@@ -30,14 +30,28 @@ import (

"github.com/carlosframework/platform/internal/svcrole"

)

-// mintDoors and fleetDoors are the two families no role may ever reach.

-// Listed as substrings of the registered pattern, because that is the shape

-// the table is keyed on. /api/cli/services/ has no doors yet — the services

-// API is a later task — so that entry is a standing guard rather than a live

-// assertion: the day those doors appear, they appear closed or this fails.

+// mintDoors, fleetDoors and policyDoors are the three families no role may

+// ever reach. Listed as substrings of the registered pattern, because that is

+// the shape the table is keyed on. /api/cli/services/ has no doors yet — the

+// services API is a later task — so that entry is a standing guard rather

+// than a live assertion: the day those doors appear, they appear closed or

+// this fails.

+//

+// These lists are the INTENT-level pin, and they exist because closedDoors

+// below is only bookkeeping. closedDoors says "these eight are registered

+// with the zero Set"; a commit that reopened a door and edited that list in

+// the same breath would stay green. These say WHICH DOORS MAY NEVER OPEN,

+// whatever the registration currently says, so reopening one takes an edit

+// that has to argue with the words next to it.

+//

+// policyDoors is instances-manifest: enabling writes the manifest that bounds

+// which domain, pool, regions and channel an app may ever be brought up on.

+// A credential the app itself holds must not rewrite the bounds it runs

+// inside, which is a fact about the door's subject and not about any role.

var (

- mintDoors = []string{"/instance-token", "/api/cli/services/"}

- fleetDoors = []string{"/api/cli/fleets/"}

+ mintDoors = []string{"/instance-token", "/api/cli/services/"}

+ fleetDoors = []string{"/api/cli/fleets/"}

+ policyDoors = []string{"/instances-manifest"}

)

// closedDoors is the exact set of /api/cli/ patterns registered with the zero

@@ -110,19 +124,24 @@ func TestEveryCLIDoorNamesItsRoles(t *testing.T) {

}

-// TestNoRoleReachesAMintOrFleetDoor is the escalation cut: a credential must

-// never be able to mint or rotate a credential — including itself — and must

-// never enrol a box.

-func TestNoRoleReachesAMintOrFleetDoor(t *testing.T) {

+// TestNoRoleReachesAMintFleetOrPolicyDoor is the escalation cut: a credential

+// must never be able to mint or rotate a credential — including itself —

+// must never enrol a box, and must never rewrite the policy that bounds what

+// its own app may be brought up as.

+func TestNoRoleReachesAMintFleetOrPolicyDoor(t *testing.T) {

s := newConsoleFixture(t).srv

+ var families []string

+ families = append(families, mintDoors...)

+ families = append(families, fleetDoors...)

+ families = append(families, policyDoors...)

for pattern, set := range s.cliRoles {

- for _, closed := range append(append([]string{}, mintDoors...), fleetDoors...) {

+ for _, closed := range families {

if !strings.Contains(pattern, closed) {

continue

}

for _, r := range svcrole.All() {

if set.Permits(r) {

- t.Errorf("%s is reachable by role %q; mint and fleet doors must be closed to every role", pattern, r)

+ t.Errorf("%s is reachable by role %q; mint, fleet and policy doors must be closed to every role", pattern, r)

}

@@ -524,6 +543,61 @@ func TestServiceRoleReachesTheDoorItNames(t *testing.T) {

}

+// TestEveryClosedDoorRefusesAServiceCredential walks the closedDoors list

+// itself and dials every pattern on it with a real service credential over

+// HTTP. Driven off the list rather than off a hand-written set of URLs, so a

+// ninth closed door added later is covered the moment it is registered — and

+// so the list cannot become a claim nobody checks.

+//

+// This is what turns closedDoors from bookkeeping into a property. On its own

+// the list only says "these are registered with the zero Set"; a door that

+// was reopened while the list was edited to match would still be green

+// against it. Here the answer comes from the running server.

+//

+// The credential holds ADMIN, the role most likely to slip through, and it is

+// account-scoped so no app binding can be doing the refusing instead of the

+// role gate. Every answer must be the closed-door 403, whose body is the same

+// bytes at every door (TestClosedDoorRefusalIsItsOwnMessage).

+func TestEveryClosedDoorRefusesAServiceCredential(t *testing.T) {

+ fx := newConsoleFixture(t)

+ tok := serviceToken(t, fx.st, OpsAccountID, "root", "", svcrole.Admin)

+ seen := 0

+ for _, pattern := range closedDoors {

+ method, path, found := strings.Cut(pattern, " ")

+ if !found {

+ t.Fatalf("closedDoors entry %q is not \"METHOD /path\"", pattern)

+ }

+ // Fill the wildcards with things that EXIST where they can, so a 403

+ // is the role gate refusing and not a 404 arriving first. Order is

+ // role before scope, so this is belt and braces.

+ u := strings.NewReplacer(

+ "{account}", fx.sqid,

+ "{app}", "kass",

+ "{fleet}", "edge",

+ "{label}", "box1",

+ ).Replace(path)

+ if strings.Contains(u, "{") {

+ t.Fatalf("closedDoors entry %q has a wildcard this test does not fill", pattern)

+ }

+ body := ""

+ if method == http.MethodPost || method == http.MethodPut {

+ body = `{}`

+ }

+ status, _, out := rawReq(t, method, fx.ts.URL+u, tok, body)

+ if status != http.StatusForbidden {

+ t.Errorf("%s = %d %s, want the closed-door 403", pattern, status, out)

+ }

+ if !strings.Contains(out, "closed to service credentials") {

+ t.Errorf("%s body = %s, want the closed-door refusal", pattern, out)

+ }

+ seen++

+ }

+ if seen != len(closedDoors) || seen != 8 {

+ t.Errorf("walked %d closed doors, want all %d (and 8 today: instance-token, instances-manifest, six fleets)",

+ seen, len(closedDoors))

+ }

// TestAdminIsRefusedAtAClosedDoor proves the escalation cut end to end, not

// just in the table. Admin is the union of every role, so it is the one

// credential most likely to slip through a door that named no roles — and

diff --git a/internal/console/svcscope_test.go b/internal/console/svcscope_test.go

index a5b94e9c7..03c9493d7 100644

--- a/internal/console/svcscope_test.go

+++ b/internal/console/svcscope_test.go

@@ -77,13 +77,20 @@ func apiApps(t *testing.T, fx *consoleFixture, bearer string) []string {

return pairs

}

+// sameStrings compares two sets regardless of order. It sorts COPIES of both

+// sides: sorting only `want` happened to work while every caller fed it

+// apiApps's pre-sorted output, and would have quietly compared an unsorted

+// `got` the first time someone passed a raw list.

func sameStrings(got, want []string) bool {

if len(got) != len(want) {

return false

}

- sort.Strings(want)

- for i := range got {

- if got[i] != want[i] {

+ a := append([]string(nil), got...)

+ b := append([]string(nil), want...)

+ sort.Strings(a)

+ sort.Strings(b)

+ for i := range a {

+ if a[i] != b[i] {

return false

}

@@ -403,3 +410,61 @@ func TestServiceBindingDoesNotOutliveTheApp(t *testing.T) {

status, h, body := rawReq(t, http.MethodGet, u, tok, "")

assertNotFound(t, "bound app after it was trashed", want, status, h, body)

}

+// TestAppScopedServiceCannotClaimANewApp is C29. The app-binding check lives

+// in apiAccountAppBy and fires on the {app} segment — and POST

+// /api/cli/apps/{account}/new has none, while being Allow(publish). Without

+// a guard at the door itself, a credential minted for ONE app and handed to

+// that app's CI could claim any other name in the account: squatting, quota

+// and billing surface, and a row the operator never authorised. Pre-branch

+// this was impossible, because no app token reached the door at all.

+//

+// The refusal is the byte-identical 404 every other binding mismatch gives —

+// it is a scope refusal, not a role refusal, and the door stays Allow(publish)

+// because an ACCOUNT-scoped credential is exactly what it is for.

+func TestAppScopedServiceCannotClaimANewApp(t *testing.T) {

+ fx := newConsoleFixture(t)

+ if _, err := fx.st.CreateAppIn(OpsAccountID, "one", "paul@keymail.dev", time.Now()); err != nil {

+ t.Fatal(err)

+ }

+ u := fx.ts.URL + "/api/cli/apps/" + fx.sqid + "/new"

+ want := canonicalNotFound(t)

+ appTok := serviceToken(t, fx.st, OpsAccountID, "just-one", "one", svcrole.Publish)

+ status, h, body := rawReq(t, http.MethodPost, u, appTok, `{"name":"two"}`)

+ assertNotFound(t, "app-scoped credential claiming a new app", want, status, h, body)

+ // And it really did not create anything.

+ apps, err := fx.st.AppsIn(OpsAccountID)

+ if err != nil {

+ t.Fatal(err)

+ }

+ for _, a := range apps {

+ if a == "two" {

+ t.Fatal("an app-scoped credential claimed a second app")

+ }

+ // A legacy app token never even reaches the guard: it holds role

+ // `instance` and this door needs `publish`, so role-before-scope refuses

+ // it out at cliRoute with the constant 403. Asserted here so the two

+ // walls are not confused for one — if the role table ever put `instance`

+ // on this door, THIS is the assertion that would move, and the guard

+ // above is what would then be doing the work.

+ legacy, err := fx.st.NewAppToken(OpsAccountID, "one", serviceMinter, "router", time.Now())

+ if err != nil {

+ t.Fatal(err)

+ }

+ if status, _, body := rawReq(t, http.MethodPost, u, legacy, `{"name":"three"}`); status != http.StatusForbidden {

+ t.Errorf("legacy app token claiming a new app = %d %s, want the role gate's 403", status, body)

+ }

+ // The ACCOUNT-scoped credential this door is designed for still works:

+ // the guard is a binding check, not a narrowing of the role.

+ hub := serviceToken(t, fx.st, OpsAccountID, "hub", "", svcrole.Publish)

+ if status, _, body := rawReq(t, http.MethodPost, u, hub, `{"name":"four"}`); status != http.StatusOK {

+ t.Fatalf("account-scoped credential creating an app = %d %s, want 200", status, body)

+ }

+ if has, err := fx.st.HasAppIn(OpsAccountID, "four"); err != nil || !has {

+ t.Fatalf("HasAppIn(four) = %v %v, want the hub's app claimed", has, err)

+ }