cli: promote --force on renew + require --reason on revoke (closes P3-1, P3-2)

Closes findings P3-1 and P3-2 from the 2026-05-05 CLI/API/MCP↔GUI parity
audit (cowork/cli-gui-parity-audit-2026-05-05/RESULTS.md). Both findings
flagged hidden defaults that the CLI was sending without exposing them
to operators: `force=false` baked into every renew payload, and a silent
fallback to `reason="unspecified"` whenever --reason was omitted.

P3-1 — promote --force on `certs renew` (full end-to-end plumbing)

The pre-2026-05-05 CLI sent `{"force": false}` in the renew body. The
API handler never decoded it — a textbook "lying field" per the
operator's CLAUDE.md "complete path, not the easy path" rule: the body
field stored a value, claimed to do something, and silently did nothing
because the wire never reached the consumer. Adding a --force flag that
also went unread would have created another lying field.

This commit takes the complete path:

  service.CertificateService.TriggerRenewal grew a `force bool` parameter
  (internal/service/certificate.go). When force=true, the
  RenewalInProgress block is overridden so operators can recover stuck
  in-flight renewals where a previous job hung without releasing the
  status flag. Archived and Expired remain terminal blockers regardless
  of force — those are semantic dead-ends that --force should not paper
  over (archived = decommissioned, expired = issue a new cert instead of
  renewing a dead one).

  handler.CertificateHandler.TriggerRenewal parses force from
  ?force=true (or ?force=1) query param, OR {"force": true} JSON body,
  whichever the client picks. Defaults to false. Passes through to the
  service.

  internal/cli/client.go::RenewCertificate(id, force bool) sends
  ?force=true on the URL when --force is set. The historical hardcoded
  `{"force": false}` body is gone — no more lying field.

  cmd/cli/main.go dispatches `certs renew <id> [--force]` (ID-first
  flag-second convention matches the existing `agents retire <id>
  [--force]`).

P3-2 — require --reason on `certs revoke` (Option A: strict refusal)

The pre-2026-05-05 CLI dropped to `--reason unspecified` whenever the
operator omitted the flag. Compliance reporting (RFC 5280 §5.3.1, PCI-
DSS §3.6, HIPAA §164.312) relies on the reason code being meaningful;
silent fallback defeats the audit trail because every revocation looks
identical.

  cmd/cli/main.go dispatch refuses to send when --reason is empty,
  prints the canonical RFC 5280 §5.3.1 reason-code menu, and exits
  non-zero.

  internal/cli/client.go exposes ValidRevokeReasons() returning the
  canonical camelCase list (unspecified, keyCompromise, caCompromise,
  affiliationChanged, superseded, cessationOfOperation, certificateHold,
  removeFromCRL, privilegeWithdrawn, aaCompromise) and
  NormalizeRevokeReason() that accepts both camelCase and snake_case
  inputs and normalises to the canonical wire form. Off-list reasons
  are rejected at dispatch with the menu re-printed.

Test pins:

  internal/cli/client_test.go::TestClient_RenewCertificate_ForceFlag —
  --force=true sends ?force=true with empty body; --force=false sends
  no query and no body.

  internal/cli/client_test.go::TestNormalizeRevokeReason +
  TestValidRevokeReasons — canonical-camelCase + snake_case + reject-
  off-enum behaviour.

  cmd/cli/dispatch_test.go::TestHandleCerts_Revoke_RequiresReason +
  TestHandleCerts_Revoke_RejectsUnknownReason +
  TestHandleCerts_Renew_ForceFlag — dispatch-layer pins for the same
  contracts.

  internal/api/handler/certificate_handler_test.go::TestTriggerRenewal_
  ForceQueryParam — query-param passthrough (no-flag, force=true,
  force=1, force=false) flows through to the service-layer parameter.

  internal/service/certificate_test.go::TestTriggerRenewal_
  ForceOverridesInProgress — force=false preserves the
  RenewalInProgress block; force=true clears it.

  Existing TestTriggerRenewal_Archived extended to assert force=true
  still blocks Archived (terminal-state guarantee).

Docs: docs/reference/cli.md updated with the --force example for renew
and the strict --reason semantics for revoke (including snake_case
input acceptance).

Acceptance gate (verified):
  - go build ./cmd/server/... ./cmd/agent/... ./cmd/cli/...
    ./cmd/mcp-server/... clean.
  - go vet ./... clean.
  - go test -short -count=1 ./... pass repo-wide.
  - bash scripts/ci-guards/openapi-handler-parity.sh clean
    (router 178, OpenAPI 144, exceptions 36 — unchanged; we add
    parameter parsing, not routes).
  - gofmt -l clean.
This commit is contained in:
shankar0123
2026-05-05 19:49:34 +00:00
parent ff75361553
commit 0e06f6c4fc
9 changed files with 456 additions and 35 deletions
@@ -32,7 +32,7 @@ type MockCertificateService struct {
UpdateCertificateFn func(ctx context.Context, id string, cert domain.ManagedCertificate) (*domain.ManagedCertificate, error)
ArchiveCertificateFn func(ctx context.Context, id string) error
GetCertificateVersionsFn func(ctx context.Context, certID string, page, perPage int) ([]domain.CertificateVersion, int64, error)
TriggerRenewalFn func(ctx context.Context, certID string, actor string) error
TriggerRenewalFn func(ctx context.Context, certID string, actor string, force bool) error
TriggerDeploymentFn func(ctx context.Context, certID string, targetID string, actor string) error
RevokeCertificateFn func(ctx context.Context, certID string, reason string, actor string) error
GetRevokedCertificatesFn func(ctx context.Context) ([]*domain.CertificateRevocation, error)
@@ -84,9 +84,9 @@ func (m *MockCertificateService) GetCertificateVersions(ctx context.Context, cer
return nil, 0, nil
}
func (m *MockCertificateService) TriggerRenewal(ctx context.Context, certID string, actor string) error {
func (m *MockCertificateService) TriggerRenewal(ctx context.Context, certID string, actor string, force bool) error {
if m.TriggerRenewalFn != nil {
return m.TriggerRenewalFn(ctx, certID, actor)
return m.TriggerRenewalFn(ctx, certID, actor, force)
}
return nil
}
@@ -690,7 +690,7 @@ func TestGetCertificateVersions_NotFound(t *testing.T) {
// Test TriggerRenewal - success case
func TestTriggerRenewal_Success(t *testing.T) {
mock := &MockCertificateService{
TriggerRenewalFn: func(_ context.Context, certID string, _ string) error {
TriggerRenewalFn: func(_ context.Context, certID string, _ string, _ bool) error {
if certID == "mc-prod-001" {
return nil
}
@@ -722,7 +722,7 @@ func TestTriggerRenewal_Success(t *testing.T) {
// Test TriggerRenewal - service error
func TestTriggerRenewal_ServiceError(t *testing.T) {
mock := &MockCertificateService{
TriggerRenewalFn: func(_ context.Context, certID string, _ string) error {
TriggerRenewalFn: func(_ context.Context, certID string, _ string, _ bool) error {
return ErrMockServiceFailed
},
}
@@ -739,6 +739,44 @@ func TestTriggerRenewal_ServiceError(t *testing.T) {
}
}
// TestTriggerRenewal_ForceQueryParam pins the 2026-05-05 parity-defaults-cleanup
// (P3-1) wire: ?force=true on the renew URL flows through to the service-layer
// `force bool` parameter so operators can override the RenewalInProgress block.
func TestTriggerRenewal_ForceQueryParam(t *testing.T) {
for _, tc := range []struct {
name string
query string
wantForce bool
}{
{"no-flag", "", false},
{"force-true", "?force=true", true},
{"force-1", "?force=1", true},
{"force-false", "?force=false", false},
} {
t.Run(tc.name, func(t *testing.T) {
var gotForce bool
mock := &MockCertificateService{
TriggerRenewalFn: func(_ context.Context, _ string, _ string, force bool) error {
gotForce = force
return nil
},
}
handler := NewCertificateHandler(mock)
req := httptest.NewRequest(http.MethodPost,
"/api/v1/certificates/mc-prod-001/renew"+tc.query, nil)
req = req.WithContext(contextWithRequestID())
w := httptest.NewRecorder()
handler.TriggerRenewal(w, req)
if w.Code != http.StatusAccepted {
t.Fatalf("status: got %d want %d", w.Code, http.StatusAccepted)
}
if gotForce != tc.wantForce {
t.Errorf("force passthrough: got %v want %v", gotForce, tc.wantForce)
}
})
}
}
// Test TriggerDeployment - success case
func TestTriggerDeployment_Success(t *testing.T) {
mock := &MockCertificateService{
+20 -2
View File
@@ -32,7 +32,7 @@ type CertificateService interface {
UpdateCertificate(ctx context.Context, id string, cert domain.ManagedCertificate) (*domain.ManagedCertificate, error)
ArchiveCertificate(ctx context.Context, id string) error
GetCertificateVersions(ctx context.Context, certID string, page, perPage int) ([]domain.CertificateVersion, int64, error)
TriggerRenewal(ctx context.Context, certID string, actor string) error
TriggerRenewal(ctx context.Context, certID string, actor string, force bool) error
TriggerDeployment(ctx context.Context, certID string, targetID string, actor string) error
RevokeCertificate(ctx context.Context, certID string, reason string, actor string) error
GetRevokedCertificates(ctx context.Context) ([]*domain.CertificateRevocation, error)
@@ -437,7 +437,25 @@ func (h CertificateHandler) TriggerRenewal(w http.ResponseWriter, r *http.Reques
actor := resolveActor(r.Context())
if err := h.svc.TriggerRenewal(r.Context(), certID, actor); err != nil {
// 2026-05-05 parity-defaults-cleanup (P3-1): operators can opt into
// forcing a renewal when the cert is stuck in RenewalInProgress (a
// previous job hung without releasing the status flag). Accepted as
// either ?force=true query param OR {"force": true} JSON body so CLI
// + GUI clients can pick whichever flow fits their idiom.
force := false
if fv := r.URL.Query().Get("force"); fv == "true" || fv == "1" {
force = true
}
if !force && r.ContentLength > 0 && r.Header.Get("Content-Type") == "application/json" {
var body struct {
Force bool `json:"force,omitempty"`
}
if err := json.NewDecoder(r.Body).Decode(&body); err == nil {
force = body.Force
}
}
if err := h.svc.TriggerRenewal(r.Context(), certID, actor, force); err != nil {
errMsg := err.Error()
if strings.Contains(errMsg, "not found") {
ErrorWithRequestID(w, http.StatusNotFound, "Certificate not found", requestID)
+95 -5
View File
@@ -179,12 +179,22 @@ func (c *Client) GetCertificate(id string) error {
}
// RenewCertificate triggers renewal for a certificate.
func (c *Client) RenewCertificate(id string) error {
body := map[string]interface{}{
"force": false,
//
// 2026-05-05 parity-defaults-cleanup (P3-1): the `force` parameter, when
// true, clears the server-side RenewalInProgress block — operators use
// this to recover from a stuck in-flight renewal where the previous job
// hung without releasing the status flag. Sent as `?force=true` query
// parameter; the historical body field `{"force": false}` is gone (it was
// a "lying field" — the API never read it). Archived and Expired remain
// terminal blockers regardless of force; --force is not a magic wand for
// terminal-state certs.
func (c *Client) RenewCertificate(id string, force bool) error {
var q url.Values
if force {
q = url.Values{"force": []string{"true"}}
}
resp, err := c.do("POST", fmt.Sprintf("/api/v1/certificates/%s/renew", id), nil, body)
resp, err := c.do("POST", fmt.Sprintf("/api/v1/certificates/%s/renew", id), q, nil)
if err != nil {
return err
}
@@ -198,14 +208,94 @@ func (c *Client) RenewCertificate(id string) error {
return c.outputJSON(result)
}
fmt.Printf("Renewal triggered for certificate %s\n", id)
if force {
fmt.Printf("Renewal force-triggered for certificate %s (RenewalInProgress block cleared)\n", id)
} else {
fmt.Printf("Renewal triggered for certificate %s\n", id)
}
if jobID, ok := result["job_id"]; ok {
fmt.Printf("Job ID: %v\n", jobID)
}
return nil
}
// canonicalRevokeReasons enumerates the RFC 5280 §5.3.1 reason codes
// accepted by `certctl-cli certs revoke --reason`. Mirrors the canonical
// camelCase surface used by the local issuer + ACME server. Underscore_lower
// variants (e.g. `key_compromise`) are accepted as a convenience and
// normalised at this layer.
//
// 2026-05-05 parity-defaults-cleanup (P3-2): exposed via ValidRevokeReasons()
// + NormalizeRevokeReason() so the CLI dispatch can validate before sending,
// AND so the empty-reason error path can print the menu of valid choices
// instead of silently sending `unspecified`.
var canonicalRevokeReasons = []string{
"unspecified",
"keyCompromise",
"caCompromise",
"affiliationChanged",
"superseded",
"cessationOfOperation",
"certificateHold",
"removeFromCRL",
"privilegeWithdrawn",
"aaCompromise",
}
// ValidRevokeReasons returns the canonical RFC 5280 §5.3.1 reason-code
// camelCase enum the CLI accepts. Used by `certctl-cli certs revoke` to
// print the menu when --reason is missing or invalid.
func ValidRevokeReasons() []string {
out := make([]string, len(canonicalRevokeReasons))
copy(out, canonicalRevokeReasons)
return out
}
// NormalizeRevokeReason maps the operator's input to the canonical
// camelCase form. Returns the canonical form + ok=true if recognised,
// otherwise the original input + ok=false. Accepts both camelCase
// ("keyCompromise") and snake_case ("key_compromise") variants.
func NormalizeRevokeReason(input string) (string, bool) {
// Direct camelCase match.
for _, r := range canonicalRevokeReasons {
if r == input {
return r, true
}
}
// snake_case → camelCase by converting the canonical entry to snake
// form and comparing.
for _, r := range canonicalRevokeReasons {
if strings.EqualFold(camelToSnake(r), input) {
return r, true
}
}
return input, false
}
// camelToSnake converts a camelCase identifier to snake_case (e.g.
// "keyCompromise" → "key_compromise") so we can compare against operator
// input that uses the snake form.
func camelToSnake(camel string) string {
out := make([]byte, 0, len(camel)+4)
for i := 0; i < len(camel); i++ {
ch := camel[i]
if ch >= 'A' && ch <= 'Z' {
if i > 0 {
out = append(out, '_')
}
out = append(out, ch+('a'-'A'))
} else {
out = append(out, ch)
}
}
return string(out)
}
// RevokeCertificate revokes a certificate.
//
// 2026-05-05 parity-defaults-cleanup (P3-2, Option A): empty reason is
// rejected at the CLI dispatch layer (see cmd/cli/main.go) — this method
// expects a pre-validated, canonical RFC 5280 reason string.
func (c *Client) RevokeCertificate(id, reason string) error {
body := map[string]interface{}{
"reason": reason,
+94 -1
View File
@@ -88,12 +88,105 @@ func TestClient_RenewCertificate(t *testing.T) {
defer server.Close()
client, _ := NewClient(server.URL, "", "table", "", false)
err := client.RenewCertificate("mc-1")
err := client.RenewCertificate("mc-1", false)
if err != nil {
t.Fatalf("RenewCertificate failed: %v", err)
}
}
// TestClient_RenewCertificate_ForceFlag pins the 2026-05-05 parity-defaults-
// cleanup (P3-1) wire: the CLI sends `?force=true` on the renew URL when
// the operator passes --force. The pre-2026-05-05 hardcoded `force=false`
// body field is gone (the API never read it — it was a "lying field").
func TestClient_RenewCertificate_ForceFlag(t *testing.T) {
for _, tc := range []struct {
name string
force bool
wantQuery string
}{
{"no-force", false, ""},
{"force-true", true, "force=true"},
} {
t.Run(tc.name, func(t *testing.T) {
var gotQuery string
var gotBody string
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotQuery = r.URL.RawQuery
buf := make([]byte, 1024)
n, _ := r.Body.Read(buf)
gotBody = string(buf[:n])
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{"job_id": "job-1"})
}))
defer server.Close()
client, _ := NewClient(server.URL, "", "table", "", false)
if err := client.RenewCertificate("mc-1", tc.force); err != nil {
t.Fatalf("RenewCertificate: %v", err)
}
if gotQuery != tc.wantQuery {
t.Errorf("query: got %q want %q", gotQuery, tc.wantQuery)
}
// Body must be empty — the lying `force=false` field is gone.
if gotBody != "" {
t.Errorf("body should be empty (no lying force field), got %q", gotBody)
}
})
}
}
// TestNormalizeRevokeReason pins the 2026-05-05 parity-defaults-cleanup
// (P3-2) reason-code validator: canonical camelCase passes through, snake_
// case normalises to camelCase, anything else returns ok=false.
func TestNormalizeRevokeReason(t *testing.T) {
for _, tc := range []struct {
in string
want string
wantOK bool
}{
{"keyCompromise", "keyCompromise", true},
{"key_compromise", "keyCompromise", true},
{"superseded", "superseded", true},
{"cessation_of_operation", "cessationOfOperation", true},
{"unspecified", "unspecified", true},
{"BogusReason", "BogusReason", false},
{"", "", false},
{"key compromise", "key compromise", false},
} {
t.Run(tc.in, func(t *testing.T) {
got, ok := NormalizeRevokeReason(tc.in)
if got != tc.want || ok != tc.wantOK {
t.Errorf("NormalizeRevokeReason(%q) = (%q, %v), want (%q, %v)",
tc.in, got, ok, tc.want, tc.wantOK)
}
})
}
}
// TestValidRevokeReasons pins that the canonical RFC 5280 §5.3.1 reason
// list is non-empty and contains the operator-critical entries (the help
// menu printed when --reason is missing depends on this).
func TestValidRevokeReasons(t *testing.T) {
got := ValidRevokeReasons()
if len(got) < 9 {
t.Errorf("expected ≥9 RFC 5280 §5.3.1 reasons, got %d: %v", len(got), got)
}
want := []string{"keyCompromise", "caCompromise", "superseded",
"cessationOfOperation", "certificateHold", "privilegeWithdrawn"}
for _, w := range want {
found := false
for _, g := range got {
if g == w {
found = true
break
}
}
if !found {
t.Errorf("missing canonical reason %q in %v", w, got)
}
}
}
func TestClient_RevokeCertificate(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method != "POST" || r.URL.Path != "/api/v1/certificates/mc-1/revoke" {
+12 -3
View File
@@ -287,7 +287,15 @@ func (s *CertificateService) GetVersions(ctx context.Context, certID string) ([]
// TriggerRenewal initiates a renewal job if the certificate is eligible.
// Creates a Renewal job (or Issuance for new certs) so the scheduler's job processor
// can pick it up and route it through the issuer connector.
func (s *CertificateService) TriggerRenewal(ctx context.Context, certID string, actor string) error {
//
// 2026-05-05 parity-defaults-cleanup (P3-1): the `force` parameter, when
// true, overrides the RenewalInProgress block — operators use this to
// recover from a stuck in-flight renewal where the previous job hung
// without releasing the status flag. Archived and Expired remain terminal
// blockers regardless of force; those are semantic dead-ends (archived =
// "this cert is decommissioned", expired = "issue a new cert, don't renew
// a dead one") that --force should not paper over.
func (s *CertificateService) TriggerRenewal(ctx context.Context, certID string, actor string, force bool) error {
cert, err := s.certRepo.Get(ctx, certID)
if err != nil {
return fmt.Errorf("failed to fetch certificate: %w", err)
@@ -301,8 +309,9 @@ func (s *CertificateService) TriggerRenewal(ctx context.Context, certID string,
return fmt.Errorf("cannot renew expired certificate; reissue instead")
}
// Check if already renewing
if cert.Status == domain.CertificateStatusRenewalInProgress {
// Check if already renewing — overridable with force=true so operators
// can clear stuck in-flight renewals.
if cert.Status == domain.CertificateStatusRenewalInProgress && !force {
return fmt.Errorf("certificate renewal already in progress")
}
+54 -2
View File
@@ -294,7 +294,7 @@ func TestTriggerRenewal(t *testing.T) {
auditService := NewAuditService(auditRepo)
certService := NewCertificateService(certRepo, policyService, auditService)
err := certService.TriggerRenewal(ctx, "cert-001", "user-1")
err := certService.TriggerRenewal(ctx, "cert-001", "user-1", false)
if err != nil {
t.Fatalf("TriggerRenewal failed: %v", err)
}
@@ -309,6 +309,53 @@ func TestTriggerRenewal(t *testing.T) {
}
}
// TestTriggerRenewal_ForceOverridesInProgress pins the 2026-05-05 parity-
// defaults-cleanup (P3-1) semantic: force=true clears the
// RenewalInProgress block so operators can recover stuck in-flight renewals.
// force=false (the historical default) preserves the block.
func TestTriggerRenewal_ForceOverridesInProgress(t *testing.T) {
ctx := context.Background()
now := time.Now()
mkCert := func() *domain.ManagedCertificate {
return &domain.ManagedCertificate{
ID: "cert-stuck",
CommonName: "example.com",
IssuerID: "iss-1",
Status: domain.CertificateStatusRenewalInProgress,
ExpiresAt: now.AddDate(0, 0, 5),
CreatedAt: now,
UpdatedAt: now,
}
}
t.Run("force=false blocks", func(t *testing.T) {
certRepo := &mockCertRepo{
Certs: map[string]*domain.ManagedCertificate{"cert-stuck": mkCert()},
Versions: make(map[string][]*domain.CertificateVersion),
}
auditRepo := &mockAuditRepo{}
policyRepo := &mockPolicyRepo{Rules: make(map[string]*domain.PolicyRule)}
svc := NewCertificateService(certRepo, NewPolicyService(policyRepo, NewAuditService(auditRepo)), NewAuditService(auditRepo))
err := svc.TriggerRenewal(ctx, "cert-stuck", "user-1", false)
if err == nil {
t.Fatal("expected error when force=false on RenewalInProgress cert")
}
})
t.Run("force=true clears block", func(t *testing.T) {
certRepo := &mockCertRepo{
Certs: map[string]*domain.ManagedCertificate{"cert-stuck": mkCert()},
Versions: make(map[string][]*domain.CertificateVersion),
}
auditRepo := &mockAuditRepo{}
policyRepo := &mockPolicyRepo{Rules: make(map[string]*domain.PolicyRule)}
svc := NewCertificateService(certRepo, NewPolicyService(policyRepo, NewAuditService(auditRepo)), NewAuditService(auditRepo))
if err := svc.TriggerRenewal(ctx, "cert-stuck", "user-1", true); err != nil {
t.Fatalf("force=true should override RenewalInProgress: %v", err)
}
})
}
func TestTriggerRenewal_Archived(t *testing.T) {
ctx := context.Background()
now := time.Now()
@@ -333,10 +380,15 @@ func TestTriggerRenewal_Archived(t *testing.T) {
auditService := NewAuditService(auditRepo)
certService := NewCertificateService(certRepo, policyService, auditService)
err := certService.TriggerRenewal(ctx, "cert-001", "user-1")
err := certService.TriggerRenewal(ctx, "cert-001", "user-1", false)
if err == nil {
t.Fatal("expected error for archived certificate")
}
// Archived is a terminal state — force=true must NOT magic it open
// (parity-defaults-cleanup P3-1 semantic guarantee).
if err := certService.TriggerRenewal(ctx, "cert-001", "user-1", true); err == nil {
t.Fatal("force=true should still block archived (terminal)")
}
}
func TestListCertificates(t *testing.T) {