mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-07 16:31:33 +00:00
650f5a198fb342eee356e0233443fed808836208
288 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 shankar0123
|
650f5a198f |
fix: collapse identical if/else branches in Account handler (CodeQL #25)
CodeQL alert #25 (go/duplicate-branches) on internal/api/handler/
acme.go::ACMEHandler.Account flagged that 'if readOnly { ... } else
{ ... }' had byte-identical bodies — both setting the same
Content-Type: application/json header. The 'readOnly' bool was
threaded through the function as a placeholder for differentiated
headers (Cache-Control etc. on the POST-as-GET path) that never
landed; both branches collapsed to the same value with no
follow-through.
Audit + fix:
- The alert is real (verified by re-reading the source); not a
false positive.
- The Copilot Autofix Anthropic surfaced was correct in spirit but
incomplete: it collapsed the if/else but left 'readOnly' as
dead code (declared at line 395, assigned at lines 400 and 436,
only read at the now-removed if). golangci-lint's 'unused'
linter would flag 'readOnly' next.
- Complete fix: collapse the if/else AND remove the now-unused
'readOnly' variable + its 2 assignments. Single unconditional
'w.Header().Set("Content-Type", "application/json")' covers
both paths (RFC 8555 §6.3 POST-as-GET + §7.3.2 / §7.3.6 update
+ deactivation all return the same account JSON shape — no spec
rationale for differentiating headers).
Verified locally: 'gofmt -l .' clean; 'go vet ./...' clean;
'go test -short -count=1 ./internal/api/handler/' green; 'grep
readOnly' on the file returns only the new explanatory comment
(no live references).
The alert was first detected in commit
|
||
|
shankar0123
|
1e1bc9b3b4 |
ci: fix Phase 4 post-push unused-symbol failures
CI on commit
|
||
|
shankar0123
|
f6ba5634fd |
ci: fix Phase 4 post-push gofmt failure (map-literal alignment)
CI on commit
|
||
|
shankar0123
|
4dc8d3fa5b |
acme-server: key rollover + revocation + ARI (Phase 4/7)
Closes the RFC 8555 + RFC 9773 surface beyond the issuance happy-path:
- POST /acme/profile/<id>/key-change (RFC 8555 §7.3.5)
- POST /acme/profile/<id>/revoke-cert (RFC 8555 §7.6)
- GET /acme/profile/<id>/renewal-info/<cert-id> (RFC 9773 ARI)
After this commit, ACME clients can rotate account keys, revoke certs
through the ACME surface (rather than only via the certctl GUI/API),
and fetch ARI for proactive renewal scheduling.
Architecture:
- Key rollover: outer JWS verified against the registered account key
(existing kid path); the inner JWS — embedded as the outer's payload
— verified against the embedded NEW jwk in a new dedicated routine
(ParseAndVerifyKeyChangeInner) that enforces RFC 8555 §7.3.5
inner-only invariants: MUST use jwk + MUST NOT use kid, payload
.account == outer.kid, payload.oldKey thumbprint-equals registered.
A single WithinTx swaps the stored thumbprint+pem and writes the
audit row. Concurrent-rollover safety via SELECT…FOR UPDATE on the
conflicting account row in UpdateAccountJWKWithTx; the loser
observes the winner's new thumbprint and is told to retry (409).
- Revocation: two auth paths. kid → AccountOwnsCertificate single-
indexed COUNT lookup over acme_orders. jwk → constant-time RFC 7638
thumbprint compare against the cert's pubkey. Both paths route
through service.RevocationSvc.RevokeCertificateWithActor so the
existing CRL/OCSP refresh + audit + metrics pipeline applies. RFC
5280 §5.3.1 numeric reason codes clamp to certctl's
domain.ValidRevocationReasons; codes 8 (removeFromCRL) + 10
(aACompromise) clamp to 'unspecified' since they aren't in the set.
- ARI is GET-only and unauth per RFC 9773 §4. Cert-id wire shape is
base64url(AKI).base64url(serial); ParseARICertID strict-decodes,
SerialHex emits the canonical certctl-shape lowercase-no-leading-
zeros hex used in certificate_versions.serial_number.
ComputeRenewalWindow has 3 branches: bound RenewalPolicy →
[notAfter - days, notAfter - days/2]; no policy → last 33% of
validity; past expiry → [now, now + 1d] (renew immediately).
Retry-After honors CERTCTL_ACME_SERVER_ARI_POLL_INTERVAL.
What ships:
- internal/api/acme/{keychange,ari}.go (+ phase4_test.go: 15 tests).
- internal/api/acme/order.go: RevokeCertRequest wire shape.
- internal/api/handler/acme.go: KeyChange, RevokeCert, RenewalInfo
+ 11 new writeServiceError mappings.
- internal/repository/postgres/acme.go: UpdateAccountJWKWithTx (FOR
UPDATE + expectedOldThumbprint precondition; ErrACMEAccountKey-
ConcurrentUpdate sentinel) + AccountOwnsCertificate.
- internal/service/acme.go: RotateAccountKey + RevokeCert +
RenewalInfo; CertificateRevoker + RenewalPolicyLookup interfaces;
SetRevocationDelegate + SetRenewalPolicyLookup wiring; 11 new
sentinels; 6 new metrics.
- internal/service/acme_phase4_test.go: service-layer tests for
RotateAccountKey (happy + duplicate-key) + RevokeCert (kid mismatch
+ jwk mismatch + jwk happy + already-revoked + reason-clamping) +
RenewalInfo (disabled + bad cert-id).
- internal/api/router/router.go: 6 new register calls (3 per-profile
+ 3 shorthand). Router parity exceptions extended in lockstep
(in-tree SpecParityExceptions + CI-only openapi-handler-exceptions
.yaml).
- cmd/server/main.go: SetRevocationDelegate(revocationSvc) +
SetRenewalPolicyLookup(renewalPolicyRepo) at startup.
- internal/config/config.go: CERTCTL_ACME_SERVER_ARI_ENABLED (default
true) + CERTCTL_ACME_SERVER_ARI_POLL_INTERVAL (default 6h);
BuildDirectory's ariEnabled flag now flips on under
cfg.ARIEnabled.
- docs/acme-server.md: phase status flipped to Phase 4; endpoints
table grows 6 rows (3 per-profile + 3 shorthand); FAQ section
appended explaining how to rotate keys, revoke certs, and consume
ARI.
Tests:
- 'go vet ./...' clean across the repo.
- 'go test -short -count=1 ./...' green across every package.
- phase4_test.go covers: keychange happy-path + 5 negatives +
MapKeyChangeErrorToProblem coverage; ARI cert-id round-trip + 6
malformed cases + BuildARICertID from a generated cert; window-
math 3 branches.
- service-layer tests confirm: RotateAccountKey atomically swaps the
thumbprint (verifies persisted state) and rejects duplicate keys;
RevokeCert routes through the stub RevocationSvc with the right
actor string + reason on the jwk path, rejects mismatched keys,
rejects already-revoked certs, clamps reason codes correctly;
RenewalInfo respects ARIEnabled + cert-id format.
Engineering history: cowork/WORKSPACE-CHANGELOG.md 'ACME-Server-4'.
|
||
|
shankar0123
|
62513ad12f |
ci: fix Phase 3 post-push CI failures (contextcheck + ST1021)
CI on commit
|
||
|
shankar0123
|
9bc845304e |
acme-server: HTTP-01 + DNS-01 + TLS-ALPN-01 challenge validation (Phase 3/7)
Wires up the actual challenge-validation machinery so profiles in
acme_auth_mode='challenge' resolve end-to-end. After this commit,
cert-manager 1.15+ with `solver: http01: ingress` against a
challenge-mode profile completes a real HTTP-01 flow and gets a cert.
DNS-01 + TLS-ALPN-01 share the same code path with the appropriate
validator selection.
Architecture (the load-bearing parts):
- 3 separate semaphore-bounded worker pools (one per challenge type),
so HTTP-01 and DNS-01 can't starve each other under load. Default
weight 10 per type; tunable via CERTCTL_ACME_SERVER_HTTP01_CONCURRENCY,
DNS01_CONCURRENCY, TLSALPN01_CONCURRENCY.
- 30s per-challenge timeout (configurable via PoolConfig.PerChallengeTimeout).
- HTTP-01 validator runs validation.IsReservedIPForDial (newly
exported wrapper preserving the existing private impl byte-for-byte
for the network scanner + ValidateSafeURL paths) on the resolved
IP — both at the initial dial and every redirect hop. SSRF probes
into private IP space are refused before the connect.
- DNS-01 validator uses a dedicated resolver pointed at
CERTCTL_ACME_SERVER_DNS01_RESOLVER (default 8.8.8.8:53) — does
NOT use the system resolver to keep behavior deterministic across
deployments. Wildcard handling: `*.example.com` queries
_acme-challenge.example.com.
- TLS-ALPN-01 validator (RFC 8737) connects with ALPN `acme-tls/1`,
inspects the id-pe-acmeIdentifier extension (OID 1.3.6.1.5.5.7.1.31),
asserts the ASN.1 OCTET STRING value equals SHA-256 of the key
authorization. Cert chain is intentionally NOT validated
(InsecureSkipVerify=true is correct per RFC 8737 — the proof is
in the extension, not the chain). Documented in docs/tls.md L-001
table + the //nolint:gosec comment carries the justification.
SSRF guard: same posture as HTTP-01.
- Validation is asynchronous: handler accepts the POST and returns
200 immediately with status=processing; the worker-pool fires a
callback that updates challenge → authz → order in a fresh
background-context WithinTx. The order auto-promotes to `ready`
when ALL authzs become valid; auto-fails to `invalid` when ANY
authz becomes invalid.
What ships:
- internal/api/acme/challenge.go: KeyAuthorization (RFC 8555 §8.1) +
DNS01TXTRecordValue (§8.4) + TLSALPN01ExtensionValue (RFC 8737 §3)
helpers; IDPEAcmeIdentifierOID; ChallengeProblemFromError mapper
(4-way: connection / dns / tls / incorrectResponse); 9 sentinel
errors covering every named failure mode.
- internal/api/acme/validators.go: ChallengeValidator interface;
Pool dispatcher with 3 semaphores + per-type in-flight + peak
gauges; HTTP01Validator + DNS01Validator + TLSALPN01Validator
implementations; Drain method called from cmd/server/main.go's
shutdown sequence.
- internal/api/acme/validators_test.go: KeyAuthorization round-trip,
DNS01 / TLS-ALPN-01 helper tests, SSRF rejection, bounded-
concurrency saturation test (peak-in-flight ≤ cap), type-isolation
test (HTTP-01 saturation doesn't block DNS-01), UnknownType test,
7-case ChallengeProblemFromError mapping.
- internal/repository/postgres/acme.go: GetChallengeByID +
UpdateChallengeWithTx + UpdateAuthzStatusWithTx.
- internal/service/acme.go: SetValidatorPool wires the *acme.Pool;
RespondToChallenge dispatches with account-ownership assertion +
KeyAuthorization computation + processing-status transition (atomic
+ audit); recordChallengeOutcome callback persists the final
challenge + cascading authz + order-promote/-fail in one WithinTx +
audit row. 4 new metrics.
- internal/api/handler/acme.go: Challenge handler; round-trips
account.JWKPEM through ParseJWKFromPEM to recover the *jose.JSONWebKey
the validator pool needs.
- internal/api/router/router.go + openapi_parity_test.go +
api/openapi-handler-exceptions.yaml: 2 new routes (per-profile +
shorthand for challenge/{chall_id}) with parity exceptions.
- cmd/server/main.go: constructs the Pool at startup with the
per-type concurrency caps from cfg.ACMEServer; ACMEService.ValidatorPool()
accessor exposed for the shutdown drain sequence.
- internal/validation/ssrf.go: exported IsReservedIPForDial wrapper
(private impl unchanged; network scanner + ValidateSafeURL paths
byte-identical with prior behavior).
- docs/tls.md: L-001 InsecureSkipVerify table extended with the
TLS-ALPN-01 validator justification (RFC 8737 §3).
- docs/acme-server.md: phase status updated; endpoints table grows
the challenge row; phases-cross-reference flips Phase 3 → live.
Tests:
- 80%+ coverage on the new files.
- BoundedConcurrency test: 10 challenges submitted against an
HTTP-01 pool of weight 3; observed peak-in-flight ≤ 3, all 10
eventually complete, post-Drain in-flight returns to 0.
- TypeIsolation test: HTTP-01 saturation does NOT block a DNS-01
submission; DNS-01 callback fires within 2s.
- SSRF rejection test: a Validate against `localhost` is refused
before the dial (ErrChallengeReservedIP or ErrChallengeConnection).
Engineering history: cowork/WORKSPACE-CHANGELOG.md "ACME-Server-3".
|
||
|
shankar0123
|
c351bba41a |
acme-server: orders + authorizations + finalize + cert download (Phase 2/7)
Closes the issuance loop in trust_authenticated mode (commits |
||
|
shankar0123
|
44a85d6f85 |
acme-server: account resource + JWS verifier (Phase 1b/7)
Layers JWS-authenticated POST machinery onto the Phase 1a foundation
(commit
|
||
|
shankar0123
|
ec88a61274 |
acme-server: foundation — directory + new-nonce + per-profile routing (Phase 1a/7)
First slice of the RFC 8555 ACME server endpoint (master plan at cowork/acme-server-endpoint-prompt.md, per-phase prompts at cowork/acme-server-prompts/). This commit lands the smallest viable end-to-end deployable slice: an ACME client running curl -sk https://certctl/acme/profile/<id>/directory curl -sk -I https://certctl/acme/profile/<id>/new-nonce successfully fetches the directory document and a Replay-Nonce. Account creation, JWS verification, orders, challenges, and revocation are all out of scope for this phase and arrive in Phases 1b–4. Closes the Rank 1 LHF from the 2026-05-03 Infisical deep-research (cowork/infisical-deep-research-results.md). Pre-fix, certctl was an ACME consumer only — no /acme/directory endpoint, no JWS verifier, no challenge validators. K8s customers running cert-manager could not point at certctl as an ACME issuer; they had to deploy a certctl agent on every node. What ships: - internal/api/acme/{directory,nonce,errors}.go (+ tests). - internal/api/handler/acme.go + acme_handler_test.go. - internal/repository/postgres/acme.go (nonce ops only — Phase 1b extends with account CRUD; Phases 2-4 extend with order / authz / challenge CRUD). - internal/service/acme.go (BuildDirectory + IssueNonce stubs; Phase 1b adds VerifyJWS / NewAccount / etc.). - migrations/000025_acme_server.{up,down}.sql ships the full 5-table ACME schema (acme_accounts / acme_orders / acme_authorizations / acme_challenges / acme_nonces) PLUS the per-profile certificate_profiles.acme_auth_mode column. Phase 1a actively uses only acme_nonces; remaining tables are empty until Phases 1b-4 plug in. - internal/config/config.go: ACMEServerConfig struct + ACMEServer field on Config. Env vars use CERTCTL_ACME_SERVER_* prefix to avoid colliding with the existing consumer-side ACMEConfig at config.go:1746 (CERTCTL_ACME_DIRECTORY_URL / PROFILE / CHALLENGE_TYPE etc.). Phase 1a wires Enabled + DefaultAuthMode + DefaultProfileID + NonceTTL + DirectoryMeta; Order/Authz TTLs + per-challenge-type concurrency caps + DNS01 resolver are reserved fields parsed in 1a so operators can set them ahead of Phases 2/3. - cmd/server/main.go: wire ACMEHandler into the HandlerRegistry literal alongside the existing certificate / EST / SCEP / etc. handlers. - internal/api/router/router.go: HandlerRegistry.ACME field + 6 Register calls (3 per-profile + 3 shorthand). - internal/api/router/openapi_parity_test.go: 6 new entries in SpecParityExceptions. ACME is a wire-protocol surface (JWS-signed JSON over HTTPS per RFC 7515) whose semantics are dictated by RFC 8555 + RFC 9773 rather than by an OpenAPI document, same precedent as SCEP/EST. The canonical reference is docs/acme-server.md. - docs/acme-server.md: Phase-1a-shaped reference. Configuration table for every CERTCTL_ACME_SERVER_* env var. Per-profile auth-mode decision tree skeleton. TLS trust bootstrap section flagging cert-manager's ClusterIssuer.spec.acme.caBundle requirement (the single biggest first-time-deploy footgun; the full cert-manager walkthrough lands in Phase 6 but the requirement is documented up front). Architecture decisions baked in: - URL family is /acme/profile/<id>/* (per-profile, canonical) with /acme/* shorthand active when CERTCTL_ACME_SERVER_DEFAULT_PROFILE_ID is set. Path matches existing per-profile precedent in EST + SCEP. - Auth mode is per-profile (acme_auth_mode column on certificate_profiles), NOT server-wide. One certctl-server can serve trust_authenticated for an internal-PKI profile and challenge for a public-trust-style profile simultaneously. The column is read at request time, not cached at server start — operators flipping a profile's mode via SQL take effect on the next order without restart. - Nonces are DB-backed (acme_nonces table). Survive server restart. The RFC 8555 §6.5 replay defense requires the store to outlast the client's nonce caching window; an in-memory-only nonce store would lose every in-flight order on restart. - Per-op atomic counters on service.ACMEService.Metrics() — certctl_acme_directory_total, certctl_acme_directory_failures_total, certctl_acme_new_nonce_total, certctl_acme_new_nonce_failures_total. Naming follows certctl frozen decision 0.10 cardinality discipline. Phase 1b will extend with new_account counters; Phase 2 with order / finalize / cert; Phase 3 with per-challenge-type counters. Audit fixes #11 + #12 (cowork/acme-server-prompts/audit-additions.md) applied: - #11: CERTCTL_ACME_SERVER_* prefix avoids the consumer-side CERTCTL_ACME_* namespace collision. - #12: prior-attempt WIP from two failed Phase-1 dispatches was discarded at phase start; this commit starts from a clean tree. Tests: - 14 unit tests in internal/api/acme/ (directory, nonce, errors). - 7 handler-level tests via httptest.NewServer + mockACMEService (mirrors the mockSCEPService pattern at scep_handler_test.go). - 7 service-layer tests with mocked repo + injected profileLookup. - All pass under -race -count=1 -short. Deferred to Phase 1b: - JWS verification (go-jose v4 — see master-prompt §8a for the API surface and audit doc for the speculation pitfalls). - new-account / account/<id> endpoints + AccountService. - Nonce *consumption* path (issue path is in this commit; consume is only invoked by JWS-verified POSTs which Phase 1b adds). Engineering history: cowork/WORKSPACE-CHANGELOG.md "ACME-Server-1a". Per-phase implementation plan: cowork/acme-server-prompts/. Master plan + audit fixes: cowork/acme-server-endpoint-prompt.md + cowork/acme-server-prompt-audit.md + cowork/acme-server-prompts/audit-additions.md. |
||
|
shankar0123
|
b8b7e1e3dd |
tlsprobe: add VerifyWithExponentialBackoff + rewire all connectors' runPostDeployVerify
Closes Top-10 fix #8 of the 2026-05-02 deployment-target audit re-run (see cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md). Pre-fix, every connector's runPostDeployVerify used linear backoff (default 3 attempts × 2s linear waits). Linear backoff misbehaves under load-balanced rollouts: the verify probe hits a random LB-backed pod, and 3 × 2s often falls into the worst case where match-fingerprint pods stop responding by attempt 3 due to LB session-stickiness cycles. This commit: 1. New shared helper internal/tlsprobe/retry.go:: VerifyWithExponentialBackoff. Default 3 attempts; 1s initial, 16s cap. Doubling pattern: 1s → 2s → 4s → 8s → 16s. probe func(ctx) error signature so connectors compose handshake + fingerprint-compare into one lambda. 2. Each connector's runPostDeployVerify (nginx, apache, haproxy, traefik, envoy, postfix, dovecot) rewired to call the shared helper. Per-connector signature unchanged. 3. New PostDeployVerifyMaxBackoff time.Duration field added to each connector's Config. Operators preserving V2 linear behavior set PostDeployVerifyMaxBackoff equal to PostDeployVerifyBackoff. 4. Tests: - tlsprobe/retry_test.go: TestVerifyWithExponentialBackoff_ GrowthAndCap + TestVerifyWithExponentialBackoff_ StopsOnFirstSuccess + TestVerifyWithExponentialBackoff_ CtxCancellation. - One Test<Connector>_VerifyExponentialBackoff_ GrowsBetweenAttempts per connector (6 total across postfix, nginx, apache, haproxy; traefik and envoy connectors use unique test signatures so test wiring deferred to future unification). 5. docs/deployment-atomicity.md Section 4 updated: 'linear backoff' → 'exponential backoff (1s → 16s cap)'; YAML example shows the new field. Backward-compat note: PostDeployVerifyBackoff was interpreted as the linear interval pre-fix; post-fix it's interpreted as the initial backoff (which doubles each attempt). Operators using the default value (2s) see waits of 2s → 4s → 8s instead of 2s → 2s → 2s. For LB-rollout cases this is the intended behavior; for single-target deploys the wall-clock is slightly longer (12s vs 6s for 3 attempts). Operators preserving V2 linear semantics: set PostDeployVerifyMaxBackoff equal to PostDeployVerifyBackoff. Verified locally: - gofmt clean. - go test -short -count=1 ./internal/tlsprobe/... ./internal/connector/target/{postfix,nginx,apache,haproxy}/... green. Audit reference: cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md Top-10 fix #8. |
||
|
shankar0123
|
b16e5b5e97 |
docs(ssh): operator playbook for InsecureIgnoreHostKey design choice
Closes Top-10 fix #7 of the 2026-05-02 deployment-target audit re-run (see cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md). Pre-fix, the SSH connector's ssh.InsecureIgnoreHostKey() at internal/connector/target/ssh/ ssh.go (realSSHClient.Connect) had only an inline comment justifying the design choice. An acquirer's diligence engineer reading the connector cold pattern-matches "MITM hazard" without seeing the comment. This commit lands a doc-side operator playbook in docs/connectors.md SSH section covering: 1. Why the connector accepts any host key (operator-configured target infrastructure; mirrors network scanner's InsecureSkipVerify and F5's Insecure flag). 2. Threat model the choice accepts (passive eavesdropper on operator-controlled network; layered SSH-key auth limits blast radius). 3. Threat model the choice does NOT accept (public-internet ephemeral hosts, multi-tenant networks, strict MITM- resistance regulatory requirements). 4. Mitigations operators can layer (custom SSHClient via NewWithClient + golang.org/x/crypto/ssh/knownhosts; SSH certificate authentication via @cert-authority pinning; network segmentation; per-target key rotation). 5. When to NOT use the SSH connector (regulatory environments, dynamic IPs, multi-tenant networks). 6. V3-Pro forward path (built-in known_hosts management, tracked in WORKSPACE-ROADMAP.md). Inline comment in ssh.go realSSHClient.Connect updated to forward-reference the new doc subsection (no logic change; same HostKeyCallback: ssh.InsecureIgnoreHostKey() call). Same shape Bundle 8 used for "Operator playbook: keytool argv password exposure" in docs/connectors.md JavaKeystore section. No code-behavior changes. No test changes. Verified locally: - gofmt / go vet clean. - go test -short ./internal/connector/target/ssh/... green. Audit reference: cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md Top-10 fix #7. |
||
|
shankar0123
|
62f0a284be |
iis,wincertstore: default-deadline ctx wrapper for PowerShell exec calls
Closes Top-10 fix #4 of the 2026-05-02 deployment-target audit re-run (see cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md). Pre-fix, both IIS and WinCertStore's realExecutor invoked PowerShell via exec.CommandContext(ctx, ...) and relied entirely on the caller's ctx to provide a deadline. If the caller forgot to attach one (context.Background() in a deeply-nested path; an operator running an ad-hoc deploy via a CLI that doesn't default-deadline its ctx), a hung WinRM session blocked the deploy worker thread indefinitely. S2 (failure isolation) bar from the audit: "does a hung WinRM take down the deploy worker pool?" — today's answer was "potentially yes" for these two connectors. Post-fix the answer is "no, capped at the configured ExecDeadline (default 60s)". This commit: 1. Adds Config.ExecDeadline (time.Duration, json: "exec_deadline") to both connectors, defaulted to 60 seconds. WinCertStore defaults via the existing applyDefaults helper; IIS defaults inline at New() and inside ValidateConfig (the IIS connector has no shared applyDefaults helper today; out-of-scope to refactor one in for this minor fix). Operators on slow Windows links can override via the JSON config field exec_deadline. 2. Wraps realExecutor.Execute with a fallback context.WithTimeout that fires ONLY when ctx has no deadline of its own. Caller- supplied deadlines always win — the wrapper is a safety net, not a hard cap. defer cancel() guards against goroutine leaks. 3. Tests: - TestIIS_RealExecutor_AttachesDefaultDeadlineWhenCallerHasNone (passes context.Background; asserts the call returns within 500ms with an error). On Linux/macOS runners powershell.exe is missing and exec.Cmd fails fast; on Windows the wrapper's ctx deadline cancels the running PowerShell process. Either path returns well under 500ms. - TestIIS_RealExecutor_RespectsCallerDeadlineWhenSet (10s fallback executor deadline, 50ms caller ctx; asserts caller deadline wins). - TestIIS_RealExecutor_NoDeadlineWiredWhenZero (deadline=0 means no fallback wrapper; caller's tight ctx still bounds). - TestIIS_New_DefaultsExecDeadlineTo60s + TestIIS_New_RespectsExplicitExecDeadline pin the constructor's defaulting behavior (uses winrm mode so the test doesn't need powershell.exe in PATH). - Same five tests in wincertstore_test.go. 4. docs/connectors.md IIS + WinCertStore sections document the new exec_deadline field with: what it is (per-PowerShell- subprocess cap), default (60 seconds), override semantics (caller ctx deadline wins). No change to behavior when the caller already attaches a deadline (the common case in production code paths). Tests using the mock executor (mockExecutor in iis_test.go / wincertstore_test.go) are unaffected — they bypass realExecutor entirely. S2 cross-cutting scorecard rating in cowork/deployment-target-audit-2026-05-02-rerun/findings.json flips from "gap" to "pass" for IIS and WinCertStore (in any future re-audit). Verified locally: - gofmt / go vet / staticcheck clean across both packages. - go test -race -count=1 ./internal/connector/target/iis/... ./internal/connector/target/wincertstore/... green. Audit reference: cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md Top-10 fix #4. |
||
|
shankar0123
|
4142837cac |
iis,wincertstore,javakeystore: SHA-256 idempotency short-circuit
Closes Top-10 fix #3 of the 2026-05-02 deployment-target audit
re-run (see cowork/deployment-target-audit-2026-05-02-rerun/
RESULTS.md). Pre-fix, the three PowerShell-driven connectors
(IIS / WinCertStore / JavaKeystore) bypass internal/deploy.Apply
because they write to the Windows cert store / Java keystore via
PowerShell + keytool rather than the local filesystem. They don't
get deploy.Apply's SHA-256 idempotency short-circuit for free, so
every renewal triggers a full Remove+Import cycle even on byte-
identical material. Operators with 60-day rotation see unnecessary
cert-store / keystore churn, briefly bumping CPU and possibly
disrupting connections in flight.
This commit adds a per-connector idempotency probe modeled on
Bundle 9's Caddy api-mode SHA-256 short-circuit (commit
|
||
|
shankar0123
|
b8293653a5 |
postfix: add atomic-test variants for Mode=dovecot (happy path + verify-rollback)
Closes Bundle 11 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix,
postfix_atomic_test.go exercised the atomic deploy path under Mode=
postfix only — the existing TestPostfix_DovecotMode at L233-246
asserted only the DeploymentID prefix, leaving applyDefaults's
dovecot-specific validate/reload command set + the rollback's
file-content-restoration unverified at the deploy-test layer.
Audit's only test-coverage gap on the otherwise-production-grade
Postfix/Dovecot connector.
This commit adds two new tests (test-only commit; no production-
code changes):
1. TestPostfix_Atomic_DovecotMode_HappyPath. Builds a Config with
Mode: "dovecot" and NO ValidateCommand / NO ReloadCommand set.
Calls ValidateConfig (which is what triggers applyDefaults via
its JSON-marshal-then-parse path) before DeployCertificate.
Captures the validate + reload commands threaded through the
SetTestRunValidate / SetTestRunReload hooks. Asserts:
- capturedValidateCmd contains "doveconf -n" (applyDefaults
populated it from the dovecot branch).
- capturedReloadCmd contains "doveadm reload".
- DeploymentID prefix "dovecot-" + result.Metadata["mode"] is
"dovecot" (Mode survived end-to-end).
2. TestPostfix_Atomic_DovecotMode_VerifyFails_Rollback. Pre-creates
cert.pem AND key.pem with known "ORIG-CERT" / "ORIG-KEY" bytes.
Builds Config with Mode: "dovecot", PostDeployVerify enabled
(Endpoint pointing at a dovecot-IMAPS-style :993 — value unused
by the probe stub), PostDeployVerifyAttempts: 1 (default is 3
attempts × 2s backoff = 4+ seconds; we don't need that for a
unit test). Probe stub returns Success: false, which
runPostDeployVerify wraps as "TLS probe failed: ...". Asserts:
- DeployCertificate returns error containing "TLS probe failed".
- cert.pem AND key.pem on disk contain the ORIG bytes
verbatim — Bundle 11's load-bearing assertion that the
rollback restored the pre-deploy file state under
Mode=dovecot. The existing TestPostfix_VerifyMismatch_Rollback
(Mode=postfix) only asserts the error; this test extends to
file-content restoration.
Existing TestPostfix_DovecotMode (L233-246) preserved as-is — the
minimal DeploymentID-prefix smoke test complements the new richer
tests without duplicating their scope.
The encoding/json import is added to support the HappyPath test's
json.Marshal call. No other dependency changes.
No production-code changes; the connector itself was already
correct for Mode=dovecot. Only the test pin was missing.
Verified locally:
- gofmt -l ./internal/connector/target/postfix/ clean
- go vet ./internal/connector/target/postfix/ clean
- go build ./cmd/agent/... clean (no signature changes)
- go test -race -count=1 ./internal/connector/target/postfix/ green
(24 tests total: 22 pre-existing + 2 new)
Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 11.
|
||
|
shankar0123
|
08a86d355d |
caddy: fix duration metric + file-mode PEM validate + api-mode idempotency
Closes Bundle 9 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Three
small independent fixes that share one connector file:
1. Duration metric (caddy.go L176). Pre-fix:
"duration_ms": fmt.Sprintf("%d", time.Since(time.Now()).Milliseconds())
This always returned ~0ms because time.Now() was called twice —
the second call captured a baseline immediately before time.Since
computed the delta. The intended baseline is `startTime` declared
at L113 and threaded through deployViaFile correctly. Post-fix:
"duration_ms": fmt.Sprintf("%d", time.Since(startTime).Milliseconds())
deployViaAPI's signature evolves to take startTime time.Time so
the api-mode path uses the same baseline as the file-mode path.
2. File-mode ValidateDeployment now validates PEM syntax. Pre-fix
(caddy.go L266-293) checked file existence only via os.Stat. A
cert file containing garbage bytes passed validation; Caddy's
file-watcher silently failed to load it; operators saw "validation
green" + "TLS handshake fails" with no obvious connection.
Post-fix: after the os.Stat checks succeed, os.ReadFile + parse
the first PEM block as an x509 cert via the shared
certutil.ParseCertificatePEM helper. Failure surfaces as
Valid=false with a clear "not valid PEM/x509" message.
3. API-mode idempotency short-circuit. Pre-fix, every deploy POSTed
to /config/apps/tls/certificates/load even when the active cert
was already what we wanted to deploy. Caddy reloads TLS state on
every POST, briefly bumping CPU and possibly disrupting connections
in flight. Post-fix: idempotencySkipPOST runs a GET first, parses
the response (handles BOTH the array-of-objects and single-object
shapes Caddy admin can return), SHA-256 compares the entry's
`cert` field to the deploy payload's cert bytes, and skips the
POST when match. Result.Metadata["idempotent"]="true" surfaces
the no-op. Conservative: any GET failure (network, non-200, parse
error, no matching entry, hash mismatch) silently falls through to
the POST, preserving today's behavior. Idempotency is a fast path,
not a correctness boundary — false negatives are safe; false
positives are dangerous.
Tests added to caddy_test.go (6 new tests, ~290 LOC):
- TestCaddy_API_DurationMetric_NonZero (httptest server with a 10ms
sleep in the POST handler; asserts duration_ms parses as int >= 5).
- TestCaddy_ValidateDeployment_FileMode_MalformedPEM_Rejected (writes
garbage to cert.pem; asserts Valid=false with PEM/x509 in message).
- TestCaddy_ValidateDeployment_FileMode_ValidPEM_Accepted (writes a
real ECDSA P-256 self-signed cert; asserts Valid=true).
- TestCaddy_API_Idempotent_SkipsPOSTWhenCertHashMatches (GET response
contains the same cert as the deploy payload; POST counter remains
0; metadata.idempotent=true; exactly 1 GET probe ran).
- TestCaddy_API_Idempotent_RunsPOSTWhenCertHashDiffers (GET response
contains a DIFFERENT cert; POST counter is 1; idempotent absent).
- TestCaddy_API_Idempotent_GETFails_FallsThroughToPOST (GET returns
500; POST still runs; deploy succeeds; idempotent absent).
Two existing tests updated to match the new contracts:
- TestCaddyConnector_DeployViaAPI_Success: mock handler now serves
BOTH GET (returns "[]" so the comparison falls through) and POST
(the original 200-OK path). The dispatch is a method-switch
inside the path-match branch.
- TestCaddyConnector_ValidateDeployment_Success: the placeholder
cert "MIIC..." used to pass the old existence-only check; post-Fix-2
it fails the PEM-parse check. Test now uses generateTestCertAndKey
to produce a real self-signed ECDSA P-256 cert.
generateTestCertAndKey helper added to the test file — same pattern
the javakeystore + wincertstore tests use, kept local because the
caddy package has no other test in the certutil family that would
make a shared helper cleaner.
Verified locally:
- gofmt -l ./internal/connector/target/caddy/ clean
- go vet ./internal/connector/target/caddy/ clean
- go build ./cmd/agent/... clean (factory wiring unchanged)
- go test -race -count=1 ./internal/connector/target/caddy/ green
(16 tests total: 11 pre-existing including the two updated +
6 new)
Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 9.
|
||
|
shankar0123
|
eb390b2db4 |
javakeystore: pre-deploy export snapshot + on-import-failure rollback + argv-password operator note
Closes Bundle 8 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix,
DeployCertificate at javakeystore.go:172-272 ran an irreversible
keytool -delete against the existing alias, then keytool
-importkeystore. If the import failed after the delete succeeded,
the keystore was missing the alias entirely — previous cert gone,
new cert never landed. docs/deployment-atomicity.md L94 promised
"keytool snapshot; rollback via keytool -delete + re-import"; the
code didn't deliver. Separately, the operator-facing keystore
password is passed via -storepass argv (a standard keytool
limitation) which is visible to ps(1) for the duration of each
subprocess; this was undocumented as an operator-playbook caveat.
This commit:
1. Pre-delete snapshot. When os.Stat(KeystorePath) succeeds,
snapshotKeystore runs keytool -exportkeystore to
<BackupDir>/.certctl-bak.<unix-nanos>.p12 BEFORE the existing
-delete step. Backup path persisted in a local variable for
the rollback path; export-step failure aborts the deploy
entirely (no mutation has happened yet — the keystore is
untouched). Snapshot skipped on first-time deploys (no
keystore file = nothing to roll back to). The "alias not
present in pre-existing keystore" case is recognised via the
well-known keytool error string and treated as a clean
first-time-on-existing-keystore signal — the deploy proceeds
without a backup, and rollback (if needed) becomes the
no-backup branch.
2. On-import-failure rollback. When keytool -importkeystore
returns error, rollbackImport(ctx, backupPath) runs:
- keytool -delete -alias <Alias> ... (best-effort; the failed
import may have created a partial alias entry).
- keytool -importkeystore from the backup PKCS#12 to restore
the previous state.
On rollback success, the deploy returns wrapped error noting
"rolled back from <backup_path>". On rollback failure,
returns operator-actionable wrapped error containing both the
import error AND the rollback error AND the backup path so
the operator can manually keytool -importkeystore from the
.p12 file to recover.
3. Backup retention. Successful deploys prune older
.certctl-bak.*.p12 files beyond Config.BackupRetention.
Sort by ModTime newest-first; keep most recent N. Defaults:
BackupRetention=0 → keep most recent 3 (the default).
BackupRetention=N → keep most recent N.
BackupRetention=-1 → opt out of pruning entirely (operators
that wire their own archival/rotation).
Pruning runs in the success path AFTER the optional reload
command so it doesn't interfere with deploy-time signals.
ReadDir / Remove failures are non-fatal (debug log only) —
the deploy already succeeded.
4. Config gains BackupRetention int and BackupDir string fields.
BackupDir defaults to filepath.Dir(KeystorePath) so backups
land on the same filesystem as the keystore (atomic-ish
writes, disk-full failures fail fast at snapshot time).
5. Helper extraction. snapshotKeystore + rollbackImport +
pruneBackups + backupDir are private methods on Connector.
Constants backupFilePrefix=".certctl-bak." and
backupFileSuffix=".p12" centralise the naming convention so
the snapshot writer, the rollback reader, and the retention
pruner all agree.
6. Operator-playbook section added to docs/connectors.md
JavaKeystore section. Documents the standard keytool
-storepass argv exposure: ps(1)-visible for the duration
of each subprocess. Lists mitigations:
- Restrict shell access to the agent host.
- Linux user namespaces / AppArmor / SystemD ProtectProc=
invisible to deny ps-visibility.
- Single-purpose container for proper PID-namespace
isolation.
- Post-deploy keystore password rotation via reload_command
for high-security environments.
- BCFKS keystore type for FIPS environments (same argv
caveat applies).
Also documents an "Atomic rollback" subsection covering the
snapshot/rollback flow, the new backup_retention /
backup_dir Config fields, and the design choice to reuse
the keystore password for the snapshot (rather than
generating a separate transient password) — operator
already trusts the connector with this secret, surface area
doesn't grow, rollback's matching -srcstorepass stays
simple.
Tests added to javakeystore_test.go (7 new tests, ~430 LOC):
- TestJKS_Snapshot_RunsBefore_Delete: mock executor records call
order; asserts -exportkeystore is call[0], -delete is call[1],
-importkeystore is call[2]. The snapshot MUST run before the
delete — otherwise the delete destroys the very state the
snapshot is meant to capture.
- TestJKS_Snapshot_FirstTimeDeploy_NoExport: no keystore file
pre-created; asserts exactly 1 keytool call (-importkeystore
only), no -exportkeystore.
- TestJKS_ImportFails_RollsBack: happy rollback path with one
same-Subject backup. Asserts rollback re-import references the
same backup path the snapshot wrote (verified via arg
comparison between call[0] and call[4]).
- TestJKS_ImportFails_RollbackAlsoFails_OperatorActionable:
wrapped-error escalation with backup path in the error
message.
- TestJKS_BackupRetention_PrunesOldBackups: 5 pre-existing
staggered-ModTime backups + 1 deploy-created → retention=3 →
exactly 3 newest survive (deploy-created + 2 newest
pre-existing); 3 oldest pre-existing pruned.
- TestJKS_BackupRetention_Zero_DefaultsTo3: BackupRetention=0
must default to 3 (not "keep none").
- TestJKS_BackupRetention_Negative_OptsOut: BackupRetention=-1
pre-existing 5 + deploy 1 = 6 total, all 6 remain.
- TestJKS_Snapshot_AliasNotInKeystore_ProceedsCleanly: keystore
exists but alias missing; -exportkeystore returns "alias does
not exist" → snapshot helper recognises this signal and
returns ("", nil) so the deploy proceeds cleanly.
mockExecutor extended with optional `onCall` hook so the
retention-pruning tests can simulate keytool -exportkeystore's
file-write side effect (via the simulateExportSideEffect helper
that parses -destkeystore from args and writes a placeholder
.p12 file). Existing tests that don't set onCall behave
identically to before — backward compatible.
docs/deployment-atomicity.md L94 unchanged from today's text —
Bundle 1 doc-realignment hasn't shipped, so the "keytool snapshot;
rollback via keytool -delete + re-import" line was never softened.
Post-Bundle-8 the claim is honest (was aspirational pre-fix).
Verified locally (sandbox lacks staticcheck install due to disk
pressure; CI runs the full lint gate):
- gofmt -l ./internal/connector/target/javakeystore/ clean
- go vet ./internal/connector/target/javakeystore/ clean
- go build ./cmd/agent/... clean
- go test -race -count=1 ./internal/connector/target/javakeystore/
green (16 tests total: 9 pre-existing + 7 new)
Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 8.
|
||
|
shankar0123
|
60ae92b0e8 |
wincertstore: pre-deploy snapshot + on-import-failure rollback
Closes Bundle 7 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix,
DeployCertificate at wincertstore.go:162-215 ran a single PowerShell
script that imported the PFX, optionally set FriendlyName, and
optionally removed expired same-Subject certs. Import-PfxCertificate
is atomic at the cert-store level, but the wider sequence (import →
friendly name → remove expired) is not. Failure in any post-import
step left the new cert in the store with no clean recovery path.
docs/deployment-atomicity.md L93 promised "Get-ChildItem snapshot
for rollback"; the code didn't deliver.
This commit:
1. Pre-deploy snapshot. New PowerShell script (tagged
`# CERTCTL_SNAPSHOT`) runs Get-ChildItem over the target store,
captures every thumbprint, and for each cert with the same
Subject as the new one calls Export-PfxCertificate to a tempdir
using a transient snapshotExportPassword (32-byte random,
distinct from the import PFX password). Output parsed into a
snapshotState{Entries: []{Thumbprint, PfxPath}, AllThumbprints,
TempDir, ExportPassword}. The new cert's Subject is parsed from
request.CertPEM via certutil.ParseCertificatePEM before any
cert-store mutation; PEM-parse failure aborts the deploy
cleanly.
2. On-import-failure rollback. When the import-script Execute
returns error, run a rollback script (tagged
`# CERTCTL_ROLLBACK`) that:
- Test-Path on the new cert path; Remove-Item if present.
- Import-PfxCertificate -FilePath <pfxPath> for each snapshot
entry (restores prior state).
- Remove-Item -Recurse on the snapshot tempdir.
3. Post-rollback verification. Re-read Get-ChildItem (tagged
`# CERTCTL_VERIFY`); assert every original thumbprint is back.
On mismatch, append a warning to the DeploymentResult message
(rollback ran but final state is suspect — operator inspection
recommended). Skipped when AllThumbprints is empty (first-time
deploy).
4. Success-path tempdir cleanup. New script tagged
`# CERTCTL_CLEANUP` runs after a successful import to remove
the snapshot tempdir on a best-effort basis. Failure here is
non-fatal (debug log only).
5. Helper extraction. rollbackImport(ctx, snapshot, newThumbprint)
+ verifyRollback(ctx, snapshot) + cleanupSnapshot(ctx, snapshot)
+ parseSnapshotOutput are private methods/functions on
Connector for clean test seams. Each script emits a unique
`# CERTCTL_*` PowerShell comment tag so test mocks can match
scripts deterministically — the snapshot/rollback/verify/cleanup
scripts all reference Cert:\<store> paths, so the comment tags
are the only deterministic substring under randomized map
iteration.
DeploymentResult shape on failure:
- import OK, rollback OK → Success=false, "PowerShell import
failed; rolled back" (clean
recoverable failure).
- import FAIL, rollback OK → same.
- rollback FAIL → operator-actionable wrapped error
containing both errors; metadata
flags manual_action_required=true
and surfaces import_error /
rollback_error verbatim.
Tests added to wincertstore_test.go:
- TestWinCertStore_ImportFails_RemovesNewCert_RestoresOldFromSnapshot
— happy rollback path with one same-Subject cert in the
snapshot. Asserts rollback script contains Remove-Item for the
new thumbprint AND Import-PfxCertificate referencing the
snapshotted PFX path.
- TestWinCertStore_ImportFails_NoExistingSameSubject_RemovesNewCertOnly
— snapshot has THUMB: lines but no SNAPSHOT: entries; rollback
removes the new cert but does NOT call Import-PfxCertificate.
- TestWinCertStore_FriendlyNameFails_NewCertRemoved_OldCertsRestored
— variant where the import script's failure originates from
Set-ItemProperty FriendlyName; same rollback path. Asserts
metadata.import_error preserves the FriendlyName-related
PowerShell output for operator visibility.
- TestWinCertStore_ImportFails_RollbackAlsoFails_OperatorActionable
— wrapped-error escalation. Asserts the error mentions both
"PowerShell import failed" and "rollback also failed", and
metadata flags manual_action_required=true.
Three existing tests (Success, ImportFailed, WithFriendlyName,
WithRemoveExpired) updated to match the new contract: success
path runs 3 PowerShell scripts (snapshot + import + cleanup),
import-failure path runs 4 (snapshot + import + rollback + verify),
and the import script lives at mock.scripts[1] not [0].
PowerShell injection note: the new cert's Subject DN is embedded
in the snapshot script as a single-quoted literal. Subject DNs can
contain apostrophes (e.g. CN=O'Reilly), so escapePowerShellSingleQuoted
doubles them per the PowerShell single-quoted-literal escape rule.
The export password and thumbprints come from
certutil.GenerateRandomPassword (alphanumeric only) and the cert's
SHA-1 thumbprint hex (alphanumeric); no escaping needed for those.
docs/deployment-atomicity.md L93 unchanged from today's text —
Bundle 1 doc-realignment hasn't shipped, so the "Get-ChildItem
snapshot for rollback" line was never softened. Post-Bundle-7 the
claim is honest (was aspirational pre-fix).
Verified locally (sandbox lacks staticcheck install due to disk
pressure; CI runs the full lint gate):
- gofmt -l ./internal/connector/target/wincertstore/ clean
- go vet ./internal/connector/target/wincertstore/ clean
- go build ./cmd/agent/... clean
- go test -race -count=1 ./internal/connector/target/wincertstore/
green
Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 7.
|
||
|
shankar0123
|
c222c8b57a |
ssh: fix staticcheck ST1008 — error is last return from restoreFromBackups
CI's golangci-lint run on commit
|
||
|
shankar0123
|
636de7f6b5 |
ssh: pre-deploy snapshot + reload-failure rollback
Closes Bundle 6 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix,
DeployCertificate at ssh.go:201-316 wrote new cert/key/chain via
SFTP then ran the operator's reload command. If reload failed, the
new files stayed on the remote — partial-success state with no
rollback path. docs/deployment-atomicity.md L92 promised "Pre-deploy
SCP backup of remote files"; the code didn't deliver.
This commit:
1. Pre-deploy snapshot. Before any WriteFile, iterate the deploy's
target paths (cert, key, optional chain). For each path:
- StatFile to detect existence. errors.Is(err, os.ErrNotExist)
means first-time deploy (rollback = Remove). Other stat
errors bail out before any write happens.
- ReadFile into an in-memory backups map[string][]byte keyed
by remote path. Original mode captured into a parallel
modes map for restore fidelity.
2. SSHClient interface evolution — three changes:
- StatFile(path) (os.FileInfo, error) — was (int64, error).
FileInfo carries Mode() needed for accurate restore. Existing
fixture tests updated to call info.Size() instead of the
bare size value.
- ReadFile(path) ([]byte, error) — new method; SFTP Open + read
via io.ReadAll. realSSHClient implements via sftpClient.Open.
- Remove(path) error — new method; SFTP Remove. Used by the
rollback path to clean up first-time-deploy partial state.
3. On-reload-failure rollback. Replace the bare error-return at
L282-295 with restoreFromBackups + retry-reload escalation:
- For paths in the snapshot map, WriteFile the original bytes
with the original mode (0600 fallback if mode capture was
incomplete).
- For paths that didn't exist pre-deploy, Remove the new file.
- Re-run the reload command (best-effort second attempt). If
it succeeds, the target is back to pre-deploy state. If it
fails, the remote is in pre-deploy file state but the daemon
may be stuck — surface as wrapped error so the operator
knows where to look.
4. DeploymentResult.Metadata gains backup_status_{cert,key,chain}
so operators can see per-path snapshot state on both success
("snapshotted" / "no_pre_existing" / "n/a") and failure
("restored" / "removed" / "restore_failed" / "remove_failed").
buildMetadataWithBackup helper centralises the metadata
shape so success and failure paths emit a consistent set
of keys.
5. Helper extraction. restoreFromBackups(ctx, paths, backups,
modes) is a private method on Connector; returns the first
error + per-key restore status map for clean test seams.
DeploymentResult shape on failure:
- rollback OK + retry-reload OK → Success=false, "reload command
failed; rolled back to pre-deploy state" (clean recoverable
failure; remote fully restored, daemon serving original cert).
- rollback OK + retry-reload FAIL → wrapped error noting "rolled
back files; retry-reload also failed; daemon may need manual
restart". Metadata flags daemon_state_unknown=true.
- rollback FAIL → operator-actionable wrapped error containing
BOTH the reload error AND the rollback error; metadata flags
manual_action_required=true.
Tests added to ssh_test.go (4 new tests, ~330 LOC):
- TestSSH_ReloadFails_FilesRestored — happy rollback path with
pre-existing remote bytes for cert/key/chain. Asserts every
path's last WriteFile call contains the captured backup bytes
verbatim, no Remove calls fired (all paths had snapshots), and
metadata reports backup_status=restored for each path.
- TestSSH_NoExistingCert_ReloadFails_NewCertRemoved — first-time
deploy variant. StatFile returns os.ErrNotExist for every path;
rollback Removes each written file but performs no WriteFile
during restore (no backup to restore from). Asserts exactly 3
WriteFile calls (deploy only) and 3 Remove calls (rollback).
- TestSSH_ReloadFails_RollbackAlsoFails_OperatorActionable —
uses a writeOrderTrackingMock to fail the SECOND WriteFile to
the cert path (i.e. the restore call, not the initial deploy).
Asserts wrapped error contains both the reload error and the
rollback error, and metadata flags manual_action_required=true.
- TestSSH_ReloadFails_RestoreThenSecondReloadFails — partial-
recovery escalation. Rollback succeeds but the post-restore
retry-reload fails. Asserts wrapped error mentions "rolled back
files; retry-reload also failed" and metadata flags
daemon_state_unknown=true.
Existing tests preserved by extending mockSSHClient with backward-
compatible per-path response maps (statByPath / readByPath /
writeFileErrByPath / executeErrSequence). Legacy global fields
(statFileSize / statFileErr / writeFileErr / executeErr) still
work when no per-path override matches, so TestValidateConfig_*
and TestDeployCertificate_Success_* don't need changes.
docs/deployment-atomicity.md L92 unchanged from today's text —
Bundle 1 doc-realignment hasn't shipped, so the "Pre-deploy SCP
backup of remote files" line was never softened. Post-Bundle-6
the claim is honest (was aspirational pre-fix).
Verified locally (sandbox lacks staticcheck install due to disk
pressure; CI runs the full lint gate):
- gofmt -l ./internal/connector/target/ssh/ clean
- go vet ./internal/connector/target/ssh/ clean
- go build ./internal/connector/target/ssh/... clean
- go build ./cmd/agent/... clean
- go test -race -count=1 ./internal/connector/target/ssh/ green
Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 6.
|
||
|
shankar0123
|
30daadbe81 |
iis: pre-deploy binding snapshot + on-failure rollback
Closes Bundle 5 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix,
DeployCertificate at iis.go:235-436 imported the cert via
Import-PfxCertificate (atomic at cert-store level) then ran a
separate PowerShell script for the SNI binding update. If the
binding script failed, the new cert was orphaned in the store AND
the old binding stayed pointed at the old thumbprint.
docs/deployment-atomicity.md L91 promised "explicit pre-deploy
backup + post-rollback re-import"; the code didn't deliver.
This commit:
1. Pre-deploy snapshot. snapshotOldBinding runs Get-WebBinding
before the import; parses the bound SSL thumbprint into a local
`oldThumbprint` variable. Empty = first-time binding (no
rollback target).
2. On-failure rollback script. When the binding-update Execute
returns error, rollbackBinding runs a single PowerShell script
that:
- Remove-Item Cert:\LocalMachine\<store>\<newThumbprint> (delete
the cert we just imported but couldn't bind).
- If oldThumbprint != "", AddSslCertificate('<oldThumbprint>',
...) to re-bind the old cert. Falls through to New-WebBinding
+ AddSslCertificate when the old binding entry is also gone.
3. Post-rollback verification. verifyRollback re-reads
Get-WebBinding; asserts the bound thumbprint matches
oldThumbprint. On mismatch, warn in the DeploymentResult
message — the rollback ran but final state is suspect, operator
inspection required. Skipped when oldThumbprint == "" (no
binding to verify against).
4. Helper extraction. snapshotOldBinding / rollbackBinding /
verifyRollback are private methods on Connector for clean test
seams. Each emits a unique `# CERTCTL_*` PowerShell comment tag
so test mocks can match scripts deterministically — multiple
scripts call Get-WebBinding so substring matching otherwise
collides under Go's randomized map iteration order.
DeploymentResult shape on failure:
- rollback OK → Success=false, Message="binding update failed;
rolled back", clean error.
- rollback FAIL → Success=false, wrapped error containing both
binding error and rollback error; metadata
flags manual_action_required=true and surfaces
rollback_error / binding_error verbatim.
Tests added to iis_test.go:
- TestIIS_BindingUpdateFails_RemovesNewCert_RebindsOld — happy
rollback path. Mock executor queued with snapshot →
OLD_THUMBPRINT:abc123, import OK, binding fails, rollback →
REBOUND_EXISTING. Asserts rollback script contains both
Remove-Item for the new thumbprint AND
AddSslCertificate('abc123', ...).
- TestIIS_BindingUpdateFails_NoOldBinding_RemovesNewCertOnly —
first-time deploy variant. Snapshot returns NO_OLD_BINDING;
rollback removes the new cert but does NOT call
AddSslCertificate; verify script never runs.
- TestIIS_BindingUpdateFails_RollbackAlsoFails_OperatorActionable
— wrapped-error escalation. Asserts the returned error mentions
both `binding update failed` and `rollback also failed`, and
metadata flags manual_action_required=true.
Two existing tests (TestIISConnector_DeployCertificate_Success and
…_SNIEnabled) updated to expect 3 commands (snapshot, import,
binding) and to look for the binding script at commands[2].
docs/deployment-atomicity.md L91 unchanged from today's text — the
"Already explicit pre-deploy backup + post-rollback re-import"
claim is now honest. (Bundle 1 doc-realignment hasn't shipped yet,
so there's no softened-pending claim to restore.)
Verified locally (sandbox lacks staticcheck install due to disk
pressure, ran via go vet + go test -race; CI runs the full lint
gate):
- gofmt -l ./internal/connector/target/iis/ clean
- go vet ./internal/connector/target/iis/... clean
- go build ./internal/connector/target/iis/... clean
- go test -race -count=1 ./internal/connector/target/iis/ green
Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 5.
|
||
|
shankar0123
|
b767f579ef |
traefik: refactor to single deploy.Apply Plan (all-files atomicity + rollback)
Closes Bundle 4 of the 2026-05-02 deployment-target coverage audit
(see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Pre-fix,
DeployCertificate called deploy.AtomicWriteFile twice — once for
cert at L123, once for key at L131 — instead of bundling both into
a single deploy.Plan and calling deploy.Apply. Three downstream
hazards:
1. If cert write succeeds and key write fails, the cert is already
on disk. The in-line best-effort cert rollback at L137-141 had
no error wrapping and the dedicated rollbackCertAndKey helper
only restored the cert.
2. Idempotency was per-file, not all-files. The verify gate
(if !certRes.Idempotent) skipped verify when cert was unchanged
but key was new — exactly the shape that produces a fresh key on
disk + a stale fingerprint served, and zero alarm.
3. Verify-failure rollback only handled the cert. Key was left in
whatever state the deploy reached.
This commit aligns Traefik with the canonical NGINX/Apache/HAProxy/
Postfix template:
- buildPlan() constructs deploy.Plan{Files: []{cert, key}}.
- deploy.Apply runs it all-or-nothing. SHA-256 idempotency is
all-files (Result.SkippedAsIdempotent).
- No PreCommit (Traefik has no validate-with-target command —
file watcher absorbs config errors).
- No PostCommit (file watcher auto-reloads on rename).
- runPostDeployVerify retained as-is (TLS handshake + SHA-256
fingerprint compare + retry/backoff).
- On verify failure, restoreFromBackups iterates
res.BackupPaths and rewrites each destination via
AtomicWriteFile{SkipIdempotent: true, BackupRetention: -1}.
Removed:
- The legacy rollbackCertAndKey helper (cert-only restore).
- The inline best-effort cert-rollback in DeployCertificate.
Tests added to traefik_atomic_test.go:
- TestTraefik_Atomic_KeyWriteFails_CertRollsBack — regression guard
for the original two-AtomicWriteFile bug. Pre-writes a sentinel
cert; sets the key path inside a read-only subdir so the key
write must fail; asserts the cert on disk still contains the
sentinel bytes (Apply's all-or-nothing rollback).
- TestTraefik_Atomic_AllFilesIdempotent — two subtests:
both_match_skips: pre-writes cert + key matching what Traefik
would write; asserts idempotent=true AND probe is never
called.
cert_match_key_new_runs_verify: pre-writes only the cert; key
is new; asserts idempotent=false AND probe IS called once.
Pre-fix per-file gate would have leaked through and skipped
the verify here.
- TestTraefik_Atomic_VerifyMismatch_BothFilesRollBack — pre-writes
sentinel cert + key; stub probe returns wrong fingerprint;
asserts BOTH files are restored to sentinel bytes after the
rollback fires. Pre-fix rollbackCertAndKey only restored the
cert; the key would still be the new bytes.
The pre-existing TestTraefik_Atomic_VerifyMismatch_Rollback (which
asserted only the cert restore) is left intact — it's a strict
subset of the new BothFilesRollBack assertion and serves as a
narrower regression guard.
docs/deployment-atomicity.md L84 unchanged — operator-facing claim
("atomic-write only; ValidateOnly returns sentinel") stays accurate.
Verified locally:
- gofmt -l ./internal/connector/target/traefik/ clean
- go vet ./... clean
- staticcheck ./internal/connector/target/traefik/... clean
- go build ./... clean
- go test -race -count=1 ./internal/connector/target/traefik/...
green (pre-existing tests + 3 new = 13 test functions; 14 with
the AllFilesIdempotent subtests)
- go test -short -count=1 ./internal/connector/target/... green
(no cross-connector regressions)
Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md
Bundle 4.
|
||
|
shankar0123
|
febf50090b |
envoy: atomic SDS JSON write + post-deploy watcher pickup poll
Closes Bundle 3 of the 2026-05-02 deployment-target coverage audit (see cowork/deployment-target-audit-2026-05-02/RESULTS.md). The audit ranked this fix #3 by acquirer impact behind the K8s real client (#1) and the docs realignment (#2 / Bundle 1). Two production-grade gaps closed: 1. SDS JSON config write was non-atomic. Cert/key/chain at envoy.go L155/L168/L183 went through deploy.AtomicWriteFile (atomic + backups + ownership preservation), but the SDS JSON at L260 went through os.WriteFile directly. A power loss / OOM / process-kill mid-write of the SDS JSON produces a torn file Envoy cannot parse, and Envoy's file-based SDS watcher refuses to load any cert (not just the rotating one) until the JSON is repaired by hand. Replaced with deploy.AtomicWriteFile and threaded ctx through writeSDSConfig. 2. No watcher pickup confirmation before returning success. Pre-fix, DeployCertificate returned the moment file writes completed. Envoy's SDS watcher is asynchronous; a caller running post-deploy TLS verify immediately after DeployCertificate could see Envoy still serving the old cert (watcher latency, load-balanced replica hit one that hadn't reloaded yet). Added the canonical post-deploy verify pattern (mirrors nginx.go::runPostDeployVerify L416): probe seam + retry/backoff + SHA-256 fingerprint compare against request.CertPEM. On verify failure, restore from per-file backups via the new restoreFromBackups helper. Envoy has no PostCommit reload to re-run; the watcher auto-reloads on the restored files. Config additions to envoy.Config (mirror nginx.Config L84-93): - PostDeployVerify *PostDeployVerifyConfig (Enabled, Endpoint, Timeout) - PostDeployVerifyAttempts int (default 3 in runPostDeployVerify) - PostDeployVerifyBackoff time.Duration (default 2s) - BackupRetention int (mirrors nginx; passed to AtomicWriteFile per file) Default behaviour unchanged for callers that don't set PostDeployVerify — verify is opt-in. nil or Enabled=false skips it entirely. Probe seam: c.probe = tlsprobe.ProbeTLS at construction; tests inject via the new SetTestProbe method. Same shape NGINX uses (nginx.go:130); also mirrors the existing Traefik SetTestProbe at traefik.go:62. WriteResult retention: every AtomicWriteFile call now retains its *deploy.WriteResult in a local []*deploy.WriteResult slice so the rollback path can restore from BackupPath across all four files (cert, key, chain, SDS JSON), not just the cert. Pre-fix the cert's WriteResult was discarded. restoreFromBackups (envoy.go new): iterates the WriteResults from a successful per-file pass, rewrites each non-idempotent destination from its BackupPath via AtomicWriteFile{SkipIdempotent:true, BackupRetention:-1}. The -1 prevents backup-of-the-backup pollution. For files that didn't exist pre-deploy (BackupPath == ""), restore = remove. Mirrors nginx.go::rollbackToBackups (L487-515) with the reload step elided. Idempotency gate: shouldRunVerify returns true unless EVERY WriteResult was Idempotent — same all-files semantics NGINX gets from res.SkippedAsIdempotent. Pre-fix Envoy had no verify at all, so there was no gate to get wrong; this introduces the correct all-files shape from the start. Tests added to envoy_atomic_test.go: - TestEnvoy_Atomic_SDSConfigWriteIsAtomic — pre-writes a sentinel SDS JSON, runs DeployCertificate, asserts a backup file with deploy.BackupSuffix appears alongside the new sds.json (proves AtomicWriteFile is now in the SDS path). - TestEnvoy_Atomic_WatcherPickupRetries — stub probe returns wrong fingerprint on attempts 1+2 and correct on attempt 3; deploy succeeds; probe called exactly 3 times. - TestEnvoy_Atomic_WatcherPickupAllAttemptsFail_RollsBack — pre-writes SENTINEL bytes for cert+key, stub probe always wrong; deploy returns wrapped error AND the destination files contain the sentinel bytes (rollback restored). - TestEnvoy_Atomic_PostDeployVerifyDisabledByDefault — Config with nil PostDeployVerify; asserts probe is never called (opt-in default preserved). A small certPEMFingerprint helper added to the test file mirrors the production envoy.certPEMToFingerprint (which is package-private — external tests can't call it). docs/deployment-atomicity.md L87 row already documents "TLS handshake | atomic-write replaces os.WriteFile" — pre-fix the claim was aspirational (verify happened in the agent verify-and-report path, not the connector; SDS JSON wasn't atomic). Post-fix the claim is honest. No doc change required. Verified locally: - gofmt -l ./internal/connector/target/envoy/ clean - go vet ./internal/connector/target/envoy/... clean - staticcheck ./internal/connector/target/envoy/... clean - go build ./... clean - go test -race -count=1 ./internal/connector/target/envoy/... green (5 pre-existing tests + 4 new = 9 total) - go test -short -count=1 ./internal/connector/target/... green Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md Bundle 3. |
||
|
shankar0123
|
475421457f |
fix(test): TestBoundedFanOut_SkipsAgentRoutedDeployments race on seenIDs slice
CI race detector flagged TestBoundedFanOut_SkipsAgentRoutedDeployments
on commit
|
||
|
shankar0123
|
a22a1be962 |
globalsign,entrust: cache mTLS keypair with mtime-based reload
Closes the #10 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. Pre-fix, GlobalSign reloaded the mTLS cert/key from disk on every API call (globalsign.go::getHTTPClient) and Entrust loaded once in ValidateConfig with no rotation handling — both shapes were broken for different reasons. Per-call disk reads under a 100- cert renewal sweep meant 200 file opens / parses / tls.X509KeyPair calls in flight, each adding 5–50ms of latency for nothing; the single-load Entrust shape served stale credentials forever after a cert rotation, requiring a process restart. This commit: - Adds a new shared package internal/connector/issuer/mtlscache/ with a Cache type holding a parsed tls.Certificate plus a precomputed *http.Transport. RWMutex serialises reloads; reads are lock-free in the hot path (read lock briefly held to copy out the *http.Client pointer, then released — the HTTP request itself happens with no lock held, per the audit prompt's anti- pattern about holding the write lock across an API call). - RefreshIfStale stats the cert file; if mtime advanced beyond the last load, the keypair is re-parsed and the transport is rebuilt. The fast path (mtime unchanged) takes the read lock for the comparison and returns immediately. Double-checked-lock pattern (read lock → stat → release → write lock → re-stat) prevents two callers who observed the same stale mtime from both reloading. - Options.TLSConfigBuilder lets the caller customise the *tls.Config built around the parsed leaf certificate. GlobalSign uses this to inject the ServerCAPath-pinning RootCAs pool that buildServerTLSConfig already produces; entrust uses the default builder. - New() performs the initial load so a broken cert path fails fast at construction rather than at first API call. - GlobalSign.Connector gains an mtls field. getHTTPClient now: (1) preserves the test-mode short-circuit when httpClient has a non-nil Transport; (2) preserves the bare-default-client short-circuit when cert paths aren't configured; (3) lazy-builds the cache on the first call so the constructor stays cheap; (4) calls RefreshIfStale on every subsequent call. The error wrap preserves the substring "client certificate" so existing TestGlobalsign_GetHTTPClient_MTLSPathConfigured_LoadsKeyPair keeps its assertion. - Entrust.Connector gains an mtls field plus a new getHTTPClient helper mirroring GlobalSign's shape. The three IssueCertificate / RevokeCertificate / pollEnrollmentOnce sites that previously hit c.httpClient.Do(req) directly now route through getHTTPClient, which falls through to the test-injected client (same logic as GlobalSign) and otherwise serves the cached mTLS client. The legacy ValidateConfig flow that pre-built c.httpClient with its own transport stays intact — its transport wins because getHTTPClient short-circuits when c.httpClient.Transport != nil. - Tests at internal/connector/issuer/mtlscache/cache_test.go cover: * fail-fast on missing paths (constructor input validation) * load on construction (positive + negative) * NoReloadWhenMtimeStable — 100 RefreshIfStale calls, LoadedAt must stay equal to the constructor's stamp (the load-bearing regression guard against per-call disk reads) * ReloadsOnMtimeAdvance — os.Chtimes forward, next refresh must observe the new LoadedAt (the load-bearing regression guard for rotation-without-process-restart) * StatErrorBubbles — missing cert file surfaces as an error rather than silently serving stale credentials * ConcurrentNoRace — 100 goroutines × 50 iterations under -race; no race detected, all calls succeed * TLSConfigBuilderUsed — custom builder is invoked at New AND on reload; verifies MinVersion=TLS1.3 takes effect * ClientHonoursTimeout — Options.HTTPTimeout reaches the constructed *http.Client - docs/connectors.md GlobalSign + Entrust sections each gain an "mTLS keypair caching (audit fix #10)" paragraph documenting the steady-state caching, mtime-based rotation contract, and operator workflow (mv -f new.crt /etc/certctl/.../client.crt). Acquirer impact: removes the per-call disk-read latency floor and makes operator-driven cert rotation a no-restart event. Combined with audit fix #9's bounded scheduler concurrency, the renewal sweep's hot path now has predictable steady-state cost: capN concurrent goroutines, each reusing the cached keypair, no per- call file I/O. Verified locally: - gofmt -l . clean - go vet ./... clean - staticcheck ./... clean - go test -race -count=1 ./internal/connector/issuer/mtlscache/... green (8 tests) - go test -count=1 -short across globalsign / entrust / sectigo / ejbca / mtlscache / connector packages: green Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #10. Closes the audit's full Top-10 list (fixes #1-10 all shipped to master). |
||
|
shankar0123
|
35e18bfc56 |
scheduler: bound renewal concurrency via CERTCTL_RENEWAL_CONCURRENCY
Closes the #9 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. Pre-fix, JobService.ProcessPendingJobs ran every claimed job sequentially in a single goroutine: safe but slow, and operators with large fleets had no lever to dial throughput up. Switching to fire-and-forget per-job goroutines would have unbounded the upstream-CA call rate and tripped DigiCert / Entrust / Sectigo rate limits — certctl's response to 429 was to retry on the next tick, re-fanning out the same calls and digging deeper into the limit. Operators need a knob. This commit: - Adds CERTCTL_RENEWAL_CONCURRENCY env var (default 25) loaded via the existing getEnvInt pattern in internal/config/config.go. Documented inline as the cap for the per-tick renewal/issuance/ deployment goroutine fan-out, with operator-tuning guidance: permissive upstream limits + large fleets (>10k certs) → 100; strict limits or async-CA-heavy fleets → 25 or lower. - Wires golang.org/x/sync/semaphore.Weighted around the per-job goroutine launch in JobService.ProcessPendingJobs. Acquire(ctx, 1) is the load-bearing piece — it BLOCKS the loop when at the cap, providing real backpressure rather than fire-and-forget. The fan-out is split into processPendingJobsSequential (legacy, preserved for unit-test wiring that doesn't call SetRenewalConcurrency) and processPendingJobsConcurrent (production, delegates to a generic boundedFanOut helper). - boundedFanOut takes the per-job work as a closure so the cap can be tested directly without standing up the renewal/deployment service graph. processed/failed counters use atomic.Int64 to avoid mutex overhead on every job completion; final log line reads both AFTER wg.Wait so the counts reflect every dispatched job. ctx-aware Acquire ensures a shutdown ctx cancel interrupts the dispatch loop promptly; in-flight goroutines drain via Wait before the function returns so no goroutine outlives the scheduler tick. - shouldSkipJob extracted as a package-private helper so the agent-routed-deployment skip logic is shared between the sequential and concurrent paths byte-for-byte (the audit prompt's "channel-based semaphore without ctx-aware acquire" anti-pattern is explicitly avoided — semaphore.Weighted.Acquire returns on ctx done; channel <- struct{}{} would block forever). - SetRenewalConcurrency setter on JobService normalises ≤0 to 1. semaphore.NewWeighted(0) constructs a semaphore that blocks every Acquire forever; the normalisation prevents a misconfigured env var from wedging the scheduler. - cmd/server/main.go wires SetRenewalConcurrency(cfg.Scheduler. RenewalConcurrency) on the freshly-built jobService, immediately after SetAuditService. Production deployments always take the bounded path; tests that build JobService directly via NewJobService keep their strict-sequential behaviour because renewalConcurrency is the zero value. - Tests in internal/service/job_concurrency_test.go: * TestBoundedFanOut_CapHolds — primary regression guard. 50 jobs × 50ms work × cap=5 → asserts peak in-flight never exceeds 5 AND reaches 5 at least once (catches both upper-bound regressions and gates that incorrectly cap below the configured value). Lock-free max via CompareAndSwap so the measurement instrument doesn't itself constrain concurrency. * TestBoundedFanOut_AllJobsRun — lower-bound: every non-skipped job is dispatched. * TestBoundedFanOut_SkipsAgentRoutedDeployments — pins the shouldSkipJob contract. * TestBoundedFanOut_CtxCancelInterrupts — ctx cancellation interrupts a stuck fan-out within the timeout budget. * TestBoundedFanOut_FailedJobsCounted — per-job errors don't abort the fan-out. * TestSetRenewalConcurrency_NormalizesNonPositive — ≤0 → 1 fail-safe pinned across negative/zero/positive inputs. - docs/features.md: scheduler-loop table augmented with the concurrency-cap env-var pointer alongside the job-processor row. - docs/architecture.md: Concurrency Safety section gains a paragraph explaining the cap, the operator-tuning guidance, the ctx-aware Acquire semantics, and the audit reference. Operator-facing impact: the first big renewal sweep no longer takes down the upstream CA's rate-limit budget. Existing deployments get the bounded path automatically (default 25); operators can override via env var without code changes. Verified locally: - gofmt -l . clean - go vet ./... clean - staticcheck ./... clean - go test -short -count=1 across service / scheduler / config / integration: green - Six new tests under TestBoundedFanOut* + TestSetRenewalConcurrency*: green Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #9. |
||
|
shankar0123
|
fefa5a5fd7 |
acme: support serial-only revocation via local cert-version lookup
Closes the #7 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. Pre-fix, ACME RevokeCertificate at acme.go:L519-L529 returned the literal error "ACME revocation by serial not supported in V1; provide certificate DER". RFC 8555 §7.6 genuinely requires the cert DER bytes (not just the serial), but a CLM platform's job is to abstract over that limitation. Operators routinely have only the serial in hand: lost PEM, rotated key, GUI revoke action driven by a row in the certs list. This commit: - Adds CertificateLookupRepo interface at the ACME connector boundary (connector boundary, NOT a service/repository import — the connector accepts whatever satisfies the shape). Production wiring in cmd/server/main.go injects the postgres CertificateRepository; tests inject a fake. - Adds CertificateRepository.GetVersionBySerial(ctx, issuerID, serial) + interface declaration in repository/interfaces.go, returning the certificate_versions row whose SerialNumber matches, scoped to the issuer via JOIN on managed_certificates. Mirrors the existing GetByIssuerAndSerial shape but returns the version (where PEMChain lives). Per RFC 5280 §5.2.3 the issuer scope is required for determinism. - Adds SetCertificateLookup + SetIssuerID setters on *acme.Connector. Mirror the pattern local.Connector already uses for OCSP responder wiring. Both must be wired before serial-only revoke works; unwired state falls back to a more actionable error pointing at the wiring requirement (the historical "not supported" wording is retired). - Rewrites RevokeCertificate end-to-end: lookup → empty-PEM check → pem.Decode → block.Type == "CERTIFICATE" check → ensureClient → golang.org/x/crypto/acme.Client.RevokeCert(ctx, accountKey, der, reasonCode). RFC 8555 §7.6 case 1 (revocation request signed with account key) — the same account key issued the cert, so authority is intrinsic. The not-found path returns an actionable operator- facing error pointing at the local-store requirement. - Adds mapRevocationReason translating RFC 5280 §5.3.1 reason strings (unspecified, keyCompromise, cACompromise, affiliationChanged, superseded, cessationOfOperation, certificateHold, removeFromCRL, privilegeWithdrawn, aACompromise) into golang.org/x/crypto/acme. CRLReasonCode. Accepts canonical camelCase + underscore_lower + ALL_CAPS_UNDERSCORE. Nil reason → 0 (unspecified). Unknown reason errors rather than silently demoting (operators rely on the reason for compliance reporting). - Wiring update in service/issuer_registry.go: SetACMECertLookup setter on the registry; Rebuild type-asserts *acme.Connector and calls SetCertificateLookup + SetIssuerID, mirroring the existing *local.Connector branch. cmd/server/main.go calls issuerRegistry.SetACMECertLookup(certificateRepo) immediately after SetIssuanceMetrics — the postgres repo satisfies the interface via GetVersionBySerial. - Tests: * acme_revoke_test.go (new): TestRevokeCertificate_NoCertLookupWired, TestRevokeCertificate_NoIssuerIDWired, TestRevokeCertificate_LookupReturnsNotFound (operator-facing "may not have been issued through certctl" hint pinned), TestRevokeCertificate_LookupArbitraryError, TestRevokeCertificate_VersionPEMEmpty (corrupt-row guard), TestRevokeCertificate_PEMMalformed_NoBlock, TestRevokeCertificate_PEMMalformed_WrongType (PRIVATE KEY block rejected as not a CERTIFICATE). * TestMapRevocationReason_TableDriven: full RFC 5280 reason set plus camelCase / underscore / ALL-CAPS variants plus nil-reason and unknown-reason cases. * acme_failure_test.go: renamed TestRevokeCertificate_AlwaysError → TestRevokeCertificate_UnwiredCertLookupFallback; the test still exercises the same backward-compat branch but now asserts the new "CertificateLookup wiring" error wording. - Mock-repo updates (3 sites): mockCertificateRepository in internal/integration/lifecycle_test.go, mockCertRepo in internal/service/testutil_test.go, mockCertRepoWithGetError in internal/service/shortlived_test.go each gain a GetVersionBySerial implementation that mirrors the GetByIssuerAndSerial logic but returns the version row. - docs/connectors.md ACME section: new "Revocation by serial number" subsection covering the workflow, the local-store requirement (cert was issued through certctl, not imported), the reason-code mapping with the three accepted spelling variants, and a pointer to the audit reference. Out of scope (intentional, per spec): - Recovering the DER from outside the local cert store (CT logs, CSR + signature reconstruction). If the cert wasn't issued through certctl, revoke-by-serial via certctl isn't possible. - Revocation via the cert's private key (RFC 8555 §7.6 case 2). The account-key path covers all certctl-issued certs because the same account key issued them. - Pebble-backed integration test for the happy path. Pebble integration is the right home for that — the unit tests in this commit pin all failure-mode branches before the network call, and the wiring branch in Rebuild is exercised by the existing TestIssuerRegistryRebuild paths. Verified locally: - gofmt -l . clean - go vet ./... clean - staticcheck ./... clean - go test -short -count=1 across connector, service, repository, integration, api/middleware, api/handler: green Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #7. |
||
|
shankar0123
|
2a384c690e |
secret: migrate EJBCA / GlobalSign / Sectigo credentials to *secret.Ref (Phase 2)
Phase 2 of the #6 acquisition-readiness fix from the 2026-05-01 issuer
coverage audit. Phase 1 (commit
|
||
|
shankar0123
|
0509790325 |
asyncpoll: refactor Sectigo / Entrust / GlobalSign to bounded polling (Phase 2)
Phase 2 of the #5 acquisition-readiness fix from the 2026-05-01 issuer
coverage audit. Phase 1 (commit
|
||
|
shankar0123
|
633a10aa4e |
secret: add Ref opaque-credential abstraction (Phase 1)
Phase 1 of the #6 acquisition-readiness fix from the 2026-05-01 issuer coverage audit. Pre-fix, GlobalSign / EJBCA / Sectigo store API keys / OAuth tokens / 3-header credentials as plain Go strings on the Connector struct. Encrypted at rest via internal/crypto/encryption.go (AES-256-GCM v3 + PBKDF2-600k), they sit in process memory in the clear after load and are sent in HTTP headers on every API call. Under DEBUG-level HTTP request logging, the headers leak. This commit ships the foundation type. Per-connector migrations (GlobalSign / EJBCA / Sectigo Config field changes from string to *secret.Ref, plus auth-header write-path changes) are Phase 2 — a separate commit per connector keeps each diff reviewable. Phase 1 (this commit): - internal/secret/secret.go with Ref: NewRef(src func() ([]byte, error)) — production: decrypt-on-demand NewRefFromString(s string) — tests / config-loading Use(fn func(buf []byte) error) — invoke fn with a fresh buffer, zero on return WriteTo(w io.Writer) — convenience for the "set a header" case String() — returns "[redacted]" MarshalJSON() — returns "[redacted]" IsEmpty() — for ValidateConfig paths - The bytes are zeroed (every byte set to 0) after Use returns — defeats casual heap-dump extraction. The `[redacted]` brackets (rather than `<redacted>`) avoid Go's json HTMLEscape behavior. - 9 unit tests covering: bytes-exposed-and-zeroed contract, the buffer-escape anti-pattern (asserts post-Use buffer is zeroed), WriteTo, String/MarshalJSON redaction, JSON-encoding inside a parent struct, nil-Ref safety on every method, source-error propagation, IsEmpty, direct test of the zero helper. Phase 2 (separate follow-up commits): - GlobalSign Config.APIKey / APISecret migration to *secret.Ref. - EJBCA Config.Token migration to *secret.Ref. - Sectigo Config.CustomerURI / Login / Password migration. - Each migration includes the auth-header write-path change (setAuthHeaders → Ref.WriteTo) and the env-var-loading update (NewRefFromString at config load time). - Outbound HTTP transport-wrapping for per-connector credential- header redaction in DEBUG logs (defense against third-party SDK leakage; not in scope for the foundation). Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #6 — Phase 1. |
||
|
shankar0123
|
711265b652 |
asyncpoll: shared bounded-polling Poller + DigiCert refactor (Phase 1)
Phase 1 of the #5 acquisition-readiness fix from the 2026-05-01 issuer coverage audit. Pre-fix, four async-CA connectors (DigiCert, Sectigo, Entrust, GlobalSign) had GetOrderStatus paths that polled the upstream on every scheduler tick with no exponential backoff, no max-retry cap, and no deadline. The scheduler's tick rate (typically 30s) was the only throttle — an unready order got hit every 30s indefinitely, and a 429 from a rate-limited upstream produced "retry on the next tick" which re-fanned-out the same call. This commit ships the shared infrastructure (asyncpoll package) and refactors DigiCert as the reference. Sectigo / Entrust / GlobalSign follow the same mechanical pattern; they land in Phase 2. Phase 1 (this commit): - internal/connector/issuer/asyncpoll/asyncpoll.go: shared Poller with exponential backoff (5s → 15s → 45s → 2m → 5m capped), ±20% jitter, configurable MaxWait deadline (default 10m), and ctx-aware cancellation. - Result enum: StillPending / Done / Failed. PollFunc returns (Result, err); Poll handles the wait loop, deadline check, and ctx propagation. - ErrMaxWait sentinel for callers that want to distinguish "deadline exhausted" from "fn errored". - asyncpoll_test.go: 11 tests covering happy path, transient error keep-polling, Failed terminates immediately, MaxWait timeout, MaxWait+lastErr wrap, ctx cancel, multiplicative backoff, jitter bounds (statistical), pct=0 deterministic, defaults applied. - DigiCert refactor: GetOrderStatus now wraps pollOrderOnce in asyncpoll.Poll. Status-code triage: 2xx + parse + status="issued" → Done with cert 2xx + parse + status="pending" → StillPending 2xx + parse + status="rejected"/"denied" → Done with status="failed" 2xx + parse fail → Failed (permanent) 4xx (not 429) → Failed (404 = order doesn't exist) 429 / 5xx / network → StillPending - Config.PollMaxWaitSeconds (env: CERTCTL_DIGICERT_POLL_MAX_WAIT_SECONDS) exposes the per-call deadline knob; default 600 (10m). - Test helper buildDigicertConnector + GetOrderStatus_Pending test set PollMaxWaitSeconds=1 so async-pending tests don't block 10 minutes on the production default. Phase 2 (separate follow-up commit, not in this PR): - Sectigo refactor (collectNotReady sentinel maps to StillPending). - Entrust refactor (approval-pending → longer per-issuer MaxWait). - GlobalSign refactor (serial-tracking; same Poller). - Per-connector cadence integration tests against fake HTTP servers. - docs/async-polling.md + docs/connectors.md updates. Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #5 — Phase 1. |
||
|
shankar0123
|
74d6b462a4 |
metrics: gofmt issuance_metrics_test.go — fix CI
Trivial whitespace fix: gofmt collapsed three trailing-comment columns that I'd hand-aligned in the test file. Local sandbox missed this because the per-file gofmt run earlier in the commit cycle was scoped to the changed-files list and didn't include the test file at the final write moment; CI's project-wide `gofmt -l .` caught it. Behavior unchanged. |
||
|
shankar0123
|
3b92048242 |
metrics: add per-issuer-type issuance counters, histogram, and failure classifier
Closes the #4 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. Before this commit, certctl's Prometheus exposition had zero per-issuer-type signal — operators answering "is DigiCert slow?" or "is Sectigo failing more than ACME?" had to grep logs by issuer name. This commit adds three series labelled by issuer type: certctl_issuance_total{issuer_type, outcome} certctl_issuance_duration_seconds{issuer_type} (histogram) certctl_issuance_failures_total{issuer_type, error_class} The histogram covers 0.05–120 second buckets to span the local-issuer fast path and async-CA slow path (DigiCert/Sectigo/Entrust polling can take minutes). error_class is a closed enum of eight values (timeout, auth, rate_limited, validation, upstream_5xx, upstream_4xx, network, other) classified once in service.ClassifyError. Cardinality budget is ~276 new series, well within Prometheus's comfortable range. Implementation: - service.IssuanceMetrics is the thread-safe counter + histogram table. Three independent views (counters / failures / durations) exposed via SnapshotCounters / SnapshotFailures / SnapshotDurations. sync.RWMutex protects the map shape; per-key sync/atomic.Uint64 primitives keep the recording hot path lock-free under concurrent service-layer goroutines. - service.IssuanceCounterEntry / IssuanceFailureEntry / IssuanceDurationEntry / IssuanceMetricsSnapshotter live in service (not handler) to avoid an import cycle: handler already imports service for admin_est.go etc., so service can't import handler back. Handler's exposer takes the snapshotter via the service-defined interface. - service.ClassifyError pure function maps error → error_class. context.DeadlineExceeded / context.Canceled → timeout; *net.OpError → network; substring matches against canonical AWS / DigiCert / Sectigo error shapes for auth / rate_limited / validation / upstream_5xx / upstream_4xx / network; unknown → other. Each branch has at least one representative test case in TestClassifyError. - IssuerConnectorAdapter.SetMetrics wires per-adapter recording (issuerType + metrics). Existing 28+ test call sites of NewIssuerConnectorAdapter keep their one-arg signature; production wiring goes through SetMetrics post-construction. - IssuerRegistry.SetIssuanceMetrics + Rebuild type-asserts to *IssuerConnectorAdapter and calls SetMetrics with the issuer type string. nil-guarded — tests that hand-build adapters without metrics get no-op recording. - IssuerConnectorAdapter.IssueCertificate / RenewCertificate wrap the underlying connector call with start := time.Now() and recordIssuance(start, err). Renewal is recorded into the same certctl_issuance_* series as initial issuance — operationally, renewal IS issuance from the connector's perspective (matches the audit prompt's guidance on series naming). - handler/metrics.go GetPrometheusMetrics gains a new exposer block emitting all three series in stable label order with correct Prometheus format (_bucket / _sum / _count for the histogram, +Inf bucket appended). Sorted via sort.Slice for stable output. nil- guarded so deploys without the wire produce clean exposition. - formatLE helper trims trailing zeros from histogram bucket labels via strconv.FormatFloat(le, 'f', -1, 64) so the `le` labels match Prometheus client conventions ("0.05", "30", "120", not "0.0500" etc.). - cmd/server/main.go wires a single IssuanceMetrics instance into both the IssuerRegistry (recording) and the MetricsHandler (exposer) using DefaultIssuanceBucketBoundaries. Tests: - TestIssuanceMetrics_RecordAndSnapshot — happy-path counter + histogram + failure recording, BucketBoundaries returns a copy (not shared storage). - TestIssuanceMetrics_HistogramCumulative — pins the cumulative-buckets contract. 100ms observation lands in 0.1 bucket and every larger bucket; 750ms only in the 1.0 bucket. Off-by-one here would corrupt every quantile query downstream. - TestIssuanceMetrics_Concurrency — 100 goroutines × 1000 ops under the race detector. Asserts atomic counter integrity across contended writes. - TestClassifyError — 17 cases covering every branch of the closed enum plus the nil-error special case. Implementation chooses the existing hand-rolled fmt.Fprintf exposition pattern (no prometheus/client_golang dependency added) to stay consistent with the OCSP / deploy counter blocks already in the file. Out of scope (separate follow-ups): - Revocation metrics (certctl_revocation_*) — symmetric to issuance but the audit didn't ask; explicit follow-up commit. - Discovery / health-check duration histograms. - prometheus/client_golang migration. Verified locally: - gofmt clean - go vet ./... clean - staticcheck ./... clean - golangci-lint run --timeout 5m ./... → 0 issues - go test -short -count=1 ./internal/service/ green - go test -short -count=1 -race -run TestIssuanceMetrics ./internal/service/ green - go test -short -count=1 ./internal/api/handler/ green - go build ./... success Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #4 (Part 3, narrative section). |
||
|
shankar0123
|
b0efdbe2f8 |
repo,service: introduce WithinTx and atomic audit rows for issue/renew/revoke
Closes the #3 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit (Part 1.5 finding #1: audit row not transactional with issuance). AuditRepository.Create previously ran on the package-level *sql.DB while the certificate insert / version insert / revocation insert ran on independent connections — a failed audit INSERT after a successful operation INSERT was silently lost. SOX §404 over IT general controls, PCI-DSS §10 audit logging, HIPAA §164.312(b) audit controls, and CA/B Forum Baseline Requirements §5.4.1 audit log records all presume audit-with-operation atomicity. Design — Option A (Querier abstraction). The chosen pattern: a shared repository.Querier interface (subset of *sql.DB and *sql.Tx) plus a postgres.WithinTx helper that begins a tx, runs fn, commits on nil error, rolls back on error or panic, and returns the wrapped result. Repository methods that participate in a service-layer transaction expose a *WithTx variant taking repository.Querier; the bare methods remain for stand-alone use. A repository.Transactor abstracts the "begin tx, run fn, commit/rollback" lifecycle so service-layer code runs multi-write operations atomically without holding *sql.DB directly. Option B (UnitOfWork) was considered but adds boilerplate without behavioral benefit for the current scope. Option C (context-carried tx) was explicitly rejected — it hides the transactional boundary from the type system, reproducing the class of bug we're fixing. This commit: - Adds internal/repository/querier.go with the Querier interface (compile-time guards that *sql.DB and *sql.Tx satisfy it) and the Transactor interface for service-layer use. - Adds internal/repository/postgres/tx.go with the WithinTx helper (begin/fn/commit/rollback with panic recovery) and a transactor type that satisfies repository.Transactor. - Adds CreateWithTx variants on AuditRepository, CertificateRepository (Create + Update + CreateVersion), and RevocationRepository. Existing bare methods now delegate to the *WithTx variant using the package-level *sql.DB so existing call sites are behavior-preserving. - Updates repository/interfaces.go: AuditRepository, CertificateRepository, and RevocationRepository declare the new *WithTx methods. Adds an atomicity contract doc-comment on AuditRepository pointing at WithinTx + the audit blocker. - Adds AuditService.RecordEventWithTx, mirroring RecordEvent but routing through CreateWithTx so the audit row is part of the caller's transaction. Same redaction + marshalling contract. - Refactors three audit-emitting service paths to use Transactor.WithinTx when SetTransactor was wired, with a legacy fallback for backward compat: * CertificateService.Create — cert insert + audit row in one tx. * RevocationSvc.RevokeCertificateWithActor — cert status update + revocation row + audit row in one tx. The OCSP cache invalidate remains best-effort (out of scope per the prompt). * RenewalService CompleteServerRenewal — cert version insert + cert update + audit row in one tx. Job status update stays outside the audit-atomicity scope (job state lives outside the operator-facing audit trail). - Adds SetTransactor on CertificateService, RevocationSvc, and RenewalService. cmd/server/main.go wires a single Transactor instance shared across all three so all audit-emitting paths run their writes in transactions backed by the same *sql.DB handle. - Updates 5 mock implementations to satisfy the new interface methods: mockCertRepo (testutil_test.go), mockCertRepoWithGetError (shortlived_test.go), fakeRevocationRepo (crl_cache_test.go), intuneE2EAuditRepo (scep_intune_e2e_test.go), and the integration- test mocks (lifecycle_test.go: mockCertificateRepository, mockAuditRepository, mockRevocationRepository). All *WithTx mocks ignore the Querier and delegate to the bare method (mocks have no DB; in-memory state is shared regardless of "tx"). - Adds a service-layer test mockTransactor with BeginTxErr and CommitErr knobs so the atomic-audit tests can assert error propagation through the transactional boundary. - Adds internal/repository/postgres/tx_test.go: unit-level test that WithinTx surfaces "begin tx" wrap when BeginTx fails, and that Transactor.WithinTx delegates correctly. Real-Postgres rollback semantics are covered by the testcontainers tests in the postgres package — sandbox disk pressure prevented adding a sqlmock dep for the in-fn / commit-failure unit test, so those scenarios are exercised through atomic_audit_test.go using the mockTransactor's CommitErr / BeginTxErr fields. - Adds internal/service/atomic_audit_test.go: * TestCertificateService_Create_AtomicWithTx — asserts audit insert failure inside the tx surfaces as the operation's error (closes the blocker contract). * TestCertificateService_Create_LegacyPathLogs — pins the backward-compat behavior when SetTransactor isn't wired: audit failure is logged-not-failed, matching pre-fix. * TestCertificateService_Create_TransactorBeginFailure — BeginTx error path: operation fails, no cert insert, no audit insert. * TestCertificateService_Create_TransactorCommitFailure — Commit error after successful in-fn writes surfaces as the operation's error. Real Postgres can fail Commit on serialization conflicts; the service must report this. Out of scope (separate follow-up commits, same shape): - Issuer CRUD audit atomicity. - Target CRUD audit atomicity. - Agent retire (already transactional via RetireAgentWithCascade; verified, not changed). - Renewal-policy CRUD audit atomicity. - Owner/team/agent-group CRUD audit atomicity. - Discovery / health-check audit atomicity. Verified locally: - gofmt -l . clean - go vet ./... clean - staticcheck ./... clean - golangci-lint run --timeout 5m ./... → 0 issues - go test -short -count=1 ./internal/service/ green - go test -short -count=1 ./internal/api/handler/ green - go test -short -count=1 ./internal/integration/ green - go test -short -count=1 ./internal/repository/postgres/ green - go build ./... success Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #3 (Part 3, narrative section). |
||
|
shankar0123
|
3669556e57 |
ejbca: wire mTLS client cert in New()
Closes the #2 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. New() at ejbca.go:L79-L88 previously constructed an http.Client with only Timeout set — no Transport, no TLSClientConfig. When AuthMode=mtls (the default), the client never presented the configured ClientCert/ClientKey. The OAuth2 path worked; mTLS always failed authentication. Tests passed because they injected a pre-built *http.Client via NewWithHTTPClient, a path the production factory never took. This commit: - Rewrites New() to load ClientCertPath + ClientKeyPath via tls.LoadX509KeyPair when AuthMode=mtls, configure *http.Transport.TLSClientConfig with MinVersion: TLS 1.2 (compatibility floor for on-prem EJBCA installs that may predate TLS 1.3), and return (*Connector, error). Constructs a fresh *http.Transport — does NOT clone http.DefaultTransport, which would leak mutation across the package boundary. - OAuth2 mode unchanged: returns a client with no transport customization (the Bearer header path is wired in setAuthHeaders). - Invalid auth_mode values return (nil, error) immediately rather than falling through to the mtls default and erroring at cert load. - Updates the factory call site at issuerfactory/factory.go for the new signature; the factory's outer (issuer.Connector, error) shape was already in place. - Adds TestNew_MTLSWiresClientCert: calls production New() (NOT NewWithHTTPClient) with real cert/key files generated via stdlib crypto/x509, asserts httpClient.Transport.TLSClientConfig.Certificates is non-empty. Includes an httptest TLS server with ClientAuth: tls.RequireAndVerifyClientCert that proves the cert is actually presented on the wire — not just stashed in a struct field. - Adds TestNew_MTLSCertLoadFailure: missing-cert path returns an error wrapping fs.ErrNotExist (verified via errors.Is). - Adds TestNew_OAuth2NoTransportTuning: OAuth2 path leaves Transport nil, ensuring no accidental mTLS bleedthrough. - Adds TestNew_InvalidAuthMode: explicit guard that auth_mode values other than "mtls"/"oauth2" return (nil, error) at New() time. - Adds export_test.go with HTTPClientForTest helper so the external ejbca_test package can inspect the connector's internal *http.Client for the wiring assertions. Compile-only during `go test`; production builds don't expose it. - Adds mustNewForValidateConfig test helper (OAuth2 placeholder connector) for the existing ValidateConfig-only tests; pre-fix they used New(nil, ...) which is no longer valid because nil config falls into the mTLS default branch that requires non-nil cert paths. - Updates ejbca_stubs_test.go (internal package) for the new (*Connector, error) signature; switches the dummy connector to OAuth2 mode so Config{} doesn't error at New(). Out of scope (separate follow-ups, per the prompt's explicit fence): - OAuth2 token refresh missing - Config.Token plaintext at runtime (needs SecretRef abstraction) - RevokeCertificate composite OrderID parsing (the issuerDN := "" line at ejbca.go:L313) Verified locally: - gofmt clean - go vet ./... clean - staticcheck ./... clean - golangci-lint run --timeout 5m ./... → 0 issues - go test -short -count=1 ./internal/connector/issuer/ejbca/ green - go test -short -count=1 ./internal/connector/issuerfactory/ green - go test -short -count=1 ./internal/service/ green - go build ./... success Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #2. |
||
|
shankar0123
|
804a1b05ce |
awsacmpca: thread ctx through factory + registry — fix CI contextcheck
Follow-up to |
||
|
shankar0123
|
590f654b0d |
awsacmpca: replace stub client with AWS SDK v2 implementation
Closes the #1 acquisition-readiness blocker from the 2026-05-01 issuer coverage audit. The production New() constructor previously hardcoded &stubClient{}, which returned "AWS SDK client not initialized (stub)" on every method. Tests passed green via NewWithClient mock injection — a path the production constructor never took. AWSACMPCA was wired into the factory, the seed file, the test suite, and marketing collateral but did not actually issue, retrieve, or revoke certificates. This commit: - Adds aws-sdk-go-v2/{config,service/acmpca,aws} to go.mod (with acmpca/types as a sub-package). go mod tidy could not be completed in the sandbox due to virtiofs concurrent-open-file ceiling on the module cache; the require blocks were arranged manually so the three directly-imported packages are non-indirect. Build, vet, staticcheck, and the full test suite are green; operator should run `go mod tidy` on the workstation to confirm cosmetic ordering before pushing. - Implements sdkClient wrapping *acmpca.Client with local input/output type translation. Each method translates the connector's local input type to the SDK's typed input, calls the SDK, and translates the SDK output back to the local output type. aws-sdk-go-v2 types do not leak out of the awsacmpca package. - Deletes stubClient (the four "AWS SDK client not initialized (stub)" methods). After this commit, there is no fall-back stub; production New() always wires the SDK. - Rewrites New() to load credentials via awsconfig.LoadDefaultConfig with awsconfig.WithRegion(config.Region) and construct the SDK client via acmpca.NewFromConfig. Returns (*Connector, error). When config is nil or config.Region is empty, New defers SDK loading; ValidateConfig builds the client lazily on the first successful validation. This preserves the test pattern of New(nil, logger) → ValidateConfig. - Wires acmpca.NewCertificateIssuedWaiter (5-minute default timeout) inside sdkClient.IssueCertificate so the connector's two-call pattern (IssueCertificate → GetCertificate) sees synchronous-via- waiter semantics. The waiter is hidden from the ACMPCAClient interface so mock implementations stay simple. - Maps RFC 5280 revocation reasons to acmpcatypes.RevocationReason via the existing mapRevocationReason helper plus a cast at the sdkClient.RevokeCertificate boundary. - Updates the issuerfactory.NewFromConfig call site at factory.go:L88 for the new (*Connector, error) signature; the factory's outer signature already returns (issuer.Connector, error) so the change is local. - Adds nil-client guards on the four client-using connector methods (IssueCertificate, RevokeCertificate, GetCACertPEM, plus the RenewCertificate path via IssueCertificate). When the connector is used before ValidateConfig has been called, these methods fail-fast with a "client not initialized" sentinel error instead of panicking. - Fixes the copy-paste env-var doc-comments at awsacmpca.go:L41,L45 (CERTCTL_GOOGLE_CAS_PROJECT / CERTCTL_GOOGLE_CAS_CA_ARN → CERTCTL_AWS_PCA_REGION / CERTCTL_AWS_PCA_CA_ARN). The actual config loader at internal/config/config.go:L1556-L1561 already used the correct env-var names; only the doc-comments were wrong. - Updates the package doc-comment at awsacmpca.go:L1-L36 to clarify the synchronous-via-waiter behavior (issuance is asynchronous at the API level; the waiter inside sdkClient.IssueCertificate hides the asynchrony). - Adds TestNew_ProductionPath/ValidConfigBuildsRealClient: calls production New() (NOT NewWithClient) with a valid config, asserts err is nil, then calls IssueCertificate with a bogus CSR and asserts the resulting error is the expected PEM-decode error rather than the deleted stubClient's "client not initialized" sentinel. This is the regression-marker test the audit's D11 blocker called out as missing — if anyone re-introduces a stub-style placeholder from production New() in the future, this test fails. - Adds TestNew_ProductionPath/NilConfigDefersClientInit: documents the lazy-init contract for the New(nil, logger) → ValidateConfig pattern. - Adds TestNew_ProductionPath/ValidateConfigBuildsClientLazily: verifies that ValidateConfig wires the SDK client when New was called with nil config. - Adds TestNew_ProductionPath/{Revoke,GetCAPEM}BeforeInitFailsFast: verifies the nil-client guards on the other client-using methods. - Adds TestNew_ErrorPaths covering AccessDeniedException-shaped errors, transient 5xx errors, and ctx-cancel propagation via the existing mockACMPCAClient. - Updates docs/connectors.md:L490-L555 with: the synchronous-via-waiter behavior, a complete IAM policy example scoped to the four ACM PCA actions, a worked POST /api/v1/issuers example, and a troubleshooting section with three known failure modes (AccessDeniedException, ResourceNotFoundException, waiter timeout). Live AWS integration testing is intentionally not added: ACM PCA is a Pro-tier feature in localstack and the existing interface-mock tests cover correctness end-to-end. Operators with AWS credentials can validate by following the worked example in docs/connectors.md. Audit reference: cowork/issuer-coverage-audit-2026-05-01/RESULTS.md Top-10 fix #1 (Part 3, narrative section). |
||
|
shankar0123
|
7cb453a336 |
chore(fmt): repo-wide gofmt -w sweep — close drift surfaced by ci-pipeline-cleanup Phase 4
Mechanical reformat. The new 'gofmt drift' CI step (added in
ci-pipeline-cleanup Phase 4, commit
|
||
|
shankar0123
|
77abb7096c |
fix(config): wire CERTCTL_DEPLOY_BACKUP_RETENTION + CERTCTL_K8S_DEPLOY_KUBELET_SYNC_TIMEOUT to satisfy G-3 docs-drift guard
CI failed on the G-3 docs-drift guard for the deploy-hardening I
release commit (88e8a417 /
|
||
|
shankar0123
|
8637131f80 |
chore: gofmt fixes across deploy-hardening I new files
Phase 13 verification surfaced gofmt-formatting drift in 6 files across the bundle's new code: - internal/api/handler/metrics.go (struct field alignment) - internal/connector/target/k8ssecret/validate_only_test.go (alignment) - internal/connector/target/nginx/nginx.go (alignment) - internal/connector/target/postfix/postfix.go (alignment) - internal/connector/target/ssh/validate_only_test.go (alignment) - internal/service/deploy_counters.go (alignment) Pure mechanical gofmt -w fixes; no behavior changes. CI's make verify gate (which runs `go fmt ./...`) didn't catch these because go fmt is more lenient than gofmt -l, but golangci-lint v2.11.4 + the explicit gofmt step in Phase 13 verification did. Phase 13 full-matrix verification all green: - gofmt -l: empty across all bundle-touched files - go vet ./internal/deploy/... ./internal/connector/target/... ./internal/service/ ./internal/api/handler/ ./cmd/agent/: clean - golangci-lint v2.11.4 (the version CI runs): 0 issues - go test -race -count=1 across deploy + nginx + apache + haproxy + agent + service: all green - INTEGRATION=1 go test -tags integration -run Deploy ./deploy/test/...: 4/4 e2e tests green Phase 14 next: release prep — Active Focus update, release notes, Reddit-beat draft, final tag handoff to operator. |
||
|
shankar0123
|
135b271197 |
feat(metrics): per-target-type deploy counters wired into /metrics/prometheus
Phase 10 of the deploy-hardening I master bundle. Mirrors the
production-hardening-II Phase 8 OCSP-counter pattern. Per frozen
decision 0.9, the metric naming convention is
`certctl_deploy_<area>_total` with target_type + sub-label.
internal/service/deploy_counters.go:
- DeployCounters struct with sync.Map of per-target-type buckets
(apache, nginx, etc.). Lock-free fast path via sync/atomic
Uint64 counters; LoadOrStore on first tick.
- 8 sub-counters per target-type bucket:
- attemptsSuccess / attemptsFailure
- validateFailures (PreCommit returned error)
- reloadFailures (PostCommit returned error → rollback ran)
- postVerifyFails (post-deploy TLS handshake failed)
- rollbackRestored (rollback succeeded)
- rollbackAlsoFail (operator-actionable escalation)
- idempotentSkips (SHA-256 match → no-op deploy)
- Snapshot returns []DeploySnapshot for the Prometheus exposer.
internal/service/deploy_counters_test.go:
- 5 tests: zero-state, per-target-type tick isolation, race-detector
smoke under concurrent ticks, cross-target bucket isolation,
snapshot-mutation-doesn't-affect-counter.
internal/api/handler/metrics.go:
- New DeployCounterSnapshotter interface (mirrors CounterSnapshotter
for the OCSP counters but uses the per-target-type tuple shape).
- New DeploySnapshotEntry struct copying the service-layer shape;
avoids importing the service package directly so the handler
stays dependency-light.
- New SetDeployCounters setter on MetricsHandler (mirrors
SetOCSPCounters wiring).
- Prometheus exposer extended with 6 new metric blocks per frozen
decision 0.9:
- certctl_deploy_attempts_total{target_type, result}
- certctl_deploy_validate_failures_total{target_type}
- certctl_deploy_reload_failures_total{target_type}
- certctl_deploy_post_verify_failures_total{target_type}
- certctl_deploy_rollback_total{target_type, outcome}
- certctl_deploy_idempotent_skip_total{target_type}
- Output sorted by target_type for stable diffs across requests.
The agent-side wire-up (cmd/agent/main.go ticking counters in the
DeployCertificate dispatch site) is intentionally deferred to a
follow-up commit — Phase 10's load-bearing change is the
infrastructure; per-connector tick wiring is a mechanical follow-on.
Build + go vet clean. go test -count=1 green for service +
handler packages.
Phase 11 next: cross-cutting integration tests at deploy/test/.
|
||
|
shankar0123
|
9f41b58b2f |
feat(ssh,wincertstore,javakeystore,k8ssecret): explicit ValidateOnly + leverage existing connectors
Phase 9 of the deploy-hardening I master bundle. The four non-file-server connectors get real ValidateOnly probes that operators use to preview a deploy without touching the live cert. Existing DeployCertificate paths already have explicit backup + rollback semantics (SCP backup / WinCertStore Get-ChildItem snapshot / keytool snapshot / K8s atomic API). SSH (validate_only.go): - Probes via SSHClient.Connect. Confirms agent reachability + credentials. Cheap (no remote command runs); released cleanly via defer Close. - A true SCP dry-run requires a no-commit upload (SCP doesn't have one). V2 ships the auth probe as the load-bearing check. - 3 new tests in validate_only_test.go. WinCertStore (validate_only.go): - Probes via PowerShell `Get-ChildItem -Path Cert:\<loc>\<store>` using the configured StoreLocation + StoreName (defaults LocalMachine\My). - Confirms agent has Windows + the IIS module + the right ACLs. - 4 new tests including default-store-path verification. JavaKeystore (validate_only.go): - Probes via `keytool -list -keystore <path> -storepass <pass>` using the configured KeystorePath / KeystorePassword and KeytoolPath (default "keytool"). - Confirms keystore exists, password is correct, JRE is on PATH. - 4 new tests covering succeeds / fails / no-path-sentinel / nil-executor-sentinel. K8s Secret (validate_only.go): - Probes via K8sClient.GetSecret on the configured Namespace + SecretName. Returns nil on success or "not found" (the CreateSecret path on Deploy will handle it). Other errors (forbidden/unreachable) surface as wrapped. - 4 new tests covering succeeds / RBAC-error wrapped / no-config-sentinel / nil-client-sentinel. Smoke test connectorsAtPhase3 list shrunk from 7 to 3 entries (ssh + wincertstore + javakeystore + k8ssecret removed). Only caddy (file-mode) + envoy + traefik remain — those three genuinely have no validate-with-target command available. Race detector clean across all 13 connectors. golangci-lint v2.11.4 clean. Phase 10 next: DeployCounters + Prometheus exposer mirroring the production-hardening-II OCSP counter pattern. |
||
|
shankar0123
|
36d79cd1ff |
feat(f5,iis): explicit ValidateOnly + leverage existing transactional rollback
Phase 8 of the deploy-hardening I master bundle. F5 + IIS already have transactional / explicit-backup-restore rollback semantics in their DeployCertificate paths. Phase 8 adds the explicit ValidateOnly dry-run probe that operators use to preview a deploy without touching the live cert. F5 (validate_only.go): - ValidateOnly probes the iControl REST API via Authenticate. Cheap (no F5 transaction created) + cached after first success. Failure surfaces as a wrapped error so operators see the actual cause (auth provider down, invalid creds, BIG-IP unreachable, etc.). nil client returns ErrValidateOnlyNotSupported. - A true cert-bind dry-run requires F5's no-commit transaction mode (v17.5+); V3-Pro can add per-version dispatch. V2 ships the reachability probe as the load-bearing safety check. - 5 new tests in validate_only_test.go covering: auth-success, auth-fail wrapped, nil-client sentinel, error-message contains BIG-IP context, recoverable auth-fail surfaces provider info. IIS (validate_only.go): - ValidateOnly runs `Get-WebSite -Name <SiteName>` via the injected PowerShellExecutor. Confirms the IIS PS module is loaded AND the site exists AND the agent has admin privileges. Failure here surfaces the actual PowerShell stderr (site not found / module missing / access denied). - A true cert-bind dry-run would need IIS to expose a no-commit New-WebBinding (it doesn't); V3-Pro can extend with a temp-install + immediate-remove. V2 ships the permission + module probe as the load-bearing check. - 5 new tests in validate_only_test.go covering: get-website succeeds, get-website fails, nil-executor sentinel, site-name quoting (handles spaces in 'Default Web Site'), output-context in error. Smoke test connectorsAtPhase3 list shrunk from 10 to 7 entries (f5 + iis + postfix removed). Caddy stays in (file-mode returns sentinel; api-mode is real-impl). Envoy + Traefik stay in (no validate-with-target command exists for either). javakeystore + k8ssecret + ssh + wincertstore stay in pending Phase 9. Coverage: F5 holds at ≥85%; IIS holds at ≥85%. Race detector clean. golangci-lint v2.11.4 clean. Phase 9 next: SSH + WinCertStore + JavaKeystore + K8s — the non-file-server connectors. |
||
|
shankar0123
|
a7cce9afdd |
feat(traefik,caddy,envoy,postfix): atomic deploy + post-deploy TLS verify + rollback + ValidateOnly
Phase 7 of the deploy-hardening I master bundle. Retrofits the remaining file-based connectors against the canonical NGINX template. Per-connector quirks codified: - Postfix/Dovecot: full retrofit with PreCommit (postfix check / doveconf -n) + PostCommit (postfix reload / doveadm reload) + post-deploy TLS verify. Quirk preserved: when ChainPath is empty, chain is appended to cert (Postfix/Dovecot's "no separate chain" mode). Per-distro user defaults: postfix, dovecot, _postfix. Default key mode 0600. ValidateOnly real impl returns sentinel when no ValidateCommand. - Traefik: simpler retrofit — no PreCommit/PostCommit because Traefik watches the cert directory via inotify and auto-reloads. Atomic-write via deploy.AtomicWriteFile + post-deploy TLS verify + cert rollback on verify mismatch. Default key mode 0600. ValidateOnly returns sentinel (no validate-with-the-target command exists for Traefik). - Caddy: retrofitted both modes. File mode replaces os.WriteFile with deploy.AtomicWriteFile (preserves the file watcher's auto- reload). API mode unchanged (POST /load already atomic at the Caddy admin server). ValidateOnly real impl: API mode probes the admin /config/ endpoint to confirm Caddy is reachable; file mode returns sentinel. - Envoy: file mode atomic-write via deploy.AtomicWriteFile. Envoy's SDS file watcher picks up the rename atomically without config reload. ValidateOnly returns sentinel (no Envoy CLI validate command exists for individual cert files). Test counts (all packages above the prompt's >=20 bar): - Postfix: 30 (12 new in postfix_atomic_test.go + 18 pre-existing) - Traefik: 22 (12 new in traefik_atomic_test.go + 10 pre-existing) - Caddy: 22 (10 new in caddy_atomic_test.go + 12 pre-existing) - Envoy: 21 (5 new in envoy_atomic_test.go + 16 pre-existing) Coverage: each connector at the prompt's >=80% target. golangci-lint v2.11.4 clean across all 4 connector packages. Smoke test connectorsAtPhase3 list shrunk from 10 to 6 entries (postfix removed alongside nginx + apache + haproxy; traefik / caddy / envoy retain their stubs in the list because their ValidateOnly returns the sentinel for V2 — the real implementation arrives only when there's a meaningful validate-with-the-target command). Wait — actually the smoke test still pins all 4 because their ValidateOnly returns the sentinel. Postfix's real impl returns nil on success (when ValidateCommand is set), so postfix MUST be removed. Caddy's API mode is real-impl. Traefik + Envoy still return sentinel always — they stay in the smoke list. Phase 8 next: F5 + IIS — explicit post-deploy TLS verify + on-failure rollback. Both already have transactional semantics internally; the Phase 8 work is making rollback explicit + adding the post-deploy verify. |
||
|
shankar0123
|
919a92bf1b |
feat(haproxy): atomic deploy + post-deploy TLS verify + rollback + ValidateOnly + test-depth uplift to 36 tests
Phase 6 of the deploy-hardening I master bundle. HAProxy connector follows the canonical Phase 4 NGINX template with the HAProxy- specific quirk: combined PEM file (cert + chain + key in one file, in that order). Test count lifts 3 → 36. HAProxy specifics: - buildCombinedPEM concatenates cert, chain, key in HAProxy's required order. The combined file goes through deploy.Apply as a single File entry (vs NGINX/Apache's 2-3 separate File entries). - Default mode 0600 unconditionally (combined file contains the private key); operators rely on this back-compat behavior. PEMFileMode override is the supported escape hatch. - Validate command is `haproxy -c -f <config>`. Reload via `systemctl reload haproxy` (NOT `restart` — reload uses socket activation to drain in-flight connections). - Default user/group: haproxy (cross-distro consistent). DeployCertificate refactor: - Replaces the duplicated os.WriteFile flow with deploy.Apply. - PreCommit runs `haproxy -c -f` validation (gated on ValidateCommand being non-empty — HAProxy historically allowed empty validate). - PostCommit runs the operator's ReloadCommand. - Post-deploy TLS verify (frozen-decision-0.3 default ON when Endpoint is configured): probes the configured target, fingerprint-matches against the deployed cert (the leaf cert block from the combined PEM), retries with backoff for load- balanced targets. - Rollback wires identical to NGINX/Apache: backup restore + reload retry on PostCommit failure; verify-fail also triggers rollback. ValidateOnly real impl: returns sentinel when no ValidateCommand; otherwise runs the operator's command without touching the live combined PEM. Tests (36 total: 33 in haproxy_atomic_test.go + 3 pre-existing in haproxy_test.go): - Atomic invariants (happy, validate-fail, reload-fail-rollback, rollback-also-fail-escalation) - Combined PEM order (cert + chain + key — verified via PEM block headers, not base64 bodies) - Mode handling (default 0600 even when existing is 0640 — back-compat; PEMFileMode override; existing-mode unchanged when override matches) - Idempotency (full skip) - Verify (match, mismatch, dial-timeout, retries, disabled, no-endpoint, rollback-runs-reload) - ValidateOnly (happy, fails, no-command-sentinel, stderr-in-error) - Concurrency (same-paths-serialize) - Edge cases (no-chain, no-key, ctx-cancelled, no-validate-command, config-validation rejects missing pem_path / reload / shell-injection) Coverage: HAProxy 88.0% (above >=85% prompt bar). Race detector clean. golangci-lint v2.11.4 clean. Smoke test connectorsAtPhase3 list shrinks 11→10 (haproxy removed alongside nginx + apache). Phase 7 next: Traefik + Caddy + Envoy + Postfix — the remaining file-based connectors get the same treatment. |
||
|
shankar0123
|
12e5f97f59 |
feat(apache): atomic deploy + post-deploy TLS verify + rollback + ValidateOnly + test-depth uplift to 34 tests
Phase 5 of the deploy-hardening I master bundle. Mirrors the Phase 4 NGINX template for Apache httpd. Test count lifts 3 → 34 (above the prompt's >=30 target; matches and slightly exceeds the IIS bar). Apache-specific quirks codified in apache.go: - Validate command convention is `apachectl configtest` (NOT `apachectl -t` — that flag exists but configtest is the documented operator-facing form). - Reload command convention is `apachectl graceful` for zero- downtime worker swap (NOT `apachectl restart` which drops in-flight TLS sessions). - Per-distro user defaults: Debian/Ubuntu apache2, RHEL/CentOS apache, Alpine httpd. pickFirstExistingUser walks the list and picks the one that resolves on the host; falls back to no-chown when none exist (cross-distro portability without operator config; same approach as nginx). - Default key file mode 0600 for back-compat with operators relying on the historical hard-coded value (matches the pre-Phase-5 implementation behavior). DeployCertificate refactor: - Replaces the duplicated os.WriteFile chain with deploy.Apply. - PreCommit runs the operator's ValidateCommand via the test seam (which wraps `sh -c <cmd>` in production). - PostCommit runs ReloadCommand the same way. - Post-deploy TLS verify (frozen-decision-0.3 default ON when Endpoint is configured): probes the configured target, compares leaf cert SHA-256 against deployed bytes, retries with exponential backoff (default 3 attempts / 2s backoff for load-balanced targets). - Rollback wires: reload-fail → restore backups + retry reload; verify-fail → restore backups + reload again. Second-failure surfaces ErrRollbackFailed for operator-actionable triage. ValidateOnly real implementation replaces the Phase 3 stub. Returns ErrValidateOnlyNotSupported when no ValidateCommand configured; otherwise runs the validate-with-the-target command without touching the live cert. Test seams (SetTestRunValidate / SetTestRunReload / SetTestProbe) allow tests to skip exec without `apachectl` on PATH; mirror the nginx pattern. Tests (34 total: 31 in apache_atomic_test.go + 3 pre-existing in apache_test.go): - Atomic invariants (happy, validate-fail-no-files-changed, reload-fail-rollback, rollback-also-fail-escalation) - SHA-256 idempotency (full skip + partial-mismatch full-deploy) - Post-deploy verify (match-success, mismatch-rollback, dial-timeout-rollback, retries-until-match, retries-exhausted-rollback, no-endpoint-skips, disabled-skips) - Ownership / mode preservation (existing-mode, override-wins, default-key-0600, default-cert-0644) - Backup retention (keeps-N, disabled-no-backups, backup-created) - Concurrency (same-paths-serialize) - ValidateOnly (happy, fails, no-command-sentinel, stderr-in-error) - Edge cases (no-chain, no-key, ctx-cancelled, verify-rollback- reload, deployment-id-prefix, metadata-populated) Coverage: Apache 86.6% (above the >=85% prompt bar). Race detector clean. golangci-lint v2.11.4 clean. Smoke test connectorsAtPhase3 list shrunk from 12 to 11 entries (apache removed; nginx + apache now have real impls). Phase 6 next: HAProxy (combined PEM atomic write + `haproxy -c -f` validate + uplift 3 → >=30). |
||
|
shankar0123
|
7444df01e2 |
feat(nginx): atomic deploy + post-deploy TLS verify + rollback + ValidateOnly + ownership preservation
Phase 4 of the deploy-hardening I master bundle. The canonical NGINX implementation that Phases 5-9 model on. Replaces the historical os.WriteFile flow at internal/connector/target/nginx/nginx.go:99 with deploy.Apply() and adds three production-grade competitor-gap features: atomic deploy with rollback, post-deploy TLS verify, file ownership preservation. NGINX connector — internal/connector/target/nginx/nginx.go: - DeployCertificate now wires deploy.Apply with PreCommit running the operator's ValidateCommand (e.g. `nginx -t`), PostCommit running ReloadCommand (e.g. `nginx -s reload`), and an explicit post-deploy TLS verify step that dials the configured endpoint, pulls the leaf cert SHA-256, and compares against what was just deployed. SHA-256 mismatch (wrong vhost / cached cert / NGINX still serving stale) triggers automatic rollback: backup files are restored + reload fired again. Failed-second-reload returns ErrRollbackFailed (operator-actionable; loud audit + alert). - ValidateOnly replaces the Phase 3 stub: runs the operator's ValidateCommand without touching the live cert. V2 contract is syntax-only validation (full pre-deploy temp-config validation is V3-Pro). Returns ErrValidateOnlyNotSupported when no ValidateCommand is configured. - New per-target Config fields: PostDeployVerify (frozen-decision- 0.3 default ON), PostDeployVerifyAttempts (default 3 — defends against load-balanced targets where the verify might hit a different pod that hasn't picked up the new cert yet), PostDeployVerifyBackoff (default 2s exponential), per-file Mode/Owner/Group overrides (KeyFileMode, CertFileMode, KeyFileOwner, etc.), and BackupRetention (default 3, -1 to disable backups entirely — documented foot-gun). - buildPlan honors per-distro nginx user (Debian: www-data, Alpine: nginx, Red Hat: nginx) by checking the local user database; falls back to no-chown when neither exists. Means the connector is portable across distros without operator config. Deploy package — internal/deploy/ownership.go: - applyOwnership now silently swallows chown failures when the agent isn't running as root. Production agents always run as root and chown failures are real bugs; dev / CI runs as a regular user where chown to a different uid will always fail with EPERM (or EINVAL on some tmpfs configs) and would otherwise force every test to run with sudo. Production-grade contract preserved (uid 0 still hard-fails on chown errors). Test suite — internal/connector/target/nginx/nginx_atomic_test.go ships 42 new named tests (NGINX total: 17 pre-existing + 42 new = 59, above the prompt's >=40 bar; matches the IIS depth bar of 41): - Atomic-deploy invariants (cert+chain+key all-or-nothing, validate-fails-no-files-changed, reload-fails-rollback, rollback-also-fails-escalation) - SHA-256 idempotency (full match skips, partial match deploys all) - Post-deploy TLS verify (fingerprint-match-success, SHA256-mismatch-rollback, dial-timeout-rollback, retries-until- match, retries-exhausted-rollback, no-endpoint-skips, disabled-skips-entirely, default-10s-timeout, endpoint-forwarded) - Ownership / mode preservation (existing-mode-preserved, override- wins, KeyFileMode override applied) - Backup retention (keeps-last-N, disabled-creates-no-backups, fresh-deploy-creates-backup) - Concurrency (same-paths-serialize via deploy package's file mutex, different-paths-parallelize) - ValidateOnly (happy-path-nil, command-fails-wrapped-error, no-config-returns-sentinel, ctx-cancelled, stderr-in-message) - Edge cases (no-chain, no-key, no-chain-path, empty-cert-PEM, ctx-cancelled, all-four-one-apply) - Result.Metadata + DeploymentID shape contracts Coverage: NGINX 91.0% (above the >=85% prompt bar). Race detector clean. golangci-lint v2.11.4 clean. Existing 17 tests still all pass (no behavior change in the legacy paths exercised there). Phase 5 next: mirror this implementation for Apache + lift its test count from 3 to >=30. Same template applies through Phases 6-9 for the remaining 11 connectors. |
||
|
shankar0123
|
49f1a60762 |
feat(target): ValidateOnly dry-run method on Connector interface (default returns ErrValidateOnlyNotSupported)
Phase 3 of the deploy-hardening I master bundle. Extends the
target.Connector interface with the dry-run method that operators
will use to preview a deploy before committing — but ships only the
default-stub for all 13 connectors. Phases 4-9 replace each stub
with the real validate-with-the-target implementation.
interface.go:
- Add ErrValidateOnlyNotSupported sentinel (frozen decision 0.6 —
connectors that cannot dry-run, like K8s, return this rather than
nil so operator triage can errors.Is for "not supported" vs
"validated successfully").
- Add ValidateOnly(ctx, request DeploymentRequest) error to
Connector interface.
13 new validate_only.go files (one per connector at
internal/connector/target/<name>/validate_only.go):
- apache, caddy, envoy, f5, haproxy, iis, javakeystore, k8ssecret,
nginx, postfix, ssh, traefik, wincertstore.
- Each file is identical except for the package declaration: a
one-method default stub returning target.ErrValidateOnlyNotSupported.
- Per-connector files (rather than a single embed-method approach)
let Phases 4-9 replace each connector's stub independently
without churning a shared base.
Tests:
- internal/connector/target/validate_only_test.go pins the sentinel
contract (errors.Is identity, Error() string, %w wrap propagation).
- internal/connector/target/validate_only_smoke_test.go (external
test package) constructs a zero-value &<pkg>.Connector{} for each
of the 13 connectors and asserts ValidateOnly returns
ErrValidateOnlyNotSupported. The test's
connectorsAtPhase3 list is the load-bearing CI guard:
- A 14th connector added without wiring ValidateOnly fails the
`len(connectorsAtPhase3) != 13` invariant.
- A connector whose real ValidateOnly lands (Phase 4 NGINX, Phase
5 Apache, etc.) MUST be removed from this list or the smoke test
fails (real impl no longer returns the sentinel). That removal
IS the bookkeeping that the operator-visible bit + behavior
change are wired together end-to-end.
Compile + go vet + golangci-lint v2.11.4 + go test all 0 issues.
Phase 4 next: NGINX canonical real-impl — replace the stub with
nginx -t -c <temp>; same time replace the existing os.WriteFile
flow in DeployCertificate with deploy.Apply(...).
|
||
|
shankar0123
|
f5c67a51b2 |
feat(deploy): atomic write + validate + rollback primitive shared across all target connectors
Phase 1 of the deploy-hardening I master bundle. Closes the load-bearing prerequisite for the seven Bundle I items by extracting one canonical atomic-deploy primitive at internal/deploy/ that all 13 target connectors will consume in Phases 4-9. The package ships: - Plan + Apply API: write all File entries to sibling .certctl-tmp.<nanos> in the destination directory (same-filesystem guarantees os.Rename atomicity), call PreCommit (validate-with-the-target), atomic-rename all temps to final, call PostCommit (reload). On PostCommit failure, restore from pre-deploy backups + re-call PostCommit. If second PostCommit also fails, return ErrRollbackFailed (operator-actionable; documented loud). - AtomicWriteFile lower-level entry for connectors that don't fit the Plan model (F5, K8s — they ship bytes through APIs, not local files). - SHA-256 idempotency: every Apply short-circuits when all File destinations already match SHA-256 of new bytes. Defends against agent-restart retry storms hammering targets with no-op reloads. - Ownership + mode preservation: existing nginx:nginx 0640 stays nginx:nginx 0640 across renewals. Per-target FileDefaults applies for first-deploy. Per-File explicit Mode/Owner/Group overrides win over both. Closes the silent-failure mode where os.WriteFile(path, bytes, 0600) at apache.go:119 (et al.) clobbered worker access. - Backup retention janitor: pre-deploy backup at <path>.certctl-bak.<nanos>; default keeps last 3 (DefaultBackupRetention); BackupRetention=-1 disables backups (rollback impossible — documented foot-gun). - File-level mutex via sync.Map: two concurrent Apply calls touching the same destination serialize. Per-target serialization (Phase 2) is finer- grained at the agent dispatch layer; this is the file-level guard. - Sentinel errors for connector errors.Is checks: ErrPlanInvalid, ErrValidateFailed, ErrReloadFailed, ErrRollbackFailed. Tests (37 named cases across deploy_test.go + coverage_test.go) pin every load-bearing invariant the prompt's Phase 1 requires, plus error-leg coverage uplifts: - TestApply_HappyPath_PreCommitSucceeds_PostCommitSucceeds_FilesAtomic - TestApply_PreCommitFails_NoFilesChanged (atomic-or-nothing on validate) - TestApply_PostCommitFails_FilesRolledBack (rollback wire) - TestApply_RollbackAlsoFails_ReturnsErrRollbackFailed (escalation path) - TestApply_IdempotentSkip_SHA256Match (idempotency short-circuit) - TestApply_PreservesExistingOwnerAndMode_WhenNotOverridden - TestApply_RespectsOverrides_OwnerGroupMode - TestApply_ConcurrentApplyToSameFile_Serializes (file-level lock) - TestApply_BackupRetention_KeepsLastN (janitor pruning) - TestApply_NoExistingFile_UsesDefaultsForOwnerGroupMode - TestAtomicWriteFile_TempFileCleanedUpOnError - TestAtomicWriteFile_RenameRaceWithReader_AtomicReadAlwaysSeesOldOrNew (POSIX-rename atomicity proof via concurrent reader) Plus white-box tests for resolveOwnership, lookupUID/GID, and deeper error legs in restoreFromBackups + applyOwnership + AtomicWriteFile. Coverage 87.3% — practical ceiling without injecting a fault-aware FS abstraction (Write/Sync/Close OS errors are unreachable from go test without sudo'd disk-fill or a custom interface seam). Above the existing service-layer 70% floor; Phases 4-9 will lift this further as they exercise the package through real-connector use. Race detector clean; gofmt + go vet + golangci-lint v2.11.4 all 0 issues. The package is the load-bearing prerequisite for Phases 4-9. Phase 2 next: per-target deploy mutex in cmd/agent/main.go. Spec: cowork/deploy-hardening-i-prompt.md Baseline + recon: cowork/deploy-hardening-i/baseline.md |
||
|
shankar0123
|
9e6c57673e |
test(service): coverage uplift for production hardening II + adjacent helpers (R-CI-extended floor)
CI's R-CI-extended coverage gate failed on 2025-04-30: service-layer
coverage was 68.7% vs the 70% floor. The drag was from new files
(internal/service/ocsp_counters.go, ocsp_response_cache.go,
export_audit_actions.go) that shipped without enough direct tests
to keep the package above the floor.
NEW internal/service/ocsp_counters_test.go (4 tests):
- TestOCSPCounters_NewIsZero — fresh counter snapshot is all zero
- TestOCSPCounters_EveryIncTicksItsLabel — table-driven test
pinning every Inc* method to its label string + the no-cross-
bleed invariant. Critical for Phase 8 Prometheus exposer
contract: a typo in either side would silently drop the
counter from /metrics/prometheus.
- TestOCSPCounters_SnapshotIsCopy — mutating the returned map
doesn't affect the underlying counters
- TestOCSPCounters_ConcurrentTicksRace — race-detector smoke
against sync/atomic primitives
NEW internal/service/ocsp_response_cache_real_test.go (10 tests):
- HappyPath_CachesAfterMiss — first fetch live-signs + writes
cache row; second fetch hits cache
- CacheWriteFailureIsNonFatal — putErrorRepo simulates disk full;
response still returned (fail-soft contract)
- StaleEntryRegenerates — entries with next_update in the past
trigger re-sign on next fetch
- InvalidateOnRevoke — pin the load-bearing security wire
- InvalidateOnRevoke_DeleteFailureSurfacesError — error-path
coverage for the delete branch
- CountByIssuer + NilRepoReturnsEmpty
- CAOperationsSvc.GetOCSPResponseWithNonce_CacheDispatchHit pins
the nil-nonce → cache dispatch wire
- CAOperationsSvc.GetOCSPResponseWithNonce_NonceBypassesCache
pins the nonce-bearing → live-sign bypass wire (cache stays
empty)
- RevocationSvc.SetOCSPCacheInvalidator_WireConnects pins the
setter through to the wired interface
NEW internal/service/coverage_extras_test.go (~12 tests) targets the
0%-coverage chunks adjacent to the bundle's modified files so the
package as a whole stays above the floor:
- cert-export typed audit emission (Phase 7) round-trip with
detail-map inspection (has_private_key + actor_kind + cipher pin)
- PKCS12CipherModernAES256 pinned-value test (drift catches a
future go-pkcs12 default change)
- audit.ListAuditEvents + GetAuditEvent (handler-interface methods
that were at 0%)
- certificate.ListCertificatesWithFilter (M20 filter delegate)
- discovery.{ListScans,GetScan,GetDiscoverySummary} (delegates)
- health_check.{Update,SetNotificationService} delegates + audit
- est.{deterministicSerial,zeroizeBytes,zeroizeKey} pure helpers
+ the live RSA + ECDSA key-zeroize branches
Sandbox total: 67.6% → 69.9% (+2.3pp). The live keygen branches
in zeroizeKey skip in the sandbox when crypto/rand isn't available
but run on CI, so the CI total should land above the 70% floor with
a small buffer.
Pre-commit verification: go build ./... clean; go test -short
-count=1 green for ./internal/service/.
|
||
|
shankar0123
|
13b29ca1bd |
fix(cert-export): satisfy staticcheck ST1022 on PKCS12CipherModernAES256
Production hardening II Phase 11 verification — golangci-lint v2.11.4 flagged the const PKCS12CipherModernAES256 doc comment with ST1022 (comments on exported identifiers should start with the identifier name). Reformatted to lead with the const name; same content. Reproduced clean: 0 issues across handler/, service/, connector/issuer/local/, api/router/, ratelimit/. |