forked from molecule-ai/molecule-core
fix(auth): tighten AdminAuth to reject workspace bearer tokens when ADMIN_TOKEN is set (#684)
Blast-radius isolation gap: AdminAuth called ValidateAnyToken which accepted any live workspace bearer token. A compromised workspace agent could present its own token to GET /admin/github-installation-token and steal the platform's GitHub App credential, or hit /approvals/pending to enumerate cross-workspace approvals. Fix: introduce a dedicated admin credential tier via ADMIN_TOKEN env var. When set, AdminAuth verifies the bearer against that secret exclusively (crypto/subtle constant-time comparison). Workspace tokens are rejected outright — no DB lookup occurs. When ADMIN_TOKEN is not set the previous behaviour is preserved as a deprecated backward-compat fallback (tier 3) so existing deployments without the env var don't break immediately. Credential tiers (evaluated in order): 1. Fail-open — no live tokens globally (fresh install / pre-Phase-30) 2. ADMIN_TOKEN match — env var set, bearer must equal it exactly 3. Fallback (deprecated) — any valid workspace token (ADMIN_TOKEN unset) Operators should set ADMIN_TOKEN=<openssl rand -base64 32> to fully close the blast-radius gap. Tier 3 will be removed in a future release. Fixes #684. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
63212130e3
commit
6259e69b42
@ -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()
|
||||
|
||||
@ -891,3 +891,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)
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
Reference in New Issue
Block a user