From f43582371a3804d48dd7c15d343511505d624100 Mon Sep 17 00:00:00 2001 From: 42-v <210031137+42-v@users.noreply.github.com> Date: Wed, 26 Aug 2026 21:46:13 +0200 Subject: [PATCH 1/2] fix(admin): revoke reaches an admin who created another admin POST /admin/admins/{id}/revoke could not revoke any admin who had opened another account. The account survived, and so did its live sessions. auth.admin_users.created_by referenced its own table with no ON DELETE clause, so NO ACTION applied and deleting a row another row still named raised 23503. AdminUserRepo.Revoke is a bare DELETE, so the whole statement failed. admin_sessions cascades, but only when the parent delete succeeds, and it did not -- so the sessions the revoke exists to kill stayed open. created_by is set on every create and 016 raises when it is null once any admin exists, so the admin graph is a tree. The bootstrap super_admin became unrevokable the moment it opened one other account, which is the first thing an operator does. That is worse than an ordinary 500 because revoke is the only containment lever the admin plane has: router.go:125-128 is list, create, revoke, with no admin update, no admin lock and no per-admin-session revoke. An admin whose credentials were known to be compromised could not be stopped at all. 042 makes the constraint ON DELETE SET NULL. Deliberately not CASCADE: that would delete the revoked admin's entire created subtree, so revoking one compromised account would silently remove every account it had ever opened. The handler now reads the row before deleting it and writes the old created_by into the audit metadata, so "who authorized this account" survives the SET NULL, and it audits the failure path as well. Returning 500 before the audit call left no trace of the one event an operator most needs to see: the containment attempt that did not work. The existing repo test passes on the unfixed code because it only ever revokes a leaf. The new one revokes upward and asserts all three properties: the creator is gone, its sessions cascaded to zero, and the child survives with created_by cleared. Removing the migration fails it. docs/admin-gateway.md asserted the opposite of the behaviour in the tree. --- README.md | 2 +- docs/UPGRADING.md | 13 ++- docs/admin-gateway.md | 2 +- docs/badges.json | 6 +- internal/adminapi/handler.go | 28 +++++- .../042_admin_revoke_reaches_a_creator.sql | 49 +++++++++++ .../postgres_admin_revoke_creator_test.go | 88 +++++++++++++++++++ 7 files changed, 177 insertions(+), 11 deletions(-) create mode 100644 migrations/042_admin_revoke_reaches_a_creator.sql create mode 100644 tests/integration/postgres_admin_revoke_creator_test.go 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..39d9a004 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": 928, "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/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) + } +} From 3ddb8abeb53c16e9f9d0fdfe935c1b3809d96fb2 Mon Sep 17 00:00:00 2001 From: 42-v <210031137+42-v@users.noreply.github.com> Date: Wed, 26 Aug 2026 22:04:26 +0200 Subject: [PATCH 2/2] test(admin): cover both audit paths the revoke fix added The coverage gate counted two statements neither covered nor excluded: the provenance read that keeps created_by before the SET NULL erases it, and the audit row written when the revoke itself fails. Both are now driven. The first asserts that a successful revoke carries revoked_admin_created_by, which is where the answer to "who authorized this account" survives once 042 nulls the column on every account the revoked admin opened. The second asserts a failed revoke writes a row at all, with an outcome and a reason -- it used to return 500 before reaching the audit call, so the operator was told the request failed and the trail said the attempt never happened. Wired through captureAudit and a directly constructed Handler, which is the idiom audit_actor_test.go already uses for this question. newTestHandler takes an audit repository but builds its logger from testAuditLog(), so rows written through the handler never reach a repository passed in -- worth knowing before writing the next audit assertion against it. --- docs/badges.json | 2 +- internal/adminapi/admin_revoke_audit_test.go | 82 ++++++++++++++++++++ 2 files changed, 83 insertions(+), 1 deletion(-) create mode 100644 internal/adminapi/admin_revoke_audit_test.go diff --git a/docs/badges.json b/docs/badges.json index 39d9a004..f62f403b 100644 --- a/docs/badges.json +++ b/docs/badges.json @@ -10,7 +10,7 @@ "packages": 43, "goFiles": 192, "goLines": 48153, - "testFiles": 928, + "testFiles": 929, "directDeps": 3, "transitiveDeps": 15, "totalTests": 6479, 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") + } +}