Bundle G: Final audit closure — L-004 + D-003/4/5/7 closed; 54/55 + 7/7

Closes the 2026-04-25 audit's final-closure cluster. Score 51/55 -> 54/55

(98% closed); deferred 4/7 -> 7/7 (100%). All severity-graded findings now

closed except M-029 (frontend per-PR migration backlog, by design incremental).

L-004 (CWE-924) — dual-key API rotation overlap window:

  internal/config/config.go::ParseNamedAPIKeys rewritten to allow same-name

  duplicate entries iff admin flag matches. Mismatched-admin entries rejected

  at startup (privilege escalation guard); exact (name,key) duplicates rejected

  (typo guard — rotation requires DIFFERENT keys under the same name). Startup

  INFO log per name with multiple entries surfaces the active rotation window.

  NewAuthWithNamedKeys was already shaped correctly (constant-time hash compare

  across all entries, same UserKey + AdminKey for either bearer); Bundle B's

  M-025 per-user rate-limit bucket and audit-trail actor inherit consistency

  across the rollover automatically. 8 new tests pin the contract end-to-end.

  docs/security.md::API key rotation walks the 6-step zero-downtime rollover.

D-003 — Mutation testing wired:

  security-deep-scan.yml gets a go-mutesting step covering ./internal/crypto/...,

  ./internal/pkcs7/..., ./internal/connector/issuer/local/... with per-package

  summary lines extracted into go-mutesting.txt artefact.

D-007 — Frontend semgrep wired (recon found Bundle 7's wiring claim was false):

  security-deep-scan.yml gets a 'semgrep p/react-security' step running

  returntocorp/semgrep:latest --config=p/react-security against /src/web/src;

  results uploaded as semgrep-react.json.

D-004 + D-005 — Operator runbook published:

  docs/testing-strategy.md (NEW) consolidates per-tool local-run procedures,

  acceptance thresholds, and triage paths for go-mutesting, ZAP baseline DAST,

  testssl.sh, and semgrep p/react-security. Closes the 'wired CI-only, no

  local-run validation' framing for D-004/D-005 by giving operators the same

  commands the CI workflow runs.

Verification:

  gofmt -l                                no diff

  go vet ./internal/config/... ./internal/api/middleware/...   clean

  go test -short -count=1 ./internal/config/... ./internal/api/middleware/...   PASS

  python3 -c 'yaml.safe_load(...)'        YAML OK

  G-3 env-var docs guard                  no phantom env-vars

Audit deliverables:

  audit-report.md: L-004 + D-003/4/5/7 boxes flipped [x]; score 51/55 -> 54/55

  findings.yaml:   5 status flips; new bundle-G-final-closure closure_log entry

  CHANGELOG.md:    Bundle G entry under [unreleased]; supersedes Bundle E + F

                   L-004-deferred framing
This commit is contained in:
Shankar
2026-04-27 02:27:44 +00:00
parent 8abeb3ec20
commit 904b2266e7
7 changed files with 637 additions and 23 deletions
@@ -0,0 +1,97 @@
package middleware
import (
"net/http"
"net/http/httptest"
"testing"
)
// Audit L-004 (CWE-924) — auth-middleware side of the dual-key rotation
// contract. ParseNamedAPIKeys allows two entries to share a name during
// the overlap window; NewAuthWithNamedKeys must accept either bearer
// token and produce the same UserKey + Admin context value either way.
func TestL004_AuthMiddleware_BothKeysValidate(t *testing.T) {
mw := NewAuthWithNamedKeys([]NamedAPIKey{
{Name: "alice", Key: "OLDKEY", Admin: true},
{Name: "alice", Key: "NEWKEY", Admin: true},
})
makeReq := func(token string) *http.Request {
req := httptest.NewRequest(http.MethodGet, "/api/v1/anything", nil)
req.Header.Set("Authorization", "Bearer "+token)
return req
}
for _, tok := range []string{"OLDKEY", "NEWKEY"} {
t.Run("token="+tok, func(t *testing.T) {
rec := httptest.NewRecorder()
handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if got := GetUser(r.Context()); got != "alice" {
t.Errorf("UserKey = %q, want alice (rotation must preserve identity across both keys)", got)
}
if !IsAdmin(r.Context()) {
t.Errorf("Admin flag lost — both rotation entries carry admin=true, context must reflect that")
}
w.WriteHeader(http.StatusOK)
}))
handler.ServeHTTP(rec, makeReq(tok))
if rec.Code != http.StatusOK {
t.Fatalf("token %s should validate during rotation overlap; got %d", tok, rec.Code)
}
})
}
}
func TestL004_AuthMiddleware_PostRotationOldKeyRejected(t *testing.T) {
// Operator has completed the rotation: old key removed from
// CERTCTL_API_KEYS_NAMED, only new key remains. Old bearer must
// now fail.
mw := NewAuthWithNamedKeys([]NamedAPIKey{
{Name: "alice", Key: "NEWKEY", Admin: true},
})
req := httptest.NewRequest(http.MethodGet, "/api/v1/anything", nil)
req.Header.Set("Authorization", "Bearer OLDKEY")
rec := httptest.NewRecorder()
handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusOK)
}))
handler.ServeHTTP(rec, req)
if rec.Code != http.StatusUnauthorized {
t.Errorf("OLDKEY post-rotation should be rejected; got %d", rec.Code)
}
}
func TestL004_AuthMiddleware_DualUserKeyedRateLimit(t *testing.T) {
// Bundle B's rate limiter keys on the UserKey. Both rotation
// entries must produce the SAME UserKey value so the per-user
// bucket stays consistent across the overlap window — otherwise
// a client rotating its key would get a fresh bucket and bypass
// the rate limit. Pin the invariant.
mw := NewAuthWithNamedKeys([]NamedAPIKey{
{Name: "alice", Key: "OLDKEY", Admin: false},
{Name: "alice", Key: "NEWKEY", Admin: false},
})
captured := []string{}
handler := mw(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
captured = append(captured, GetUser(r.Context()))
w.WriteHeader(http.StatusOK)
}))
for _, tok := range []string{"OLDKEY", "NEWKEY"} {
req := httptest.NewRequest(http.MethodGet, "/", nil)
req.Header.Set("Authorization", "Bearer "+tok)
handler.ServeHTTP(httptest.NewRecorder(), req)
}
if len(captured) != 2 {
t.Fatalf("expected 2 captured UserKey values, got %d", len(captured))
}
if captured[0] != captured[1] {
t.Errorf("UserKey diverged across rotation: OLDKEY=%q NEWKEY=%q — rate-limit bucket would split",
captured[0], captured[1])
}
}
+75 -6
View File
@@ -1527,6 +1527,33 @@ func (c *Config) GetLogLevel() slog.Level {
// The ":admin" suffix is optional; if present, the key has admin privileges.
// Returns a typed []NamedAPIKey so main.go can pass it directly to the
// middleware layer without type assertion gymnastics.
//
// Audit L-004 (CWE-924) — graceful key rotation contract:
//
// Two entries MAY share the same Name during a rotation overlap window:
// CERTCTL_API_KEYS_NAMED="alice:OLDKEY:admin,alice:NEWKEY:admin"
// When duplicates appear, both keys validate at the auth middleware
// (NewAuthWithNamedKeys iterates every entry on every request, so the
// match is by hash regardless of name collisions). Both produce the
// same UserKey context value (the shared name), which keeps the audit
// trail and per-user rate-limit bucket (Bundle B M-025) consistent
// across the rollover.
//
// The duplicate-name path is restricted: every entry sharing a name
// MUST carry the same admin flag — mixing admin=true with admin=false
// under the same identity would let a non-admin caller present the
// admin-flagged key and bypass the gate (or vice-versa). The contract
// is "rotate ONE key at a time"; the privilege level stays constant
// within the overlap window.
//
// Exact (name,key) duplicates are still rejected — that's a typo,
// not a rotation. Rotation requires DIFFERENT keys under the same
// name.
//
// Once the rollover is complete, the operator removes the OLDKEY
// entry and restarts. Single-entry steady state resumes.
//
// See docs/security.md::API key rotation for the full operator runbook.
func ParseNamedAPIKeys(input string) ([]NamedAPIKey, error) {
if input == "" {
return nil, nil
@@ -1534,7 +1561,17 @@ func ParseNamedAPIKeys(input string) ([]NamedAPIKey, error) {
parts := splitComma(input)
var keys []NamedAPIKey
seen := make(map[string]bool)
// nameToAdmin pins the admin flag for any name we've seen before; it
// is consulted on subsequent duplicate-name entries to enforce the
// "matching admin" contract above.
nameToAdmin := make(map[string]bool)
// nameSeen records whether we've seen a name at all (used to
// distinguish first-occurrence from duplicate-occurrence; we need
// this separate from nameToAdmin because admin=false is a valid
// recorded state).
nameSeen := make(map[string]bool)
// pairSeen rejects exact (name,key) duplicates as typos.
pairSeen := make(map[string]bool)
for _, part := range parts {
part = trimSpace(part)
@@ -1566,15 +1603,30 @@ func ParseNamedAPIKeys(input string) ([]NamedAPIKey, error) {
return nil, fmt.Errorf("invalid key name: %s (must be alphanumeric, hyphens, underscores)", name)
}
if seen[name] {
return nil, fmt.Errorf("duplicate key name: %s", name)
}
seen[name] = true
if key == "" {
return nil, fmt.Errorf("empty key for name: %s", name)
}
// Typo guard: same (name,key) pair twice is never legitimate —
// rotation requires DIFFERENT keys under the same name.
pairKey := name + "\x00" + key
if pairSeen[pairKey] {
return nil, fmt.Errorf("duplicate (name,key) entry for name %q — rotation requires DIFFERENT keys under the same name", name)
}
pairSeen[pairKey] = true
// Duplicate-name path: allowed iff admin flag matches the prior
// entry for the same name (L-004 rotation overlap contract).
if nameSeen[name] {
priorAdmin := nameToAdmin[name]
if priorAdmin != admin {
return nil, fmt.Errorf("duplicate key name %q with mismatched admin flag — rotation overlap requires both entries carry the same privilege level (prior=%v, this=%v)", name, priorAdmin, admin)
}
} else {
nameSeen[name] = true
nameToAdmin[name] = admin
}
keys = append(keys, NamedAPIKey{
Name: name,
Key: key,
@@ -1582,6 +1634,23 @@ func ParseNamedAPIKeys(input string) ([]NamedAPIKey, error) {
})
}
// Rotation-window observability: emit a one-shot startup INFO log
// per name with multiple entries so operators can see the active
// overlap state in logs. (Single-entry steady state stays silent.)
nameCounts := make(map[string]int)
for _, k := range keys {
nameCounts[k.Name]++
}
for name, count := range nameCounts {
if count > 1 {
slog.Info("api-key rotation window active",
"name", name,
"entries", count,
"see", "docs/security.md::api-key-rotation",
)
}
}
return keys, nil
}
@@ -0,0 +1,122 @@
package config
import (
"strings"
"testing"
)
// Audit L-004 (CWE-924): graceful API key rotation overlap window.
// Pre-bundle ParseNamedAPIKeys rejected duplicate names. Post-bundle
// duplicates are allowed iff the admin flag matches across entries —
// this gives operators a zero-downtime rotation primitive without
// requiring schema, GUI, or DB-resident key storage.
//
// These tests pin the contract end-to-end through ParseNamedAPIKeys.
// The auth-middleware side is exercised separately in
// internal/api/middleware via auth_l004_rotation_test.go.
func TestL004_DualKeyRotation_SameAdmin_Accepted(t *testing.T) {
cases := []struct {
name string
input string
}{
{"both_admin", "alice:OLDKEY:admin,alice:NEWKEY:admin"},
{"both_non_admin", "ci-runner:OLD,ci-runner:NEW"},
{"three_keys_admin", "ops:K1:admin,ops:K2:admin,ops:K3:admin"},
{"mixed_with_other_users", "alice:OLDKEY:admin,bob:UNRELATED,alice:NEWKEY:admin"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
keys, err := ParseNamedAPIKeys(tc.input)
if err != nil {
t.Fatalf("expected dual-key rotation to parse, got error: %v", err)
}
if len(keys) < 2 {
t.Errorf("expected ≥2 entries, got %d", len(keys))
}
})
}
}
func TestL004_DualKeyRotation_AdminMismatch_Rejected(t *testing.T) {
cases := []struct {
name string
input string
}{
{"first_admin_then_user", "alice:OLD:admin,alice:NEW"},
{"first_user_then_admin", "alice:OLD,alice:NEW:admin"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
_, err := ParseNamedAPIKeys(tc.input)
if err == nil {
t.Fatal("expected admin-flag mismatch to be rejected")
}
if !strings.Contains(err.Error(), "mismatched admin flag") {
t.Errorf("error must cite admin flag mismatch, got: %v", err)
}
})
}
}
func TestL004_DualKeyRotation_IdenticalNameAndKey_Rejected(t *testing.T) {
// Same name + same key is a typo, not a rotation. The rotation
// case is DIFFERENT keys under the same name.
_, err := ParseNamedAPIKeys("alice:SAMEKEY:admin,alice:SAMEKEY:admin")
if err == nil {
t.Fatal("expected (name,key) duplicate to be rejected")
}
if !strings.Contains(err.Error(), "duplicate (name,key)") {
t.Errorf("error must cite (name,key) duplicate, got: %v", err)
}
}
func TestL004_DualKeyRotation_SteadyStateUnchanged(t *testing.T) {
// Single-key (no rotation) and multi-distinct-name configs must
// continue to parse the same way they did pre-bundle.
cases := []struct {
name string
input string
want int
}{
{"single", "alice:KEY:admin", 1},
{"two_distinct_names", "alice:KEY1:admin,bob:KEY2", 2},
{"three_distinct_names", "alice:K1:admin,bob:K2,carol:K3:admin", 3},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
keys, err := ParseNamedAPIKeys(tc.input)
if err != nil {
t.Fatalf("steady-state parse failed: %v", err)
}
if len(keys) != tc.want {
t.Errorf("got %d entries, want %d", len(keys), tc.want)
}
})
}
}
func TestL004_DualKeyRotation_PreservesAllEntries(t *testing.T) {
// Round-trip: every input entry must appear in the parsed output.
keys, err := ParseNamedAPIKeys("alice:OLDKEY:admin,alice:NEWKEY:admin")
if err != nil {
t.Fatalf("parse: %v", err)
}
if len(keys) != 2 {
t.Fatalf("got %d, want 2", len(keys))
}
gotKeys := map[string]bool{keys[0].Key: true, keys[1].Key: true}
for _, want := range []string{"OLDKEY", "NEWKEY"} {
if !gotKeys[want] {
t.Errorf("missing key %q in parsed entries: %+v", want, keys)
}
}
for _, k := range keys {
if k.Name != "alice" {
t.Errorf("entry %+v has wrong name; want alice", k)
}
if !k.Admin {
t.Errorf("entry %+v lost admin flag", k)
}
}
}