diff --git a/cmd/cli/dispatch_test.go b/cmd/cli/dispatch_test.go index fa63c47..86e2402 100644 --- a/cmd/cli/dispatch_test.go +++ b/cmd/cli/dispatch_test.go @@ -163,14 +163,79 @@ func TestHandleCerts_Revoke_HitsClientPath(t *testing.T) { })) t.Cleanup(srv.Close) c := newDispatchTestClient(t, srv) - if err := handleCerts(c, []string{"revoke", "mc-x", "--reason", "compromise"}); err != nil { + // 2026-05-05 parity-defaults-cleanup (P3-2): reason must be a canonical + // RFC 5280 §5.3.1 code (camelCase or snake_case both accepted; this + // test asserts the snake_case path normalises to the camelCase wire + // format that the local issuer + ACME server expect). + if err := handleCerts(c, []string{"revoke", "mc-x", "--reason", "key_compromise"}); err != nil { t.Errorf("handleCerts({revoke ...}): err=%v", err) } if lastMethod != "POST" || !strings.Contains(lastPath, "/revoke") { t.Errorf("expected POST .../revoke, got %s %s", lastMethod, lastPath) } - if !strings.Contains(lastBody, "compromise") { - t.Errorf("expected reason in body, got %q", lastBody) + if !strings.Contains(lastBody, "keyCompromise") { + t.Errorf("expected normalised reason 'keyCompromise' in body, got %q", lastBody) + } +} + +// TestHandleCerts_Revoke_RequiresReason pins the 2026-05-05 parity-defaults- +// cleanup (P3-2, Option A) strict-reason contract: empty --reason is a +// fatal error, not a silent fallback to "unspecified". +func TestHandleCerts_Revoke_RequiresReason(t *testing.T) { + srv := stubServer(t, 200, `{}`) + c := newDispatchTestClient(t, srv) + err := handleCerts(c, []string{"revoke", "mc-x"}) + if err == nil { + t.Fatal("expected error when --reason is omitted; got nil (regression on P3-2 strict path)") + } + if !strings.Contains(err.Error(), "reason") { + t.Errorf("expected error to mention 'reason', got %q", err.Error()) + } +} + +// TestHandleCerts_Revoke_RejectsUnknownReason pins that off-RFC reason +// codes are rejected at the CLI dispatch layer (P3-2 anti-typo guard). +func TestHandleCerts_Revoke_RejectsUnknownReason(t *testing.T) { + srv := stubServer(t, 200, `{}`) + c := newDispatchTestClient(t, srv) + err := handleCerts(c, []string{"revoke", "mc-x", "--reason", "compromise"}) + if err == nil { + t.Fatal("expected error for non-canonical reason; got nil") + } + if !strings.Contains(err.Error(), "compromise") { + t.Errorf("expected error to echo bad reason 'compromise', got %q", err.Error()) + } +} + +// TestHandleCerts_Renew_ForceFlag pins the 2026-05-05 parity-defaults- +// cleanup (P3-1) wire: --force on the renew dispatch sends ?force=true. +// CLI convention: ID is positional and precedes the flags (matches +// `agents retire [--force]`), so the flag MUST come after the ID. +func TestHandleCerts_Renew_ForceFlag(t *testing.T) { + for _, tc := range []struct { + name string + args []string + wantQuery string + }{ + {"no-force", []string{"renew", "mc-x"}, ""}, + {"force-after-id", []string{"renew", "mc-x", "--force"}, "force=true"}, + } { + t.Run(tc.name, func(t *testing.T) { + var lastQuery string + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + lastQuery = r.URL.RawQuery + w.WriteHeader(200) + _, _ = w.Write([]byte(`{}`)) + })) + t.Cleanup(srv.Close) + c := newDispatchTestClient(t, srv) + if err := handleCerts(c, tc.args); err != nil { + t.Fatalf("handleCerts: %v", err) + } + if lastQuery != tc.wantQuery { + t.Errorf("query: got %q want %q", lastQuery, tc.wantQuery) + } + }) } } diff --git a/cmd/cli/main.go b/cmd/cli/main.go index 5b5334c..9a0d726 100644 --- a/cmd/cli/main.go +++ b/cmd/cli/main.go @@ -144,22 +144,70 @@ func handleCerts(client *cli.Client, args []string) error { } return client.GetCertificate(subArgs[0]) case "renew": + // 2026-05-05 parity-defaults-cleanup (P3-1): expose --force as an + // explicit operator flag instead of the historical hardcoded + // `force=false` body field. force=true overrides the server-side + // RenewalInProgress block — used to recover stuck in-flight + // renewals. Archived/Expired remain terminal regardless. + // + // CLI convention: `certs renew [--force]` — the ID is a + // positional arg that precedes the flags. Mirrors `agents retire + // `'s pattern (Go's flag package stops at the first non-flag + // token, so we pull subArgs[0] as the ID and hand subArgs[1:] to + // the flag parser). if len(subArgs) == 0 { - fmt.Fprintf(os.Stderr, "usage: certs renew \n") - return nil - } - return client.RenewCertificate(subArgs[0]) - case "revoke": - if len(subArgs) == 0 { - fmt.Fprintf(os.Stderr, "usage: certs revoke [--reason ]\n") + fmt.Fprintf(os.Stderr, "usage: certs renew [--force]\n") return nil } id := subArgs[0] - reason := "unspecified" - if len(subArgs) > 2 && subArgs[1] == "--reason" { - reason = subArgs[2] + fs := flag.NewFlagSet("certs renew", flag.ContinueOnError) + force := fs.Bool("force", false, "Force renewal even when the cert is currently in RenewalInProgress (clears stuck in-flight renewals; does NOT override Archived/Expired terminal states)") + if err := fs.Parse(subArgs[1:]); err != nil { + return err } - return client.RevokeCertificate(id, reason) + return client.RenewCertificate(id, *force) + case "revoke": + // 2026-05-05 parity-defaults-cleanup (P3-2, Option A): --reason is + // strictly required. Empty reason refuses to dispatch and prints + // the RFC 5280 §5.3.1 reason-code menu so operators pick a real + // value. The pre-2026-05-05 silent fallback to "unspecified" + // defeated compliance reporting (PCI-DSS §3.6, HIPAA §164.312) + // because every revocation looked the same in the audit trail. + // + // CLI convention: `certs revoke --reason ` — same + // ID-first ordering as `certs renew`. + if len(subArgs) == 0 { + fmt.Fprintf(os.Stderr, "usage: certs revoke --reason \n") + fmt.Fprintf(os.Stderr, "\nValid RFC 5280 §5.3.1 reasons:\n") + for _, r := range cli.ValidRevokeReasons() { + fmt.Fprintf(os.Stderr, " %s\n", r) + } + return nil + } + id := subArgs[0] + fs := flag.NewFlagSet("certs revoke", flag.ContinueOnError) + reason := fs.String("reason", "", "RFC 5280 revocation reason (required). Valid values: keyCompromise, caCompromise, affiliationChanged, superseded, cessationOfOperation, certificateHold, removeFromCRL, privilegeWithdrawn, aaCompromise, unspecified") + if err := fs.Parse(subArgs[1:]); err != nil { + return err + } + if *reason == "" { + fmt.Fprintf(os.Stderr, "error: --reason is required (no silent fallback to 'unspecified' — pick a real RFC 5280 §5.3.1 code).\n\n") + fmt.Fprintf(os.Stderr, "Valid reasons:\n") + for _, r := range cli.ValidRevokeReasons() { + fmt.Fprintf(os.Stderr, " %s\n", r) + } + return fmt.Errorf("--reason is required") + } + canonical, ok := cli.NormalizeRevokeReason(*reason) + if !ok { + fmt.Fprintf(os.Stderr, "error: %q is not a valid RFC 5280 §5.3.1 reason code.\n\n", *reason) + fmt.Fprintf(os.Stderr, "Valid reasons (camelCase or snake_case both accepted):\n") + for _, r := range cli.ValidRevokeReasons() { + fmt.Fprintf(os.Stderr, " %s\n", r) + } + return fmt.Errorf("invalid --reason: %q", *reason) + } + return client.RevokeCertificate(id, canonical) case "bulk-revoke": return client.BulkRevokeCertificates(subArgs) default: diff --git a/docs/reference/cli.md b/docs/reference/cli.md index fca83c4..8c66ad3 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -73,21 +73,29 @@ certctl-cli certs list --fields id,common_name,expires_at,status ```bash certctl-cli certs renew mc-api-prod # Returns the job id; track with: certctl-cli jobs get + +# Recovery: clear a stuck in-flight renewal so a new one can start +certctl-cli certs renew mc-api-prod --force ``` +`--force` clears the server-side `RenewalInProgress` block — used when a previous renewal job hung without releasing the status flag. `--force` does NOT override `Archived` or `Expired` (those are terminal states; archived = decommissioned, expired = issue a new cert instead of renewing a dead one). + ### Revoke ```bash -# Single revoke +# Single revoke — --reason is REQUIRED (no silent fallback to 'unspecified') certctl-cli certs revoke mc-api-prod --reason keyCompromise +# snake_case is accepted and normalised to camelCase before dispatch +certctl-cli certs revoke mc-api-prod --reason key_compromise + # Bulk revoke by filter certctl-cli certs revoke --profile prof-deprecated --reason superseded certctl-cli certs revoke --team t-payments --reason cessationOfOperation -certctl-cli certs revoke --issuer iss-old-vault --reason cACompromise +certctl-cli certs revoke --issuer iss-old-vault --reason caCompromise ``` -Reason codes are the canonical RFC 5280 §5.3.1 set: `unspecified`, `keyCompromise`, `cACompromise`, `affiliationChanged`, `superseded`, `cessationOfOperation`, `certificateHold`, `removeFromCRL`, `privilegeWithdrawn`, `aACompromise`. Anything else returns an error. +`--reason` is mandatory: omitting it prints the canonical RFC 5280 §5.3.1 menu and exits non-zero. Compliance reporting (PCI-DSS §3.6, HIPAA §164.312) relies on the reason code being meaningful, so the CLI no longer falls back silently. Valid camelCase set: `unspecified`, `keyCompromise`, `caCompromise`, `affiliationChanged`, `superseded`, `cessationOfOperation`, `certificateHold`, `removeFromCRL`, `privilegeWithdrawn`, `aaCompromise`. snake_case variants (`key_compromise`, `cessation_of_operation`, etc.) are accepted and normalised. ### Bulk import diff --git a/internal/api/handler/certificate_handler_test.go b/internal/api/handler/certificate_handler_test.go index 54733f9..ad63e37 100644 --- a/internal/api/handler/certificate_handler_test.go +++ b/internal/api/handler/certificate_handler_test.go @@ -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{ diff --git a/internal/api/handler/certificates.go b/internal/api/handler/certificates.go index fe1a1df..a7e8325 100644 --- a/internal/api/handler/certificates.go +++ b/internal/api/handler/certificates.go @@ -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) diff --git a/internal/cli/client.go b/internal/cli/client.go index 0beb2a4..338bbc6 100644 --- a/internal/cli/client.go +++ b/internal/cli/client.go @@ -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, diff --git a/internal/cli/client_test.go b/internal/cli/client_test.go index 4d04d13..997b17a 100644 --- a/internal/cli/client_test.go +++ b/internal/cli/client_test.go @@ -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" { diff --git a/internal/service/certificate.go b/internal/service/certificate.go index f494680..fc15909 100644 --- a/internal/service/certificate.go +++ b/internal/service/certificate.go @@ -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") } diff --git a/internal/service/certificate_test.go b/internal/service/certificate_test.go index fcff4be..07c2926 100644 --- a/internal/service/certificate_test.go +++ b/internal/service/certificate_test.go @@ -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) {