Skip to content

fix(admin): attribute email-plane audit rows to an id the column can hold #92

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
42-v wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(admin): attribute email-plane audit rows to an id the column can hold #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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-48229-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-48253-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) |
<!-- /badges -->
Expand Down
4 changes: 2 additions & 2 deletions docs/badges.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"reachableCoverageNum": 100.00,
"packages": 43,
"goFiles": 192,
"goLines": 48229,
"goLines": 48253,
"testFiles": 928,
"directDeps": 3,
"transitiveDeps": 15,
Expand All @@ -20,7 +20,7 @@
"tests": 4910,
"coverage": "100.00% of reachable",
"coverageNum": 100.00,
"lines": 48229,
"lines": 48253,
"deps": 3,
"transitiveDeps": 15
},
Expand Down
93 changes: 91 additions & 2 deletions internal/adminapi/audit_actor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"net/http"
"net/http/httptest"
"regexp"
"strings"
"testing"

Expand Down Expand Up @@ -32,7 +33,7 @@ func actorReq(method, target, body string) *http.Request {
r.RemoteAddr = "203.0.113.7:5555"
r.Header.Set("User-Agent", "audit-probe/1.0")
return r.WithContext(WithAdmin(r.Context(), &model.AdminUser{
ID: "adm-actor-1", Username: "actor", Role: "super_admin",
ID: wantActor, Username: "actor", Role: "super_admin",
}))
}

Expand All @@ -43,8 +44,17 @@ func actorReq(method, target, body string) *http.Request {
// so the record proved something happened while dropping the one fact asked
// first after an incident: which admin did it, from where. The row must
// attribute the action to the authenticated admin.
// wantActor is UUID-shaped, because audit.audit_log.user_id is UUID. The
// previous value was "adm-actor-1", which the column cannot hold either -- so a
// mock that accepts any string let a handler pass a username here and stay green
// while the real insert failed 22P02 and the row never existed. Asserting the
// shape is what makes this gate about the column rather than about equality.
const wantActor = "7e2f9a10-2222-4000-8000-0000000000ab"

var actorUUIDShape = regexp.MustCompile(
`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`)

func TestPrivilegedMutations_AuditAttributeTheActor(t *testing.T) {
const wantActor = "adm-actor-1"
const wantIP = "203.0.113.7:5555"
const wantUA = "audit-probe/1.0"

Expand All @@ -53,6 +63,14 @@ func TestPrivilegedMutations_AuditAttributeTheActor(t *testing.T) {
if e.UserID != wantActor {
t.Errorf("audit %s: actor = %q, want %q, no record of which admin made the change", e.EventType, e.UserID, wantActor)
}
// The column is UUID. A username here is not a value it can hold: the
// insert fails 22P02, the error is discarded as best-effort, and the
// handler still answers 200 -- so the trail is silently empty for
// exactly the route somebody would want it for.
if !actorUUIDShape.MatchString(e.UserID) {
t.Errorf("audit %s: actor = %q is not UUID-shaped, and audit.audit_log.user_id is "+
"UUID -- this row cannot be stored", e.EventType, e.UserID)
}
if e.IP != wantIP {
t.Errorf("audit %s: ip = %q, want %q", e.EventType, e.IP, wantIP)
}
Expand Down Expand Up @@ -102,6 +120,77 @@ func TestPrivilegedMutations_AuditAttributeTheActor(t *testing.T) {
assertAttributed(t, got[0])
})

// The email plane. These four are why the shape assertion above exists: they
// were the only audit call sites in the package passing a username instead of
// an id, so every row they wrote failed 22P02 and vanished, while the handler
// answered 200. email:write rewrites the body of password-reset and
// verification mail for every user of an app, which makes this the route in
// the admin API whose trail matters most and the one that had none.
t.Run("email branding set", func(t *testing.T) {
var got []*model.AuditEntry
h := &Handler{emailBranding: newFakeBrandingRepo(), auditLog: captureAudit(&got)}
rec := httptest.NewRecorder()
h.PutEmailBranding(rec, withPathValue(
actorReq(http.MethodPut, "/admin/email-branding/acme", `{"app_name":"Acme"}`),
map[string]string{"app": "acme"}))
if rec.Code != http.StatusOK {
t.Fatalf("status = %d (%s)", rec.Code, rec.Body.String())
}
if len(got) != 1 {
t.Fatalf("audit rows = %d, want 1", len(got))
}
assertAttributed(t, got[0])
})

t.Run("email branding delete", func(t *testing.T) {
var got []*model.AuditEntry
repo := newFakeBrandingRepo()
h := &Handler{emailBranding: repo, auditLog: captureAudit(&got)}
rec := httptest.NewRecorder()
h.DeleteEmailBranding(rec, withPathValue(
actorReq(http.MethodDelete, "/admin/email-branding/acme", ""),
map[string]string{"app": "acme"}))
if rec.Code != http.StatusOK && rec.Code != http.StatusNoContent {
t.Fatalf("status = %d (%s)", rec.Code, rec.Body.String())
}
if len(got) != 1 {
t.Fatalf("audit rows = %d, want 1", len(got))
}
assertAttributed(t, got[0])
})

t.Run("email template set", func(t *testing.T) {
var got []*model.AuditEntry
h := &Handler{emailTemplates: newFakeTemplateRepo(), auditLog: captureAudit(&got)}
rec := httptest.NewRecorder()
h.PutEmailTemplate(rec, withPathValue(
actorReq(http.MethodPut, "/admin/email-templates/acme/verification", `{"subject":"Hi","html_content":"<p>Hi</p>"}`),
map[string]string{"app": "acme", "name": "verification"}))
if rec.Code != http.StatusOK {
t.Fatalf("status = %d (%s)", rec.Code, rec.Body.String())
}
if len(got) != 1 {
t.Fatalf("audit rows = %d, want 1", len(got))
}
assertAttributed(t, got[0])
})

t.Run("email template delete", func(t *testing.T) {
var got []*model.AuditEntry
h := &Handler{emailTemplates: newFakeTemplateRepo(), auditLog: captureAudit(&got)}
rec := httptest.NewRecorder()
h.DeleteEmailTemplate(rec, withPathValue(
actorReq(http.MethodDelete, "/admin/email-templates/acme/verification", ""),
map[string]string{"app": "acme", "name": "verification"}))
if rec.Code != http.StatusOK && rec.Code != http.StatusNoContent {
t.Fatalf("status = %d (%s)", rec.Code, rec.Body.String())
}
if len(got) != 1 {
t.Fatalf("audit rows = %d, want 1", len(got))
}
assertAttributed(t, got[0])
})

t.Run("users import", func(t *testing.T) {
var got []*model.AuditEntry
h := &Handler{
Expand Down
32 changes: 28 additions & 4 deletions internal/adminapi/email.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,37 @@ import (
const maxSubjectLen = 255

// actor returns the username of the authenticated admin, or "" if absent.
//
// For display columns only. audit.audit_log.user_id is UUID, so a username
// there is not a value that column can hold -- see actorID.
func (h *Handler) actor(r *http.Request) string {
if a := GetAdmin(r.Context()); a != nil {
return a.Username
}
return ""
}

// actorID returns the UUID of the authenticated admin, or "" if absent.
//
// The audit call sites in this file passed actor() -- a username. The bootstrap
// admin's username is literally "admin", audit.audit_log.user_id is UUID, and
// the insert failed 22P02 on every one of them. The logger runs synchronously
// here (the admin gateway wires a zero flush interval) and every call site
// discards the error under #nosec G104, so the handler answered 200 and the row
// simply did not exist.
//
// Of the audit call sites in this package these four were the only ones not
// passing an id, which is why nothing else in the admin plane showed the
// symptom. It matters most here: email:write rewrites the body of password-reset
// and verification mail for every user of an app, which makes it the one route
// in this API a phisher would want, and the one whose trail was empty.
func (h *Handler) actorID(r *http.Request) string {
if a := GetAdmin(r.Context()); a != nil {
return a.ID
}
return ""
}

// ===================== Email branding =====================

type emailBrandingView struct {
Expand Down Expand Up @@ -137,7 +161,7 @@ func (h *Handler) PutEmailBranding(w http.ResponseWriter, r *http.Request) {
return
}
if h.auditLog != nil {
h.auditLog.Log(r.Context(), "admin:email_branding_set", h.actor(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
h.auditLog.Log(r.Context(), "admin:email_branding_set", h.actorID(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
map[string]any{"app": app})
}
if stored, err := h.emailBranding.Get(r.Context(), app); err == nil && stored != nil {
Expand All @@ -162,7 +186,7 @@ func (h *Handler) DeleteEmailBranding(w http.ResponseWriter, r *http.Request) {
return
}
if h.auditLog != nil {
h.auditLog.Log(r.Context(), "admin:email_branding_delete", h.actor(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
h.auditLog.Log(r.Context(), "admin:email_branding_delete", h.actorID(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
map[string]any{"app": app})
}
httputil.WriteJSON(w, http.StatusOK, map[string]any{"status": "deleted"})
Expand Down Expand Up @@ -297,7 +321,7 @@ func (h *Handler) PutEmailTemplate(w http.ResponseWriter, r *http.Request) {
return
}
if h.auditLog != nil {
h.auditLog.Log(r.Context(), "admin:email_template_set", h.actor(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
h.auditLog.Log(r.Context(), "admin:email_template_set", h.actorID(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
map[string]any{"app": app, "template": name, "enabled": enabled})
}
if stored, err := h.emailTemplates.Get(r.Context(), app, name); err == nil && stored != nil {
Expand All @@ -322,7 +346,7 @@ func (h *Handler) DeleteEmailTemplate(w http.ResponseWriter, r *http.Request) {
return
}
if h.auditLog != nil {
h.auditLog.Log(r.Context(), "admin:email_template_delete", h.actor(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
h.auditLog.Log(r.Context(), "admin:email_template_delete", h.actorID(r), "", r.RemoteAddr, r.UserAgent(), "", "", // #nosec G104 -- audit is best-effort
map[string]any{"app": app, "template": name})
}
httputil.WriteJSON(w, http.StatusOK, map[string]any{"status": "deleted"})
Expand Down
Loading