From 9b898393c190d0ee4fb7815d5d8c0f4cb21ecb26 Mon Sep 17 00:00:00 2001 From: Remylus Losius Date: Fri, 19 Jun 2026 16:35:12 -0400 Subject: [PATCH] =?UTF-8?q?feat(remediation):=20conditional=20approval=20(?= =?UTF-8?q?A-keep)=20=E2=80=94=20free-core=20auto-approves?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements the A-keep ADR: free-core single-rule remediation no longer requires a separate human approval, so a single operator can request and Fix a finding directly (removing the self-review deadlock). The approve/reject flow with separation of duties is retained for the licensed bulk/auto track. - Request(...requiresApproval bool): false (free core) inserts an 'approved' row directly (reviewed_at set, reviewed_by NULL, auto-approved review_note) and emits remediation.requested + remediation.approved; true (licensed bulk/auto) inserts 'pending_approval' and goes through Approve/Reject. - The single-rule request handler passes false. - Tests: AC-01 covers auto-approve + the approval-required path; the HTTP AC-05/AC-06 approve and pending-execute paths seed a pending_approval request (the free-core POST auto-approves). Frontend unchanged (the hook already renders approved -> Fix and keeps the pending_approval/approve UI for the licensed track). Note: the ADR + governance docs land in #604; their status flips to 'implemented' once both merge. --- CHANGELOG.md | 8 +++ internal/remediation/service.go | 48 +++++++++++--- internal/remediation/service_test.go | 47 +++++++++----- internal/server/api_remediation_test.go | 73 +++++++++++++--------- internal/server/remediation_handlers.go | 5 +- internal/worker/remediation_worker_test.go | 2 +- specs/api/remediation.spec.yaml | 6 +- 7 files changed, 132 insertions(+), 57 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a8750821..0f5e1ab3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ Versioning: [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Changed +- Remediation: free-core single-rule remediation is now **auto-approved** on + request, so an operator can apply a fix without a separate approver. This + removes the self-review deadlock for single-operator workspaces (you could + request a fix but never approve your own request). The request lifecycle and + the approve/reject flow with separation of duties are retained for the + licensed bulk/automated remediation track (which requests with approval + required). See `docs/engineering/remediation_governance_adr.md` ("A-keep"). + - CI release safety: the release workflow now fails closed on a `v*` tag push when no GPG signing key is configured, rather than publishing unsigned packages. Manual `workflow_dispatch` trial builds stay permissive (warn + diff --git a/internal/remediation/service.go b/internal/remediation/service.go index 65550fa0..fdb3669c 100644 --- a/internal/remediation/service.go +++ b/internal/remediation/service.go @@ -75,8 +75,21 @@ func scanListRequest(row pgx.Row) (Request, error) { // recording a best-effort projected per-framework lift. Returns // ErrDuplicateOpen when an open request already exists for the same host+rule. // NEVER contacts the host. Emits remediation.requested. +// Request opens a remediation request for one rule on one host. +// +// Approval is conditional on the remediation track (ADR +// docs/engineering/remediation_governance_adr.md, "A-keep"): +// +// - requiresApproval=false (FREE-CORE single-rule manual remediation): the +// request is AUTO-APPROVED on creation — inserted directly in 'approved' +// with reviewed_at set and an explanatory review_note, so the requester can +// Fix it without a separate approver. This makes single-operator workspaces +// workable (no self-review deadlock). +// - requiresApproval=true (LICENSED bulk/auto track): the request is inserted +// 'pending_approval' and must go through Approve/Reject with separation of +// duties (the requester cannot review their own request) before it can run. func (s *Service) Request(ctx context.Context, hostID uuid.UUID, ruleID string, - scanRunID *uuid.UUID, requestedBy uuid.UUID) (Request, error) { + scanRunID *uuid.UUID, requestedBy uuid.UUID, requiresApproval bool) (Request, error) { ruleID = strings.TrimSpace(ruleID) if ruleID == "" { return Request{}, ErrInvalidInput @@ -87,13 +100,27 @@ func (s *Service) Request(ctx context.Context, hostID uuid.UUID, ruleID string, proj, _ := s.ProjectLift(ctx, hostID, ruleID) id := uuid.Must(uuid.NewV7()) - row := s.pool.QueryRow(ctx, ` - INSERT INTO remediation_requests - (id, host_id, rule_id, scan_run_id, status, requested_by, - projected_cis, projected_stig, projected_nist) - VALUES ($1, $2, $3, $4, 'pending_approval', $5, $6, $7, $8) - RETURNING `+selectCols, - id, hostID, ruleID, scanRunID, requestedBy, proj.CIS, proj.STIG, proj.NIST) + var row pgx.Row + if requiresApproval { + row = s.pool.QueryRow(ctx, ` + INSERT INTO remediation_requests + (id, host_id, rule_id, scan_run_id, status, requested_by, + projected_cis, projected_stig, projected_nist) + VALUES ($1, $2, $3, $4, 'pending_approval', $5, $6, $7, $8) + RETURNING `+selectCols, + id, hostID, ruleID, scanRunID, requestedBy, proj.CIS, proj.STIG, proj.NIST) + } else { + row = s.pool.QueryRow(ctx, ` + INSERT INTO remediation_requests + (id, host_id, rule_id, scan_run_id, status, requested_by, + reviewed_at, review_note, + projected_cis, projected_stig, projected_nist) + VALUES ($1, $2, $3, $4, 'approved', $5, now(), + 'auto-approved: free-core single-rule remediation requires no separate approval', + $6, $7, $8) + RETURNING `+selectCols, + id, hostID, ruleID, scanRunID, requestedBy, proj.CIS, proj.STIG, proj.NIST) + } rq, err := scanRequest(row) if err != nil { if isUniqueViolation(err) { @@ -103,6 +130,11 @@ func (s *Service) Request(ctx context.Context, hostID uuid.UUID, ruleID string, } s.emitEvent(ctx, audit.RemediationRequested, rq, requestedBy, "requested") + if !requiresApproval { + // Record the auto-approval honestly in the audit trail; there is no + // human reviewer (reviewed_by stays NULL). + s.emitEvent(ctx, audit.RemediationApproved, rq, requestedBy, "auto_approved") + } return rq, nil } diff --git a/internal/remediation/service_test.go b/internal/remediation/service_test.go index 5c04ca1c..273b668a 100644 --- a/internal/remediation/service_test.go +++ b/internal/remediation/service_test.go @@ -102,33 +102,50 @@ func TestRequest_InsertDuplicateInvalidReopen(t *testing.T) { var calls []emitCall svc := NewService(pool, fakeEmitter(&calls)) - rq, err := svc.Request(ctx, hostID, "sshd-permit-root-no", nil, user) + // Free-core single-rule remediation (requiresApproval=false) is + // AUTO-APPROVED on creation. + rq, err := svc.Request(ctx, hostID, "sshd-permit-root-no", nil, user, false) if err != nil { t.Fatalf("Request: %v", err) } - if rq.Status != StatusPendingApproval || rq.RuleID != "sshd-permit-root-no" { - t.Errorf("requested remediation = %+v", rq) + if rq.Status != StatusApproved || rq.RuleID != "sshd-permit-root-no" { + t.Errorf("requested remediation = %+v, want status=approved", rq) } - if len(calls) != 1 || calls[0].Code != audit.RemediationRequested { - t.Errorf("audit calls = %+v, want one requested", calls) + if rq.ReviewedAt == nil || rq.ReviewedBy != nil { + t.Errorf("auto-approved: want reviewed_at set + reviewed_by nil, got reviewed_at=%v reviewed_by=%v", rq.ReviewedAt, rq.ReviewedBy) + } + if !strings.Contains(rq.ReviewNote, "auto-approved") { + t.Errorf("auto-approved review_note = %q, want an auto-approved note", rq.ReviewNote) + } + // Auto-approve emits remediation.requested then remediation.approved. + if len(calls) != 2 || calls[0].Code != audit.RemediationRequested || calls[1].Code != audit.RemediationApproved { + t.Errorf("audit calls = %+v, want requested + approved", calls) } - // Duplicate open: rejected. - if _, err := svc.Request(ctx, hostID, "sshd-permit-root-no", nil, user); !errors.Is(err, ErrDuplicateOpen) { + // Duplicate open (the auto-approved request is still open): rejected. + if _, err := svc.Request(ctx, hostID, "sshd-permit-root-no", nil, user, false); !errors.Is(err, ErrDuplicateOpen) { t.Errorf("duplicate Request err = %v, want ErrDuplicateOpen", err) } // Invalid input (empty rule). - if _, err := svc.Request(ctx, hostID, " ", nil, user); !errors.Is(err, ErrInvalidInput) { + if _, err := svc.Request(ctx, hostID, " ", nil, user, false); !errors.Is(err, ErrInvalidInput) { t.Errorf("empty rule err = %v, want ErrInvalidInput", err) } - // Reopen after the prior is rejected: a fresh request succeeds. + // Approval-required (requiresApproval=true) inserts pending_approval and + // reopens after a terminal state: reject -> a fresh request succeeds. reviewer := seedUser(t, pool, "reviewer") - if _, err := svc.Reject(ctx, rq.ID, reviewer, "not now"); err != nil { + pend, err := svc.Request(ctx, hostID, "needs-approval", nil, user, true) + if err != nil { + t.Fatalf("approval-required Request: %v", err) + } + if pend.Status != StatusPendingApproval { + t.Errorf("approval-required status = %v, want pending_approval", pend.Status) + } + if _, err := svc.Reject(ctx, pend.ID, reviewer, "not now"); err != nil { t.Fatalf("Reject: %v", err) } - if _, err := svc.Request(ctx, hostID, "sshd-permit-root-no", nil, user); err != nil { + if _, err := svc.Request(ctx, hostID, "needs-approval", nil, user, true); err != nil { t.Errorf("reopen after reject failed: %v", err) } }) @@ -146,7 +163,7 @@ func TestLifecycle_Transitions(t *testing.T) { svc := NewService(pool, fakeEmitter(&calls)) // approve path - a, _ := svc.Request(ctx, hostID, "rule-a", nil, requester) + a, _ := svc.Request(ctx, hostID, "rule-a", nil, requester, true) got, err := svc.Approve(ctx, a.ID, reviewer, "ok") if err != nil || got.Status != StatusApproved || got.ReviewedBy == nil || *got.ReviewedBy != reviewer { t.Fatalf("Approve = %+v, err %v", got, err) @@ -157,7 +174,7 @@ func TestLifecycle_Transitions(t *testing.T) { } // reject path - b, _ := svc.Request(ctx, hostID, "rule-b", nil, requester) + b, _ := svc.Request(ctx, hostID, "rule-b", nil, requester, true) if got, err := svc.Reject(ctx, b.ID, reviewer, "no"); err != nil || got.Status != StatusRejected { t.Fatalf("Reject = %+v, err %v", got, err) } @@ -198,7 +215,7 @@ func TestSeparationOfDuties(t *testing.T) { hostID := seedHost(t, pool, user) svc := NewService(pool, fakeEmitter(&[]emitCall{})) - rq, _ := svc.Request(ctx, hostID, "rule-s", nil, user) + rq, _ := svc.Request(ctx, hostID, "rule-s", nil, user, true) // self-approve blocked if _, err := svc.Approve(ctx, rq.ID, user, "me"); !errors.Is(err, ErrSelfReview) { t.Errorf("self-approve err = %v, want ErrSelfReview", err) @@ -260,7 +277,7 @@ func TestProjectLift_AndOverlayNeverMutatesRuleState(t *testing.T) { } // Request persists the projection snapshot. - rq, err := svc.Request(ctx, hostID, "rule-active", nil, requester) + rq, err := svc.Request(ctx, hostID, "rule-active", nil, requester, false) if err != nil { t.Fatalf("Request: %v", err) } diff --git a/internal/server/api_remediation_test.go b/internal/server/api_remediation_test.go index 234f6196..8a91b5e3 100644 --- a/internal/server/api_remediation_test.go +++ b/internal/server/api_remediation_test.go @@ -62,8 +62,10 @@ func TestAPI_Remediation_LifecycleAndRBAC(t *testing.T) { if err := json.NewDecoder(or.Body).Decode(&created); err != nil { t.Fatalf("decode created: %v", err) } - if created.Status != "pending_approval" || created.RuleID != "sshd-permit-root-no" { - t.Errorf("created = %+v", created) + // Free core: a single-rule request is auto-approved on creation (no + // separate approver). See remediation_governance_adr.md. + if created.Status != "approved" || created.RuleID != "sshd-permit-root-no" { + t.Errorf("created = %+v, want status=approved (auto-approved free core)", created) } // --- duplicate open -> 409 --- @@ -81,16 +83,22 @@ func TestAPI_Remediation_LifecycleAndRBAC(t *testing.T) { t.Errorf("anonymous list status = %d, want 401/403", ar.StatusCode) } - // --- approve: ops_lead is 403 (no remediation:approve) --- - oa := doReq(t, asRole(t, "POST", base+"/"+created.ID+":approve", auth.RoleOpsLead, map[string]any{})) + // --- approve endpoint RBAC + separation of duties. The free-core POST + // above auto-approves, so it never yields a pending row; seed an + // approval-required (pending_approval) request directly to exercise the + // approve endpoint (this is the licensed bulk/auto track's shape). --- + pendID := seedPendingRemediation(t, pool, hostID, roleUserIDs[auth.RoleOpsLead], "needs-approval") + pendPath := base + "/" + pendID.String() + + // ops_lead (the requester; no remediation:approve) -> 403 + oa := doReq(t, asRole(t, "POST", pendPath+":approve", auth.RoleOpsLead, map[string]any{})) oa.Body.Close() if oa.StatusCode != http.StatusForbidden { t.Fatalf("ops_lead approve status = %d, want 403", oa.StatusCode) } - // --- approve: security_admin (different user from the ops_lead - // requester) succeeds --- - sa := doReq(t, asRole(t, "POST", base+"/"+created.ID+":approve", + // security_admin (different user from the requester) approves -> 200 + sa := doReq(t, asRole(t, "POST", pendPath+":approve", auth.RoleSecurityAdmin, map[string]any{"note": "reviewed"})) defer sa.Body.Close() if sa.StatusCode != http.StatusOK { @@ -102,21 +110,17 @@ func TestAPI_Remediation_LifecycleAndRBAC(t *testing.T) { t.Errorf("approved status = %q, want approved", approved.Status) } - // --- re-approve -> 409 wrong state --- - ra := doReq(t, asRole(t, "POST", base+"/"+created.ID+":approve", auth.RoleSecurityAdmin, map[string]any{})) + // re-approve -> 409 wrong state + ra := doReq(t, asRole(t, "POST", pendPath+":approve", auth.RoleSecurityAdmin, map[string]any{})) ra.Body.Close() if ra.StatusCode != http.StatusConflict { t.Errorf("re-approve status = %d, want 409", ra.StatusCode) } - // --- separation of duties at the HTTP layer: a security_admin - // requests, then tries to approve their own -> 409 self_review --- - selfReq := doReq(t, asRole(t, "POST", base, auth.RoleSecurityAdmin, - map[string]any{"host_id": hostID.String(), "rule_id": "self-rule"})) - defer selfReq.Body.Close() - var selfRR apiRem - _ = json.NewDecoder(selfReq.Body).Decode(&selfRR) - selfAp := doReq(t, asRole(t, "POST", base+"/"+selfRR.ID+":approve", auth.RoleSecurityAdmin, map[string]any{})) + // separation of duties at the HTTP layer: the security_admin requester + // cannot approve their own pending request -> 409 self_review. + selfID := seedPendingRemediation(t, pool, hostID, roleUserIDs[auth.RoleSecurityAdmin], "self-rule") + selfAp := doReq(t, asRole(t, "POST", base+"/"+selfID.String()+":approve", auth.RoleSecurityAdmin, map[string]any{})) selfAp.Body.Close() if selfAp.StatusCode != http.StatusConflict { t.Errorf("self-approve status = %d, want 409 (separation of duties)", selfAp.StatusCode) @@ -181,23 +185,18 @@ func TestAPI_Remediation_ExecuteFreeCore(t *testing.T) { t.Fatalf("viewer execute status = %d, want 403", o.StatusCode) } - // security_admin has the perm, but the request is still pending - // (not approved) -> 409 wrong_state. NOT 402. - pre := doReq(t, asRole(t, "POST", execURL, auth.RoleSecurityAdmin, map[string]any{})) + // Executing a NON-approved (pending) request -> 409 wrong_state, NOT 402 + // (free core, no license gate). The free-core request above auto-approves, + // so seed a pending row to exercise this path. + pendID := seedPendingRemediation(t, pool, hostID, roleUserIDs[auth.RoleOpsLead], "rule-pending") + pre := doReq(t, asRole(t, "POST", base+"/"+pendID.String()+":execute", auth.RoleSecurityAdmin, map[string]any{})) pre.Body.Close() if pre.StatusCode != http.StatusConflict { t.Fatalf("execute-before-approve status = %d, want 409", pre.StatusCode) } - // Approve it (security_admin != ops_lead requester). - ap := doReq(t, asRole(t, "POST", base+"/"+created.ID+":approve", - auth.RoleSecurityAdmin, map[string]any{"note": "ok"})) - ap.Body.Close() - if ap.StatusCode != http.StatusOK { - t.Fatalf("approve status = %d, want 200", ap.StatusCode) - } - - // Now execute -> 202 Accepted, a remediation job enqueued. + // The free-core request is already approved (auto). Execute it directly + // -> 202 Accepted, a remediation job enqueued. ex := doReq(t, asRole(t, "POST", execURL, auth.RoleSecurityAdmin, map[string]any{})) var acc struct { RequestID string `json:"request_id"` @@ -226,6 +225,22 @@ func TestAPI_Remediation_ExecuteFreeCore(t *testing.T) { }) } +// seedPendingRemediation inserts a pending_approval remediation request +// directly (the approval-required / licensed track's shape). The free-core HTTP +// POST auto-approves, so this is how the approve-endpoint and pending-execute +// paths are exercised at the HTTP layer. +func seedPendingRemediation(t *testing.T, pool *pgxpool.Pool, hostID, requestedBy uuid.UUID, ruleID string) uuid.UUID { + t.Helper() + id := uuid.Must(uuid.NewV7()) + if _, err := pool.Exec(context.Background(), + `INSERT INTO remediation_requests (id, host_id, rule_id, status, requested_by) + VALUES ($1, $2, $3, 'pending_approval', $4)`, + id, hostID, ruleID, requestedBy); err != nil { + t.Fatalf("seed pending remediation: %v", err) + } + return id +} + // countRemediationJobs counts pending remediation jobs on the queue. func countRemediationJobs(t *testing.T, pool *pgxpool.Pool) int { t.Helper() diff --git a/internal/server/remediation_handlers.go b/internal/server/remediation_handlers.go index fa83b0e9..27dd2fce 100644 --- a/internal/server/remediation_handlers.go +++ b/internal/server/remediation_handlers.go @@ -207,7 +207,10 @@ func (h *handlers) RequestRemediation(w http.ResponseWriter, r *http.Request) { s := uuid.UUID(*req.ScanRunId) scanRunID = &s } - rq, err := h.remediationSvc.Request(ctx, hostID, req.RuleId, scanRunID, requestedBy) + // Free-core: single-rule manual remediation is auto-approved (no separate + // approver). Bulk/auto remediation (OpenWatch+) will request with approval + // required via its own gated handler. + rq, err := h.remediationSvc.Request(ctx, hostID, req.RuleId, scanRunID, requestedBy, false) if mapRemediationErr(w, err) { return } diff --git a/internal/worker/remediation_worker_test.go b/internal/worker/remediation_worker_test.go index 621fbbae..5d7d87bf 100644 --- a/internal/worker/remediation_worker_test.go +++ b/internal/worker/remediation_worker_test.go @@ -36,7 +36,7 @@ func seedApprovedRequest(t *testing.T, pool *pgxpool.Pool, svc *remediation.Serv requester := seedUniqueUser(t, pool) reviewer := seedUniqueUser(t, pool) ctx := context.Background() - rq, err := svc.Request(ctx, hostID, ruleID, nil, requester) + rq, err := svc.Request(ctx, hostID, ruleID, nil, requester, true) if err != nil { t.Fatalf("seed request: %v", err) } diff --git a/specs/api/remediation.spec.yaml b/specs/api/remediation.spec.yaml index ae042ff2..29b5e52e 100644 --- a/specs/api/remediation.spec.yaml +++ b/specs/api/remediation.spec.yaml @@ -121,7 +121,7 @@ spec: acceptance_criteria: - id: AC-01 - description: 'Request inserts a pending_approval row (host_id, rule_id, requested_by, optional scan_run_id and projected-lift snapshot) and emits remediation.requested; a second Request for the same host+rule while one is open (pending_approval/approved/dry_run_complete/executing) returns ErrDuplicateOpen; an empty rule_id returns ErrInvalidInput; a fresh Request after the prior one reached a terminal state succeeds' + description: 'Approval is conditional on the remediation track. A free-core Request (requiresApproval=false) is AUTO-APPROVED: it inserts an approved row (host_id, rule_id, requested_by, optional scan_run_id and projected-lift snapshot, reviewed_at set, reviewed_by NULL, an auto-approved review_note) and emits remediation.requested then remediation.approved. A requiresApproval=true Request (the licensed bulk/auto track) inserts pending_approval instead. A second Request for the same host+rule while one is open (pending_approval/approved/dry_run_complete/executing) returns ErrDuplicateOpen; an empty rule_id returns ErrInvalidInput; a fresh Request after the prior one reached a terminal state succeeds' priority: critical references_constraints: [C-02] - id: AC-02 @@ -137,11 +137,11 @@ spec: priority: critical references_constraints: [C-01, C-07] - id: AC-05 - description: 'Endpoint RBAC + lifecycle: POST /api/v1/remediation/requests requires remediation:request and 404s an unknown host_id; GET /requests, GET /requests/{id}, GET /requests/{id}/steps require remediation:read; POST :approve and :reject require remediation:approve; anonymous callers are rejected on all. A full request->approve happy path round-trips through the HTTP layer' + description: 'Endpoint RBAC + lifecycle: POST /api/v1/remediation/requests requires remediation:request, 404s an unknown host_id, and (free core) returns the request AUTO-APPROVED; GET /requests, GET /requests/{id}, GET /requests/{id}/steps require remediation:read; POST :approve and :reject require remediation:approve and enforce separation of duties (the requester cannot approve their own request, 409); anonymous callers are rejected on all. The approve endpoint is exercised on an approval-required (pending_approval) request since the free-core request auto-approves' priority: critical references_constraints: [C-05, C-03] - id: AC-06 - description: 'Execute/rollback are FREE core: a caller WITH remediation:execute executing an approved request gets 202 and a remediation job is enqueued; a caller LACKING remediation:execute is 403; executing a non-approved request is 409; rolling back (remediation:rollback) a non-executed request is 409. No remediation act endpoint returns 402. :dry-run returns 501. request/approve/reject and the GET reads return their normal status codes.' + description: 'Execute/rollback are FREE core: a caller WITH remediation:execute executing an approved request gets 202 and a remediation job is enqueued. A free-core request is auto-approved, so it executes directly with no separate approval step. A caller LACKING remediation:execute is 403; executing a non-approved (pending_approval) request is 409; rolling back (remediation:rollback) a non-executed request is 409. No remediation act endpoint returns 402. :dry-run returns 501. request/approve/reject and the GET reads return their normal status codes.' priority: critical references_constraints: [C-06] - id: AC-07