| index 306d77990..0812a6d38 100644 |
| --- a/internal/console/svcroutes_test.go |
| +++ b/internal/console/svcroutes_test.go |
| @@ -87,10 +87,14 @@ func TestEveryCLIDoorNamesItsRoles(t *testing.T) { |
| empty, wantClosed) |
| } |
| |
| - registered := registeredCLIPatterns(t) |
| + // scanCLISource fails on its own for a door registered outside |
| + // cliRoute; this pairs what it DID find against what the running server |
| + // recorded. The equality is what makes a walker that silently found |
| + // nothing fail too — no magic minimum needed. |
| + registered := scanCLISource(t).gated |
| for _, pattern := range registered { |
| if _, ok := s.cliRoles[pattern]; !ok { |
| - t.Errorf("%s: registered without going through cliRoute", pattern) |
| + t.Errorf("%s: the source registers it, the server did not record it", pattern) |
| } |
| } |
| if len(s.cliRoles) != len(registered) { |
| @@ -171,53 +175,46 @@ func TestInstanceRoleMatchesTodaysAppTokenReach(t *testing.T) { |
| } |
| } |
| |
| -// registeredCLIPatterns reads the package's own source rather than the mux, |
| -// because http.ServeMux cannot be enumerated. The check it enables is worth |
| -// the unusual shape: it catches a door added tomorrow with plain |
| -// mux.HandleFunc, which is the exact way default-deny would be lost. |
| -func registeredCLIPatterns(t *testing.T) []string { |
| - t.Helper() |
| - src, err := os.ReadFile("authcli.go") |
| - if err != nil { |
| - t.Fatalf("read authcli.go: %v", err) |
| - } |
| - var out []string |
| - for _, line := range strings.Split(string(src), "\n") { |
| - if !strings.Contains(line, "/api/cli/") { |
| - continue |
| - } |
| - if strings.Contains(line, "s.mux.HandleFunc(") { |
| - t.Errorf("door registered with mux.HandleFunc, bypassing the role gate:\n %s", strings.TrimSpace(line)) |
| - continue |
| - } |
| - if !strings.Contains(line, "s.cliRoute(") { |
| - continue |
| - } |
| - // The pattern is the first quoted string on the line. |
| - i := strings.Index(line, `"`) |
| - if i < 0 { |
| - continue |
| - } |
| - j := strings.Index(line[i+1:], `"`) |
| - if j < 0 { |
| - continue |
| - } |
| - out = append(out, line[i+1:i+1+j]) |
| - } |
| - if len(out) < 40 { |
| - t.Fatalf("found only %d cliRoute patterns; the parser is broken, not the code", len(out)) |
| - } |
| - return out |
| +// ---- reading the package's own source --------------------------------------- |
| +// |
| +// http.ServeMux cannot be enumerated: there is no way to ask a live server |
| +// which patterns it holds, so a test that wants to prove EVERY door is gated |
| +// has to read the source that registers them. That is an unusual shape for a |
| +// test and it earns its place — it is the only thing that catches a door |
| +// added tomorrow with plain mux.HandleFunc, which is the exact way |
| +// default-deny would be lost. |
| +// |
| +// It is one go/ast pass over every non-test file in the package, not a scan |
| +// of one file's lines. Both parts of that matter. Package-wide, because the |
| +// services API arrives in a file of its own with a registration function of |
| +// its own, and those are the doors that MINT credentials: a scanner that read |
| +// only authcli.go would report 47 gated doors out of 47 and be silent about |
| +// them. And AST rather than strings, because a registration split across two |
| +// lines escapes a substring match, and because a line scanner keying on |
| +// "s.mux.HandleFunc(" fails the build for any COMMENT that happens to quote |
| +// it — a standing tax on the wording of the most heavily commented function |
| +// in the file. |
| + |
| +// muxDoor is one pattern registered straight onto the mux, with the handler |
| +// expression it was given. |
| +type muxDoor struct { |
| + pattern string |
| + handler ast.Expr |
| } |
| |
| -// TestAuthCLIFamilyResolvesOnlyPersons is the OTHER half of default-deny. |
| -// The /auth/cli/ handlers — whoami, revoke, tokens/revoke, accounts (which |
| -// creates an ACCOUNT) — sit outside cliRoute and need no role bindings, |
| -// because every one of them that reads a bearer at all gates on cliUser, |
| -// which answers only for a person's token. That is currently true by |
| -// construction; this makes it true by test, so the day someone reaches for |
| -// CLIPrincipal over there the build says so. |
| -func TestAuthCLIFamilyResolvesOnlyPersons(t *testing.T) { |
| +// cliSource is what that single pass yields. |
| +type cliSource struct { |
| + fset *token.FileSet |
| + funcs map[string]*ast.FuncDecl // by bare name |
| + gated []string // patterns registered through cliRoute |
| + plain []muxDoor // patterns registered outside /api/cli/ |
| +} |
| + |
| +// scanCLISource parses the package and classifies every route registration in |
| +// it. Anything it cannot classify is an error rather than a silent skip: a |
| +// registration this test cannot read is a registration it cannot vouch for. |
| +func scanCLISource(t *testing.T) *cliSource { |
| + t.Helper() |
| fset := token.NewFileSet() |
| pkgs, err := parser.ParseDir(fset, ".", func(fi os.FileInfo) bool { |
| return !strings.HasSuffix(fi.Name(), "_test.go") |
| @@ -229,67 +226,168 @@ func TestAuthCLIFamilyResolvesOnlyPersons(t *testing.T) { |
| if !ok { |
| t.Fatal("package console not found by the parser") |
| } |
| - funcs := map[string]*ast.FuncDecl{} |
| + src := &cliSource{fset: fset, funcs: map[string]*ast.FuncDecl{}} |
| for _, f := range pkg.Files { |
| for _, d := range f.Decls { |
| if fd, ok := d.(*ast.FuncDecl); ok && fd.Name != nil && fd.Body != nil { |
| - funcs[fd.Name.Name] = fd |
| + src.funcs[fd.Name.Name] = fd |
| } |
| } |
| } |
| - reg, ok := funcs["registerCLIRoutes"] |
| - if !ok { |
| - t.Fatal("registerCLIRoutes not found; this test is reading the wrong package") |
| + for _, f := range pkg.Files { |
| + for _, d := range f.Decls { |
| + fd, ok := d.(*ast.FuncDecl) |
| + if !ok || fd.Body == nil { |
| + continue |
| + } |
| + // cliRoute is the gate itself. Its own mux.HandleFunc call is |
| + // the one registration in the package whose pattern is a |
| + // variable rather than a literal, and it is the mechanism these |
| + // tests are asserting ABOUT — reading it as a door would be |
| + // reading the lock as a way in. It is the only exemption. |
| + if fd.Name != nil && fd.Name.Name == "cliRoute" { |
| + continue |
| + } |
| + src.inspect(t, fd.Body) |
| + } |
| } |
| + return src |
| +} |
| |
| - var named []string |
| - ast.Inspect(reg.Body, func(n ast.Node) bool { |
| +func (src *cliSource) inspect(t *testing.T, body *ast.BlockStmt) { |
| + t.Helper() |
| + ast.Inspect(body, func(n ast.Node) bool { |
| call, ok := n.(*ast.CallExpr) |
| if !ok { |
| return true |
| } |
| sel, ok := call.Fun.(*ast.SelectorExpr) |
| - if !ok || sel.Sel.Name != "HandleFunc" || len(call.Args) != 2 { |
| - return true |
| - } |
| - lit, ok := call.Args[0].(*ast.BasicLit) |
| - if !ok || lit.Kind != token.STRING { |
| - t.Errorf("mux.HandleFunc at %s: pattern is not a string literal, so this test cannot classify it", |
| - fset.Position(call.Pos())) |
| - return true |
| - } |
| - pattern, err := strconv.Unquote(lit.Value) |
| - if err != nil { |
| - t.Errorf("mux.HandleFunc at %s: unquote %s: %v", fset.Position(call.Pos()), lit.Value, err) |
| + if !ok { |
| return true |
| } |
| - if strings.Contains(pattern, "/api/cli/") { |
| - t.Errorf("%s: registered with mux.HandleFunc, bypassing the role gate", pattern) |
| - return true |
| + switch sel.Sel.Name { |
| + case "cliRoute": |
| + if len(call.Args) != 3 { |
| + t.Errorf("cliRoute at %s takes %d arguments; this test expects (pattern, allow, handler)", |
| + src.fset.Position(call.Pos()), len(call.Args)) |
| + return true |
| + } |
| + pattern, ok := stringLit(call.Args[0]) |
| + if !ok { |
| + t.Errorf("cliRoute at %s: pattern is not a string literal, so this test cannot classify it", |
| + src.fset.Position(call.Pos())) |
| + return true |
| + } |
| + src.gated = append(src.gated, pattern) |
| + if !strings.Contains(pattern, "/api/cli/") { |
| + t.Errorf("%s: registered through cliRoute but is not under /api/cli/", pattern) |
| + } |
| + case "Handle", "HandleFunc": |
| + pattern, isLit := stringLit(call.Args[0]) |
| + // The bypass guard, and the reason this pass is package-wide: an |
| + // /api/cli/ door registered anywhere but cliRoute names no roles |
| + // and is therefore gated by nothing at all. |
| + if isLit && strings.Contains(pattern, "/api/cli/") { |
| + t.Errorf("%s at %s: registered with %s, bypassing the role gate", |
| + pattern, src.fset.Position(call.Pos()), sel.Sel.Name) |
| + return true |
| + } |
| + if !isMuxRegistration(sel) { |
| + return true |
| + } |
| + if !isLit { |
| + t.Errorf("mux.%s at %s: pattern is not a string literal, so this test cannot classify it", |
| + sel.Sel.Name, src.fset.Position(call.Pos())) |
| + return true |
| + } |
| + if len(call.Args) == 2 { |
| + src.plain = append(src.plain, muxDoor{pattern: pattern, handler: call.Args[1]}) |
| + } |
| } |
| - switch h := call.Args[1].(type) { |
| + return true |
| + }) |
| +} |
| + |
| +// isMuxRegistration reports whether a selector is a call on something's `mux` |
| +// field — s.mux.HandleFunc and friends — as opposed to any other method that |
| +// happens to share the name. |
| +func isMuxRegistration(sel *ast.SelectorExpr) bool { |
| + recv, ok := sel.X.(*ast.SelectorExpr) |
| + return ok && recv.Sel.Name == "mux" |
| +} |
| + |
| +// stringLit unwraps an argument that is a plain quoted string. |
| +func stringLit(e ast.Expr) (string, bool) { |
| + lit, ok := e.(*ast.BasicLit) |
| + if !ok || lit.Kind != token.STRING { |
| + return "", false |
| + } |
| + s, err := strconv.Unquote(lit.Value) |
| + if err != nil { |
| + return "", false |
| + } |
| + return s, true |
| +} |
| + |
| +// TestAuthCLIFamilyResolvesOnlyPersons is the OTHER half of default-deny. |
| +// The /auth/cli/ handlers — whoami, revoke, tokens/revoke, accounts (which |
| +// creates an ACCOUNT) — sit outside cliRoute and need no role bindings, |
| +// because every one of them that reads a bearer at all gates on cliUser, |
| +// which answers only for a person's token. That is currently true by |
| +// construction; this makes it true by test, so the day someone reaches for |
| +// CLIPrincipal over there the build says so. |
| +// |
| +// It asserts over EVERY door the package registers outside /api/cli/, not |
| +// just the CLI family, because the property is the same one everywhere: a |
| +// handler the role gate does not stand in front of must not be resolving a |
| +// service credential. A browser page passes it trivially, which is the |
| +// correct answer for a browser page. |
| +func TestAuthCLIFamilyResolvesOnlyPersons(t *testing.T) { |
| + src := scanCLISource(t) |
| + for _, d := range src.plain { |
| + switch h := d.handler.(type) { |
| case *ast.SelectorExpr: |
| - named = append(named, h.Sel.Name) |
| - assertResolvesOnlyPersons(t, fset, pattern, funcs[h.Sel.Name]) |
| + assertResolvesOnlyPersons(t, src, d.pattern, src.funcs[h.Sel.Name]) |
| case *ast.Ident: |
| - named = append(named, h.Name) |
| - assertResolvesOnlyPersons(t, fset, pattern, funcs[h.Name]) |
| + assertResolvesOnlyPersons(t, src, d.pattern, src.funcs[h.Name]) |
| case *ast.FuncLit: |
| - assertBodyResolvesOnlyPersons(t, fset, pattern, h.Body) |
| + assertBodyResolvesOnlyPersons(t, src, d.pattern, h.Body) |
| default: |
| - t.Errorf("%s: handler is neither a named function nor a literal; this test cannot see inside it", pattern) |
| + t.Errorf("%s: handler is neither a named function nor a literal; this test cannot see inside it", d.pattern) |
| + } |
| + } |
| + |
| + // A concrete presence check rather than a count: the CLI identity family |
| + // is the part of this surface the assertion exists for, so name it. If |
| + // the walker breaks, or one of these moves under cliRoute without the |
| + // move being intended, this says so — where a minimum-length guard would |
| + // have gone on passing. |
| + seen := map[string]bool{} |
| + for _, d := range src.plain { |
| + seen[d.pattern] = true |
| + } |
| + for _, pattern := range []string{ |
| + "POST /auth/cli/start", |
| + "POST /auth/cli/poll", |
| + "GET /auth/cli", |
| + "POST /auth/cli/approve", |
| + "GET /settings", |
| + "GET /auth/cli/tokens", |
| + "POST /auth/cli/tokens/revoke", |
| + "GET /auth/cli/whoami", |
| + "POST /auth/cli/revoke", |
| + "POST /auth/cli/accounts", |
| + } { |
| + if !seen[pattern] { |
| + t.Errorf("%s: not found as a plain mux registration; either it moved or the walker is broken", pattern) |
| } |
| - return true |
| - }) |
| - if len(named) < 8 { |
| - t.Fatalf("found only %d named non-/api/cli/ handlers; the walker is broken, not the code", len(named)) |
| } |
| |
| // The positive half: the three doors of the family that DO read a |
| // bearer must read it through cliUser. Without this the test above |
| // would still pass if a handler stopped authenticating altogether. |
| for _, name := range []string{"handleCLIWhoami", "handleCLIRevoke", "handleCLINewAccount"} { |
| - fd, ok := funcs[name] |
| + fd, ok := src.funcs[name] |
| if !ok { |
| t.Errorf("%s: not found in the package", name) |
| continue |
| @@ -302,13 +400,13 @@ func TestAuthCLIFamilyResolvesOnlyPersons(t *testing.T) { |
| |
| // assertResolvesOnlyPersons is assertBodyResolvesOnlyPersons with the lookup |
| // of a named handler folded in. |
| -func assertResolvesOnlyPersons(t *testing.T, fset *token.FileSet, what string, fd *ast.FuncDecl) { |
| +func assertResolvesOnlyPersons(t *testing.T, src *cliSource, what string, fd *ast.FuncDecl) { |
| t.Helper() |
| if fd == nil { |
| t.Errorf("%s: handler function not found in the package", what) |
| return |
| } |
| - assertBodyResolvesOnlyPersons(t, fset, what, fd.Body) |
| + assertBodyResolvesOnlyPersons(t, src, what, fd.Body) |
| } |
| |
| // assertBodyResolvesOnlyPersons fails if a handler outside /api/cli/ resolves |
| @@ -316,7 +414,7 @@ func assertResolvesOnlyPersons(t *testing.T, fset *token.FileSet, what string, f |
| // SERVICE credential and CLITokenAddress is the raw resolver cliUser wraps; |
| // a handler over here calling either one has stopped being person-only, and |
| // that is precisely the drift this catches. |
| -func assertBodyResolvesOnlyPersons(t *testing.T, fset *token.FileSet, what string, body *ast.BlockStmt) { |
| +func assertBodyResolvesOnlyPersons(t *testing.T, src *cliSource, what string, body *ast.BlockStmt) { |
| t.Helper() |
| if body == nil { |
| return |
| @@ -329,7 +427,7 @@ func assertBodyResolvesOnlyPersons(t *testing.T, fset *token.FileSet, what strin |
| switch sel.Sel.Name { |
| case "CLIPrincipal", "CLITokenAddress", "principalFrom": |
| t.Errorf("%s: resolves a bearer through %s at %s — the /auth/cli/ family must go through cliUser, which answers only for a person", |
| - what, sel.Sel.Name, fset.Position(sel.Pos())) |
| + what, sel.Sel.Name, src.fset.Position(sel.Pos())) |
| } |
| return true |
| }) |
| @@ -423,6 +521,46 @@ func TestAdminIsRefusedAtAClosedDoor(t *testing.T) { |
| } |
| } |
| |
| +// TestClosedDoorRefusalIsItsOwnMessage: a door no role reaches must not say |
| +// "this door needs one of []". That renders from the general refusal when |
| +// Roles() is empty, and it misleads — it implies some role exists that would |
| +// work, and sends whoever reads it looking for the role that opens the mint |
| +// door. The closed-door answer is its own sentence, and it is CONSTANT: the |
| +// same bytes for every role and every account segment, so it discloses |
| +// neither what the caller holds nor whether the account it named is real. |
| +func TestClosedDoorRefusalIsItsOwnMessage(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + now := time.Now() |
| + if err := fx.st.CreateAccount(42, "cdef", "widgets", "bob@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + admin := serviceToken(t, fx.st, OpsAccountID, "root", "", svcrole.Admin) |
| + shipper := serviceToken(t, fx.st, OpsAccountID, "shipper", "kass", svcrole.Publish) |
| + |
| + var bodies []string |
| + for _, c := range []struct{ tok, acct string }{ |
| + {admin, fx.sqid}, {shipper, fx.sqid}, {admin, "cdef"}, {shipper, "zzzz"}, |
| + } { |
| + status, body := postRaw(t, fx.ts.URL+"/api/cli/apps/"+c.acct+"/kass/instance-token", |
| + map[string]string{}, c.tok) |
| + if status != http.StatusForbidden { |
| + t.Fatalf("closed door = %d %s, want 403", status, body) |
| + } |
| + bodies = append(bodies, body) |
| + } |
| + for i := 1; i < len(bodies); i++ { |
| + if bodies[i] != bodies[0] { |
| + t.Errorf("closed-door refusal varies with the caller or the account:\n %s\n %s", bodies[0], bodies[i]) |
| + } |
| + } |
| + if strings.Contains(bodies[0], "[]") || strings.Contains(bodies[0], "needs one of") { |
| + t.Errorf("closed-door refusal = %s; it must not offer a role list, there is no role that works", bodies[0]) |
| + } |
| + if !strings.Contains(bodies[0], "closed to service credentials") { |
| + t.Errorf("closed-door refusal = %s; it should say the door is closed to service credentials", bodies[0]) |
| + } |
| +} |
| + |
| // TestPersonNeverMeetsTheRoleGate: a person's authority is their membership |
| // set, evaluated per request by the handler. The role gate is for service |
| // principals only and must be invisible to a human at a terminal — including |
| @@ -434,11 +572,26 @@ func TestPersonNeverMeetsTheRoleGate(t *testing.T) { |
| fx.ts.URL + "/api/cli/apps/" + fx.sqid + "/kass/ledgers", |
| fx.ts.URL + "/api/cli/apps/" + fx.sqid + "/kass/config", |
| fx.ts.URL + "/api/cli/apps/" + fx.sqid + "/kass/store", |
| + // The zero-Set doors are the ones that matter here. They refuse |
| + // EVERY role, so a gate that mistook a person for a service |
| + // principal would lock every human out of `carlos instances token` |
| + // and `carlos fleets` — the loudest possible regression, and the |
| + // one a test named for the person's exemption should be the place |
| + // to find covered. |
| + fx.ts.URL + "/api/cli/fleets/" + fx.sqid, |
| } { |
| if status, body := getRaw(t, u, tok); status == http.StatusForbidden { |
| t.Errorf("person at %s = 403 %s; the role gate must not see a person at all", u, body) |
| } |
| } |
| + for _, u := range []string{ |
| + fx.ts.URL + "/api/cli/apps/" + fx.sqid + "/kass/instance-token", |
| + fx.ts.URL + "/api/cli/fleets/" + fx.sqid + "/new", |
| + } { |
| + if status, body := postRaw(t, u, map[string]string{"name": "edge"}, tok); status == http.StatusForbidden { |
| + t.Errorf("person at %s = 403 %s; a closed door is closed to SERVICE credentials, not to people", u, body) |
| + } |
| + } |
| } |
| |
| // TestCLIRouteUnauthorizedMatchesCLIUser: the gate's 401 and cliUser's 401 |
amadan