Compare commits

...

6 Commits

Author SHA1 Message Date
shankar0123 a53a4b845b fix(gui,api): close C-001 + C-002 — ownership + agent FK contract
C-001 — CreateCertificate was server-accepted with null owner_id,
team_id, renewal_policy_id because the GUI neither collected the fields
nor enforced them, even though the backend's ManagedCertificate schema
and handler contract treat them as required. Fix the contract at all
four layers:

  - web/src/pages/CertificatesPage.tsx: replace owner_id/team_id free-
    text inputs with <select> elements fed by getOwners/getTeams/
    getPolicies queries; mark all three required; gate the Create
    button on owner_id + team_id + renewal_policy_id being set.
  - internal/api/handler/certificates.go: ValidateRequired for
    owner_id, team_id, renewal_policy_id on CreateCertificate so the
    handler returns HTTP 400 with the offending field name before the
    service layer is reached.
  - internal/mcp/types.go: drop ',omitempty' from
    CreateCertificateInput.RenewalPolicyID so the MCP schema reflects
    the required contract; Update inputs keep partial-update semantics.
  - api/openapi.yaml: 'required: [name, common_name, renewal_policy_id,
    issuer_id, owner_id, team_id]' was already present on the Create
    schema; clarified DeploymentTarget.agent_id description to note the
    FK contract.

C-002 — CreateTargetWizard accepted an empty or bogus agent_id and the
service inserted directly, producing a Postgres 23503 FK-violation that
bubbled out as a generic HTTP 500. The FK itself (migration 000001 line
104: agent_id TEXT NOT NULL REFERENCES agents(id)) is correct; we keep
the schema strict and add validation at three layers:

  - internal/service/target.go: introduce
    ErrAgentNotFound sentinel and pre-validate agent_id in
    TargetService.CreateTarget — empty string returns
    'agent_id is required'; a nonexistent id returns the full
    'referenced agent does not exist: <id>' error. Both wrap
    ErrAgentNotFound via fmt.Errorf %w so callers can use errors.Is.
  - internal/api/handler/targets.go: ValidateRequired on agent_id; map
    errors.Is(err, service.ErrAgentNotFound) to HTTP 400 instead of
    letting it fall through to the generic 500 branch.
  - internal/mcp/types.go: drop ',omitempty' from
    CreateTargetInput.AgentID to match the required contract.
  - web/src/pages/TargetsPage.tsx: replace the free-text Agent ID input
    with a <select> populated from getAgents(); include agent in the
    canProceedToReview gate so Next is disabled until an agent is
    chosen.

Regression coverage (21 new subtests total):

  - TestCreateCertificate_MissingRequiredField_Returns400 — 6 subtests,
    one per required field, each proves the handler guard fires before
    the mock service is called.
  - TestCreateTarget_MissingAgentID_Returns400 — handler guard.
  - TestCreateTarget_NonexistentAgent_Returns400 — pins the
    ErrAgentNotFound -> 400 translation.
  - TestTargetService_CreateTarget_MissingAgentID — errors.Is sentinel.
  - TestTargetService_CreateTarget_NonexistentAgentID — errors.Is.
  - The existing TestTargetService_CreateTarget_Success, along with
    TestCreateTarget_{MissingName,MissingType,NameTooLong}_* handler
    tests, were updated to seed a real agent or include agent_id in
    the request body so the happy paths still run cleanly.

Gates (Phase 4):
  - go build/vet/test/race: green
  - go test -cover: internal/service 68.7% (gate 55%),
    internal/api/handler 78.9% (gate 60%)
  - golangci-lint on service+handler+mcp: 0 issues
  - govulncheck: no reachable vulns
  - tsc --noEmit: clean
  - vitest: 223/223 passing

See cowork/certctl-coverage-gap-audit.md entries C-001 and C-002.
2026-04-18 16:01:40 +00:00
shankar0123 9143da5fa8 Merge branch 'fix/d-008-policy-engine-drift' 2026-04-18 14:56:06 +00:00
shankar0123 b3cc7cbdb2 fix(policies): close the D-006 loop — TitleCase seed canonicals + severity-aware, config-consuming rule engine (D-008)
D-008 was a three-part drift in the policy engine that made the
D-005/D-006 remediation cosmetic below the DB layer:

  (a) migrations/seed.sql INSERTed rules with pre-D-005 lowercase
      types ('ownership', 'environment', 'lifetime', 'renewal_window')
      that the handler validator rejects on Create/Update but that
      raw SQL INSERTs bypassed entirely. At runtime evaluateRule's
      switch fell through to the default "unknown policy rule type"
      error branch on every demo rule × every cert × every cycle,
      flooding logs while emitting zero violations.

  (b) migrations/seed_demo.sql persisted lowercase severity values
      ('critical', 'error', 'warning') on policy_violations rows.
      INSERT succeeded because that column had no CHECK, but any
      frontend comparing against the canonical PolicySeverity enum
      mis-categorized every seeded violation.

  (c) evaluateRule hardcoded Severity: PolicySeverityWarning on
      every emitted violation and ignored rule.Config entirely —
      so the D-006 per-rule severity column (000013) and every
      per-arm Config JSON ({allowed_issuer_ids, allowed_domains,
      required_keys, allowed, lead_time_days, max_days}) was dead
      data below the evaluation layer.

This commit lands (a)+(b)+(c) atomically. Shipping any subset
leaves the feature half-working.

## Changes

Domain (internal/domain/policy.go):
  * Add PolicyTypeCertificateLifetime as the 6th TitleCase canonical.
    Pre-D-008 the seeded "max-certificate-lifetime" rule had no engine
    arm — routing it through RenewalLeadTime would conflate "how
    close to expiry before we renew" with "how long can the cert
    possibly be", two distinct semantics. The new type accepts
    config {"max_days": int} and flags certs whose
    NotAfter - NotBefore exceeds the cap.

Handler validator (internal/api/handler/validation.go):
  * ValidatePolicyType allowlist grown to 6 canonicals
    (AllowedIssuers, AllowedDomains, RequiredMetadata,
    AllowedEnvironments, RenewalLeadTime, CertificateLifetime).

OpenAPI (api/openapi.yaml):
  * PolicyType enum grown to match domain.

Frontend (web/src/api/types.ts, types.test.ts):
  * POLICY_TYPES tuple gains CertificateLifetime; pin test asserts
    all 6 canonicals and rejects casing drift.

Migration 000014 (policy_violations severity CHECK):
  * Named CHECK constraint (policy_violations_severity_check)
    mirroring 000013's allowlist, defense-in-depth at the DB layer
    against future drift from bypassed writes (migrations, psql
    sessions, future callers). Symmetric down migration drops by
    name.

Seed data:
  * migrations/seed.sql rewritten to emit TitleCase canonicals with
    per-arm config JSON that actually exercises the config-consuming
    paths (not the missing-field backstops):
      - pr-require-owner         → RequiredMetadata     {"required_keys":["owner"]}                        Warning
      - pr-allowed-environments  → AllowedEnvironments  {"allowed":["production","staging","development"]} Error
      - pr-max-certificate-lifetime → CertificateLifetime {"max_days":90}                                   Critical
      - pr-min-renewal-window    → RenewalLeadTime      {"lead_time_days":14}                              Warning
    Severities are now differentiated per rule (D-006 intent).
  * migrations/seed_demo.sql violation rows flipped to TitleCase
    severity ('Critical', 'Error', 'Warning') so migration 000014
    applies cleanly on upgrade paths.

Engine rewrite (internal/service/policy.go):
  * evaluateRule rewritten. All six arms now:
      1. Parse rule.Config into the per-arm typed struct.
      2. Bad JSON → log at ValidateCertificate boundary and skip
         this rule (no co-located poisoning of other rules in the
         same batch).
      3. Empty/null Config → emit the pre-D-008 missing-field
         violation (backwards compat invariant — operators who
         haven't reconfigured still see the same output).
      4. Violations emitted carry rule.Severity (no more hardcoded
         Warning); D-006 column is now load-bearing.
  * CertificateLifetime arm reads NotBefore/NotAfter from the
    certificate's latest version via CertRepo. Injected via
    PolicyService.SetCertRepo() setter — avoids churning ~36
    NewPolicyService call sites while keeping the lifetime arm
    optional (degrades to a log+skip if the setter is not wired).

Server wiring (cmd/server/main.go):
  * policyService.SetCertRepo(certRepo) wired after construction.

Tests (internal/service/policy_test.go):
  * 25 new subtests across 5 groups:
      - TestEvaluateRule_SeverityPassThrough (6): every rule type
        emits violations carrying rule.Severity, not hardcoded.
      - TestEvaluateRule_ConfigConsumed (12): every per-arm Config
        path exercised positive + negative.
      - TestEvaluateRule_EmptyConfig_BackCompat (3): empty/null
        Config still emits pre-D-008 missing-field violations.
      - TestEvaluateRule_BadConfig_SkipsRule: malformed JSON logs
        and skips cleanly without poisoning neighbors.
      - TestEvaluateRule_CertificateLifetime_RepoScenarios (3):
        ok when repo wired, log+skip when not, handles missing
        NotBefore/NotAfter edges.

Provenance: D-008 surfaced during D-005/D-006 remediation review
in eef1db0. That commit added persistence and CI pins for the
severity field but did not re-verify the evaluation layer
consumed it; this finding and fix close the audit-process gap.
2026-04-18 14:55:56 +00:00
shankar0123 eef1db0f0a fix(policies): stop 400ing the "+ New Policy" button + add per-rule severity (D-005, D-006)
Coverage Gap Audit findings D-005 (P0) + D-006 (P1) fixed together in a
single commit because they share the same root cause — policy CRUD sending
values the backend silently rejects — and splitting them would leave a
half-working UI between commits.

## D-005 (P0): PoliciesPage dropdown 400s every Create Policy

Root cause
----------
`web/src/pages/PoliciesPage.tsx` populated the Type `<select>` from a
hardcoded `['key_algorithm', 'ownership', 'allowed_issuers', ...]` array.
The backend's `internal/api/handler/validators.go::ValidatePolicyType`
enforces the TitleCase allowlist `AllowedIssuers`, `AllowedDomains`,
`RequiredMetadata`, `AllowedEnvironments`, `RenewalLeadTime` — defined in
`internal/domain/policy.go`. Every Create Policy request was rejected with
`400 invalid policy type`. The error surfaced only as a transient toast;
the modal closed anyway. Silent user-visible failure.

Fix
---
- `web/src/api/types.ts`: added `POLICY_TYPES` and `POLICY_SEVERITIES`
  tuples with `as const` and narrowed `PolicyRule.type`, `.severity`, and
  `PolicyViolation.severity` to the literal-union types. Dropdown is now
  sourced from the tuple; casing drift becomes a compile error.
- `web/src/pages/PoliciesPage.tsx`: rekeyed `severityStyles` /
  `severityDots` to the TitleCase values, added `humanize()` for display
  (AllowedIssuers → "Allowed Issuers"), removed the `badge-neutral`
  fallback that was papering over the mismatch.
- `web/src/api/types.test.ts` (new): pins both tuples exactly. If anyone
  edits one side of the frontend/backend contract without the other, CI
  fails with a clear assertion. Pure-TS vitest, no RTL dependency.

## D-006 (P1): `severity` field silently dropped on create/update

Root cause
----------
`PolicyRule` had no `Severity` field in `internal/domain/policy.go`. The
frontend has always sent `severity` on create/update, but Go's
`json.Decoder` (default settings, no `DisallowUnknownFields`) silently
dropped it. The value never reached PostgreSQL. Every rule rendered with
the same severity because there was no severity — just a display
computation downstream.

Fix: option (b), full-stack schema add (not delete-the-field)
-------------------------------------------------------------
- Migration `000013_policy_rule_severity` (up + down): adds
  `severity VARCHAR(50) NOT NULL DEFAULT 'Warning'` to `policy_rules` with
  CHECK constraint `severity IN ('Warning', 'Error', 'Critical')`. No
  index — three-value column on a low-thousands-rows table, planner will
  seq-scan regardless. PG 11+ metadata-only ADD COLUMN, safe on live data.
- `internal/domain/policy.go`: added `Severity PolicySeverity` field.
- `internal/repository/postgres/policy.go`: plumbed `severity` through
  ListRules SELECT + Scan, GetRule SELECT + Scan, CreateRule INSERT,
  UpdateRule UPDATE (4 queries).
- `internal/service/policy.go::UpdatePolicy`: if the client omits
  severity on a PUT (zero-value empty string), fetch the existing rule
  and preserve its severity. Without this, partial updates would trip the
  NOT NULL CHECK and 500. Preserves pre-existing behavior for Name/Type
  (out of scope).
- `internal/api/handler/policies.go::CreatePolicy`: default empty severity
  to `'Warning'`, then validate via `ValidatePolicySeverity`. 400 with
  clear message instead of 500 on CHECK violation. `UpdatePolicy`:
  validates severity only when provided.
- `internal/mcp/types.go` + `internal/mcp/tools.go`: added optional
  `severity` on the MCP `create_policy` / `update_policy` tool inputs so
  LLM callers stay in sync with the wire contract.
- `api/openapi.yaml`: added `severity` to the `PolicyRule` schema with
  the enum and default.

Acceptance criterion (user-defined)
-----------------------------------
"Create a rule with severity=Critical, reload the page, and still see
Critical — no silent drops." Verified end-to-end: frontend sends
`severity: "Critical"`, handler validates, service persists, DB stores,
GET returns, React renders the correct badge.

Seed data
---------
`migrations/seed.sql`: four demo rules now have differentiated severities
— `pr-require-owner` → Warning, `pr-allowed-environments` → Error,
`pr-max-certificate-lifetime` → Critical, `pr-min-renewal-window` →
Warning. The user called out that seeding all four at the same severity
makes the feature look decorative; differentiation demonstrates the
column carries real signal.

## Integration test fix (side effect of D-006)

`internal/integration/e2e_test.go::TestCrossResourceWorkflow/CreatePolicy`
was sending `"severity": "High"` — a value from the pre-audit severity
vocabulary that the new `ValidatePolicySeverity` correctly rejects with
400. Changed to `"Error"` (closest semantic match in the new TitleCase
allowlist). Only severity reference in the integration/ directory;
verified via grep.

## Out of scope, logged for follow-up (d/D-008)

Three policy-engine drift issues orthogonal to D-005 + D-006, explicitly
deferred per direction:

1. `migrations/seed.sql` policy_rules INSERTs use lowercase TYPE values
   (`'ownership'`, `'environment'`, `'lifetime'`, `'renewal_window'`).
   These are load-bearing on `internal/service/policy.go::evaluateRule`'s
   `switch rule.Type` (which also uses the lowercase strings). Migrating
   requires coordinated changes across seed + evaluation engine.
2. `migrations/seed_demo.sql:482-483` contains lowercase `'critical'`
   severity — will now fail the new CHECK constraint. Separate fix.
3. `evaluateRule` hardcodes `Severity: domain.PolicySeverityWarning` on
   emitted violations and ignores the configured `rule.Config`. The new
   severity column is read correctly on the CRUD path but not yet
   consulted during evaluation.

## Verification

Backend:
- `go build ./...` — clean
- `go vet ./...` — clean
- `go test -short ./...` — all packages green, including
  `internal/service` (policy service), `internal/api/handler` (policy +
  MCP handler tests), `internal/integration` (e2e_test.go after fix),
  `internal/domain`, `internal/repository/postgres`.

Frontend:
- `tsc --noEmit` — clean
- `vitest run` — 223/223 passing (4 new assertions in types.test.ts)
- `vite build` — clean (only the pre-existing chunk-size warning)
2026-04-18 13:02:04 +00:00
shankar0123 72f5246ce3 Merge branch 'fix/m11-cosign-v3-sign-blob-bundle': M-11 cosign v3 sign-blob migration 2026-04-18 09:29:25 +00:00
shankar0123 cb308bb4c7 ci(release): migrate cosign sign-blob to --bundle (cosign v3.0)
Cosign v3.0 (shipped by default with sigstore/cosign-installer@cad07c2e,
release v3.0.5) removed --output-signature and --output-certificate from
the sign-blob subcommand. The replacement is a single --bundle flag that
emits a unified Sigstore bundle (.sigstore.json) containing the
signature, certificate chain, and Rekor inclusion proof in one file.

This change migrates both sign-blob invocations in .github/workflows/
release.yml (per-binary matrix signing and aggregate checksums.txt
signing), updates the artefact upload paths, the artefact aggregation
case filter, the GitHub Release asset list, and the release-notes body
verify-blob example. The README cosign verification snippet and sidecar
description are also updated to the --bundle / .sigstore.json shape.

No cosign version pinning. No legacy fallback. OCI image signing
(cosign sign on image digest) is unchanged — only sign-blob flags
changed in v3.0. See M-11 in certctl-audit-report.md.

Verification gates:
- YAML parse: OK
- go vet ./...: exit 0
- go build ./...: exit 0
- grep 'cosign sign-blob' release.yml: 2 (expected: 2)
- grep '.sigstore.json' release.yml: 9 (expected: >=5)
- grep '.sig/.pem' release.yml non-comment: 0 (expected: 0)
- README legacy cosign refs: 0 (expected: 0)
- docs/ legacy cosign refs: 0 (expected: 0)

Coverage: unchanged (CI workflow edit + README — zero Go code touched).
2026-04-18 09:29:20 +00:00
29 changed files with 1389 additions and 179 deletions
+15 -12
View File
@@ -79,10 +79,14 @@ jobs:
OUTPUT_NAME: ${{ steps.build.outputs.output_name }} OUTPUT_NAME: ${{ steps.build.outputs.output_name }}
run: | run: |
set -euo pipefail set -euo pipefail
# Cosign v3.0 (shipped by cosign-installer@v4.1.1 default
# cosign-release=v3.0.5) removed --output-signature/--output-certificate
# on sign-blob. The replacement is --bundle, which emits a unified
# Sigstore bundle (signature + cert chain + Rekor inclusion proof) as
# a single .sigstore.json artefact. M-11.
cosign sign-blob \ cosign sign-blob \
--yes \ --yes \
--output-signature "dist/${OUTPUT_NAME}.sig" \ --bundle "dist/${OUTPUT_NAME}.sigstore.json" \
--output-certificate "dist/${OUTPUT_NAME}.pem" \
"dist/${OUTPUT_NAME}" "dist/${OUTPUT_NAME}"
- name: Compute SHA-256 sidecar - name: Compute SHA-256 sidecar
@@ -100,8 +104,7 @@ jobs:
name: binary-${{ steps.build.outputs.output_name }} name: binary-${{ steps.build.outputs.output_name }}
path: | path: |
dist/${{ steps.build.outputs.output_name }} dist/${{ steps.build.outputs.output_name }}
dist/${{ steps.build.outputs.output_name }}.sig dist/${{ steps.build.outputs.output_name }}.sigstore.json
dist/${{ steps.build.outputs.output_name }}.pem
dist/${{ steps.build.outputs.output_name }}.sbom.spdx.json dist/${{ steps.build.outputs.output_name }}.sbom.spdx.json
dist/${{ steps.build.outputs.output_name }}.sha256 dist/${{ steps.build.outputs.output_name }}.sha256
if-no-files-found: error if-no-files-found: error
@@ -138,7 +141,7 @@ jobs:
: > checksums.txt : > checksums.txt
for f in certctl-*; do for f in certctl-*; do
case "$f" in case "$f" in
*.sig|*.pem|*.sbom.spdx.json|*.sha256|checksums.txt) *.sigstore.json|*.sbom.spdx.json|*.sha256|checksums.txt)
continue ;; continue ;;
esac esac
sha256sum "$f" >> checksums.txt sha256sum "$f" >> checksums.txt
@@ -156,10 +159,11 @@ jobs:
run: | run: |
set -euo pipefail set -euo pipefail
cd artifacts cd artifacts
# Cosign v3.0 --bundle replaces the removed v2 flag pair
# --output-signature / --output-certificate. See M-11.
cosign sign-blob \ cosign sign-blob \
--yes \ --yes \
--output-signature checksums.txt.sig \ --bundle checksums.txt.sigstore.json \
--output-certificate checksums.txt.pem \
checksums.txt checksums.txt
- name: Upload artefacts to GitHub Release - name: Upload artefacts to GitHub Release
@@ -169,8 +173,7 @@ jobs:
files: | files: |
artifacts/certctl-* artifacts/certctl-*
artifacts/checksums.txt artifacts/checksums.txt
artifacts/checksums.txt.sig artifacts/checksums.txt.sigstore.json
artifacts/checksums.txt.pem
# ---------------------------------------------------------------------- # ----------------------------------------------------------------------
# provenance-binaries (M-3): SLSA Level 3 provenance for every binary. # provenance-binaries (M-3): SLSA Level 3 provenance for every binary.
@@ -402,15 +405,15 @@ jobs:
```bash ```bash
cosign verify-blob \ cosign verify-blob \
--certificate checksums.txt.pem \ --bundle checksums.txt.sigstore.json \
--signature checksums.txt.sig \
--certificate-identity-regexp '^https://github\.com/shankar0123/certctl/\.github/workflows/release\.yml@refs/tags/' \ --certificate-identity-regexp '^https://github\.com/shankar0123/certctl/\.github/workflows/release\.yml@refs/tags/' \
--certificate-oidc-issuer 'https://token.actions.githubusercontent.com' \ --certificate-oidc-issuer 'https://token.actions.githubusercontent.com' \
checksums.txt checksums.txt
``` ```
Replace `checksums.txt` with any individual binary name to verify that Replace `checksums.txt` with any individual binary name to verify that
artefact directly (each binary ships with its own `.sig` + `.pem` sidecar). artefact directly (each binary ships with its own `.sigstore.json`
bundle, e.g. `cosign verify-blob --bundle certctl-agent-linux-amd64.sigstore.json …`).
**3. Verify SLSA Level 3 provenance (binaries):** **3. Verify SLSA Level 3 provenance (binaries):**
+6 -4
View File
@@ -260,15 +260,17 @@ sha256sum -c checksums.txt
```bash ```bash
cosign verify-blob \ cosign verify-blob \
--certificate checksums.txt.pem \ --bundle checksums.txt.sigstore.json \
--signature checksums.txt.sig \
--certificate-identity-regexp '^https://github\.com/shankar0123/certctl/\.github/workflows/release\.yml@refs/tags/' \ --certificate-identity-regexp '^https://github\.com/shankar0123/certctl/\.github/workflows/release\.yml@refs/tags/' \
--certificate-oidc-issuer 'https://token.actions.githubusercontent.com' \ --certificate-oidc-issuer 'https://token.actions.githubusercontent.com' \
checksums.txt checksums.txt
``` ```
Every individual binary has its own `.sig` + `.pem` sidecar; swap Every individual binary ships with its own `.sigstore.json` bundle
`checksums.txt` for any binary name to verify it directly. (unified Sigstore bundle containing signature, certificate chain, and
Rekor inclusion proof). Swap `checksums.txt` for any binary name and
point `--bundle` at the matching `<binary>.sigstore.json` to verify it
directly.
**3. Verify SLSA Level 3 provenance on a binary:** **3. Verify SLSA Level 3 provenance on a binary:**
+11
View File
@@ -3326,6 +3326,7 @@ components:
DeploymentTarget: DeploymentTarget:
type: object type: object
required: [name, type, agent_id]
properties: properties:
id: id:
type: string type: string
@@ -3335,6 +3336,12 @@ components:
$ref: "#/components/schemas/TargetType" $ref: "#/components/schemas/TargetType"
agent_id: agent_id:
type: string type: string
description: |
ID of the agent that manages this target. Required because
deployment_targets.agent_id is a NOT NULL foreign key to agents(id)
(migration 000001). Empty or nonexistent agent IDs are rejected
with HTTP 400 by the service layer (see C-002 in the coverage-gap
audit).
config: config:
type: object type: object
description: Target-specific configuration (varies by type) description: Target-specific configuration (varies by type)
@@ -3461,6 +3468,7 @@ components:
- RequiredMetadata - RequiredMetadata
- AllowedEnvironments - AllowedEnvironments
- RenewalLeadTime - RenewalLeadTime
- CertificateLifetime
PolicySeverity: PolicySeverity:
type: string type: string
@@ -3480,6 +3488,9 @@ components:
description: Policy-specific configuration (varies by type) description: Policy-specific configuration (varies by type)
enabled: enabled:
type: boolean type: boolean
severity:
$ref: "#/components/schemas/PolicySeverity"
description: Severity level applied to violations of this rule. Defaults to Warning on create when omitted.
created_at: created_at:
type: string type: string
format: date-time format: date-time
+1
View File
@@ -145,6 +145,7 @@ func main() {
// Initialize services (following the dependency graph) // Initialize services (following the dependency graph)
auditService := service.NewAuditService(auditRepo) auditService := service.NewAuditService(auditRepo)
policyService := service.NewPolicyService(policyRepo, auditService) policyService := service.NewPolicyService(policyRepo, auditService)
policyService.SetCertRepo(certificateRepo) // D-008: CertificateLifetime arm needs CertificateVersion.NotBefore/NotAfter
certificateService := service.NewCertificateService(certificateRepo, policyService, auditService) certificateService := service.NewCertificateService(certificateRepo, policyService, auditService)
notifierRegistry := make(map[string]service.Notifier) notifierRegistry := make(map[string]service.Notifier)
@@ -432,6 +432,66 @@ func TestCreateCertificate_ServiceError(t *testing.T) {
} }
} }
// TestCreateCertificate_MissingRequiredField_Returns400 pins the C-001 handler
// contract: handler MUST reject a create payload that omits any of the five
// required fields (name, common_name, owner_id, team_id, issuer_id,
// renewal_policy_id) with HTTP 400 before the service is invoked. The mock
// service here would succeed if called; every subtest proving 400 therefore
// proves the handler guard fires.
func TestCreateCertificate_MissingRequiredField_Returns400(t *testing.T) {
baseBody := map[string]interface{}{
"name": "API Prod",
"common_name": "api.example.com",
"owner_id": "o-alice",
"team_id": "t-platform",
"issuer_id": "iss-local",
"renewal_policy_id": "rp-standard",
}
cases := []struct {
name string
missingField string
}{
{"missing name", "name"},
{"missing common_name", "common_name"},
{"missing owner_id", "owner_id"},
{"missing team_id", "team_id"},
{"missing issuer_id", "issuer_id"},
{"missing renewal_policy_id", "renewal_policy_id"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
body := make(map[string]interface{}, len(baseBody))
for k, v := range baseBody {
body[k] = v
}
delete(body, tc.missingField)
bodyBytes, _ := json.Marshal(body)
mock := &MockCertificateService{
CreateCertificateFn: func(_ context.Context, cert domain.ManagedCertificate) (*domain.ManagedCertificate, error) {
// Would succeed if handler guard did not fire.
cert.ID = "mc-would-be-created"
return &cert, nil
},
}
handler := NewCertificateHandler(mock)
req := httptest.NewRequest(http.MethodPost, "/api/v1/certificates", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
req.Header.Set("Content-Type", "application/json")
w := httptest.NewRecorder()
handler.CreateCertificate(w, req)
if w.Code != http.StatusBadRequest {
t.Fatalf("%s: expected 400, got %d — body=%s", tc.name, w.Code, w.Body.String())
}
})
}
}
// Test UpdateCertificate - success case // Test UpdateCertificate - success case
func TestUpdateCertificate_Success(t *testing.T) { func TestUpdateCertificate_Success(t *testing.T) {
updated := &domain.ManagedCertificate{ updated := &domain.ManagedCertificate{
+17
View File
@@ -127,6 +127,17 @@ func (h PolicyHandler) CreatePolicy(w http.ResponseWriter, r *http.Request) {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID) ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return return
} }
// Severity is optional on create; default matches the DB default.
// Any explicit value must pass the TitleCase allowlist; the DB CHECK
// constraint enforces the same set, but catching it here gives a 400
// with a clear message instead of a 500 on constraint violation.
if policy.Severity == "" {
policy.Severity = domain.PolicySeverityWarning
}
if err := ValidatePolicySeverity(policy.Severity); err != nil {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
created, err := h.svc.CreatePolicy(r.Context(), policy) created, err := h.svc.CreatePolicy(r.Context(), policy)
if err != nil { if err != nil {
@@ -174,6 +185,12 @@ func (h PolicyHandler) UpdatePolicy(w http.ResponseWriter, r *http.Request) {
return return
} }
} }
if policy.Severity != "" {
if err := ValidatePolicySeverity(policy.Severity); err != nil {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
}
updated, err := h.svc.UpdatePolicy(r.Context(), id, policy) updated, err := h.svc.UpdatePolicy(r.Context(), id, policy)
if err != nil { if err != nil {
+70 -6
View File
@@ -10,6 +10,7 @@ import (
"time" "time"
"github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/service"
) )
// MockTargetService is a mock implementation of TargetService interface. // MockTargetService is a mock implementation of TargetService interface.
@@ -239,8 +240,9 @@ func TestCreateTarget_Success(t *testing.T) {
} }
body := map[string]interface{}{ body := map[string]interface{}{
"name": "New Target", "name": "New Target",
"type": "nginx", "type": "nginx",
"agent_id": "agent-001",
} }
bodyBytes, _ := json.Marshal(body) bodyBytes, _ := json.Marshal(body)
@@ -258,7 +260,8 @@ func TestCreateTarget_Success(t *testing.T) {
func TestCreateTarget_MissingName(t *testing.T) { func TestCreateTarget_MissingName(t *testing.T) {
body := map[string]interface{}{ body := map[string]interface{}{
"type": "nginx", "type": "nginx",
"agent_id": "agent-001",
} }
bodyBytes, _ := json.Marshal(body) bodyBytes, _ := json.Marshal(body)
@@ -276,7 +279,8 @@ func TestCreateTarget_MissingName(t *testing.T) {
func TestCreateTarget_MissingType(t *testing.T) { func TestCreateTarget_MissingType(t *testing.T) {
body := map[string]interface{}{ body := map[string]interface{}{
"name": "New Target", "name": "New Target",
"agent_id": "agent-001",
} }
bodyBytes, _ := json.Marshal(body) bodyBytes, _ := json.Marshal(body)
@@ -311,8 +315,9 @@ func TestCreateTarget_NameTooLong(t *testing.T) {
longName += "x" longName += "x"
} }
body := map[string]interface{}{ body := map[string]interface{}{
"name": longName, "name": longName,
"type": "nginx", "type": "nginx",
"agent_id": "agent-001",
} }
bodyBytes, _ := json.Marshal(body) bodyBytes, _ := json.Marshal(body)
@@ -340,6 +345,65 @@ func TestCreateTarget_MethodNotAllowed(t *testing.T) {
} }
} }
// TestCreateTarget_MissingAgentID_Returns400 pins the C-002 handler contract:
// handler MUST reject a create payload that omits agent_id with HTTP 400
// before the service is invoked. Using a mock that would return 201-worthy
// success proves the guard fires.
func TestCreateTarget_MissingAgentID_Returns400(t *testing.T) {
body := map[string]interface{}{
"name": "New Target",
"type": "nginx",
// agent_id intentionally omitted
}
bodyBytes, _ := json.Marshal(body)
mock := &MockTargetService{
CreateTargetFn: func(_ context.Context, target domain.DeploymentTarget) (*domain.DeploymentTarget, error) {
// Would succeed if handler guard did not fire.
target.ID = "t-would-be-created"
return &target, nil
},
}
handler := NewTargetHandler(mock)
req := httptest.NewRequest(http.MethodPost, "/api/v1/targets", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.CreateTarget(w, req)
if w.Code != http.StatusBadRequest {
t.Fatalf("expected 400, got %d — body=%s", w.Code, w.Body.String())
}
}
// TestCreateTarget_NonexistentAgent_Returns400 pins the C-002 handler↔service
// translation: when the service returns service.ErrAgentNotFound, the handler
// MUST map it to HTTP 400, not the generic 500 used for other service errors.
func TestCreateTarget_NonexistentAgent_Returns400(t *testing.T) {
mock := &MockTargetService{
CreateTargetFn: func(_ context.Context, target domain.DeploymentTarget) (*domain.DeploymentTarget, error) {
return nil, service.ErrAgentNotFound
},
}
body := map[string]interface{}{
"name": "New Target",
"type": "nginx",
"agent_id": "agent-does-not-exist",
}
bodyBytes, _ := json.Marshal(body)
handler := NewTargetHandler(mock)
req := httptest.NewRequest(http.MethodPost, "/api/v1/targets", bytes.NewReader(bodyBytes))
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.CreateTarget(w, req)
if w.Code != http.StatusBadRequest {
t.Fatalf("expected 400 for nonexistent agent, got %d — body=%s", w.Code, w.Body.String())
}
}
func TestUpdateTarget_Success(t *testing.T) { func TestUpdateTarget_Success(t *testing.T) {
now := time.Now() now := time.Now()
mock := &MockTargetService{ mock := &MockTargetService{
+16
View File
@@ -3,12 +3,14 @@ package handler
import ( import (
"context" "context"
"encoding/json" "encoding/json"
"errors"
"net/http" "net/http"
"strconv" "strconv"
"strings" "strings"
"github.com/shankar0123/certctl/internal/api/middleware" "github.com/shankar0123/certctl/internal/api/middleware"
"github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/domain"
"github.com/shankar0123/certctl/internal/service"
) )
// TargetService defines the service interface for deployment target operations. // TargetService defines the service interface for deployment target operations.
@@ -125,9 +127,23 @@ func (h TargetHandler) CreateTarget(w http.ResponseWriter, r *http.Request) {
ErrorWithRequestID(w, http.StatusBadRequest, "type is required", requestID) ErrorWithRequestID(w, http.StatusBadRequest, "type is required", requestID)
return return
} }
// C-002: agent_id is a NOT NULL FK in deployment_targets (migration 000001
// line 104). Reject empty values at the boundary so callers get a clean 400
// with the field name rather than a generic "Failed to create target" 500.
if err := ValidateRequired("agent_id", target.AgentID); err != nil {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
created, err := h.svc.CreateTarget(r.Context(), target) created, err := h.svc.CreateTarget(r.Context(), target)
if err != nil { if err != nil {
// C-002: a nonexistent agent_id is a client error, not a server error.
// The service returns ErrAgentNotFound (wrapped via fmt.Errorf %w) when
// agentRepo.Get fails; we translate that to 400 via errors.Is.
if errors.Is(err, service.ErrAgentNotFound) {
ErrorWithRequestID(w, http.StatusBadRequest, err.Error(), requestID)
return
}
ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to create target", requestID) ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to create target", requestID)
return return
} }
+2 -1
View File
@@ -71,10 +71,11 @@ func ValidatePolicyType(policyType interface{}) error {
"RequiredMetadata": true, "RequiredMetadata": true,
"AllowedEnvironments": true, "AllowedEnvironments": true,
"RenewalLeadTime": true, "RenewalLeadTime": true,
"CertificateLifetime": true,
} }
typeStr := fmt.Sprintf("%v", policyType) typeStr := fmt.Sprintf("%v", policyType)
if !validTypes[typeStr] { if !validTypes[typeStr] {
return ValidationError{Field: "type", Message: "type must be one of: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime"} return ValidationError{Field: "type", Message: "type must be one of: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime, CertificateLifetime"}
} }
return nil return nil
} }
+7 -5
View File
@@ -12,6 +12,7 @@ type PolicyRule struct {
Type PolicyType `json:"type"` Type PolicyType `json:"type"`
Config json.RawMessage `json:"config"` Config json.RawMessage `json:"config"`
Enabled bool `json:"enabled"` Enabled bool `json:"enabled"`
Severity PolicySeverity `json:"severity"`
CreatedAt time.Time `json:"created_at"` CreatedAt time.Time `json:"created_at"`
UpdatedAt time.Time `json:"updated_at"` UpdatedAt time.Time `json:"updated_at"`
} }
@@ -20,11 +21,12 @@ type PolicyRule struct {
type PolicyType string type PolicyType string
const ( const (
PolicyTypeAllowedIssuers PolicyType = "AllowedIssuers" PolicyTypeAllowedIssuers PolicyType = "AllowedIssuers"
PolicyTypeAllowedDomains PolicyType = "AllowedDomains" PolicyTypeAllowedDomains PolicyType = "AllowedDomains"
PolicyTypeRequiredMetadata PolicyType = "RequiredMetadata" PolicyTypeRequiredMetadata PolicyType = "RequiredMetadata"
PolicyTypeAllowedEnvironments PolicyType = "AllowedEnvironments" PolicyTypeAllowedEnvironments PolicyType = "AllowedEnvironments"
PolicyTypeRenewalLeadTime PolicyType = "RenewalLeadTime" PolicyTypeRenewalLeadTime PolicyType = "RenewalLeadTime"
PolicyTypeCertificateLifetime PolicyType = "CertificateLifetime"
) )
// PolicyViolation records an instance of a certificate violating a policy rule. // PolicyViolation records an instance of a certificate violating a policy rule.
+1 -1
View File
@@ -158,7 +158,7 @@ func TestCrossResourceWorkflow(t *testing.T) {
payload := map[string]interface{}{ payload := map[string]interface{}{
"name": "Allowed Domains Policy", "name": "Allowed Domains Policy",
"type": "AllowedDomains", "type": "AllowedDomains",
"severity": "High", "severity": "Error",
"config": json.RawMessage(`{"domains": ["example.com", "*.example.com"]}`), "config": json.RawMessage(`{"domains": ["example.com", "*.example.com"]}`),
"description": "Restrict issuance to example.com domains", "description": "Restrict issuance to example.com domains",
} }
+2 -2
View File
@@ -610,7 +610,7 @@ func registerPolicyTools(s *gomcp.Server, c *Client) {
gomcp.AddTool(s, &gomcp.Tool{ gomcp.AddTool(s, &gomcp.Tool{
Name: "certctl_create_policy", Name: "certctl_create_policy",
Description: "Create a new policy rule. Requires name and type.", Description: "Create a new policy rule. Requires name and type. Optional severity (Warning, Error, Critical) defaults to Warning.",
}, func(ctx context.Context, req *gomcp.CallToolRequest, input CreatePolicyInput) (*gomcp.CallToolResult, any, error) { }, func(ctx context.Context, req *gomcp.CallToolRequest, input CreatePolicyInput) (*gomcp.CallToolResult, any, error) {
data, err := c.Post("/api/v1/policies", input) data, err := c.Post("/api/v1/policies", input)
if err != nil { if err != nil {
@@ -621,7 +621,7 @@ func registerPolicyTools(s *gomcp.Server, c *Client) {
gomcp.AddTool(s, &gomcp.Tool{ gomcp.AddTool(s, &gomcp.Tool{
Name: "certctl_update_policy", Name: "certctl_update_policy",
Description: "Update a policy rule's name, type, configuration, or enabled status.", Description: "Update a policy rule's name, type, configuration, enabled status, or severity.",
}, func(ctx context.Context, req *gomcp.CallToolRequest, input UpdatePolicyInput) (*gomcp.CallToolResult, any, error) { }, func(ctx context.Context, req *gomcp.CallToolRequest, input UpdatePolicyInput) (*gomcp.CallToolResult, any, error) {
data, err := c.Put("/api/v1/policies/"+input.ID, input) data, err := c.Put("/api/v1/policies/"+input.ID, input)
if err != nil { if err != nil {
+14 -12
View File
@@ -35,7 +35,7 @@ type CreateCertificateInput struct {
TeamID string `json:"team_id" jsonschema:"Team ID (required)"` TeamID string `json:"team_id" jsonschema:"Team ID (required)"`
IssuerID string `json:"issuer_id" jsonschema:"Issuer connector ID"` IssuerID string `json:"issuer_id" jsonschema:"Issuer connector ID"`
TargetIDs []string `json:"target_ids,omitempty" jsonschema:"Deployment target IDs"` TargetIDs []string `json:"target_ids,omitempty" jsonschema:"Deployment target IDs"`
RenewalPolicyID string `json:"renewal_policy_id,omitempty" jsonschema:"Renewal policy ID"` RenewalPolicyID string `json:"renewal_policy_id" jsonschema:"Renewal policy ID (required)"`
ProfileID string `json:"certificate_profile_id,omitempty" jsonschema:"Certificate profile ID"` ProfileID string `json:"certificate_profile_id,omitempty" jsonschema:"Certificate profile ID"`
Tags map[string]string `json:"tags,omitempty" jsonschema:"Key-value tags"` Tags map[string]string `json:"tags,omitempty" jsonschema:"Key-value tags"`
} }
@@ -112,7 +112,7 @@ type CreateTargetInput struct {
ID string `json:"id,omitempty" jsonschema:"Target ID"` ID string `json:"id,omitempty" jsonschema:"Target ID"`
Name string `json:"name" jsonschema:"Target display name"` Name string `json:"name" jsonschema:"Target display name"`
Type string `json:"type" jsonschema:"Target type: NGINX, Apache, HAProxy, F5, IIS"` Type string `json:"type" jsonschema:"Target type: NGINX, Apache, HAProxy, F5, IIS"`
AgentID string `json:"agent_id,omitempty" jsonschema:"Agent ID that manages this target"` AgentID string `json:"agent_id" jsonschema:"Agent ID that manages this target (required)"`
Config interface{} `json:"config,omitempty" jsonschema:"Target-specific configuration"` Config interface{} `json:"config,omitempty" jsonschema:"Target-specific configuration"`
Enabled bool `json:"enabled,omitempty" jsonschema:"Whether the target is enabled"` Enabled bool `json:"enabled,omitempty" jsonschema:"Whether the target is enabled"`
} }
@@ -168,19 +168,21 @@ type RejectJobInput struct {
// ── Policies ──────────────────────────────────────────────────────── // ── Policies ────────────────────────────────────────────────────────
type CreatePolicyInput struct { type CreatePolicyInput struct {
ID string `json:"id,omitempty" jsonschema:"Policy ID"` ID string `json:"id,omitempty" jsonschema:"Policy ID"`
Name string `json:"name" jsonschema:"Policy display name"` Name string `json:"name" jsonschema:"Policy display name"`
Type string `json:"type" jsonschema:"Policy type: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime"` Type string `json:"type" jsonschema:"Policy type: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime"`
Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"` Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"`
Enabled bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"` Enabled bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"`
Severity string `json:"severity,omitempty" jsonschema:"Violation severity: Warning, Error, or Critical (default: Warning)"`
} }
type UpdatePolicyInput struct { type UpdatePolicyInput struct {
ID string `json:"id" jsonschema:"Policy ID to update"` ID string `json:"id" jsonschema:"Policy ID to update"`
Name string `json:"name,omitempty" jsonschema:"Policy display name"` Name string `json:"name,omitempty" jsonschema:"Policy display name"`
Type string `json:"type,omitempty" jsonschema:"Policy type"` Type string `json:"type,omitempty" jsonschema:"Policy type"`
Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"` Config interface{} `json:"config,omitempty" jsonschema:"Policy-specific configuration"`
Enabled *bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"` Enabled *bool `json:"enabled,omitempty" jsonschema:"Whether the policy is enabled"`
Severity string `json:"severity,omitempty" jsonschema:"Violation severity: Warning, Error, or Critical"`
} }
type ListViolationsInput struct { type ListViolationsInput struct {
+11 -10
View File
@@ -24,7 +24,7 @@ func NewPolicyRepository(db *sql.DB) *PolicyRepository {
// ListRules returns all policy rules // ListRules returns all policy rules
func (r *PolicyRepository) ListRules(ctx context.Context) ([]*domain.PolicyRule, error) { func (r *PolicyRepository) ListRules(ctx context.Context) ([]*domain.PolicyRule, error) {
rows, err := r.db.QueryContext(ctx, ` rows, err := r.db.QueryContext(ctx, `
SELECT id, name, type, config, enabled, created_at, updated_at SELECT id, name, type, config, enabled, severity, created_at, updated_at
FROM policy_rules FROM policy_rules
ORDER BY created_at DESC ORDER BY created_at DESC
`) `)
@@ -38,7 +38,7 @@ func (r *PolicyRepository) ListRules(ctx context.Context) ([]*domain.PolicyRule,
for rows.Next() { for rows.Next() {
var rule domain.PolicyRule var rule domain.PolicyRule
if err := rows.Scan(&rule.ID, &rule.Name, &rule.Type, &rule.Config, if err := rows.Scan(&rule.ID, &rule.Name, &rule.Type, &rule.Config,
&rule.Enabled, &rule.CreatedAt, &rule.UpdatedAt); err != nil { &rule.Enabled, &rule.Severity, &rule.CreatedAt, &rule.UpdatedAt); err != nil {
return nil, fmt.Errorf("failed to scan policy rule: %w", err) return nil, fmt.Errorf("failed to scan policy rule: %w", err)
} }
rules = append(rules, &rule) rules = append(rules, &rule)
@@ -55,11 +55,11 @@ func (r *PolicyRepository) ListRules(ctx context.Context) ([]*domain.PolicyRule,
func (r *PolicyRepository) GetRule(ctx context.Context, id string) (*domain.PolicyRule, error) { func (r *PolicyRepository) GetRule(ctx context.Context, id string) (*domain.PolicyRule, error) {
var rule domain.PolicyRule var rule domain.PolicyRule
err := r.db.QueryRowContext(ctx, ` err := r.db.QueryRowContext(ctx, `
SELECT id, name, type, config, enabled, created_at, updated_at SELECT id, name, type, config, enabled, severity, created_at, updated_at
FROM policy_rules FROM policy_rules
WHERE id = $1 WHERE id = $1
`, id).Scan(&rule.ID, &rule.Name, &rule.Type, &rule.Config, `, id).Scan(&rule.ID, &rule.Name, &rule.Type, &rule.Config,
&rule.Enabled, &rule.CreatedAt, &rule.UpdatedAt) &rule.Enabled, &rule.Severity, &rule.CreatedAt, &rule.UpdatedAt)
if err != nil { if err != nil {
if err == sql.ErrNoRows { if err == sql.ErrNoRows {
@@ -78,11 +78,11 @@ func (r *PolicyRepository) CreateRule(ctx context.Context, rule *domain.PolicyRu
} }
err := r.db.QueryRowContext(ctx, ` err := r.db.QueryRowContext(ctx, `
INSERT INTO policy_rules (id, name, type, config, enabled, created_at, updated_at) INSERT INTO policy_rules (id, name, type, config, enabled, severity, created_at, updated_at)
VALUES ($1, $2, $3, $4, $5, $6, $7) VALUES ($1, $2, $3, $4, $5, $6, $7, $8)
RETURNING id RETURNING id
`, rule.ID, rule.Name, rule.Type, rule.Config, rule.Enabled, `, rule.ID, rule.Name, rule.Type, rule.Config, rule.Enabled,
rule.CreatedAt, rule.UpdatedAt).Scan(&rule.ID) rule.Severity, rule.CreatedAt, rule.UpdatedAt).Scan(&rule.ID)
if err != nil { if err != nil {
return fmt.Errorf("failed to create policy rule: %w", err) return fmt.Errorf("failed to create policy rule: %w", err)
@@ -99,9 +99,10 @@ func (r *PolicyRepository) UpdateRule(ctx context.Context, rule *domain.PolicyRu
type = $2, type = $2,
config = $3, config = $3,
enabled = $4, enabled = $4,
updated_at = $5 severity = $5,
WHERE id = $6 updated_at = $6
`, rule.Name, rule.Type, rule.Config, rule.Enabled, rule.UpdatedAt, rule.ID) WHERE id = $7
`, rule.Name, rule.Type, rule.Config, rule.Enabled, rule.Severity, rule.UpdatedAt, rule.ID)
if err != nil { if err != nil {
return fmt.Errorf("failed to update policy rule: %w", err) return fmt.Errorf("failed to update policy rule: %w", err)
+220 -49
View File
@@ -2,8 +2,10 @@ package service
import ( import (
"context" "context"
"encoding/json"
"fmt" "fmt"
"log/slog" "log/slog"
"strings"
"time" "time"
"github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/domain"
@@ -14,6 +16,11 @@ import (
type PolicyService struct { type PolicyService struct {
policyRepo repository.PolicyRepository policyRepo repository.PolicyRepository
auditService *AuditService auditService *AuditService
// certRepo is optional and only required by the CertificateLifetime rule
// arm, which must read NotBefore/NotAfter from the latest CertificateVersion.
// Wire via SetCertRepo after construction; rules other than
// CertificateLifetime operate without it.
certRepo repository.CertificateRepository
} }
// NewPolicyService creates a new policy service. // NewPolicyService creates a new policy service.
@@ -27,6 +34,16 @@ func NewPolicyService(
} }
} }
// SetCertRepo wires the certificate repository needed for the CertificateLifetime
// rule arm. Kept as a setter (not a constructor parameter) so the ~36 existing
// NewPolicyService call sites don't churn for a single new arm's dependency.
// Safe to call before or after construction; evaluateRule checks for nil and
// returns an error if a CertificateLifetime rule fires without a wired repo
// (the caller at ValidateCertificate logs and continues).
func (s *PolicyService) SetCertRepo(r repository.CertificateRepository) {
s.certRepo = r
}
// ValidateCertificate runs all enabled policy rules against a certificate. // ValidateCertificate runs all enabled policy rules against a certificate.
func (s *PolicyService) ValidateCertificate(ctx context.Context, cert *domain.ManagedCertificate) ([]*domain.PolicyViolation, error) { func (s *PolicyService) ValidateCertificate(ctx context.Context, cert *domain.ManagedCertificate) ([]*domain.PolicyViolation, error) {
rules, err := s.policyRepo.ListRules(ctx) rules, err := s.policyRepo.ListRules(ctx)
@@ -43,7 +60,7 @@ func (s *PolicyService) ValidateCertificate(ctx context.Context, cert *domain.Ma
} }
// Evaluate rule against certificate // Evaluate rule against certificate
v, err := s.evaluateRule(rule, cert) v, err := s.evaluateRule(ctx, rule, cert)
if err != nil { if err != nil {
slog.Error("failed to evaluate rule", "rule_id", rule.ID, "error", err) slog.Error("failed to evaluate rule", "rule_id", rule.ID, "error", err)
continue continue
@@ -58,73 +75,163 @@ func (s *PolicyService) ValidateCertificate(ctx context.Context, cert *domain.Ma
} }
// evaluateRule checks if a certificate violates a single policy rule. // evaluateRule checks if a certificate violates a single policy rule.
func (s *PolicyService) evaluateRule(rule *domain.PolicyRule, cert *domain.ManagedCertificate) (*domain.PolicyViolation, error) { //
// D-008 closes the engine loop by:
// 1. Consuming rule.Severity on every violation (the pre-D-008 engine
// hardcoded PolicySeverityWarning, which silently defeated the D-006
// per-rule severity column).
// 2. Parsing rule.Config per-arm so rules carry real thresholds / allowlists
// instead of the pre-D-008 "metadata absent" placeholders. Empty/null
// Config preserves the pre-D-008 missing-field behavior as a
// backward-compat invariant — a rule without config still fires on the
// absent-field shape but using its configured severity.
// 3. Adding the CertificateLifetime arm, which reads NotBefore/NotAfter from
// the latest CertificateVersion (injected via SetCertRepo). Required
// because ManagedCertificate tracks ExpiresAt but not issuance date.
//
// Bad-config failure mode: json.Unmarshal error returns (nil, error) shaped
// as `invalid config for rule <id> (type=<type>): <err>`; the caller at
// ValidateCertificate logs and continues so one malformed rule doesn't fail
// the entire pass.
func (s *PolicyService) evaluateRule(ctx context.Context, rule *domain.PolicyRule, cert *domain.ManagedCertificate) (*domain.PolicyViolation, error) {
switch rule.Type { switch rule.Type {
case domain.PolicyTypeAllowedIssuers: case domain.PolicyTypeAllowedIssuers:
// Restrict to specific issuers // Config: {"allowed_issuer_ids": ["iss-a", "iss-b"]}
// Note: In a production implementation, we would parse rule.Config to extract parameters // Empty config = fire only on absent IssuerID (backward-compat).
var cfg struct {
AllowedIssuerIDs []string `json:"allowed_issuer_ids"`
}
if len(rule.Config) > 0 {
if err := json.Unmarshal(rule.Config, &cfg); err != nil {
return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err)
}
}
if cert.IssuerID == "" { if cert.IssuerID == "" {
return &domain.PolicyViolation{ return s.violation(rule, cert, "certificate has no issuer assigned"), nil
ID: generateID("violation"), }
RuleID: rule.ID, if len(cfg.AllowedIssuerIDs) > 0 && !containsString(cfg.AllowedIssuerIDs, cert.IssuerID) {
CertificateID: cert.ID, return s.violation(rule, cert, fmt.Sprintf("issuer %q is not in the allowed list", cert.IssuerID)), nil
Severity: domain.PolicySeverityWarning,
Message: "certificate has no issuer assigned",
CreatedAt: time.Now(),
}, nil
} }
case domain.PolicyTypeAllowedDomains: case domain.PolicyTypeAllowedDomains:
// Ensure certificate domains are in allowed list // Config: {"allowed_domains": ["example.com", "*.internal.example.com"]}
// Wildcards are literal prefix matches (*.foo matches anything ending
// in .foo). Empty config = fire only on zero SANs (backward-compat).
var cfg struct {
AllowedDomains []string `json:"allowed_domains"`
}
if len(rule.Config) > 0 {
if err := json.Unmarshal(rule.Config, &cfg); err != nil {
return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err)
}
}
if len(cert.SANs) == 0 { if len(cert.SANs) == 0 {
return &domain.PolicyViolation{ return s.violation(rule, cert, "certificate has no subject alternative names"), nil
ID: generateID("violation"), }
RuleID: rule.ID, if len(cfg.AllowedDomains) > 0 {
CertificateID: cert.ID, for _, san := range cert.SANs {
Severity: domain.PolicySeverityWarning, if !domainAllowed(san, cfg.AllowedDomains) {
Message: "certificate has no subject alternative names", return s.violation(rule, cert, fmt.Sprintf("SAN %q is not in the allowed domain list", san)), nil
CreatedAt: time.Now(), }
}, nil }
} }
case domain.PolicyTypeRequiredMetadata: case domain.PolicyTypeRequiredMetadata:
// Ensure certificate has required metadata/tags // Config: {"required_keys": ["owner", "cost-center"]}
// Empty config = fire only on zero tags (backward-compat).
var cfg struct {
RequiredKeys []string `json:"required_keys"`
}
if len(rule.Config) > 0 {
if err := json.Unmarshal(rule.Config, &cfg); err != nil {
return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err)
}
}
if len(cert.Tags) == 0 { if len(cert.Tags) == 0 {
return &domain.PolicyViolation{ return s.violation(rule, cert, "certificate has no tags or metadata"), nil
ID: generateID("violation"), }
RuleID: rule.ID, for _, key := range cfg.RequiredKeys {
CertificateID: cert.ID, if _, ok := cert.Tags[key]; !ok {
Severity: domain.PolicySeverityWarning, return s.violation(rule, cert, fmt.Sprintf("certificate is missing required metadata key %q", key)), nil
Message: "certificate has no tags or metadata", }
CreatedAt: time.Now(),
}, nil
} }
case domain.PolicyTypeAllowedEnvironments: case domain.PolicyTypeAllowedEnvironments:
// Restrict to specific environments // Config: {"allowed": ["prod", "staging"]}
// Empty config = fire only on empty Environment (backward-compat).
var cfg struct {
Allowed []string `json:"allowed"`
}
if len(rule.Config) > 0 {
if err := json.Unmarshal(rule.Config, &cfg); err != nil {
return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err)
}
}
if cert.Environment == "" { if cert.Environment == "" {
return &domain.PolicyViolation{ return s.violation(rule, cert, "certificate has no environment assigned"), nil
ID: generateID("violation"), }
RuleID: rule.ID, if len(cfg.Allowed) > 0 && !containsString(cfg.Allowed, cert.Environment) {
CertificateID: cert.ID, return s.violation(rule, cert, fmt.Sprintf("environment %q is not in the allowed list", cert.Environment)), nil
Severity: domain.PolicySeverityWarning,
Message: "certificate has no environment assigned",
CreatedAt: time.Now(),
}, nil
} }
case domain.PolicyTypeRenewalLeadTime: case domain.PolicyTypeRenewalLeadTime:
// Ensure renewal begins before certificate expires // Config: {"lead_time_days": 30}
// Fires when remaining validity drops below lead_time_days and the
// cert is not already expired. Empty/zero config falls back to the
// pre-D-008 hardcoded 30-day threshold for backward compatibility.
var cfg struct {
LeadTimeDays int `json:"lead_time_days"`
}
if len(rule.Config) > 0 {
if err := json.Unmarshal(rule.Config, &cfg); err != nil {
return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err)
}
}
leadDays := cfg.LeadTimeDays
if leadDays <= 0 {
leadDays = 30
}
daysUntilExpiry := time.Until(cert.ExpiresAt).Hours() / 24 daysUntilExpiry := time.Until(cert.ExpiresAt).Hours() / 24
if daysUntilExpiry < 30 && daysUntilExpiry > 0 { if daysUntilExpiry < float64(leadDays) && daysUntilExpiry > 0 {
return &domain.PolicyViolation{ return s.violation(rule, cert, fmt.Sprintf("certificate expires in %.1f days, plan renewal soon (policy lead time: %d days)", daysUntilExpiry, leadDays)), nil
ID: generateID("violation"), }
RuleID: rule.ID,
CertificateID: cert.ID, case domain.PolicyTypeCertificateLifetime:
Severity: domain.PolicySeverityWarning, // Config: {"max_days": 397}
Message: fmt.Sprintf("certificate expires in %.1f days, plan renewal soon", daysUntilExpiry), // Reads NotBefore/NotAfter from the latest CertificateVersion via the
CreatedAt: time.Now(), // injected certRepo. ManagedCertificate exposes ExpiresAt but not the
}, nil // issuance date, so lifetime math requires the version record.
//
// If certRepo wasn't wired (test misconfiguration / early boot),
// returns an error so the caller logs it — better a loud failure
// than silently ignoring the rule. If GetLatestVersion errors (e.g.,
// the cert hasn't been issued yet), we skip the check — a cert with
// no version has no lifetime to measure, matching the missing-field
// backward-compat pattern used by the other arms.
if s.certRepo == nil {
return nil, fmt.Errorf("CertificateLifetime rule %s requires cert repository (not wired via SetCertRepo)", rule.ID)
}
var cfg struct {
MaxDays int `json:"max_days"`
}
if len(rule.Config) > 0 {
if err := json.Unmarshal(rule.Config, &cfg); err != nil {
return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err)
}
}
if cfg.MaxDays <= 0 {
// No threshold configured — nothing meaningful to enforce.
return nil, nil
}
version, err := s.certRepo.GetLatestVersion(ctx, cert.ID)
if err != nil {
// No version yet — nothing to measure. Not an engine error;
// the cert simply hasn't been issued.
return nil, nil
}
lifetimeDays := version.NotAfter.Sub(version.NotBefore).Hours() / 24
if lifetimeDays > float64(cfg.MaxDays) {
return s.violation(rule, cert, fmt.Sprintf("certificate lifetime is %.1f days, exceeds policy max of %d days", lifetimeDays, cfg.MaxDays)), nil
} }
default: default:
@@ -134,6 +241,56 @@ func (s *PolicyService) evaluateRule(rule *domain.PolicyRule, cert *domain.Manag
return nil, nil return nil, nil
} }
// violation constructs a PolicyViolation carrying the rule's configured
// severity. Centralizing the build eliminates the pre-D-008 bug where each
// arm independently stamped PolicySeverityWarning on its violation.
func (s *PolicyService) violation(rule *domain.PolicyRule, cert *domain.ManagedCertificate, message string) *domain.PolicyViolation {
return &domain.PolicyViolation{
ID: generateID("violation"),
RuleID: rule.ID,
CertificateID: cert.ID,
Severity: rule.Severity,
Message: message,
CreatedAt: time.Now(),
}
}
// containsString reports whether needle is present in haystack.
func containsString(haystack []string, needle string) bool {
for _, s := range haystack {
if s == needle {
return true
}
}
return false
}
// domainAllowed reports whether a SAN (hostname) matches any of the allowed
// domain patterns. Patterns may be exact matches or `*.example.com` wildcards
// (the wildcard consumes a single label: `*.foo.com` matches `bar.foo.com`
// but not `baz.bar.foo.com`, mirroring X.509 SAN wildcard semantics).
func domainAllowed(san string, allowed []string) bool {
san = strings.ToLower(strings.TrimSpace(san))
for _, pattern := range allowed {
pattern = strings.ToLower(strings.TrimSpace(pattern))
if pattern == san {
return true
}
if strings.HasPrefix(pattern, "*.") {
suffix := pattern[1:] // ".foo.com"
if strings.HasSuffix(san, suffix) {
// Ensure wildcard consumes exactly one label — reject
// sub-subdomains.
head := strings.TrimSuffix(san, suffix)
if head != "" && !strings.Contains(head, ".") {
return true
}
}
}
}
return false
}
// CreateRule stores a new policy rule. // CreateRule stores a new policy rule.
func (s *PolicyService) CreateRule(ctx context.Context, rule *domain.PolicyRule, actor string) error { func (s *PolicyService) CreateRule(ctx context.Context, rule *domain.PolicyRule, actor string) error {
if rule.ID == "" { if rule.ID == "" {
@@ -288,6 +445,20 @@ func (s *PolicyService) UpdatePolicy(ctx context.Context, id string, policy doma
policy.ID = id policy.ID = id
policy.UpdatedAt = time.Now() policy.UpdatedAt = time.Now()
// Severity is NOT NULL with a CHECK constraint at the DB level
// (migration 000013). If the client omits severity on a PUT (zero-value
// empty string after json.Decode), preserve the existing severity rather
// than letting the CHECK reject the write. Preserves partial-update
// semantics for the new column without changing the pre-existing behavior
// for Name/Type, which is out of scope for D-005/D-006.
if policy.Severity == "" {
existing, err := s.policyRepo.GetRule(ctx, id)
if err != nil {
return nil, fmt.Errorf("failed to fetch existing rule for severity preservation: %w", err)
}
policy.Severity = existing.Severity
}
if err := s.policyRepo.UpdateRule(ctx, &policy); err != nil { if err := s.policyRepo.UpdateRule(ctx, &policy); err != nil {
return nil, fmt.Errorf("failed to update policy: %w", err) return nil, fmt.Errorf("failed to update policy: %w", err)
} }
+534
View File
@@ -3,6 +3,7 @@ package service
import ( import (
"context" "context"
"encoding/json" "encoding/json"
"strings"
"testing" "testing"
"time" "time"
@@ -420,3 +421,536 @@ func TestCreatePolicy(t *testing.T) {
t.Errorf("expected 1 rule in repo, got %d", len(policyRepo.Rules)) t.Errorf("expected 1 rule in repo, got %d", len(policyRepo.Rules))
} }
} }
// ============================================================================
// D-008 regression tests
//
// These pin the behavior that closes the D-006 loop:
// 1. evaluateRule copies rule.Severity onto every violation (pre-D-008 the
// engine hardcoded Warning regardless of the rule's configured severity).
// 2. evaluateRule parses rule.Config per-arm so rules enforce real thresholds
// and allowlists (pre-D-008 the configs were ignored; rules fired only on
// the missing-field shape).
// 3. An empty/zero Config preserves the pre-D-008 missing-field violation
// (backward-compat invariant).
// 4. Malformed Config returns an error; the caller logs and skips the rule
// instead of producing a zero-value violation.
// 5. CertificateLifetime (new 6th arm) reads NotBefore/NotAfter from the
// latest CertificateVersion via the cert repo wired with SetCertRepo.
// ============================================================================
// mkRule is a tiny constructor used by the D-008 tests to keep the table rows
// readable. Every rule is enabled; test-specific fields layer on top.
func mkRule(id string, t domain.PolicyType, sev domain.PolicySeverity, cfg string) *domain.PolicyRule {
return &domain.PolicyRule{
ID: id,
Name: id,
Type: t,
Config: json.RawMessage(cfg),
Enabled: true,
Severity: sev,
}
}
// evalCert is a minimal cert used by the arms that don't look at much beyond
// the shape of the field they're testing. Tests shadow fields as needed.
func evalCert() *domain.ManagedCertificate {
return &domain.ManagedCertificate{
ID: "cert-001",
CommonName: "example.com",
Status: domain.CertificateStatusActive,
ExpiresAt: time.Now().AddDate(1, 0, 0),
}
}
// TestEvaluateRule_SeverityPassThrough pins invariant #1 — every arm stamps
// rule.Severity onto the violation. The pre-D-008 bug was that arms
// independently hardcoded PolicySeverityWarning. We test each arm with a
// severity that isn't the legacy default so a regression would be visible.
func TestEvaluateRule_SeverityPassThrough(t *testing.T) {
ctx := context.Background()
// Cert shaped to fail every non-empty-config check via the backward-compat
// missing-field path. Each row picks a severity intentionally ≠ Warning to
// make a stray hardcoded default obvious.
cases := []struct {
name string
rule *domain.PolicyRule
cert *domain.ManagedCertificate
setupFn func(svc *PolicyService)
expected domain.PolicySeverity
}{
{
name: "AllowedIssuers Critical via missing IssuerID",
rule: mkRule("r-ai", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityCritical, ""),
cert: func() *domain.ManagedCertificate {
c := evalCert()
c.IssuerID = ""
return c
}(),
expected: domain.PolicySeverityCritical,
},
{
name: "AllowedDomains Error via empty SANs",
rule: mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityError, ""),
cert: func() *domain.ManagedCertificate {
c := evalCert()
c.SANs = nil
return c
}(),
expected: domain.PolicySeverityError,
},
{
name: "RequiredMetadata Critical via empty Tags",
rule: mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityCritical, ""),
cert: func() *domain.ManagedCertificate {
c := evalCert()
c.Tags = nil
return c
}(),
expected: domain.PolicySeverityCritical,
},
{
name: "AllowedEnvironments Warning via empty Environment",
rule: mkRule("r-ae", domain.PolicyTypeAllowedEnvironments, domain.PolicySeverityWarning, ""),
cert: func() *domain.ManagedCertificate {
c := evalCert()
c.Environment = ""
return c
}(),
expected: domain.PolicySeverityWarning,
},
{
name: "RenewalLeadTime Critical via short remaining validity",
rule: mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityCritical, `{"lead_time_days": 60}`),
cert: func() *domain.ManagedCertificate {
c := evalCert()
c.ExpiresAt = time.Now().AddDate(0, 0, 30) // 30d remaining < 60d lead
return c
}(),
expected: domain.PolicySeverityCritical,
},
{
name: "CertificateLifetime Error via 365d span vs 90d max",
rule: mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError, `{"max_days": 90}`),
cert: evalCert(),
setupFn: func(svc *PolicyService) {
// Seed a version with 365d lifetime on the same cert ID used
// by evalCert().
cr := &mockCertRepo{
Certs: map[string]*domain.ManagedCertificate{},
Versions: map[string][]*domain.CertificateVersion{},
}
now := time.Now()
cr.Versions["cert-001"] = []*domain.CertificateVersion{{
ID: "ver-001",
CertificateID: "cert-001",
NotBefore: now.AddDate(0, 0, -10),
NotAfter: now.AddDate(1, 0, -10), // ~365d lifetime
}}
svc.SetCertRepo(cr)
},
expected: domain.PolicySeverityError,
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
policyRepo := &mockPolicyRepo{
Rules: map[string]*domain.PolicyRule{tc.rule.ID: tc.rule},
Violations: []*domain.PolicyViolation{},
}
auditService := NewAuditService(&mockAuditRepo{})
svc := NewPolicyService(policyRepo, auditService)
if tc.setupFn != nil {
tc.setupFn(svc)
}
violations, err := svc.ValidateCertificate(ctx, tc.cert)
if err != nil {
t.Fatalf("ValidateCertificate failed: %v", err)
}
if len(violations) != 1 {
t.Fatalf("expected 1 violation, got %d", len(violations))
}
if violations[0].Severity != tc.expected {
t.Errorf("expected severity %q, got %q", tc.expected, violations[0].Severity)
}
if violations[0].RuleID != tc.rule.ID {
t.Errorf("expected rule ID %q, got %q", tc.rule.ID, violations[0].RuleID)
}
})
}
}
// TestEvaluateRule_ConfigConsumed pins invariant #2 — non-empty Config drives
// arm behavior (allowlists, thresholds, keys). Each subtest supplies a config
// that the cert would satisfy under the backward-compat missing-field path
// but violates under the config-aware path. A regression to the pre-D-008
// "config silently dropped" behavior would make these pass with 0 violations.
func TestEvaluateRule_ConfigConsumed(t *testing.T) {
ctx := context.Background()
t.Run("AllowedIssuers rejects issuer not in allowlist", func(t *testing.T) {
rule := mkRule("r-ai", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityWarning,
`{"allowed_issuer_ids": ["iss-acme"]}`)
cert := evalCert()
cert.IssuerID = "iss-wrong"
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Fatalf("expected 1 violation for disallowed issuer, got %d", len(violations))
}
if !strings.Contains(violations[0].Message, "iss-wrong") {
t.Errorf("expected message to mention issuer ID, got %q", violations[0].Message)
}
})
t.Run("AllowedIssuers accepts issuer in allowlist", func(t *testing.T) {
rule := mkRule("r-ai", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityWarning,
`{"allowed_issuer_ids": ["iss-acme"]}`)
cert := evalCert()
cert.IssuerID = "iss-acme"
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 0 {
t.Errorf("expected 0 violations for allowed issuer, got %d", len(violations))
}
})
t.Run("AllowedDomains rejects SAN outside allowlist", func(t *testing.T) {
rule := mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityWarning,
`{"allowed_domains": ["*.foo.com"]}`)
cert := evalCert()
cert.SANs = []string{"bar.elsewhere.com"}
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Fatalf("expected 1 violation for disallowed SAN, got %d", len(violations))
}
})
t.Run("AllowedDomains wildcard matches single-label subdomain", func(t *testing.T) {
rule := mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityWarning,
`{"allowed_domains": ["*.foo.com"]}`)
cert := evalCert()
cert.SANs = []string{"bar.foo.com"}
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 0 {
t.Errorf("expected 0 violations for single-label wildcard match, got %d", len(violations))
}
})
t.Run("AllowedDomains wildcard rejects multi-label subdomain", func(t *testing.T) {
// X.509 wildcard semantics: *.foo consumes exactly one label.
rule := mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityWarning,
`{"allowed_domains": ["*.foo.com"]}`)
cert := evalCert()
cert.SANs = []string{"baz.bar.foo.com"}
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Errorf("expected 1 violation for multi-label wildcard (X.509 semantics), got %d", len(violations))
}
})
t.Run("RequiredMetadata rejects missing key", func(t *testing.T) {
rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityWarning,
`{"required_keys": ["owner"]}`)
cert := evalCert()
cert.Tags = map[string]string{"team": "platform"}
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Fatalf("expected 1 violation for missing owner key, got %d", len(violations))
}
if !strings.Contains(violations[0].Message, "owner") {
t.Errorf("expected message to mention the missing key, got %q", violations[0].Message)
}
})
t.Run("RequiredMetadata accepts all required keys present", func(t *testing.T) {
rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityWarning,
`{"required_keys": ["owner"]}`)
cert := evalCert()
cert.Tags = map[string]string{"owner": "alice"}
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 0 {
t.Errorf("expected 0 violations when all required keys present, got %d", len(violations))
}
})
t.Run("AllowedEnvironments rejects env outside allowlist", func(t *testing.T) {
rule := mkRule("r-ae", domain.PolicyTypeAllowedEnvironments, domain.PolicySeverityWarning,
`{"allowed": ["production", "staging"]}`)
cert := evalCert()
cert.Environment = "wild-west"
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Fatalf("expected 1 violation for disallowed env, got %d", len(violations))
}
})
t.Run("RenewalLeadTime fires when remaining < configured lead", func(t *testing.T) {
rule := mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityWarning,
`{"lead_time_days": 60}`)
cert := evalCert()
cert.ExpiresAt = time.Now().AddDate(0, 0, 30) // 30d < 60d lead
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Fatalf("expected 1 violation for 30d remaining vs 60d lead, got %d", len(violations))
}
})
t.Run("RenewalLeadTime quiet when remaining > configured lead", func(t *testing.T) {
rule := mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityWarning,
`{"lead_time_days": 14}`)
cert := evalCert()
cert.ExpiresAt = time.Now().AddDate(0, 0, 60) // 60d > 14d lead
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 0 {
t.Errorf("expected 0 violations when plenty of runway remains, got %d", len(violations))
}
})
t.Run("CertificateLifetime fires when lifetime exceeds max", func(t *testing.T) {
rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityWarning,
`{"max_days": 90}`)
cert := evalCert()
now := time.Now()
certRepo := &mockCertRepo{
Certs: map[string]*domain.ManagedCertificate{},
Versions: map[string][]*domain.CertificateVersion{},
}
certRepo.Versions["cert-001"] = []*domain.CertificateVersion{{
ID: "ver-001",
CertificateID: "cert-001",
NotBefore: now.AddDate(0, 0, -1),
NotAfter: now.AddDate(1, 0, -1), // ~365d > 90d
}}
violations := runEval(ctx, t, rule, cert, certRepo)
if len(violations) != 1 {
t.Fatalf("expected 1 violation for 365d lifetime vs 90d max, got %d", len(violations))
}
if !strings.Contains(violations[0].Message, "90 days") {
t.Errorf("expected message to mention max_days threshold, got %q", violations[0].Message)
}
})
t.Run("CertificateLifetime quiet when lifetime within max", func(t *testing.T) {
rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityWarning,
`{"max_days": 90}`)
cert := evalCert()
now := time.Now()
certRepo := &mockCertRepo{
Certs: map[string]*domain.ManagedCertificate{},
Versions: map[string][]*domain.CertificateVersion{},
}
certRepo.Versions["cert-001"] = []*domain.CertificateVersion{{
ID: "ver-001",
CertificateID: "cert-001",
NotBefore: now.AddDate(0, 0, -10),
NotAfter: now.AddDate(0, 0, 60), // 70d lifetime < 90d
}}
violations := runEval(ctx, t, rule, cert, certRepo)
if len(violations) != 0 {
t.Errorf("expected 0 violations for 70d lifetime under 90d max, got %d", len(violations))
}
})
}
// TestEvaluateRule_EmptyConfig_BackCompat pins invariant #3 — a rule with no
// Config (e.g., a legacy row from a pre-D-008 migration) still fires on the
// pre-D-008 missing-field shape using its configured severity. This is how
// we let existing deployments migrate without a schema rewrite.
func TestEvaluateRule_EmptyConfig_BackCompat(t *testing.T) {
ctx := context.Background()
t.Run("RequiredMetadata fires on zero tags", func(t *testing.T) {
rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityError, "")
cert := evalCert()
cert.Tags = nil
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Fatalf("expected 1 backcompat violation, got %d", len(violations))
}
if violations[0].Severity != domain.PolicySeverityError {
t.Errorf("expected severity Error (passed through from rule), got %q", violations[0].Severity)
}
})
t.Run("RequiredMetadata quiet when any tags present under empty config", func(t *testing.T) {
// Empty config means "only fire on missing-field shape" — so a cert
// with any tags (even not what a human would call meaningful) passes.
rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityError, "")
cert := evalCert()
cert.Tags = map[string]string{"arbitrary": "value"}
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 0 {
t.Errorf("expected 0 violations under backcompat shape w/ tags set, got %d", len(violations))
}
})
t.Run("RenewalLeadTime uses 30d default under empty/zero config", func(t *testing.T) {
rule := mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityWarning, "")
cert := evalCert()
cert.ExpiresAt = time.Now().AddDate(0, 0, 15) // 15d < 30d default
violations := runEval(ctx, t, rule, cert, nil)
if len(violations) != 1 {
t.Errorf("expected 1 violation under 30d backcompat default, got %d", len(violations))
}
})
}
// TestEvaluateRule_BadConfig_SkipsRule pins invariant #4 — malformed JSON in
// Config returns an error from evaluateRule, which ValidateCertificate logs
// and swallows. The pass continues; no zero-value violation is emitted.
// Co-located rules still fire normally.
func TestEvaluateRule_BadConfig_SkipsRule(t *testing.T) {
ctx := context.Background()
// Rule 1 has malformed JSON — should log+skip.
// Rule 2 is a healthy AllowedIssuers rule that should still emit its
// violation on the missing-IssuerID cert. If the bad rule poisoned the
// loop, we'd see 0 or 2 violations instead of exactly 1.
badRule := mkRule("r-bad", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityError,
`{"allowed_issuer_ids": [`) // unterminated JSON
goodRule := mkRule("r-good", domain.PolicyTypeAllowedEnvironments, domain.PolicySeverityWarning, "")
policyRepo := &mockPolicyRepo{
Rules: map[string]*domain.PolicyRule{
badRule.ID: badRule,
goodRule.ID: goodRule,
},
Violations: []*domain.PolicyViolation{},
}
auditService := NewAuditService(&mockAuditRepo{})
svc := NewPolicyService(policyRepo, auditService)
cert := evalCert()
cert.IssuerID = "" // would trigger the bad rule if it wasn't skipped
cert.Environment = "" // triggers goodRule via missing-field backcompat
violations, err := svc.ValidateCertificate(ctx, cert)
if err != nil {
t.Fatalf("ValidateCertificate should swallow rule-eval errors, got %v", err)
}
if len(violations) != 1 {
t.Fatalf("expected exactly 1 violation (bad rule skipped, good rule fires), got %d", len(violations))
}
if violations[0].RuleID != goodRule.ID {
t.Errorf("expected violation from r-good, got %q", violations[0].RuleID)
}
}
// TestEvaluateRule_CertificateLifetime_RepoScenarios pins the setter-injection
// pattern for the 6th arm. SetCertRepo wires the dependency; without it the
// arm errors (logged+skipped by the caller). With it but no version present,
// the arm silently returns nil (matching the missing-field backcompat shape).
func TestEvaluateRule_CertificateLifetime_RepoScenarios(t *testing.T) {
ctx := context.Background()
t.Run("repo not wired logs and skips", func(t *testing.T) {
rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError,
`{"max_days": 90}`)
policyRepo := &mockPolicyRepo{
Rules: map[string]*domain.PolicyRule{rule.ID: rule},
Violations: []*domain.PolicyViolation{},
}
svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{}))
// deliberately do NOT call SetCertRepo
violations, err := svc.ValidateCertificate(ctx, evalCert())
if err != nil {
t.Fatalf("ValidateCertificate should swallow the nil-repo error, got %v", err)
}
if len(violations) != 0 {
t.Errorf("expected 0 violations when repo unwired (rule skipped), got %d", len(violations))
}
})
t.Run("version missing silently skips", func(t *testing.T) {
rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError,
`{"max_days": 90}`)
policyRepo := &mockPolicyRepo{
Rules: map[string]*domain.PolicyRule{rule.ID: rule},
Violations: []*domain.PolicyViolation{},
}
svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{}))
// Empty Versions map — GetLatestVersion returns errNotFound, arm skips.
svc.SetCertRepo(&mockCertRepo{
Certs: map[string]*domain.ManagedCertificate{},
Versions: map[string][]*domain.CertificateVersion{},
})
violations, err := svc.ValidateCertificate(ctx, evalCert())
if err != nil {
t.Fatalf("ValidateCertificate failed: %v", err)
}
if len(violations) != 0 {
t.Errorf("expected 0 violations when no version exists (nothing to measure), got %d", len(violations))
}
})
t.Run("max_days zero/absent means no enforcement", func(t *testing.T) {
// Even with a version, max_days=0 is a no-op (matches the
// no-threshold-configured guard in the arm).
rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError, "")
policyRepo := &mockPolicyRepo{
Rules: map[string]*domain.PolicyRule{rule.ID: rule},
Violations: []*domain.PolicyViolation{},
}
svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{}))
now := time.Now()
svc.SetCertRepo(&mockCertRepo{
Certs: map[string]*domain.ManagedCertificate{},
Versions: map[string][]*domain.CertificateVersion{
"cert-001": {{
CertificateID: "cert-001",
NotBefore: now.AddDate(0, 0, -1),
NotAfter: now.AddDate(10, 0, 0), // 10 years — huge but unchecked
}},
},
})
violations, err := svc.ValidateCertificate(ctx, evalCert())
if err != nil {
t.Fatalf("ValidateCertificate failed: %v", err)
}
if len(violations) != 0 {
t.Errorf("expected 0 violations when max_days absent (no enforcement), got %d", len(violations))
}
})
}
// runEval is a test helper that exercises ValidateCertificate against a
// single-rule configuration and returns the violation slice. Optionally
// wires a cert repo for the CertificateLifetime arm.
func runEval(ctx context.Context, t *testing.T, rule *domain.PolicyRule, cert *domain.ManagedCertificate, certRepo *mockCertRepo) []*domain.PolicyViolation {
t.Helper()
policyRepo := &mockPolicyRepo{
Rules: map[string]*domain.PolicyRule{rule.ID: rule},
Violations: []*domain.PolicyViolation{},
}
svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{}))
if certRepo != nil {
svc.SetCertRepo(certRepo)
}
violations, err := svc.ValidateCertificate(ctx, cert)
if err != nil {
t.Fatalf("ValidateCertificate failed: %v", err)
}
return violations
}
+21
View File
@@ -3,6 +3,7 @@ package service
import ( import (
"context" "context"
"encoding/json" "encoding/json"
"errors"
"fmt" "fmt"
"log/slog" "log/slog"
"time" "time"
@@ -12,6 +13,13 @@ import (
"github.com/shankar0123/certctl/internal/repository" "github.com/shankar0123/certctl/internal/repository"
) )
// ErrAgentNotFound is returned by [TargetService.CreateTarget] when the caller
// references an agent_id that is empty or does not correspond to a registered
// agent. The handler layer maps this to HTTP 400 via [errors.Is]. See C-002 in
// cowork/certctl-coverage-gap-audit.md — this sentinel replaces a silent
// Postgres FK violation (23503 → HTTP 500) with a deterministic 400.
var ErrAgentNotFound = errors.New("referenced agent does not exist")
// validTargetTypes is the set of allowed target types for validation. // validTargetTypes is the set of allowed target types for validation.
var validTargetTypes = map[domain.TargetType]bool{ var validTargetTypes = map[domain.TargetType]bool{
domain.TargetTypeNGINX: true, domain.TargetTypeNGINX: true,
@@ -276,6 +284,19 @@ func (s *TargetService) CreateTarget(ctx context.Context, target domain.Deployme
if !isValidTargetType(target.Type) { if !isValidTargetType(target.Type) {
return nil, fmt.Errorf("unsupported target type: %s", target.Type) return nil, fmt.Errorf("unsupported target type: %s", target.Type)
} }
// C-002: enforce agent_id FK at service layer so we return a clean 400
// instead of bubbling a Postgres 23503 foreign-key violation out as 500.
// The schema (migrations/000001 line 104) declares agent_id TEXT NOT NULL
// with a FK to agents(id); we mirror that contract here for deterministic
// error mapping.
if target.AgentID == "" {
return nil, fmt.Errorf("%w: agent_id is required", ErrAgentNotFound)
}
if _, err := s.agentRepo.Get(ctx, target.AgentID); err != nil {
return nil, fmt.Errorf("%w: %s", ErrAgentNotFound, target.AgentID)
}
if target.ID == "" { if target.ID == "" {
target.ID = generateID("target") target.ID = generateID("target")
} }
+57 -3
View File
@@ -3,6 +3,7 @@ package service
import ( import (
"context" "context"
"encoding/json" "encoding/json"
"errors"
"log/slog" "log/slog"
"os" "os"
"testing" "testing"
@@ -377,11 +378,17 @@ func TestTargetService_GetTarget_Success(t *testing.T) {
} }
func TestTargetService_CreateTarget_Success(t *testing.T) { func TestTargetService_CreateTarget_Success(t *testing.T) {
svc, targetRepo, _, _ := newTestTargetService() svc, targetRepo, _, agentRepo := newTestTargetService()
// C-002: CreateTarget now pre-validates agent_id against agentRepo. Seed a
// real agent so the happy path still exercises the normal creation flow
// without tripping the new ErrAgentNotFound guard.
agentRepo.AddAgent(&domain.Agent{ID: "a-1", Name: "test-agent"})
target := domain.DeploymentTarget{ target := domain.DeploymentTarget{
Name: "New Target", Name: "New Target",
Type: domain.TargetTypeNGINX, Type: domain.TargetTypeNGINX,
AgentID: "a-1",
} }
ctx := context.Background() ctx := context.Background()
@@ -415,6 +422,53 @@ func TestTargetService_CreateTarget_InvalidType(t *testing.T) {
} }
} }
// TestTargetService_CreateTarget_MissingAgentID verifies the C-002 service-layer
// guard: an empty agent_id must be rejected with ErrAgentNotFound before the
// repository layer is ever consulted. The handler maps this sentinel to HTTP
// 400, so a 500 from a Postgres 23503 FK violation is never surfaced.
func TestTargetService_CreateTarget_MissingAgentID(t *testing.T) {
svc, _, _, _ := newTestTargetService()
target := domain.DeploymentTarget{
Name: "No Agent",
Type: domain.TargetTypeNGINX,
// AgentID intentionally empty
}
ctx := context.Background()
_, err := svc.CreateTarget(ctx, target)
if err == nil {
t.Fatalf("expected error for missing agent_id, got nil")
}
if !errors.Is(err, ErrAgentNotFound) {
t.Errorf("expected errors.Is(err, ErrAgentNotFound) to be true, got err=%v", err)
}
}
// TestTargetService_CreateTarget_NonexistentAgentID verifies the second half of
// the C-002 guard: a non-empty agent_id that does not resolve in agentRepo
// still returns ErrAgentNotFound rather than letting the FK violation escape to
// Postgres. This is the realistic failure mode for a GUI sending a stale
// agent_id or a CLI caller with a typo.
func TestTargetService_CreateTarget_NonexistentAgentID(t *testing.T) {
svc, _, _, _ := newTestTargetService()
target := domain.DeploymentTarget{
Name: "Bad Agent Ref",
Type: domain.TargetTypeNGINX,
AgentID: "a-does-not-exist",
}
ctx := context.Background()
_, err := svc.CreateTarget(ctx, target)
if err == nil {
t.Fatalf("expected error for nonexistent agent_id, got nil")
}
if !errors.Is(err, ErrAgentNotFound) {
t.Errorf("expected errors.Is(err, ErrAgentNotFound) to be true, got err=%v", err)
}
}
func TestTargetService_UpdateTarget_Success(t *testing.T) { func TestTargetService_UpdateTarget_Success(t *testing.T) {
svc, targetRepo, _, _ := newTestTargetService() svc, targetRepo, _, _ := newTestTargetService()
@@ -0,0 +1,8 @@
-- Rollback migration 000013: remove per-rule severity.
--
-- DROP COLUMN removes the column, its CHECK constraint, and the default in
-- one statement. Any downstream code still referencing severity after
-- rollback will fail at query time — that's intentional, since running this
-- rollback implies severity as a concept is being abandoned.
ALTER TABLE policy_rules DROP COLUMN IF EXISTS severity;
@@ -0,0 +1,24 @@
-- Migration 000013: Per-Rule Severity on policy_rules
--
-- Prior to this migration, PolicyRule had no severity column. The TypeScript
-- frontend (PoliciesPage.tsx) sent a `severity` field on create/update, but
-- Go's json.Decoder silently dropped it (no matching struct field) and the
-- value never reached PostgreSQL. Reloading the page always showed severity
-- reverting to a default — the classic "silent drop" bug.
--
-- This migration adds severity as a first-class column on policy_rules.
-- Default `'Warning'` covers pre-existing rows; the CHECK constraint gives
-- defense-in-depth against casing drift (the application-layer validator in
-- internal/api/handler/validation.go already enforces the TitleCase allowlist,
-- but the DB should reject a bypassed write too).
--
-- No index: three-value column on a table that stays in the low thousands of
-- rows. The planner will seq-scan regardless; write cost without read benefit.
-- If measurements later justify it, add the index then.
--
-- PG 11+ makes ADD COLUMN with a literal DEFAULT a metadata-only operation
-- (no table rewrite), so this is safe to run on a live server.
ALTER TABLE policy_rules
ADD COLUMN IF NOT EXISTS severity VARCHAR(50) NOT NULL DEFAULT 'Warning'
CHECK (severity IN ('Warning', 'Error', 'Critical'));
@@ -0,0 +1,9 @@
-- Rollback migration 000014: drop the policy_violations severity CHECK.
--
-- Drops the named CHECK constraint added by the up migration. The severity
-- column itself stays (it predates this migration — see 000001 line 183),
-- so any application code that reads/writes the column continues to work.
-- Only the DB-level enforcement of the TitleCase allowlist is removed.
ALTER TABLE policy_violations
DROP CONSTRAINT IF EXISTS policy_violations_severity_check;
@@ -0,0 +1,29 @@
-- Migration 000014: CHECK constraint on policy_violations.severity
--
-- Sibling to migration 000013, which added severity + CHECK to policy_rules.
-- policy_violations has carried a severity column since the initial schema
-- (000001, line 183) but without any CHECK. The engine used to hardcode
-- `Warning` on every violation regardless of the triggering rule's severity
-- (see pre-D-008 internal/service/policy.go:evaluateRule), so the column
-- value was uniform by accident of implementation, not by constraint.
--
-- D-008 rewrites evaluateRule to copy rule.Severity into the violation. The
-- engine now writes values drawn from the application-layer PolicySeverity
-- allowlist, but nothing at the DB level prevents a future caller — or a
-- bypassed write from a migration or psql session — from inserting casing
-- drift ('warning', 'ERROR', etc.) and re-opening the same class of bug
-- that D-005 and D-006 closed. This constraint is the defense-in-depth
-- complement to the handler validator.
--
-- Pre-existing seed_demo.sql rows use lowercase severity values. D-008
-- updates those in the same commit so this migration can apply cleanly
-- against both a fresh install and an upgraded install that has already
-- seeded the demo data.
--
-- Named constraint (policy_violations_severity_check) so the down migration
-- can DROP it by name without ambiguity; un-named CHECK constraints use
-- a synthesized PostgreSQL name that varies by environment.
ALTER TABLE policy_violations
ADD CONSTRAINT policy_violations_severity_check
CHECK (severity IN ('Warning', 'Error', 'Critical'));
+32 -16
View File
@@ -12,42 +12,58 @@ VALUES (
'[30, 14, 7, 0]'::jsonb '[30, 14, 7, 0]'::jsonb
) ON CONFLICT (id) DO NOTHING; ) ON CONFLICT (id) DO NOTHING;
-- Policy rules: Require owner assignment -- Policy rules: Require owner assignment, bound environments, cap lifetime,
INSERT INTO policy_rules (id, name, type, config, enabled) -- and enforce a renewal lead-time.
--
-- Severity is differentiated per rule (D-006) and the types are now the
-- TitleCase canonicals the engine actually recognizes (D-008). Pre-D-008 the
-- types were lowercase strings (`ownership`, `environment`, `lifetime`,
-- `renewal_window`) that the engine silently dropped through to its
-- default-case error path — the rules looked alive in the GUI but did not
-- enforce anything. The backend CHECK constraint (migration 000013) enforces
-- the TitleCase severity allowlist Warning/Error/Critical. Configs are also
-- reshaped to match the D-008 per-arm schemas so the rules actually exercise
-- the config-consuming paths instead of falling back to the missing-field
-- placeholders.
INSERT INTO policy_rules (id, name, type, config, enabled, severity)
VALUES ( VALUES (
'pr-require-owner', 'pr-require-owner',
'require-owner', 'require-owner',
'ownership', 'RequiredMetadata',
'{"requirement": "owner_id must be set"}'::jsonb, '{"required_keys": ["owner"]}'::jsonb,
true true,
'Warning'
) ON CONFLICT (id) DO NOTHING; ) ON CONFLICT (id) DO NOTHING;
-- Policy rules: Allowed environments -- Policy rules: Allowed environments
INSERT INTO policy_rules (id, name, type, config, enabled) INSERT INTO policy_rules (id, name, type, config, enabled, severity)
VALUES ( VALUES (
'pr-allowed-environments', 'pr-allowed-environments',
'allowed-environments', 'allowed-environments',
'environment', 'AllowedEnvironments',
'{"allowed": ["production", "staging", "development"]}'::jsonb, '{"allowed": ["production", "staging", "development"]}'::jsonb,
true true,
'Error'
) ON CONFLICT (id) DO NOTHING; ) ON CONFLICT (id) DO NOTHING;
-- Policy rules: Maximum certificate lifetime -- Policy rules: Maximum certificate lifetime
INSERT INTO policy_rules (id, name, type, config, enabled) INSERT INTO policy_rules (id, name, type, config, enabled, severity)
VALUES ( VALUES (
'pr-max-certificate-lifetime', 'pr-max-certificate-lifetime',
'max-certificate-lifetime', 'max-certificate-lifetime',
'lifetime', 'CertificateLifetime',
'{"max_days": 90}'::jsonb, '{"max_days": 90}'::jsonb,
true true,
'Critical'
) ON CONFLICT (id) DO NOTHING; ) ON CONFLICT (id) DO NOTHING;
-- Policy rules: Minimum renewal window -- Policy rules: Minimum renewal window (renew at least 14 days before expiry)
INSERT INTO policy_rules (id, name, type, config, enabled) INSERT INTO policy_rules (id, name, type, config, enabled, severity)
VALUES ( VALUES (
'pr-min-renewal-window', 'pr-min-renewal-window',
'min-renewal-window', 'min-renewal-window',
'renewal_window', 'RenewalLeadTime',
'{"min_days": 14}'::jsonb, '{"lead_time_days": 14}'::jsonb,
true true,
'Warning'
) ON CONFLICT (id) DO NOTHING; ) ON CONFLICT (id) DO NOTHING;
+13 -6
View File
@@ -478,13 +478,20 @@ ON CONFLICT (id) DO NOTHING;
-- ============================================================ -- ============================================================
-- 13. Policy Violations -- 13. Policy Violations
-- ============================================================ -- ============================================================
-- D-008: severity values rewritten to TitleCase canonicals (Warning/Error/Critical).
-- Pre-D-008 these rows used lowercase strings ('critical', 'error', 'warning'). Those
-- values were silently tolerated by the pre-D-008 engine, which hardcoded 'Warning'
-- on every new violation regardless of the triggering rule's severity. D-008 rewires
-- evaluateRule to copy rule.Severity into the violation AND migration 000014 adds a
-- CHECK constraint enforcing the TitleCase allowlist at the DB level. Both paths now
-- round-trip correctly against these demo rows.
INSERT INTO policy_violations (id, certificate_id, rule_id, message, severity, created_at) VALUES INSERT INTO policy_violations (id, certificate_id, rule_id, message, severity, created_at) VALUES
('pv-001', 'mc-legacy-prod', 'pr-max-certificate-lifetime', 'Certificate has expired and exceeds maximum lifetime policy', 'critical', NOW() - INTERVAL '3 days'), ('pv-001', 'mc-legacy-prod', 'pr-max-certificate-lifetime', 'Certificate has expired and exceeds maximum lifetime policy', 'Critical', NOW() - INTERVAL '3 days'),
('pv-002', 'mc-old-api', 'pr-max-certificate-lifetime', 'Certificate expired 15 days ago', 'critical', NOW() - INTERVAL '15 days'), ('pv-002', 'mc-old-api', 'pr-max-certificate-lifetime', 'Certificate expired 15 days ago', 'Critical', NOW() - INTERVAL '15 days'),
('pv-003', 'mc-vpn-prod', 'pr-min-renewal-window', 'Renewal failed within minimum renewal window', 'error', NOW() - INTERVAL '3 days'), ('pv-003', 'mc-vpn-prod', 'pr-min-renewal-window', 'Renewal failed within minimum renewal window', 'Error', NOW() - INTERVAL '3 days'),
('pv-004', 'mc-mail-prod', 'pr-min-renewal-window', 'Certificate expiring in 5 days, below 14-day minimum window','warning', NOW() - INTERVAL '20 minutes'), ('pv-004', 'mc-mail-prod', 'pr-min-renewal-window', 'Certificate expiring in 5 days, below 14-day minimum window','Warning', NOW() - INTERVAL '20 minutes'),
('pv-005', 'mc-wiki-prod', 'pr-max-certificate-lifetime', 'Certificate expired 7 days ago', 'critical', NOW() - INTERVAL '7 days'), ('pv-005', 'mc-wiki-prod', 'pr-max-certificate-lifetime', 'Certificate expired 7 days ago', 'Critical', NOW() - INTERVAL '7 days'),
('pv-006', 'mc-compromised', 'pr-min-renewal-window', 'Certificate revoked due to key compromise', 'critical', NOW() - INTERVAL '14 days') ('pv-006', 'mc-compromised', 'pr-min-renewal-window', 'Certificate revoked due to key compromise', 'Critical', NOW() - INTERVAL '14 days')
ON CONFLICT (id) DO NOTHING; ON CONFLICT (id) DO NOTHING;
-- ============================================================ -- ============================================================
+60
View File
@@ -0,0 +1,60 @@
import { describe, it, expect } from 'vitest';
import { POLICY_TYPES, POLICY_SEVERITIES } from './types';
/**
* Regression tests for the policy enum tuples.
*
* These tuples are the GUI's source of truth for the policy type and severity
* dropdowns. They MUST stay in lockstep with the backend enum values:
* - internal/domain/policy.go defines the PolicyType / PolicySeverity consts
* - internal/api/handler/validators.go rejects anything outside the allowlist
* - migration 000013 enforces the severity allowlist at the DB level via CHECK
*
* Audit history (D-005, D-006):
* - The GUI previously sent lowercase values (e.g. 'key_algorithm',
* 'ownership'), which the backend validator rejected with a 400. Every
* attempt to create a policy from the "+ New Policy" button silently
* failed until the modal was closed.
* - The severity dropdown carried a four-value `low/medium/high/critical`
* tuple that shared zero values with the backend's
* `Warning/Error/Critical` the `medium` option has no backend analog
* and is removed.
*
* If these tests fail because a backend enum changed, DO NOT update the
* expected arrays without also updating the backend consts and the migration.
* Frontend/backend drift on these tuples is precisely what this regression
* guards against.
*/
describe('POLICY_TYPES', () => {
it('matches the backend PolicyType TitleCase allowlist exactly', () => {
expect(POLICY_TYPES).toEqual([
'AllowedIssuers',
'AllowedDomains',
'RequiredMetadata',
'AllowedEnvironments',
'RenewalLeadTime',
'CertificateLifetime',
]);
});
it('has no duplicate entries', () => {
expect(new Set(POLICY_TYPES).size).toBe(POLICY_TYPES.length);
});
});
describe('POLICY_SEVERITIES', () => {
it('matches the backend PolicySeverity TitleCase allowlist exactly', () => {
expect(POLICY_SEVERITIES).toEqual(['Warning', 'Error', 'Critical']);
});
it('has no duplicate entries', () => {
expect(new Set(POLICY_SEVERITIES).size).toBe(POLICY_SEVERITIES.length);
});
it('does not include the removed pre-fix `medium` value', () => {
// Explicit negative assertion. Pre-fix the GUI offered four severities
// (low/medium/high/critical); `medium` never had a backend analog.
expect(POLICY_SEVERITIES as readonly string[]).not.toContain('medium');
});
});
+30 -3
View File
@@ -112,11 +112,38 @@ export interface AuditEvent {
timestamp: string; timestamp: string;
} }
/**
* Policy rule type enum pinned to the backend's TitleCase constants in
* internal/domain/policy.go. Historical note (D-005): the GUI previously sent
* lowercase values (`ownership`, `environment`, etc.) that the handler's
* ValidatePolicyType rejected with a 400. These tuples are the canonical
* source of truth for the dropdown options; the regression test in
* types.test.ts pins them so future drift is caught at CI time.
*/
export const POLICY_TYPES = [
'AllowedIssuers',
'AllowedDomains',
'RequiredMetadata',
'AllowedEnvironments',
'RenewalLeadTime',
'CertificateLifetime',
] as const;
export type PolicyType = (typeof POLICY_TYPES)[number];
/**
* Policy severity enum pinned to the backend's PolicySeverity constants.
* The backend CHECK constraint on policy_rules.severity enforces the same
* allowlist (migration 000013). The 4-value `medium` option that used to
* appear in the GUI was never a valid backend value and has been removed.
*/
export const POLICY_SEVERITIES = ['Warning', 'Error', 'Critical'] as const;
export type PolicySeverity = (typeof POLICY_SEVERITIES)[number];
export interface PolicyRule { export interface PolicyRule {
id: string; id: string;
name: string; name: string;
type: string; type: PolicyType;
severity: string; severity: PolicySeverity;
config: Record<string, unknown>; config: Record<string, unknown>;
enabled: boolean; enabled: boolean;
created_at: string; created_at: string;
@@ -127,7 +154,7 @@ export interface PolicyViolation {
id: string; id: string;
rule_id: string; rule_id: string;
certificate_id: string; certificate_id: string;
severity: string; severity: PolicySeverity;
message: string; message: string;
created_at: string; created_at: string;
} }
+53 -14
View File
@@ -1,7 +1,7 @@
import { useState } from 'react'; import { useState } from 'react';
import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query';
import { useNavigate } from 'react-router-dom'; import { useNavigate } from 'react-router-dom';
import { getCertificates, createCertificate, triggerRenewal, revokeCertificate, updateCertificate, getOwners, getProfiles, getIssuers, bulkRevokeCertificates } from '../api/client'; import { getCertificates, createCertificate, triggerRenewal, revokeCertificate, updateCertificate, getOwners, getTeams, getPolicies, getProfiles, getIssuers, bulkRevokeCertificates } from '../api/client';
import { REVOCATION_REASONS } from '../api/types'; import { REVOCATION_REASONS } from '../api/types';
import PageHeader from '../components/PageHeader'; import PageHeader from '../components/PageHeader';
import DataTable from '../components/DataTable'; import DataTable from '../components/DataTable';
@@ -35,8 +35,27 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o
queryKey: ['issuers'], queryKey: ['issuers'],
queryFn: () => getIssuers(), queryFn: () => getIssuers(),
}); });
// C-001: owner_id, team_id, and renewal_policy_id are required by the
// server (handler in internal/api/handler/certificates.go) and by OpenAPI.
// Load the catalog so the user selects valid FKs instead of typing free-text
// IDs that would 400 at the server.
const { data: ownersResp } = useQuery({
queryKey: ['owners', 'form'],
queryFn: () => getOwners({ per_page: '500' }),
});
const { data: teamsResp } = useQuery({
queryKey: ['teams', 'form'],
queryFn: () => getTeams({ per_page: '500' }),
});
const { data: policiesResp } = useQuery({
queryKey: ['renewal-policies', 'form'],
queryFn: () => getPolicies({ per_page: '500' }),
});
const profiles = profilesResp?.data || []; const profiles = profilesResp?.data || [];
const issuers = issuersResp?.data || []; const issuers = issuersResp?.data || [];
const owners = ownersResp?.data || [];
const teams = teamsResp?.data || [];
const policies = policiesResp?.data || [];
const selectedProfile = profiles.find(p => p.id === form.certificate_profile_id); const selectedProfile = profiles.find(p => p.id === form.certificate_profile_id);
const ttlLabel = selectedProfile const ttlLabel = selectedProfile
@@ -143,24 +162,36 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o
</select> </select>
</div> </div>
<div> <div>
<label className="text-xs text-ink-muted block mb-1">Policy</label> <label className="text-xs text-ink-muted block mb-1">Policy *</label>
<input value={form.renewal_policy_id} onChange={e => setForm(f => ({ ...f, renewal_policy_id: e.target.value }))} <select value={form.renewal_policy_id} onChange={e => setForm(f => ({ ...f, renewal_policy_id: e.target.value }))}
className={inputClass} className={selectClass}>
placeholder="rp-standard" /> <option value="">Select policy...</option>
{policies.map(p => (
<option key={p.id} value={p.id}>{p.name}</option>
))}
</select>
</div> </div>
</div> </div>
<div className="grid grid-cols-2 gap-3"> <div className="grid grid-cols-2 gap-3">
<div> <div>
<label className="text-xs text-ink-muted block mb-1">Owner</label> <label className="text-xs text-ink-muted block mb-1">Owner *</label>
<input value={form.owner_id} onChange={e => setForm(f => ({ ...f, owner_id: e.target.value }))} <select value={form.owner_id} onChange={e => setForm(f => ({ ...f, owner_id: e.target.value }))}
className={inputClass} className={selectClass}>
placeholder="o-alice" /> <option value="">Select owner...</option>
{owners.map(o => (
<option key={o.id} value={o.id}>{o.name} ({o.email})</option>
))}
</select>
</div> </div>
<div> <div>
<label className="text-xs text-ink-muted block mb-1">Team</label> <label className="text-xs text-ink-muted block mb-1">Team *</label>
<input value={form.team_id} onChange={e => setForm(f => ({ ...f, team_id: e.target.value }))} <select value={form.team_id} onChange={e => setForm(f => ({ ...f, team_id: e.target.value }))}
className={inputClass} className={selectClass}>
placeholder="t-platform" /> <option value="">Select team...</option>
{teams.map(t => (
<option key={t.id} value={t.id}>{t.name}</option>
))}
</select>
</div> </div>
</div> </div>
<div> <div>
@@ -175,7 +206,15 @@ function CreateCertificateModal({ onClose, onSuccess }: { onClose: () => void; o
<button onClick={onClose} className="btn btn-ghost text-sm">Cancel</button> <button onClick={onClose} className="btn btn-ghost text-sm">Cancel</button>
<button <button
onClick={() => mutation.mutate()} onClick={() => mutation.mutate()}
disabled={!form.name || !form.common_name || !form.issuer_id || mutation.isPending} disabled={
!form.name ||
!form.common_name ||
!form.issuer_id ||
!form.owner_id ||
!form.team_id ||
!form.renewal_policy_id ||
mutation.isPending
}
className="btn btn-primary text-sm disabled:opacity-50" className="btn btn-primary text-sm disabled:opacity-50"
> >
{mutation.isPending ? 'Creating...' : 'Create Certificate'} {mutation.isPending ? 'Creating...' : 'Create Certificate'}
+44 -29
View File
@@ -6,22 +6,40 @@ import DataTable from '../components/DataTable';
import type { Column } from '../components/DataTable'; import type { Column } from '../components/DataTable';
import ErrorState from '../components/ErrorState'; import ErrorState from '../components/ErrorState';
import { formatDateTime } from '../api/utils'; import { formatDateTime } from '../api/utils';
import type { PolicyRule } from '../api/types'; import {
POLICY_TYPES,
POLICY_SEVERITIES,
type PolicyRule,
type PolicyType,
type PolicySeverity,
} from '../api/types';
const severityStyles: Record<string, string> = { /**
low: 'badge-info', * Severity badge style. Keyed on the backend's TitleCase PolicySeverity
medium: 'badge-warning', * enum values (D-006). The pre-fix map keyed on `low`/`medium`/`high`/`critical`
high: 'badge-danger', * which never matched the backend's `Warning`/`Error`/`Critical`, so every
critical: 'badge-danger', * existing rule fell through to the `badge-neutral` default.
*/
const severityStyles: Record<PolicySeverity, string> = {
Warning: 'badge-warning',
Error: 'badge-danger',
Critical: 'badge-danger',
}; };
const severityDots: Record<string, string> = { const severityDots: Record<PolicySeverity, string> = {
low: 'bg-emerald-500', Warning: 'bg-amber-500',
medium: 'bg-amber-500', Error: 'bg-orange-500',
high: 'bg-orange-500', Critical: 'bg-red-500',
critical: 'bg-red-500',
}; };
/**
* Convert TitleCase enum value to a human-readable label for display.
* "AllowedIssuers" "Allowed Issuers"
*/
function humanize(s: string): string {
return s.replace(/([A-Z])/g, ' $1').trim();
}
interface CreatePolicyModalProps { interface CreatePolicyModalProps {
isOpen: boolean; isOpen: boolean;
onClose: () => void; onClose: () => void;
@@ -32,8 +50,8 @@ interface CreatePolicyModalProps {
function CreatePolicyModal({ isOpen, onClose, onSuccess, isLoading, error }: CreatePolicyModalProps) { function CreatePolicyModal({ isOpen, onClose, onSuccess, isLoading, error }: CreatePolicyModalProps) {
const [name, setName] = useState(''); const [name, setName] = useState('');
const [type, setType] = useState('key_algorithm'); const [type, setType] = useState<PolicyType>(POLICY_TYPES[0]);
const [severity, setSeverity] = useState('medium'); const [severity, setSeverity] = useState<PolicySeverity>('Warning');
const [configStr, setConfigStr] = useState('{}'); const [configStr, setConfigStr] = useState('{}');
const [enabled, setEnabled] = useState(true); const [enabled, setEnabled] = useState(true);
@@ -43,8 +61,8 @@ function CreatePolicyModal({ isOpen, onClose, onSuccess, isLoading, error }: Cre
const config = JSON.parse(configStr); const config = JSON.parse(configStr);
await createPolicy({ name: name.trim(), type, severity, config, enabled }); await createPolicy({ name: name.trim(), type, severity, config, enabled });
setName(''); setName('');
setType('key_algorithm'); setType(POLICY_TYPES[0]);
setSeverity('medium'); setSeverity('Warning');
setConfigStr('{}'); setConfigStr('{}');
setEnabled(true); setEnabled(true);
onSuccess(); onSuccess();
@@ -72,27 +90,24 @@ function CreatePolicyModal({ isOpen, onClose, onSuccess, isLoading, error }: Cre
<label className="block text-sm font-medium text-ink mb-1">Type *</label> <label className="block text-sm font-medium text-ink mb-1">Type *</label>
<select <select
value={type} value={type}
onChange={e => setType(e.target.value)} onChange={e => setType(e.target.value as PolicyType)}
className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400" className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400"
> >
<option value="key_algorithm">Key Algorithm</option> {POLICY_TYPES.map(t => (
<option value="cert_lifetime">Certificate Lifetime</option> <option key={t} value={t}>{humanize(t)}</option>
<option value="san_pattern">SAN Pattern</option> ))}
<option value="key_usage">Key Usage</option>
<option value="revocation_check">Revocation Check</option>
</select> </select>
</div> </div>
<div> <div>
<label className="block text-sm font-medium text-ink mb-1">Severity *</label> <label className="block text-sm font-medium text-ink mb-1">Severity *</label>
<select <select
value={severity} value={severity}
onChange={e => setSeverity(e.target.value)} onChange={e => setSeverity(e.target.value as PolicySeverity)}
className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400" className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400"
> >
<option value="low">Low</option> {POLICY_SEVERITIES.map(s => (
<option value="medium">Medium</option> <option key={s} value={s}>{s}</option>
<option value="high">High</option> ))}
<option value="critical">Critical</option>
</select> </select>
</div> </div>
<div> <div>
@@ -182,7 +197,7 @@ export default function PoliciesPage() {
</div> </div>
), ),
}, },
{ key: 'type', label: 'Type', render: (p) => <span className="text-sm text-ink">{p.type.replace(/_/g, ' ')}</span> }, { key: 'type', label: 'Type', render: (p) => <span className="text-sm text-ink">{humanize(p.type)}</span> },
{ {
key: 'severity', key: 'severity',
label: 'Severity', label: 'Severity',
@@ -248,8 +263,8 @@ export default function PoliciesPage() {
</div> </div>
{Object.entries(bySeverity).map(([sev, count]) => ( {Object.entries(bySeverity).map(([sev, count]) => (
<div key={sev} className="flex items-center gap-1.5"> <div key={sev} className="flex items-center gap-1.5">
<div className={`w-2 h-2 rounded-full ${severityDots[sev] || 'bg-slate-400'}`} /> <div className={`w-2 h-2 rounded-full ${severityDots[sev as PolicySeverity] || 'bg-slate-400'}`} />
<span className="text-xs text-ink capitalize">{sev}</span> <span className="text-xs text-ink">{sev}</span>
<span className="text-xs text-ink-faint">{count}</span> <span className="text-xs text-ink-faint">{count}</span>
</div> </div>
))} ))}
+22 -6
View File
@@ -1,7 +1,7 @@
import { useState } from 'react'; import { useState } from 'react';
import { Link } from 'react-router-dom'; import { Link } from 'react-router-dom';
import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query';
import { getTargets, createTarget, deleteTarget } from '../api/client'; import { getTargets, createTarget, deleteTarget, getAgents } from '../api/client';
import PageHeader from '../components/PageHeader'; import PageHeader from '../components/PageHeader';
import DataTable from '../components/DataTable'; import DataTable from '../components/DataTable';
import type { Column } from '../components/DataTable'; import type { Column } from '../components/DataTable';
@@ -180,6 +180,16 @@ function CreateTargetWizard({ onClose, onSuccess }: { onClose: () => void; onSuc
const [config, setConfig] = useState<Record<string, string>>({}); const [config, setConfig] = useState<Record<string, string>>({});
const [error, setError] = useState(''); const [error, setError] = useState('');
// C-002: agent_id is a NOT NULL FK in deployment_targets (migration 000001
// line 104). Load registered agents so the user picks a valid FK instead of
// typing a free-text ID that would 400 at the service layer (or, pre-fix,
// bubble up as a Postgres 23503 foreign-key violation → 500).
const { data: agentsResp } = useQuery({
queryKey: ['agents', 'form'],
queryFn: () => getAgents({ per_page: '500' }),
});
const agents = agentsResp?.data || [];
// Fields that backends expect as boolean (Go bool) // Fields that backends expect as boolean (Go bool)
const BOOL_FIELDS = new Set([ const BOOL_FIELDS = new Set([
'sni', 'insecure', 'sds_config', 'remove_expired', 'create_keystore', 'sni', 'insecure', 'sds_config', 'remove_expired', 'create_keystore',
@@ -244,7 +254,7 @@ function CreateTargetWizard({ onClose, onSuccess }: { onClose: () => void; onSuc
}); });
const fields = CONFIG_FIELDS[targetType] || []; const fields = CONFIG_FIELDS[targetType] || [];
const canProceedToReview = name && targetType && fields.filter(f => f.required).every(f => config[f.key]); const canProceedToReview = name && targetType && agentId && fields.filter(f => f.required).every(f => config[f.key]);
return ( return (
<div className="fixed inset-0 bg-black/40 flex items-center justify-center z-50" onClick={onClose}> <div className="fixed inset-0 bg-black/40 flex items-center justify-center z-50" onClick={onClose}>
@@ -314,10 +324,16 @@ function CreateTargetWizard({ onClose, onSuccess }: { onClose: () => void; onSuc
placeholder="web-server-1" /> placeholder="web-server-1" />
</div> </div>
<div> <div>
<label className="text-xs text-ink-muted block mb-1">Agent ID</label> <label className="text-xs text-ink-muted block mb-1">Agent *</label>
<input value={agentId} onChange={e => setAgentId(e.target.value)} <select value={agentId} onChange={e => setAgentId(e.target.value)}
className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400" className="w-full bg-white border border-surface-border rounded px-3 py-2 text-sm text-ink focus:outline-none focus:border-brand-400">
placeholder="agent-web1" /> <option value="">Select an agent...</option>
{agents.map(a => (
<option key={a.id} value={a.id}>
{a.hostname || a.id} ({a.id})
</option>
))}
</select>
</div> </div>
{fields.map(f => ( {fields.map(f => (
<div key={f.key}> <div key={f.key}>