diff --git a/docs/architecture.md b/docs/architecture.md index 4e3eacf98..bf93dae9c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -265,7 +265,8 @@ GET /v1/admin/azure-orphan-sweep POST /v1/admin/azure-orphan-sweep ``` -`GET /v1/pool` and `/v1/admin/*` require the admin token. User tokens scope +`GET /v1/pool` and `/v1/admin/*` require admin authorization, from the admin token +or an existing immutable-owner GitHub admin grant. Non-admin user tokens scope list, lookup, heartbeat, release, run mutation, and usage to the token's owner/org. Run reads also permit every recorded backing lease owner so shared-lease and replacement activity remains auditable without granting those diff --git a/docs/commands/admin.md b/docs/commands/admin.md index c24a3138c..1782ef0d3 100644 --- a/docs/commands/admin.md +++ b/docs/commands/admin.md @@ -4,7 +4,7 @@ ## Requirements -Every `admin` subcommand needs both a configured coordinator and a separate admin bearer token. The token is read from `broker.adminToken` in config or the `CRABBOX_COORDINATOR_ADMIN_TOKEN` environment variable. The ordinary operator/shared token (`broker.token` / `CRABBOX_COORDINATOR_TOKEN`) is not sufficient for admin routes — commands fail with a configuration error when only the shared token is present. +Coordinator-backed `admin` commands require server-authorized admin access. An explicit `broker.adminToken` or `CRABBOX_COORDINATOR_ADMIN_TOKEN` takes precedence over the normal broker credential and token command. Otherwise, the CLI uses its configured broker authentication, including a GitHub login whose immutable owner has an existing `CRABBOX_GITHUB_ADMIN_OWNERS` grant. The coordinator still rejects shared tokens and GitHub users without that grant; the CLI does not grant privileges or retry a denied explicit admin token with another credential. Policy-printing commands run locally without authentication. ## At a glance diff --git a/docs/commands/image.md b/docs/commands/image.md index b52e0d3b4..106141a10 100644 --- a/docs/commands/image.md +++ b/docs/commands/image.md @@ -23,11 +23,12 @@ crabbox image delete 123456789 --provider hetzner --region fsn1 ``` Every `image` subcommand except direct Hetzner snapshot deletion requires a -configured coordinator (broker) **and** admin-token auth. Set `broker.adminToken` or `CRABBOX_COORDINATOR_ADMIN_TOKEN` -locally; the Worker validates it against `CRABBOX_ADMIN_TOKEN`. Without an admin -token the command exits early with `admin command requires broker.adminToken or -CRABBOX_COORDINATOR_ADMIN_TOKEN`. These commands are intentionally unavailable -to normal GitHub browser-login users. +configured coordinator (broker) and server-authorized admin access. An explicit +`broker.adminToken` or `CRABBOX_COORDINATOR_ADMIN_TOKEN` takes precedence; +otherwise the CLI uses its configured broker authentication. GitHub login works +when the coordinator already grants that immutable owner admin access through +`CRABBOX_GITHUB_ADMIN_OWNERS`. Shared tokens and GitHub users without that grant +remain unauthorized. See [admin](admin.md#requirements). Image bytes live in the provider account, never in git or coordinator durable state. AWS images are AMIs backed by EBS snapshots; Azure promotion uses diff --git a/docs/features/auth-admin.md b/docs/features/auth-admin.md index 904424563..e6eaf7036 100644 --- a/docs/features/auth-admin.md +++ b/docs/features/auth-admin.md @@ -114,8 +114,11 @@ The broker matches an incoming bearer token in this precedence: On the CLI side the admin token is read from `broker.adminToken` in config or the `CRABBOX_COORDINATOR_ADMIN_TOKEN` / `CRABBOX_ADMIN_TOKEN` environment variable; the normal broker token comes from `broker.token` or -`CRABBOX_COORDINATOR_TOKEN`. Admin commands fail fast if no admin token is -configured. +`CRABBOX_COORDINATOR_TOKEN`. Admin commands prefer the explicit admin token, +disabling the normal token command when it is present. Otherwise they use the +normal configured authentication, including a GitHub session with an existing +immutable-owner admin grant. Authorization remains with the broker, and a denied +explicit admin token does not trigger a retry with the normal credential. Never distribute the shared or admin token to untrusted users. Keep the admin token narrower and more closely held than the shared automation token. @@ -162,9 +165,9 @@ POST /v1/leases/{id}/tailscale owner, manage share, or admin GET/PUT/DELETE /v1/leases/{id}/share owner, manage share, or admin GET /v1/runs and logs/events own runs only GET /v1/usage own usage only -GET /v1/pool admin token only -POST /v1/leases with hostId admin token only -/v1/admin/* admin token only +GET /v1/pool admin authorization +POST /v1/leases with hostId admin, or owner's retained Mac host +/v1/admin/* admin authorization ``` A lease is **visible** to a caller who is the owner (matching immutable owner @@ -207,7 +210,7 @@ keys between users. ## Trusted-operator and admin commands -These require the admin token: +These require admin authorization: ```sh crabbox admin leases --state active diff --git a/docs/features/broker-auth-routing.md b/docs/features/broker-auth-routing.md index 14c00ecce..61971e337 100644 --- a/docs/features/broker-auth-routing.md +++ b/docs/features/broker-auth-routing.md @@ -129,8 +129,8 @@ callers. Raw, unverified Cloudflare Access email headers are stripped and never The Node-specific alternative is trusted reverse-proxy identity. Requests from configured proxy CIDRs may use `CRABBOX_TRUSTED_USER_HEADER` without a Crabbox -bearer token. The resulting identity is non-admin; admin routes still require -`CRABBOX_ADMIN_TOKEN`. +bearer token. The resulting identity is non-admin; admin routes require a +separate admin token or an authorized GitHub admin session. ## GitHub browser login @@ -287,8 +287,8 @@ smoke additionally proves the same route can lease, run, and release a real mach routes; Node may use any TLS/WebSocket-capable ingress. - The Access service token only clears Cloudflare Access; it is not a Crabbox admin token. - Trusted proxy identity is Node-only, CIDR-gated, and never grants admin. -- Signed GitHub user tokens are never admin tokens — admin routes require the separate admin - token. +- Signed GitHub user tokens grant admin access only when their immutable owner + matches the coordinator's `CRABBOX_GITHUB_ADMIN_OWNERS` grant. ## Related docs diff --git a/internal/cli/admin.go b/internal/cli/admin.go index ee4d975f1..055492dba 100644 --- a/internal/cli/admin.go +++ b/internal/cli/admin.go @@ -724,11 +724,10 @@ func configuredAdminCoordinator() (*CoordinatorClient, error) { if err != nil { return nil, err } - if cfg.CoordAdminToken == "" { - return nil, exit(2, "admin command requires broker.adminToken or CRABBOX_COORDINATOR_ADMIN_TOKEN") + if cfg.CoordAdminToken != "" { + cfg.CoordToken = cfg.CoordAdminToken + cfg.CoordTokenCommand = nil } - cfg.CoordToken = cfg.CoordAdminToken - cfg.CoordTokenCommand = nil coord, ok, err := newCoordinatorClient(cfg) if err != nil { return nil, err @@ -736,5 +735,9 @@ func configuredAdminCoordinator() (*CoordinatorClient, error) { if !ok { return nil, exit(2, "admin command requires a configured coordinator") } + // Reject missing credentials before callers perform guest-side preparation. + if !coord.hasConfiguredAuth() { + return nil, exit(2, "admin command requires broker authentication; run crabbox login or configure broker credentials") + } return coord, nil } diff --git a/internal/cli/admin_test.go b/internal/cli/admin_test.go index 4861f0bba..693a4ffe5 100644 --- a/internal/cli/admin_test.go +++ b/internal/cli/admin_test.go @@ -4,13 +4,113 @@ import ( "bytes" "context" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" ) +func TestAdminLeasesUsesConfiguredAuthorization(t *testing.T) { + for _, tt := range []struct { + name string + noCredentials bool + tokenCommand bool + adminToken string + responseCode int + }{ + {name: "missing broker credentials", noCredentials: true, responseCode: http.StatusOK}, + {name: "configured session", responseCode: http.StatusOK}, + {name: "configured token command", tokenCommand: true, responseCode: http.StatusOK}, + {name: "explicit admin token overrides session", adminToken: "explicit-admin", responseCode: http.StatusOK}, + {name: "explicit admin token overrides command", tokenCommand: true, adminToken: "explicit-admin", responseCode: http.StatusOK}, + {name: "server denies non-admin session", responseCode: http.StatusForbidden}, + {name: "denied explicit token does not fall back", adminToken: "denied-admin", responseCode: http.StatusForbidden}, + } { + t.Run(tt.name, func(t *testing.T) { + clearConfigEnv(t) + t.Setenv("HOME", t.TempDir()) + t.Setenv("XDG_CONFIG_HOME", t.TempDir()) + requests := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + wantToken := "configured-session" + if tt.adminToken != "" { + wantToken = tt.adminToken + } + if r.Method != http.MethodGet || r.URL.Path != "/v1/admin/leases" || r.Header.Get("Authorization") != "Bearer "+wantToken { + t.Errorf("unexpected admin request: %s %s, authorization matched=%t", r.Method, r.URL.Path, r.Header.Get("Authorization") == "Bearer "+wantToken) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(tt.responseCode) + if tt.responseCode == http.StatusForbidden { + _, _ = io.WriteString(w, `{"error":"forbidden"}`) + return + } + _, _ = io.WriteString(w, `{"leases":[]}`) + })) + defer server.Close() + broker := map[string]any{"url": server.URL, "token": "configured-session"} + if tt.noCredentials { + delete(broker, "token") + } + if tt.tokenCommand { + broker["token"] = "superseded-session" + command := []string{os.Args[0], "-test.run=^TestCoordinatorTokenCommandHelper$"} + t.Setenv("CRABBOX_TOKEN_HELPER", "1") + t.Setenv("CRABBOX_TOKEN_HELPER_VALUE", "configured-session") + if tt.adminToken != "" { + // An explicit admin credential must not execute the normal credential command. + command = []string{filepath.Join(t.TempDir(), "must-not-run")} + } + encodedCommand, err := json.Marshal(command) + if err != nil { + t.Fatal(err) + } + t.Setenv("CRABBOX_COORDINATOR_TOKEN_COMMAND", string(encodedCommand)) + } + if tt.adminToken != "" { + broker["adminToken"] = tt.adminToken + } + config, err := json.Marshal(map[string]any{"broker": broker}) + if err != nil { + t.Fatal(err) + } + configPath := filepath.Join(t.TempDir(), "config.yaml") + if err := os.WriteFile(configPath, config, 0600); err != nil { + t.Fatal(err) + } + t.Setenv("CRABBOX_CONFIG", configPath) + app := App{Stdout: io.Discard, Stderr: io.Discard} + err = app.adminLeases(context.Background(), []string{"--json"}) + if tt.noCredentials { + var exitErr ExitError + if !errors.As(err, &exitErr) || exitErr.Code != 2 || !strings.Contains(err.Error(), "broker authentication") { + t.Errorf("err=%v, want missing broker authentication before request", err) + } + if requests != 0 { + t.Errorf("requests=%d, want no unauthenticated admin request", requests) + } + return + } + if tt.responseCode == http.StatusForbidden { + var httpErr CoordinatorHTTPError + if !errors.As(err, &httpErr) || httpErr.StatusCode != http.StatusForbidden { + t.Fatalf("err=%v, want coordinator authorization denial", err) + } + } else if err != nil { + t.Fatal(err) + } + if requests != 1 { + t.Fatalf("requests=%d, want one authoritative admin request", requests) + } + }) + } +} + func TestAdminMacHostsRequiresForceForAllocate(t *testing.T) { app := App{Stdout: io.Discard, Stderr: io.Discard} err := app.adminMacHosts(context.Background(), []string{"allocate", "--availability-zone", "eu-west-1a"}) diff --git a/internal/cli/checkpoint_test.go b/internal/cli/checkpoint_test.go index babe7be4f..633a13363 100644 --- a/internal/cli/checkpoint_test.go +++ b/internal/cli/checkpoint_test.go @@ -2170,7 +2170,8 @@ func TestCheckpointCreateModeFallsBackToArchiveForSSH(t *testing.T) { } } -func TestCreateAWSAMICheckpointValidatesAdminBeforeCloudInit(t *testing.T) { +func TestCreateAWSAMICheckpointRejectsMissingBrokerAuthBeforeCloudInit(t *testing.T) { + clearConfigEnv(t) t.Setenv("CRABBOX_CONFIG", filepath.Join(t.TempDir(), "missing.yaml")) t.Setenv("CRABBOX_COORDINATOR", "https://coordinator.example") t.Setenv("CRABBOX_COORDINATOR_ADMIN_TOKEN", "") @@ -2181,11 +2182,9 @@ func TestCreateAWSAMICheckpointValidatesAdminBeforeCloudInit(t *testing.T) { cancel() _, err := (App{Stdout: io.Discard, Stderr: io.Discard}).createAWSAMICheckpoint(ctx, cfg, SSHTarget{TargetOS: targetLinux}, "cbx_123", "", "repo", true, false, 0) - if err == nil { - t.Fatal("expected missing admin token to fail") - } - if !strings.Contains(err.Error(), "adminToken") { - t.Fatalf("err=%v, want admin validation before cloud-init", err) + var exitErr ExitError + if !errors.As(err, &exitErr) || exitErr.Code != 2 || !strings.Contains(err.Error(), "broker authentication") { + t.Fatalf("err=%v, want missing broker authentication before cloud-init", err) } }