diff --git a/platform/internal/middleware/wsauth_middleware.go b/platform/internal/middleware/wsauth_middleware.go index 4cfd1d2c..aed59f33 100644 --- a/platform/internal/middleware/wsauth_middleware.go +++ b/platform/internal/middleware/wsauth_middleware.go @@ -1,6 +1,7 @@ package middleware import ( + "crypto/subtle" "database/sql" "log" "net/http" @@ -64,20 +65,31 @@ func WorkspaceAuth(database *sql.DB) gin.HandlerFunc { // AdminAuth returns a Gin middleware for global/admin routes (e.g. // /settings/secrets, /admin/secrets) that have no per-workspace scope. // -// Same lazy-bootstrap contract as WorkspaceAuth: if no live token exists -// anywhere on the platform (fresh install / pre-Phase-30 upgrade), requests -// are let through so existing deployments keep working. Once any workspace -// has a live token every request to these routes MUST present a valid bearer -// token — no Origin-based bypass. (#623) +// # Credential tier (evaluated in order) // -// Any valid workspace bearer token is accepted — the route is not scoped to -// a specific workspace so we only verify the token is live and unrevoked. +// 1. Lazy-bootstrap fail-open: if no live workspace token exists anywhere on +// the platform (fresh install / pre-Phase-30 upgrade), every request passes +// through so existing deployments keep working. +// +// 2. ADMIN_TOKEN env var (recommended, closes #684): when set, the bearer +// MUST equal this value exactly (constant-time comparison). Workspace +// bearer tokens are intentionally rejected even if valid — a compromised +// workspace agent must not be able to read global secrets, steal GitHub App +// installation tokens, or enumerate pending approvals across the platform. +// Set ADMIN_TOKEN to a strong random secret (e.g. openssl rand -base64 32). +// +// 3. Fallback — workspace token (deprecated, backward-compat): when +// ADMIN_TOKEN is not set and workspace tokens do exist globally, any valid +// workspace bearer token is still accepted. This preserves existing +// behaviour for deployments that have not yet configured ADMIN_TOKEN, but +// it leaves the blast-radius isolation gap described in #684 open. Set +// ADMIN_TOKEN to eliminate this fallback. // // NOTE: canvasOriginAllowed / isSameOriginCanvas are intentionally NOT called // here. The Origin header is trivially forgeable by any container on the // Docker network; using it as an auth bypass would let an attacker reach // /settings/secrets, /bundles/import, /events, etc. without a bearer token. -// Those short-circuits belong ONLY in CanvasOrBearer (cosmetic routes). +// Those short-circuits belong ONLY in CanvasOrBearer (cosmetic routes). (#623) func AdminAuth(database *sql.DB) gin.HandlerFunc { return func(c *gin.Context) { ctx := c.Request.Context() @@ -88,18 +100,34 @@ func AdminAuth(database *sql.DB) gin.HandlerFunc { c.AbortWithStatusJSON(http.StatusInternalServerError, gin.H{"error": "auth check failed"}) return } - if hasLive { - // Bearer token is the ONLY accepted credential for admin routes. - tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization")) - if tok != "" { - if err := wsauth.ValidateAnyToken(ctx, database, tok); err != nil { - c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"}) - return - } - c.Next() + if !hasLive { + // Tier 1: fail-open on fresh install / pre-Phase-30 upgrade. + c.Next() + return + } + + // Bearer token is the ONLY accepted credential for admin routes. + tok := wsauth.BearerTokenFromHeader(c.GetHeader("Authorization")) + if tok == "" { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "admin auth required"}) + return + } + + // Tier 2 (#684 fix): dedicated ADMIN_TOKEN — workspace bearer tokens + // must not grant access to admin routes. + if adminSecret := os.Getenv("ADMIN_TOKEN"); adminSecret != "" { + if subtle.ConstantTimeCompare([]byte(tok), []byte(adminSecret)) != 1 { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"}) return } - c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "admin auth required"}) + c.Next() + return + } + + // Tier 3 (deprecated): ADMIN_TOKEN not configured — fall back to any + // valid workspace token. Operators should set ADMIN_TOKEN to close #684. + if err := wsauth.ValidateAnyToken(ctx, database, tok); err != nil { + c.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "invalid admin auth token"}) return } c.Next() diff --git a/platform/internal/middleware/wsauth_middleware_test.go b/platform/internal/middleware/wsauth_middleware_test.go index df3f6786..740c574a 100644 --- a/platform/internal/middleware/wsauth_middleware_test.go +++ b/platform/internal/middleware/wsauth_middleware_test.go @@ -940,3 +940,233 @@ func TestAdminAuth_623_ValidBearer_WithOrigin_Passes(t *testing.T) { t.Errorf("unmet sqlmock expectations: %v", err) } } + +// ── Issue #684 — AdminAuth accepts any workspace bearer as admin credential ── +// +// Root cause: AdminAuth called ValidateAnyToken which matched any live +// workspace token. A compromised workspace agent could present its own bearer +// and reach /admin/github-installation-token, /approvals/pending, etc. +// +// Fix: when ADMIN_TOKEN env var is set the middleware verifies the bearer +// against that secret exclusively (constant-time). Workspace tokens are +// rejected even if valid. When ADMIN_TOKEN is not set the old behaviour is +// preserved for backward-compat (deprecated fallback, tier 3). + +// TestAdminAuth_684_AdminTokenSet_WorkspaceTokenRejected — the primary +// regression test: when ADMIN_TOKEN is configured, a valid workspace bearer +// token MUST be rejected with 401 on admin routes (#684). +func TestAdminAuth_684_AdminTokenSet_WorkspaceTokenRejected(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + t.Setenv("ADMIN_TOKEN", "super-secret-admin-token-xyz") + + // Platform has live workspace tokens — AdminAuth is active. + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + // ValidateAnyToken must NOT be called — workspace tokens must be rejected + // before any DB lookup when ADMIN_TOKEN is set. + + r := gin.New() + r.GET("/admin/github-installation-token", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"token": "ghp_live_token"}) + }) + + w := httptest.NewRecorder() + // #684 attack: compromised workspace agent sends its own bearer. + req, _ := http.NewRequest(http.MethodGet, "/admin/github-installation-token", nil) + req.Header.Set("Authorization", "Bearer some-valid-workspace-bearer-token") + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#684 workspace token w/ ADMIN_TOKEN set: expected 401, got %d: %s", + w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAdminAuth_684_AdminTokenSet_CorrectAdminTokenAccepted — when ADMIN_TOKEN +// is set, presenting the exact ADMIN_TOKEN value must grant access (200). +func TestAdminAuth_684_AdminTokenSet_CorrectAdminTokenAccepted(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + const adminSecret = "super-secret-admin-token-xyz" + t.Setenv("ADMIN_TOKEN", adminSecret) + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + // No DB token lookup — ADMIN_TOKEN check is env-only, no DB round-trip. + + r := gin.New() + r.GET("/admin/github-installation-token", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"token": "ghp_live_token"}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/admin/github-installation-token", nil) + req.Header.Set("Authorization", "Bearer "+adminSecret) + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("#684 correct ADMIN_TOKEN: expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAdminAuth_684_AdminTokenSet_WrongAdminToken_Returns401 — when ADMIN_TOKEN +// is set, presenting a different value must return 401. +func TestAdminAuth_684_AdminTokenSet_WrongAdminToken_Returns401(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + t.Setenv("ADMIN_TOKEN", "correct-admin-secret") + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + r := gin.New() + r.GET("/admin/liveness", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"subsystems": gin.H{}}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/admin/liveness", nil) + req.Header.Set("Authorization", "Bearer wrong-admin-secret") + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#684 wrong ADMIN_TOKEN: expected 401, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAdminAuth_684_AdminTokenSet_NoBearer_Returns401 — when ADMIN_TOKEN is +// set, a request with no bearer must still return 401. +func TestAdminAuth_684_AdminTokenSet_NoBearer_Returns401(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + t.Setenv("ADMIN_TOKEN", "correct-admin-secret") + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + r := gin.New() + r.GET("/approvals/pending", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"approvals": []interface{}{}}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/approvals/pending", nil) + r.ServeHTTP(w, req) + + if w.Code != http.StatusUnauthorized { + t.Errorf("#684 no bearer w/ ADMIN_TOKEN set: expected 401, got %d: %s", + w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAdminAuth_684_AdminTokenNotSet_FallsBackToWorkspaceToken — when +// ADMIN_TOKEN is NOT set, a valid workspace token is still accepted (deprecated +// tier-3 fallback for backward compatibility). +func TestAdminAuth_684_AdminTokenNotSet_FallsBackToWorkspaceToken(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + // ADMIN_TOKEN explicitly unset — tier-3 fallback active. + t.Setenv("ADMIN_TOKEN", "") + + workspaceToken := "any-live-workspace-token" + tokenHash := sha256.Sum256([]byte(workspaceToken)) + + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + mock.ExpectQuery(validateAnyTokenSelectQuery). + WithArgs(tokenHash[:]). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("tok-ws-1")) + + mock.ExpectExec(validateTokenUpdateQuery). + WithArgs("tok-ws-1"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + r := gin.New() + r.GET("/admin/secrets", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"ok": true}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/admin/secrets", nil) + req.Header.Set("Authorization", "Bearer "+workspaceToken) + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("#684 fallback (no ADMIN_TOKEN): expected 200, got %d: %s", + w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestAdminAuth_684_FailOpen_AdminTokenSet_NoGlobalTokens — even when +// ADMIN_TOKEN is set, a fresh install (no tokens globally) must still +// fail-open (tier-1 contract unchanged). +func TestAdminAuth_684_FailOpen_AdminTokenSet_NoGlobalTokens(t *testing.T) { + mockDB, mock, err := sqlmock.New() + if err != nil { + t.Fatalf("sqlmock.New: %v", err) + } + defer mockDB.Close() + + t.Setenv("ADMIN_TOKEN", "some-admin-secret") + + // HasAnyLiveTokenGlobal returns 0 — fresh install. + mock.ExpectQuery(hasAnyLiveTokenGlobalQuery). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + + r := gin.New() + r.GET("/admin/secrets", AdminAuth(mockDB), func(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"ok": true}) + }) + + w := httptest.NewRecorder() + req, _ := http.NewRequest(http.MethodGet, "/admin/secrets", nil) + // No bearer — but fail-open should still pass. + r.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Errorf("#684 fail-open w/ ADMIN_TOKEN set (no global tokens): expected 200, got %d: %s", + w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +}