diff --git a/README.md b/README.md index 38328b06..913a8f24 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,7 @@ Vault42 issues its own tokens and is an OAuth2 *client* of other providers. It i | ![Go](https://img.shields.io/badge/Go-1.26.6-00ADD8?style=flat&logo=go&logoColor=white) | ![Vue](https://img.shields.io/badge/Vue-3.5.41-4FC08D?style=flat&logo=vuedotjs&logoColor=white) | ![.NET](https://img.shields.io/badge/.NET-10.0-512BD4?style=flat&logo=dotnet&logoColor=white) | ![License](https://img.shields.io/badge/License-MIT-155724?style=flat&labelColor=000) | | ![Go Tests](https://img.shields.io/badge/Tests-4910-155724?style=flat&labelColor=000) | ![Vue Tests](https://img.shields.io/badge/Tests-1305-155724?style=flat&labelColor=000) | ![C# Tests](https://img.shields.io/badge/Tests-264-155724?style=flat&labelColor=000) | ![Total](https://img.shields.io/badge/Total-6479_tests-155724?style=flat&labelColor=000) | | ![Go Coverage](https://img.shields.io/badge/Coverage-100.00%25_reachable-155724?style=flat&labelColor=000) | ![Vue Coverage](https://img.shields.io/badge/Coverage-99.76%25-155724?style=flat&labelColor=000) | ![C# Coverage](https://img.shields.io/badge/Coverage-100.00%25-155724?style=flat&labelColor=000) | ![Locales](https://img.shields.io/badge/Locales-38-555?style=flat&labelColor=000) | -| ![Go Lines](https://img.shields.io/badge/Lines-48129-555?style=flat&labelColor=000) | ![Vue Lines](https://img.shields.io/badge/Lines-6800-555?style=flat&labelColor=000) | ![C# Lines](https://img.shields.io/badge/Lines-2435-555?style=flat&labelColor=000) | ![Standards](https://img.shields.io/badge/Standards-11-555?style=flat&labelColor=000) | +| ![Go Lines](https://img.shields.io/badge/Lines-48153-555?style=flat&labelColor=000) | ![Vue Lines](https://img.shields.io/badge/Lines-6800-555?style=flat&labelColor=000) | ![C# Lines](https://img.shields.io/badge/Lines-2435-555?style=flat&labelColor=000) | ![Standards](https://img.shields.io/badge/Standards-11-555?style=flat&labelColor=000) | | ![Go Deps](https://img.shields.io/badge/Deps-3-555?style=flat&labelColor=000) | ![Vue Deps](https://img.shields.io/badge/Deps-3-555?style=flat&labelColor=000) | ![C# Deps](https://img.shields.io/badge/Deps-6-555?style=flat&labelColor=000) | ![Requirements](https://img.shields.io/badge/Requirements-456-555?style=flat&labelColor=000) | | ![Go Transitive Deps](https://img.shields.io/badge/Transitive-15-555?style=flat&labelColor=000) | ![Vue Transitive Deps](https://img.shields.io/badge/Transitive-95-555?style=flat&labelColor=000) | ![C# Transitive Deps](https://img.shields.io/badge/Transitive-26-555?style=flat&labelColor=000) | ![Total Deps](https://img.shields.io/badge/Deps-148_total-555?style=flat&labelColor=000) | diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 910e6b2d..51606ab2 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -32,12 +32,17 @@ chart. **Secret.** No new keys. The Deployment mounts the same eight it did in 1.0.3. -**Schema.** v1.0.3 shipped 39 migrations; this release ships 39, so an upgrade applies 0. -There is no schema change in 1.0.4 and nothing to migrate. Take the backup anyway: the point +**Schema.** v1.0.3 shipped 39 migrations; this release ships 40, so an upgrade applies 1. +042 changes `auth.admin_users.created_by` from `NO ACTION` to `ON DELETE SET NULL`. It moves +no data and adds no column. Until it runs, `POST /admin/admins/{id}/revoke` answers 500 for +any admin who has created another admin, and that account and its live sessions stay -- +revoke is the only containment lever the admin plane has. Take the backup anyway: the point of step 1 is that you have one when something else goes wrong. -**Behaviour.** None. 1.0.4 is documentation, three generator fixes and the gates that hold -them; no request path, token, database or configuration behaviour differs from 1.0.3. The +**Behaviour.** One fix, no new surface: `POST /admin/admins/{id}/revoke` now succeeds +against an admin who has created other admins, and those accounts survive with their +`created_by` cleared -- the account that authorized them is recorded in the revoke's audit +row instead. No route, token or configuration changes shape. The one thing an operator may notice is that `docs/deps.md` and the README badge figures now report numbers that differ slightly from 1.0.3's, because those were being counted wrongly -- see the 1.0.4 entry in [CHANGELOG.md](../CHANGELOG.md). diff --git a/docs/admin-gateway.md b/docs/admin-gateway.md index 5c49bfa2..99d8bb48 100644 --- a/docs/admin-gateway.md +++ b/docs/admin-gateway.md @@ -312,7 +312,7 @@ The UPDATE half is a real ceiling: it compares against `OLD.role`, which comes f - **TOTP secrets**: Encrypted at rest with AES-256 (master key) - **TOTP replay prevention**: Each accepted TOTP code's time-step counter is stored per admin. Replayed codes (same or earlier counter) are rejected within the ±1 period window - **Account lockout**: Configurable failed attempts threshold and lockout duration. Lockout counter is atomic (SQL `RETURNING` clause) -- immune to race conditions under concurrent login attempts -- **Admin revocation**: Deleting an admin CASCADE deletes all sessions -- no race window between session revoke and admin revoke +- **Admin revocation**: Deleting an admin CASCADE deletes all sessions -- no race window between session revoke and admin revoke. `created_by` is `ON DELETE SET NULL` (migration 042) so revoking an admin who opened other accounts succeeds and those accounts survive with their provenance moved into the audit row; before 042 it was `NO ACTION`, and revoking any admin who had created another failed with a 500 that left the account and its live sessions in place - **Audit trail**: All admin mutations logged with admin ID, timestamp, IP, user agent - **No external dependencies**: Uses stdlib HTTP only (no frameworks) - **RBAC hardcoded**: Permission maps defined in Go code, not database -- immune to SQL injection escalation diff --git a/docs/badges.json b/docs/badges.json index f8886141..f62f403b 100644 --- a/docs/badges.json +++ b/docs/badges.json @@ -9,8 +9,8 @@ "reachableCoverageNum": 100.00, "packages": 43, "goFiles": 192, - "goLines": 48129, - "testFiles": 927, + "goLines": 48153, + "testFiles": 929, "directDeps": 3, "transitiveDeps": 15, "totalTests": 6479, @@ -20,7 +20,7 @@ "tests": 4910, "coverage": "100.00% of reachable", "coverageNum": 100.00, - "lines": 48129, + "lines": 48153, "deps": 3, "transitiveDeps": 15 }, diff --git a/internal/adminapi/admin_revoke_audit_test.go b/internal/adminapi/admin_revoke_audit_test.go new file mode 100644 index 00000000..10018a1c --- /dev/null +++ b/internal/adminapi/admin_revoke_audit_test.go @@ -0,0 +1,82 @@ +package adminapi + +import ( + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/42-v/vault42/internal/model" +) + +// Revoking an admin who opened other accounts nulls their created_by, so the +// answer to "who authorized this account" leaves the row. It has to land +// somewhere, and the audit trail is the only place left. +func TestRevokeAdmin_RecordsTheProvenanceItIsAboutToErase(t *testing.T) { + const target = "00000000-0000-0000-0000-0000000000aa" + + repo := newFakeAdminRepo() + repo.users[target] = &model.AdminUser{ + ID: target, Username: "doomed", Role: "viewer", CreatedBy: "root", + } + + var captured []*model.AuditEntry + h := &Handler{admins: repo, auditLog: captureAudit(&captured)} + + rec := httptest.NewRecorder() + r := withActor(httptest.NewRequest(http.MethodPost, "/admin/admins/"+target+"/revoke", nil)) + r.SetPathValue("id", target) + h.RevokeAdmin(rec, r) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d: %s", rec.Code, rec.Body.String()) + } + if len(captured) == 0 { + t.Fatal("no audit entry for a successful revoke") + } + last := captured[len(captured)-1] + if got := last.Metadata["revoked_admin_created_by"]; got != "root" { + t.Errorf("revoked_admin_created_by = %v, want \"root\". Migration 042 nulls that "+ + "column on the accounts the revoked admin opened, so this row is where the "+ + "provenance survives.", got) + } + if got := last.Metadata["outcome"]; got != "revoked" { + t.Errorf("outcome = %v, want \"revoked\"", got) + } +} + +// A revoke that fails is the event an operator most needs to see: this route is +// the only containment lever there is, so a 500 means a compromised account is +// still live and nothing else can stop it. It used to return before the audit +// call and leave no trace at all. +func TestRevokeAdmin_AuditsAFailedContainmentAttempt(t *testing.T) { + const target = "00000000-0000-0000-0000-0000000000bb" + + repo := newFakeAdminRepo() + repo.users[target] = &model.AdminUser{ID: target, Username: "doomed", Role: "viewer"} + repo.errRevoke = errors.New("violates foreign key constraint") + + var captured []*model.AuditEntry + h := &Handler{admins: repo, auditLog: captureAudit(&captured)} + + rec := httptest.NewRecorder() + r := withActor(httptest.NewRequest(http.MethodPost, "/admin/admins/"+target+"/revoke", nil)) + r.SetPathValue("id", target) + h.RevokeAdmin(rec, r) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", rec.Code) + } + if len(captured) == 0 { + t.Fatal("a failed revoke wrote no audit row at all. The operator is told the request " + + "failed and the trail says the attempt never happened.") + } + last := captured[len(captured)-1] + if got := last.Metadata["outcome"]; got != "failed" { + t.Errorf("outcome = %v, want \"failed\"", got) + } + if got, _ := last.Metadata["error"].(string); got == "" { + t.Error("the audit row carries no reason, so it cannot distinguish a constraint " + + "violation from the database being down") + } +} diff --git a/internal/adminapi/handler.go b/internal/adminapi/handler.go index 3a6662b1..651e5dd3 100644 --- a/internal/adminapi/handler.go +++ b/internal/adminapi/handler.go @@ -1121,17 +1121,41 @@ func (h *Handler) RevokeAdmin(w http.ResponseWriter, r *http.Request) { return } + // Read the row before deleting it. 042 makes created_by ON DELETE SET NULL, + // so revoking a creator nulls that column on every account it opened; this + // is where the answer to "who authorized this account" is preserved. A read + // failure is not fatal -- containment matters more than provenance -- so the + // revoke proceeds with the field absent rather than refusing. + var revokedCreatedBy string + if target, err := h.admins.GetByID(r.Context(), id); err == nil && target != nil { + revokedCreatedBy = target.CreatedBy + } + // Revoke admin first — sessions CASCADE delete via FK. // This eliminates the race window where an in-flight request could // pass SessionAuth between session revoke and admin revoke. if err := h.admins.Revoke(r.Context(), id); err != nil { + // Audited on the way out. A failed revoke is the event an operator most + // needs to see: this route is the only containment lever there is, so a + // 500 here means a compromised account is still live and nothing else + // can stop it. Returning before the audit call left no trace at all. + _ = h.auditLog.Log(r.Context(), audit.AdminAccountRevoke, actor.ID, id, r.RemoteAddr, r.UserAgent(), "", "", map[string]interface{}{ + "revoked_admin_id": id, + "outcome": "failed", + "error": err.Error(), + }) httputil.WriteError(w, http.StatusInternalServerError, "internal_error") return } - _ = h.auditLog.Log(r.Context(), audit.AdminAccountRevoke, actor.ID, id, r.RemoteAddr, r.UserAgent(), "", "", map[string]interface{}{ + meta := map[string]interface{}{ "revoked_admin_id": id, - }) + "outcome": "revoked", + } + if revokedCreatedBy != "" { + meta["revoked_admin_created_by"] = revokedCreatedBy + } + _ = h.auditLog.Log(r.Context(), audit.AdminAccountRevoke, actor.ID, id, r.RemoteAddr, r.UserAgent(), "", "", meta) httputil.WriteJSON(w, http.StatusOK, map[string]string{"status": "revoked"}) } diff --git a/migrations/042_admin_revoke_reaches_a_creator.sql b/migrations/042_admin_revoke_reaches_a_creator.sql new file mode 100644 index 00000000..5f0b210a --- /dev/null +++ b/migrations/042_admin_revoke_reaches_a_creator.sql @@ -0,0 +1,49 @@ +-- ============================================================================ +-- 042: an admin who created another admin can still be revoked +-- ============================================================================ +-- +-- POST /admin/admins/{id}/revoke could not revoke any admin who had created +-- another admin. 001:252 declares +-- +-- created_by UUID REFERENCES auth.admin_users(id) +-- +-- with no ON DELETE clause, so the default NO ACTION applies, and +-- AdminUserRepo.Revoke is a bare DELETE. Deleting a row that another row still +-- names raises 23503 and the whole statement fails: the account stays, and so +-- do its live sessions -- admin_sessions.admin_id cascades (001:257), but only +-- if the parent delete succeeds, and it does not. +-- +-- created_by is never null in practice. adminapi/handler.go sets it on every +-- create and 016:84-88 raises when it is null once any admin exists, so the +-- admin graph is a tree. The bootstrap super_admin becomes unrevokable the +-- moment it creates one other admin, which is the first thing an operator does. +-- +-- That matters more than an ordinary 500 because revoke is the only containment +-- lever there is. adminapi/router.go:125-128 is the entire admin-management +-- surface: list, create, revoke. There is no admin update, no admin lock and no +-- per-admin-session revoke, so an admin whose credentials are known to be +-- compromised cannot be stopped at all. +-- +-- The existing coverage passes because it only ever revokes a leaf: +-- tests/integration/postgres_admin_test.go sets target.CreatedBy = admin.ID and +-- revokes the target, never the creator. +-- +-- SET NULL rather than CASCADE, deliberately. CASCADE would delete the revoked +-- admin's entire created subtree -- revoking one compromised account would +-- silently remove every account it had ever created, which is a far worse +-- outcome than the bug. SET NULL costs provenance on the children instead, and +-- RevokeAdmin now writes the old created_by into the audit metadata so the +-- answer to "who authorized this account" survives in the trail. +-- +-- Neither role-escalation trigger fires on this. deny_role_escalation_on_insert +-- is BEFORE INSERT only (016:129-132) and deny_role_escalation short-circuits +-- unless the role changes (001:394-428); an FK-driven SET NULL touches neither. +-- +-- Idempotent: the DROP is conditional and the ADD follows it, so re-running +-- lands on the same constraint. + +ALTER TABLE auth.admin_users DROP CONSTRAINT IF EXISTS admin_users_created_by_fkey; + +ALTER TABLE auth.admin_users + ADD CONSTRAINT admin_users_created_by_fkey + FOREIGN KEY (created_by) REFERENCES auth.admin_users(id) ON DELETE SET NULL; diff --git a/tests/integration/postgres_admin_revoke_creator_test.go b/tests/integration/postgres_admin_revoke_creator_test.go new file mode 100644 index 00000000..b90ddc9a --- /dev/null +++ b/tests/integration/postgres_admin_revoke_creator_test.go @@ -0,0 +1,88 @@ +package integration_test + +import ( + "context" + "testing" + + "github.com/42-v/vault42/internal/repository/postgres" +) + +// TestRevokeReachesAnAdminWhoCreatedAnother proves the containment lever works +// on the account it is most likely to be aimed at. +// +// auth.admin_users.created_by referenced the same table with no ON DELETE +// clause, so the default NO ACTION applied and deleting a row another row still +// named raised 23503. AdminUserRepo.Revoke is a bare DELETE, so the statement +// failed, the account stayed, and its live sessions stayed with it -- +// admin_sessions cascades, but only if the parent delete succeeds. +// +// created_by is set on every create and migration 016 raises when it is null +// once any admin exists, so the admin graph is a tree: the bootstrap +// super_admin becomes unrevokable the moment it opens one other account, which +// is the first thing an operator does. And revoke is the only lever there is -- +// the admin surface is list, create, revoke, with no update, no lock and no +// per-admin-session revoke. +// +// The existing repo test passes because it only ever revokes a leaf. This one +// revokes upward, which is the case that failed. +func TestRevokeReachesAnAdminWhoCreatedAnother(t *testing.T) { + adminPool, _, cleanup := setupPostgres(t) + defer cleanup() + ctx := context.Background() + repo := postgres.NewAdminUserRepo(&postgres.DB{Pool: adminPool}) + + creator := makeAdmin("creator-"+randomID()[:8], "super_admin") + if err := repo.Create(ctx, creator); err != nil { + t.Fatalf("create creator: %v", err) + } + child := makeAdmin("child-"+randomID()[:8], "viewer") + child.CreatedBy = creator.ID + if err := repo.Create(ctx, child); err != nil { + t.Fatalf("create child: %v", err) + } + + // A live session on the account being revoked: cascading it is the whole + // point of deleting the admin row first, and it cascades only if that + // delete succeeds. + var sessions int + if _, err := adminPool.Exec(ctx, + `INSERT INTO auth.admin_sessions (id, admin_id, token_hash, ip, created_at, expires_at) + VALUES (gen_random_uuid(), $1, 'hash', '203.0.113.9', NOW(), NOW() + INTERVAL '1 hour')`, + creator.ID); err != nil { + t.Fatalf("seed session: %v", err) + } + + if err := repo.Revoke(ctx, creator.ID); err != nil { + t.Fatalf("revoking an admin who created another failed: %v\n"+ + "This is the only containment lever there is: an admin whose credentials "+ + "are known to be compromised cannot be stopped by any other route.", err) + } + + if got, _ := repo.GetByID(ctx, creator.ID); got != nil { + t.Fatal("the creator is still present after Revoke") + } + + if err := adminPool.QueryRow(ctx, + `SELECT COUNT(*) FROM auth.admin_sessions WHERE admin_id = $1`, creator.ID).Scan(&sessions); err != nil { + t.Fatalf("count sessions: %v", err) + } + if sessions != 0 { + t.Fatalf("%d live sessions survived the revoke; the cascade only runs when the "+ + "parent delete succeeds", sessions) + } + + // SET NULL, not CASCADE. The child must survive: cascading would delete the + // revoked admin's entire created subtree, so revoking one compromised + // account would silently remove every account it had ever opened. + got, err := repo.GetByID(ctx, child.ID) + if err != nil { + t.Fatalf("read child: %v", err) + } + if got == nil { + t.Fatal("the child account was deleted with its creator. The constraint must be " + + "ON DELETE SET NULL; CASCADE here turns one revoke into a purge.") + } + if got.CreatedBy != "" { + t.Fatalf("child created_by = %q, want it cleared by SET NULL", got.CreatedBy) + } +}