fix(security,domain): redact Agent.APIKeyHash from JSON wire shape (G-2)

Pre-G-2 internal/domain/connector.go::Agent::APIKeyHash was tagged
`json:"api_key_hash"` and shipped on every wire surface that returned
domain.Agent — GET /api/v1/agents (PagedResponse{Data: agents}),
GET /api/v1/agents/{id}, GET /api/v1/agents/retired, and the
POST /api/v1/agents registration response. Every authenticated client
(browser, CLI --json, MCP tool calls) received the SHA-256-of-the-API-key
string. The browser silently dropped it because web/src/api/types.ts
omits the field, but CLI and MCP consumers print full JSON so the hash
was visible there. Even though the value is a hash and not the plaintext
key, shipping it gives an attacker an offline brute-force target if the
API-key entropy is low (certctl doesn't enforce a minimum on operator-
supplied keys), and there's no business reason for any client to ever
receive it — the value is server-internal, used only for the lookup at
internal/repository/postgres/agent.go::GetByAPIKey. (Audit:
cat-s5-apikey_leak in coverage-gap-audit-2026-04-24-v5/unified-audit.md.)

We chose the audit's recommended fix (json:"-") plus a defense-in-depth
MarshalJSON plus a CI guardrail. Three layers because struct-tag
redaction alone is one rebase away from being silently reverted, the
custom MarshalJSON catches the case where a parent struct embeds Agent
under a different tag, and the CI grep blocks reintroduction at the spec
or frontend boundary even without a code review catching it.

Files changed:

Phase 1 — Domain redaction:
- internal/domain/connector.go: APIKeyHash tag flipped from
  `json:"api_key_hash"` to `json:"-"`. New Agent.MarshalJSON
  with value receiver + type-alias-recursion-break that explicitly
  zeroes APIKeyHash on the marshal-time copy. Long-form docblock
  explaining the G-2 closure rationale + cross-references to
  service.RegisterAgent (populator), repository.AgentRepository::
  GetByAPIKey (consumer), docs/architecture.md (DB-shape vs
  API-shape distinction), and the audit finding.

Phase 2 — Domain tests (5 test functions):
- internal/domain/connector_test.go: TestAgent_MarshalJSON_RedactsAPIKeyHash
  pins the marshal-boundary contract on a value receiver. ...RedactsViaPointer
  pins the *Agent path. ...RedactsInSlice pins the []Agent path that the
  ListAgents handler actually emits via PagedResponse. ...DoesNotMutateReceiver
  pins the by-value-receiver contract so a future refactor that switches
  to pointer-receiver gets caught. ...RoundTrip pins the wire-shape
  guarantee that APIKeyHash is dropped on encode and cannot reappear on
  decode. Single sentinel value ("sha256:LEAKED-CREDENTIAL-DERIVATIVE-
  SENTINEL") flows through every fixture for grep-ability on regression.

Phase 3 — Handler tests (4 test functions):
- internal/api/handler/agent_handler_test.go: TestListAgents_DoesNotLeakAPIKeyHash,
  TestGetAgent_DoesNotLeakAPIKeyHash, TestRegisterAgent_DoesNotLeakAPIKeyHash,
  TestListRetiredAgents_DoesNotLeakAPIKeyHash. Each asserts (a) the
  literal substring "api_key_hash" is absent from the httptest-captured
  body, (b) the leak sentinel value is absent, (c) the non-leaked fields
  ARE present (sanity that the handler is serving real data, not just
  empty payloads). Shared sentinel "sha256:LEAKED-CREDENTIAL-DERIVATIVE-
  HANDLER-SENTINEL" so a single grep over a failing test's output
  identifies the leak surface immediately.

Phase 4 — Spec / docs:
- api/openapi.yaml: api_key_hash property REMOVED from Agent schema
  (was at line 3690). Inline G-2 comment naming the closure + the
  database-vs-API-shape distinction so a future spec edit doesn't
  silently re-introduce the field.
- docs/architecture.md: ER-diagram block already documents the agents
  table including api_key_hash (DB shape — correct). Added a sibling
  note paragraph immediately below the diagram explaining that several
  columns are intentionally server-internal (api_key_hash redaction
  + issuers.config / deployment_targets.config encrypted shadow), with
  cross-references to the redaction enforcement site, the OpenAPI
  schema, the frontend interface, and the CI guardrail.
- web/src/api/types.ts: Agent interface unchanged in shape (already
  omitted the field) but added a leading comment block explaining
  WHY the omission is intentional — stops a future frontend dev from
  "completing" the interface from the OpenAPI spec or the Go struct.

Phase 5 — CI guardrail:
- .github/workflows/ci.yml: new "Forbidden api_key_hash JSON-shape
  regression guard (G-2)" step. Scoped patterns catch the actual
  regression shapes — Go struct tag (json:"api_key_hash"), frontend
  interface declaration, OpenAPI schema property, YAML enum/array
  membership. Repository / migration / seed / service / integration /
  unit-test / comment lines exempt. Verified locally on the real tree
  (passes) and against 4 synthetic regression patterns (each fires
  the guardrail). Mirrors the G-1 pattern from .github/workflows/
  ci.yml lines 47-108.

Phase 5b — Sweep verification (no changes, results documented for the
next reader):
- internal/api/middleware/audit.go: doesn't serialize Agent struct;
  records request body only. No leak.
- service.RegisterAgent audit-event payload: `map[string]interface{}{
  "name": name, "hostname": hostname}` — name + hostname only,
  no APIKeyHash. No leak.
- All 9 slog sites that mention agent: scalar attrs only ("agent_id",
  "error", "agent_hostname"), never the full struct. No leak.
- internal/mcp, internal/cli, cmd/cli, cmd/mcp-server: zero matches
  for APIKeyHash / api_key_hash. Both pass server JSON verbatim, so
  the wire-side fix transitively closes them.

Verification (all gates pass):
- go build ./...
- go vet ./...
- go test -short ./... — every package green
- go test -short -race ./internal/domain/... ./internal/api/handler/... — clean
- govulncheck ./... — no vulnerabilities in our code
- helm lint deploy/helm/certctl/ — clean
- helm template smoke render — succeeds
- python3 yaml.safe_load on api/openapi.yaml — parses
- OpenAPI Agent schema scan: no api_key_hash property
- CI guardrail mirror: clean on real tree, fires on all 4 synthetic
  regression patterns
- Domain pkg coverage: Agent.MarshalJSON 100%, connector.go total 87.5%
- Handler pkg coverage: 79.2%

Sample response body (httptest captured during verification, GET
/api/v1/agents/{id} via the new handler test):

  {"id":"agent-demo","name":"demo-agent","hostname":"demo.host",
  "status":"Online","last_heartbeat_at":"2026-04-24T11:59:30Z",
  "registered_at":"2026-04-24T12:00:00Z","os":"linux",
  "architecture":"amd64","ip_address":"10.0.0.42",
  "version":"v2.0.49"}

Note the absence of any api_key_hash key, even though the in-memory
struct passed to the handler had APIKeyHash set to a sentinel.

Out of scope (intentionally untouched):
- internal/repository/postgres/agent.go SELECT/INSERT/UPDATE/scan
  paths and GetByAPIKey lookup — DB column stays, repo still
  populates the struct, auth lookup still works. The redaction is a
  marshal-boundary concern.
- migrations/000001_initial_schema.up.sql + migrations/seed_*.sql —
  DB schema and seed data unchanged.
- internal/service/agent.go::RegisterAgent — service-side hashing
  and persistence unchanged.
- Other domain types with potential credential-derivative fields
  (Issuer.Config, DeploymentTarget.Config, notifier configs). Not
  flagged by the audit; some are already protected (e.g.,
  DeploymentTarget.EncryptedConfig []byte `json:"-"`). File a
  separate audit pass if recon surfaces additional leaks.
- Per-resource DTO layer across every handler. Single audit
  finding, single domain type.
- A separate possible follow-up: the v2 RegisterAgent endpoint
  doesn't return the plaintext API key to the agent, which may
  mean self-bootstrap via POST /api/v1/agents is broken. Verified
  during recon; out of scope for G-2; should be its own ticket.

Refs: coverage-gap-audit-2026-04-24-v5/unified-audit.md
      §2 P1 cluster, cat-s5-apikey_leak
      Audit recommendation: 'json:"-" or API-response DTO
      excluding APIKeyHash' — went with the json:"-" + MarshalJSON
      defense-in-depth pair plus CI guardrail and structural docs.
This commit is contained in:
shankar0123
2026-04-25 01:56:26 +00:00
parent 697fa792ea
commit 87213128cc
7 changed files with 467 additions and 3 deletions
+158
View File
@@ -893,3 +893,161 @@ func TestAgentReportJobStatus_ServiceError(t *testing.T) {
func stringPtr(s string) *string {
return &s
}
// G-2 (P1): cat-s5-apikey_leak audit closure tests. Pre-G-2,
// Agent.APIKeyHash was tagged `json:"api_key_hash"` and shipped on
// every wire surface that returned domain.Agent. Post-G-2 the tag is
// "-" and Agent.MarshalJSON enforces redaction via a marshal-time copy
// (see internal/domain/connector_test.go for the type-level pin). These
// four tests are the wire-shape contract — they capture the actual HTTP
// response body via httptest and assert the credential-derivative hash
// is absent.
//
// One sentinel value (g2HandlerLeakSentinel) flows through every fixture
// so a single grep over a failing test's output identifies the leak
// surface immediately.
const g2HandlerLeakSentinel = "sha256:LEAKED-CREDENTIAL-DERIVATIVE-HANDLER-SENTINEL"
func TestListAgents_DoesNotLeakAPIKeyHash(t *testing.T) {
now := time.Now()
mock := &MockAgentService{
ListAgentsFn: func(page, perPage int) ([]domain.Agent, int64, error) {
return []domain.Agent{
{ID: "a-1", Name: "agent-one", Hostname: "host-1",
Status: domain.AgentStatusOnline, RegisteredAt: now,
APIKeyHash: g2HandlerLeakSentinel + "-1"},
{ID: "a-2", Name: "agent-two", Hostname: "host-2",
Status: domain.AgentStatusOnline, RegisteredAt: now,
APIKeyHash: g2HandlerLeakSentinel + "-2"},
}, 2, nil
},
}
h := NewAgentHandler(mock)
req := httptest.NewRequest(http.MethodGet, "/api/v1/agents?page=1&per_page=50", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
h.ListAgents(w, req)
if w.Code != http.StatusOK {
t.Fatalf("ListAgents status = %d, want 200", w.Code)
}
body := w.Body.String()
if bytes.Contains([]byte(body), []byte("api_key_hash")) {
t.Errorf("ListAgents response leaked \"api_key_hash\" key (G-2 regressed):\n%s", body)
}
if bytes.Contains([]byte(body), []byte(g2HandlerLeakSentinel)) {
t.Errorf("ListAgents response leaked sentinel %q:\n%s", g2HandlerLeakSentinel, body)
}
// Sanity: the non-leaked fields ARE present (handler did serve real data).
for _, want := range []string{"a-1", "a-2", "agent-one", "agent-two"} {
if !bytes.Contains([]byte(body), []byte(want)) {
t.Errorf("ListAgents response missing expected field %q (handler may not be serving data):\n%s", want, body)
}
}
}
func TestGetAgent_DoesNotLeakAPIKeyHash(t *testing.T) {
now := time.Now()
mock := &MockAgentService{
GetAgentFn: func(id string) (*domain.Agent, error) {
return &domain.Agent{
ID: id, Name: "single-agent", Hostname: "single.host",
Status: domain.AgentStatusOnline, RegisteredAt: now,
APIKeyHash: g2HandlerLeakSentinel,
}, nil
},
}
h := NewAgentHandler(mock)
req := httptest.NewRequest(http.MethodGet, "/api/v1/agents/a-prod-001", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
h.GetAgent(w, req)
if w.Code != http.StatusOK {
t.Fatalf("GetAgent status = %d, want 200, body=%s", w.Code, w.Body.String())
}
body := w.Body.String()
if bytes.Contains([]byte(body), []byte("api_key_hash")) {
t.Errorf("GetAgent response leaked \"api_key_hash\" key:\n%s", body)
}
if bytes.Contains([]byte(body), []byte(g2HandlerLeakSentinel)) {
t.Errorf("GetAgent response leaked sentinel:\n%s", body)
}
if !bytes.Contains([]byte(body), []byte("single-agent")) {
t.Errorf("GetAgent response missing the agent name (handler may not be serving data):\n%s", body)
}
}
func TestRegisterAgent_DoesNotLeakAPIKeyHash(t *testing.T) {
// Registration is the most likely path for a freshly-hashed key to
// leak: the service mints a new APIKeyHash inside RegisterAgent
// (service/agent.go:405) and the handler returns the agent struct
// verbatim. Pin that the redaction holds even on a "freshly created"
// agent payload.
now := time.Now()
mock := &MockAgentService{
RegisterAgentFn: func(in domain.Agent) (*domain.Agent, error) {
return &domain.Agent{
ID: "agent-new", Name: in.Name, Hostname: in.Hostname,
Status: domain.AgentStatusOnline, RegisteredAt: now,
APIKeyHash: g2HandlerLeakSentinel,
}, nil
},
}
h := NewAgentHandler(mock)
body := bytes.NewBufferString(`{"name":"freshly-registered","hostname":"new.host"}`)
req := httptest.NewRequest(http.MethodPost, "/api/v1/agents", body)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
h.RegisterAgent(w, req)
if w.Code != http.StatusCreated {
t.Fatalf("RegisterAgent status = %d, want 201, body=%s", w.Code, w.Body.String())
}
respBody := w.Body.String()
if bytes.Contains([]byte(respBody), []byte("api_key_hash")) {
t.Errorf("RegisterAgent response leaked \"api_key_hash\" key:\n%s", respBody)
}
if bytes.Contains([]byte(respBody), []byte(g2HandlerLeakSentinel)) {
t.Errorf("RegisterAgent response leaked sentinel:\n%s", respBody)
}
if !bytes.Contains([]byte(respBody), []byte("agent-new")) {
t.Errorf("RegisterAgent response missing the new agent ID (handler may not be serving data):\n%s", respBody)
}
}
func TestListRetiredAgents_DoesNotLeakAPIKeyHash(t *testing.T) {
// I-004 surface — separate handler from ListAgents; same leak risk.
now := time.Now()
retiredAt := now.Add(-1 * time.Hour)
reason := "test cascade"
mock := &MockAgentService{
ListRetiredAgentsFn: func(page, perPage int) ([]domain.Agent, int64, error) {
return []domain.Agent{
{ID: "ret-1", Name: "retired-one", Hostname: "host-r1",
Status: domain.AgentStatusOffline, RegisteredAt: now,
RetiredAt: &retiredAt, RetiredReason: &reason,
APIKeyHash: g2HandlerLeakSentinel},
}, 1, nil
},
}
h := NewAgentHandler(mock)
req := httptest.NewRequest(http.MethodGet, "/api/v1/agents/retired?page=1&per_page=50", nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
h.ListRetiredAgents(w, req)
if w.Code != http.StatusOK {
t.Fatalf("ListRetiredAgents status = %d, want 200, body=%s", w.Code, w.Body.String())
}
body := w.Body.String()
if bytes.Contains([]byte(body), []byte("api_key_hash")) {
t.Errorf("ListRetiredAgents response leaked \"api_key_hash\" key:\n%s", body)
}
if bytes.Contains([]byte(body), []byte(g2HandlerLeakSentinel)) {
t.Errorf("ListRetiredAgents response leaked sentinel:\n%s", body)
}
if !bytes.Contains([]byte(body), []byte("ret-1")) {
t.Errorf("ListRetiredAgents response missing the retired agent ID:\n%s", body)
}
}