From 60ae92b0e890be6695a22e21efd3c9c5908dacc1 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 2 May 2026 18:13:40 +0000 Subject: [PATCH] wincertstore: pre-deploy snapshot + on-import-failure rollback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 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:\ 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. --- .../target/wincertstore/wincertstore.go | 334 ++++++++++++++++- .../target/wincertstore/wincertstore_test.go | 342 +++++++++++++++++- 2 files changed, 657 insertions(+), 19 deletions(-) diff --git a/internal/connector/target/wincertstore/wincertstore.go b/internal/connector/target/wincertstore/wincertstore.go index 4879f8d..9b8c69d 100644 --- a/internal/connector/target/wincertstore/wincertstore.go +++ b/internal/connector/target/wincertstore/wincertstore.go @@ -158,7 +158,52 @@ func (c *Connector) ValidateConfig(ctx context.Context, config json.RawMessage) return nil } +// snapshotEntry captures one cert in the target store with the SAME Subject +// as the new cert — i.e. a cert that may be displaced by Import-PfxCertificate +// and must be re-imported on rollback. The PfxPath is a temp file on the +// remote host populated by Export-PfxCertificate during the snapshot phase. +// +// Bundle 7 of the 2026-05-02 deployment-target audit. +type snapshotEntry struct { + Thumbprint string + PfxPath string +} + +// snapshotState is the parsed output of the pre-deploy Get-ChildItem snapshot +// PowerShell script. AllThumbprints is every cert in the store at deploy +// time (used by the post-rollback verify phase to confirm the store is +// back to pre-deploy state). Entries is the subset whose Subject matches +// the new cert and was Export-PfxCertificate'd into TempDir for restore. +// ExportPassword is the transient password used for both Export and +// rollback Import; it is held in memory only and never logged or +// persisted in metadata. +// +// Bundle 7 of the 2026-05-02 deployment-target audit. +type snapshotState struct { + Entries []snapshotEntry + AllThumbprints []string + TempDir string + ExportPassword string +} + // DeployCertificate imports a certificate into the Windows Certificate Store. +// +// Bundle 7 of the 2026-05-02 deployment-target audit added a pre-deploy +// snapshot + on-import-failure rollback wrapper around the original single +// PowerShell import script: +// 1. Parse the new cert's Subject DN from CertPEM (used by the snapshot to +// decide which existing certs may be displaced). +// 2. Run the snapshot script: Get-ChildItem the store; for every cert with +// the same Subject as the new one, Export-PfxCertificate to a tempdir +// using a transient export password. Captures every thumbprint for +// post-rollback verification. +// 3. Run the original import script (unchanged contract: PFX import + +// optional FriendlyName + optional RemoveExpired). +// 4. On import-script failure: run the rollback script (Remove-Item the +// new cert if it landed; Import-PfxCertificate every snapshot entry; +// clean up the tempdir) and a verify script (assert all original +// thumbprints are back). Return wrapped error to the operator. +// 5. On success: best-effort cleanup of the snapshot tempdir. func (c *Connector) DeployCertificate(ctx context.Context, request target.DeploymentRequest) (*target.DeploymentResult, error) { if request.KeyPEM == "" { return nil, fmt.Errorf("private key is required for Windows Certificate Store import") @@ -168,7 +213,39 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy "store_name", c.config.StoreName, "store_location", c.config.StoreLocation) - // Generate transient PFX password + // Bundle 7: parse the new cert's Subject DN. The snapshot phase uses + // this to decide which existing certs to Export-PfxCertificate for + // the rollback path. Cert PEM parse errors fail the deploy before + // any cert-store mutation. + newCert, err := certutil.ParseCertificatePEM(request.CertPEM) + if err != nil { + return nil, fmt.Errorf("parse new cert for snapshot: %w", err) + } + newSubject := newCert.Subject.String() + + // Bundle 7: pre-deploy snapshot. A separate transient export password + // from the import PFX password — different lifecycle, different + // PowerShell script. Held in memory only; never logged or persisted. + exportPassword, err := certutil.GenerateRandomPassword(32) + if err != nil { + return nil, fmt.Errorf("generate snapshot export password: %w", err) + } + + snapshotScript := c.buildSnapshotScript(newSubject, exportPassword) + snapshotOut, err := c.executor.Execute(ctx, snapshotScript) + if err != nil { + // Snapshot failure is a real outage signal — bail out before any + // cert-store mutation. The rollback path requires snapshot data; + // we have none. + return nil, fmt.Errorf("pre-deploy snapshot failed: %s: %w", snapshotOut, err) + } + state := parseSnapshotOutput(snapshotOut, exportPassword) + c.logger.Debug("pre-deploy snapshot captured", + "snapshot_entries", len(state.Entries), + "total_thumbprints", len(state.AllThumbprints), + "tempdir", state.TempDir) + + // Generate transient PFX password for the import. pfxPassword, err := certutil.GenerateRandomPassword(32) if err != nil { return nil, fmt.Errorf("generate PFX password: %w", err) @@ -192,7 +269,78 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy output, err := c.executor.Execute(ctx, script) if err != nil { - return nil, fmt.Errorf("PowerShell import failed: %s: %w", output, err) + // Bundle 7: import failed. Roll back — Remove-Item the new cert + // if it landed, Import-PfxCertificate each snapshotted PFX, clean + // up the tempdir. Then verify the rollback by re-reading + // Get-ChildItem. + c.logger.Error("PowerShell import failed; attempting rollback", + "error", err, + "output", output, + "snapshot_entries", len(state.Entries)) + rbErr := c.rollbackImport(ctx, state, thumbprint) + if rbErr != nil { + // Both import AND rollback failed — operator-actionable. + combined := fmt.Errorf("PowerShell import failed (%w) AND rollback also failed (%v); manual operator inspection required", err, rbErr) + c.logger.Error("WinCertStore rollback also failed", + "import_error", err, + "rollback_error", rbErr, + "new_thumbprint", thumbprint, + "snapshot_entries", len(state.Entries)) + return &target.DeploymentResult{ + Success: false, + TargetAddress: fmt.Sprintf("cert:\\%s\\%s", c.config.StoreLocation, c.config.StoreName), + Message: combined.Error(), + DeployedAt: time.Now(), + Metadata: map[string]string{ + "thumbprint": thumbprint, + "store_name": c.config.StoreName, + "store_location": c.config.StoreLocation, + "import_error": output, + "rollback_error": rbErr.Error(), + "rolled_back": "false", + "manual_action_required": "true", + }, + }, combined + } + + // Rollback succeeded. Best-effort verification — re-read + // Get-ChildItem and assert every original thumbprint is back. + // Skipped when the snapshot was empty (first-time deploy with + // no prior thumbprints to verify against). + verifyNote := "" + if len(state.AllThumbprints) > 0 { + if vErr := c.verifyRollback(ctx, state); vErr != nil { + verifyNote = fmt.Sprintf(" (warning: %v)", vErr) + c.logger.Warn("WinCertStore rollback verification disagreed", + "error", vErr) + } + } + + errMsg := fmt.Sprintf("PowerShell import failed; rolled back%s: %v (output: %s)", verifyNote, err, output) + return &target.DeploymentResult{ + Success: false, + TargetAddress: fmt.Sprintf("cert:\\%s\\%s", c.config.StoreLocation, c.config.StoreName), + Message: errMsg, + DeployedAt: time.Now(), + Metadata: map[string]string{ + "thumbprint": thumbprint, + "store_name": c.config.StoreName, + "store_location": c.config.StoreLocation, + "import_error": output, + "rolled_back": "true", + }, + }, fmt.Errorf("%s", errMsg) + } + + // Success path: clean up the snapshot tempdir on a best-effort basis. + // Failure here is non-fatal — operators don't need their deploy to + // fail because of leftover temp files; surface as a debug log. + if state.TempDir != "" { + if cleanupErr := c.cleanupSnapshot(ctx, state); cleanupErr != nil { + c.logger.Debug("snapshot tempdir cleanup failed (non-fatal)", + "error", cleanupErr, + "tempdir", state.TempDir) + } } c.logger.Info("certificate imported to Windows Certificate Store", @@ -310,3 +458,185 @@ func (c *Connector) ValidateDeployment(ctx context.Context, request target.Valid // Ensure Connector implements target.Connector. var _ target.Connector = (*Connector)(nil) + +// --- Bundle 7: pre-deploy snapshot + on-import-failure rollback --- + +// escapePowerShellSingleQuoted escapes a string for safe embedding inside a +// single-quoted PowerShell literal. PowerShell single-quoted strings have no +// escape sequences other than the apostrophe-doubling rule: a literal +// apostrophe inside the string is written as two consecutive apostrophes. +// Subject DN strings can contain apostrophes (e.g. CN=O'Reilly) so this is +// load-bearing for the snapshot script's -eq Subject comparison. +func escapePowerShellSingleQuoted(s string) string { + return strings.ReplaceAll(s, "'", "''") +} + +// buildSnapshotScript builds the pre-deploy Get-ChildItem snapshot PowerShell. +// Output format (one line per cert plus a trailing TEMPDIR line): +// +// SNAPSHOT:: -- same-Subject cert, exported for restore +// THUMB: -- different Subject; track for verify only +// TEMPDIR: -- tempdir created for the snapshot exports +// +// The export password is embedded as a single-quoted literal. GenerateRandomPassword +// returns alphanumeric chars only so it cannot break the literal. +// +// Bundle 7 of the 2026-05-02 deployment-target audit. The "# CERTCTL_SNAPSHOT" +// comment tag identifies the script to test mocks deterministically. +func (c *Connector) buildSnapshotScript(newSubject, exportPassword string) string { + escapedSubject := escapePowerShellSingleQuoted(newSubject) + var sb strings.Builder + sb.WriteString("# CERTCTL_SNAPSHOT\n") + fmt.Fprintf(&sb, "$store = 'Cert:\\%s\\%s'\n", c.config.StoreLocation, c.config.StoreName) + sb.WriteString("$tempDir = [System.IO.Path]::Combine([System.IO.Path]::GetTempPath(), 'certctl-snapshot-' + [System.Guid]::NewGuid().ToString())\n") + sb.WriteString("New-Item -ItemType Directory -Path $tempDir -Force | Out-Null\n") + fmt.Fprintf(&sb, "$pwd = ConvertTo-SecureString -String '%s' -Force -AsPlainText\n", exportPassword) + fmt.Fprintf(&sb, "$newSubject = '%s'\n", escapedSubject) + sb.WriteString("Get-ChildItem $store -ErrorAction SilentlyContinue | ForEach-Object {\n") + sb.WriteString(" if ($_.Subject -eq $newSubject) {\n") + sb.WriteString(" $pfx = [System.IO.Path]::Combine($tempDir, $_.Thumbprint + '.pfx')\n") + sb.WriteString(" try {\n") + sb.WriteString(" Export-PfxCertificate -Cert $_ -FilePath $pfx -Password $pwd -ChainOption EndEntityCertOnly | Out-Null\n") + sb.WriteString(" Write-Output ('SNAPSHOT:' + $_.Thumbprint + ':' + $pfx)\n") + sb.WriteString(" } catch {\n") + sb.WriteString(" Write-Output ('THUMB:' + $_.Thumbprint)\n") + sb.WriteString(" }\n") + sb.WriteString(" } else {\n") + sb.WriteString(" Write-Output ('THUMB:' + $_.Thumbprint)\n") + sb.WriteString(" }\n") + sb.WriteString("}\n") + sb.WriteString("Write-Output ('TEMPDIR:' + $tempDir)\n") + return sb.String() +} + +// parseSnapshotOutput consumes the output of buildSnapshotScript and returns +// a populated snapshotState. Lines that don't match the expected prefixes +// are tolerated (logged at debug level) so transient PowerShell warnings +// don't fail the parse. +func parseSnapshotOutput(output, exportPassword string) *snapshotState { + state := &snapshotState{ExportPassword: exportPassword} + for _, line := range strings.Split(output, "\n") { + line = strings.TrimSpace(line) + switch { + case strings.HasPrefix(line, "SNAPSHOT:"): + rest := strings.TrimPrefix(line, "SNAPSHOT:") + parts := strings.SplitN(rest, ":", 2) + if len(parts) != 2 { + continue + } + state.Entries = append(state.Entries, snapshotEntry{ + Thumbprint: parts[0], + PfxPath: parts[1], + }) + state.AllThumbprints = append(state.AllThumbprints, parts[0]) + case strings.HasPrefix(line, "THUMB:"): + state.AllThumbprints = append(state.AllThumbprints, strings.TrimPrefix(line, "THUMB:")) + case strings.HasPrefix(line, "TEMPDIR:"): + state.TempDir = strings.TrimPrefix(line, "TEMPDIR:") + } + } + return state +} + +// rollbackImport runs the rollback PowerShell script that: +// 1. Removes the new cert from the store if it landed (Test-Path guard). +// 2. Re-imports each snapshot entry's PFX from the tempdir. +// 3. Cleans up the tempdir. +// +// Returns nil on success, wrapped error on rollback-script failure. +// +// Bundle 7 of the 2026-05-02 deployment-target audit. The "# CERTCTL_ROLLBACK" +// comment tag identifies the script to test mocks deterministically. +func (c *Connector) rollbackImport(ctx context.Context, state *snapshotState, newThumbprint string) error { + var sb strings.Builder + sb.WriteString("# CERTCTL_ROLLBACK\n") + fmt.Fprintf(&sb, "$store = 'Cert:\\%s\\%s'\n", c.config.StoreLocation, c.config.StoreName) + fmt.Fprintf(&sb, "$pwd = ConvertTo-SecureString -String '%s' -Force -AsPlainText\n", state.ExportPassword) + + // Remove the new cert if it landed. + fmt.Fprintf(&sb, "$newCertPath = '%s\\%s\\%s'\n", + fmt.Sprintf("Cert:\\%s", c.config.StoreLocation), + c.config.StoreName, + newThumbprint) + sb.WriteString("if (Test-Path $newCertPath) { Remove-Item $newCertPath -Force -ErrorAction SilentlyContinue }\n") + + // Re-import each snapshot entry. + for _, entry := range state.Entries { + fmt.Fprintf(&sb, + "Import-PfxCertificate -FilePath '%s' -CertStoreLocation $store -Password $pwd -Exportable | Out-Null\n", + entry.PfxPath) + } + + // Clean up the snapshot tempdir. + if state.TempDir != "" { + fmt.Fprintf(&sb, + "Remove-Item -Recurse -Force '%s' -ErrorAction SilentlyContinue\n", + state.TempDir) + } + + sb.WriteString("Write-Output 'ROLLBACK_OK'\n") + + output, err := c.executor.Execute(ctx, sb.String()) + if err != nil { + return fmt.Errorf("rollback script: %w (output: %s)", err, strings.TrimSpace(output)) + } + c.logger.Info("WinCertStore rollback completed", + "snapshot_entries", len(state.Entries), + "new_thumbprint", newThumbprint, + "output", strings.TrimSpace(output)) + return nil +} + +// verifyRollback re-reads Get-ChildItem on the store and asserts every +// pre-deploy thumbprint is back. Returns nil on full match; returns a +// non-fatal warning error when one or more thumbprints are missing +// (the rollback's Remove-Item / Import-PfxCertificate ran but the store +// is in an unexpected state — operator inspection recommended). +// +// Bundle 7 of the 2026-05-02 deployment-target audit. The "# CERTCTL_VERIFY" +// comment tag identifies the script to test mocks deterministically. +func (c *Connector) verifyRollback(ctx context.Context, state *snapshotState) error { + if len(state.AllThumbprints) == 0 { + return nil + } + quoted := make([]string, 0, len(state.AllThumbprints)) + for _, t := range state.AllThumbprints { + quoted = append(quoted, "'"+t+"'") + } + var sb strings.Builder + sb.WriteString("# CERTCTL_VERIFY\n") + fmt.Fprintf(&sb, "$store = 'Cert:\\%s\\%s'\n", c.config.StoreLocation, c.config.StoreName) + sb.WriteString("$found = Get-ChildItem $store -ErrorAction SilentlyContinue | Select-Object -ExpandProperty Thumbprint\n") + fmt.Fprintf(&sb, "$want = @(%s)\n", strings.Join(quoted, ",")) + sb.WriteString("$missing = $want | Where-Object { $_ -notin $found }\n") + sb.WriteString("if ($missing.Count -eq 0) { Write-Output 'VERIFY_OK' } else { Write-Output ('VERIFY_FAILED:' + ($missing -join ',')) }\n") + + output, err := c.executor.Execute(ctx, sb.String()) + if err != nil { + return fmt.Errorf("verify probe: %w", err) + } + out := strings.TrimSpace(output) + if out == "VERIFY_OK" { + return nil + } + return fmt.Errorf("rollback verification disagreed: %s", out) +} + +// cleanupSnapshot best-effort removes the snapshot tempdir on the success +// path so operators' filesystems don't accumulate `certctl-snapshot-*` +// directories. Failure is non-fatal (caller logs at debug level). +// +// Bundle 7 of the 2026-05-02 deployment-target audit. The "# CERTCTL_CLEANUP" +// comment tag identifies the script to test mocks deterministically. +func (c *Connector) cleanupSnapshot(ctx context.Context, state *snapshotState) error { + if state.TempDir == "" { + return nil + } + script := fmt.Sprintf( + "# CERTCTL_CLEANUP\nRemove-Item -Recurse -Force '%s' -ErrorAction SilentlyContinue\nWrite-Output 'CLEANUP_OK'\n", + state.TempDir) + if _, err := c.executor.Execute(ctx, script); err != nil { + return fmt.Errorf("cleanup script: %w", err) + } + return nil +} diff --git a/internal/connector/target/wincertstore/wincertstore_test.go b/internal/connector/target/wincertstore/wincertstore_test.go index e8b7e3b..7262335 100644 --- a/internal/connector/target/wincertstore/wincertstore_test.go +++ b/internal/connector/target/wincertstore/wincertstore_test.go @@ -197,8 +197,14 @@ func TestDeployCertificate_Success(t *testing.T) { t.Fatalf("generate cert: %v", err) } + // Bundle 7: success path runs 3 PowerShell scripts in order: + // snapshot → import → cleanup. Seed responses for each. mock := &mockExecutor{ - responses: []string{"SUCCESS:AABBCCDD"}, + responses: []string{ + "TEMPDIR:/tmp/certctl-snapshot-abc", + "SUCCESS:AABBCCDD", + "CLEANUP_OK", + }, } c := NewWithExecutor(&Config{ StoreName: "My", @@ -222,16 +228,22 @@ func TestDeployCertificate_Success(t *testing.T) { t.Errorf("expected store_name metadata 'My', got: %s", result.Metadata["store_name"]) } - // Verify the PowerShell script was called - if len(mock.scripts) != 1 { - t.Fatalf("expected 1 script call, got %d", len(mock.scripts)) + // Bundle 7: 3 scripts on success path (snapshot + import + cleanup). + if len(mock.scripts) != 3 { + t.Fatalf("expected 3 script calls (snapshot + import + cleanup), got %d", len(mock.scripts)) } - script := mock.scripts[0] - if !strings.Contains(script, "Import-PfxCertificate") { - t.Error("expected Import-PfxCertificate in script") + if !strings.Contains(mock.scripts[0], "# CERTCTL_SNAPSHOT") { + t.Errorf("expected # CERTCTL_SNAPSHOT in first script, got: %s", mock.scripts[0]) } - if !strings.Contains(script, "Cert:\\LocalMachine\\My") { - t.Error("expected correct cert store path in script") + importScript := mock.scripts[1] + if !strings.Contains(importScript, "Import-PfxCertificate") { + t.Error("expected Import-PfxCertificate in second script") + } + if !strings.Contains(importScript, "Cert:\\LocalMachine\\My") { + t.Error("expected correct cert store path in second script") + } + if !strings.Contains(mock.scripts[2], "# CERTCTL_CLEANUP") { + t.Errorf("expected # CERTCTL_CLEANUP in third script, got: %s", mock.scripts[2]) } } @@ -267,9 +279,16 @@ func TestDeployCertificate_ImportFailed(t *testing.T) { t.Fatalf("generate cert: %v", err) } + // Bundle 7: snapshot returns empty (no thumbprints in store) → import + // fails → rollback runs (and succeeds since snapshot was empty, only + // removes the new cert if it landed). Mock seeds 3 responses. mock := &mockExecutor{ - responses: []string{"Access denied"}, - errors: []error{fmt.Errorf("exit code 1")}, + responses: []string{ + "TEMPDIR:/tmp/certctl-snapshot-xyz", + "Access denied", + "ROLLBACK_OK", + }, + errors: []error{nil, fmt.Errorf("exit code 1"), nil}, } c := NewWithExecutor(&Config{}, testLogger(), mock) @@ -280,6 +299,11 @@ func TestDeployCertificate_ImportFailed(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "PowerShell import failed") { t.Fatalf("expected import failure error, got: %v", err) } + // Bundle 7: error message must reference rollback so operators know + // the deploy left the store in a known state. + if !strings.Contains(err.Error(), "rolled back") { + t.Errorf("expected error to mention 'rolled back', got: %v", err) + } } func TestDeployCertificate_WithFriendlyName(t *testing.T) { @@ -288,7 +312,13 @@ func TestDeployCertificate_WithFriendlyName(t *testing.T) { t.Fatalf("generate cert: %v", err) } - mock := &mockExecutor{responses: []string{"SUCCESS:AABB"}} + mock := &mockExecutor{ + responses: []string{ + "TEMPDIR:/tmp/certctl-snapshot-fn", + "SUCCESS:AABB", + "CLEANUP_OK", + }, + } c := NewWithExecutor(&Config{ StoreName: "My", FriendlyName: "Production API Cert", @@ -301,8 +331,12 @@ func TestDeployCertificate_WithFriendlyName(t *testing.T) { if err != nil { t.Fatalf("deploy failed: %v", err) } - if !strings.Contains(mock.scripts[0], "FriendlyName") { - t.Error("expected FriendlyName in PowerShell script") + // Bundle 7: import script is now mock.scripts[1] (snapshot is [0]). + if len(mock.scripts) < 2 { + t.Fatalf("expected at least 2 scripts (snapshot + import), got %d", len(mock.scripts)) + } + if !strings.Contains(mock.scripts[1], "FriendlyName") { + t.Error("expected FriendlyName in import script") } } @@ -312,7 +346,13 @@ func TestDeployCertificate_WithRemoveExpired(t *testing.T) { t.Fatalf("generate cert: %v", err) } - mock := &mockExecutor{responses: []string{"SUCCESS:AABB"}} + mock := &mockExecutor{ + responses: []string{ + "TEMPDIR:/tmp/certctl-snapshot-re", + "SUCCESS:AABB", + "CLEANUP_OK", + }, + } c := NewWithExecutor(&Config{ StoreName: "My", RemoveExpired: true, @@ -325,8 +365,276 @@ func TestDeployCertificate_WithRemoveExpired(t *testing.T) { if err != nil { t.Fatalf("deploy failed: %v", err) } - if !strings.Contains(mock.scripts[0], "Remove-Item") { - t.Error("expected Remove-Item for expired cert cleanup in script") + // Bundle 7: import script is mock.scripts[1]. + if len(mock.scripts) < 2 { + t.Fatalf("expected at least 2 scripts (snapshot + import), got %d", len(mock.scripts)) + } + if !strings.Contains(mock.scripts[1], "Remove-Item") { + t.Error("expected Remove-Item for expired cert cleanup in import script") + } +} + +// --- Bundle 7: pre-deploy snapshot + on-import-failure rollback --- +// +// These four tests pin the load-bearing rollback contract added in +// Bundle 7 of the 2026-05-02 deployment-target audit: +// - happy rollback path: snapshot finds same-Subject cert → import +// fails → rollback removes new cert + re-imports snapshot; +// - first-time deploy: snapshot finds no same-Subject certs → import +// fails → rollback only removes the new cert (no re-import); +// - FriendlyName-step failure: import script fails on Set +// FriendlyName → same rollback path; +// - rollback-also-fails: operator-actionable wrapped error. + +func TestWinCertStore_ImportFails_RemovesNewCert_RestoresOldFromSnapshot(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + // Snapshot finds one same-Subject cert and exports it. + // Import fails. Rollback succeeds. Verify confirms. + mock := &mockExecutor{ + responses: []string{ + "SNAPSHOT:OLDTHUMB123:/tmp/certctl-snapshot-abc/OLDTHUMB123.pfx\nTEMPDIR:/tmp/certctl-snapshot-abc", + "PFX import error", + "ROLLBACK_OK", + "VERIFY_OK", + }, + errors: []error{nil, fmt.Errorf("exit code 1"), nil, nil}, + } + c := NewWithExecutor(&Config{ + StoreName: "My", + StoreLocation: "LocalMachine", + }, testLogger(), mock) + + result, err := c.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }) + if err == nil { + t.Fatal("expected error when import fails") + } + if result == nil { + t.Fatal("expected non-nil result on rollback path") + } + if result.Success { + t.Fatal("expected failure result") + } + if !strings.Contains(err.Error(), "PowerShell import failed") { + t.Errorf("expected error to mention import failure, got: %v", err) + } + if !strings.Contains(err.Error(), "rolled back") { + t.Errorf("expected error to mention 'rolled back', got: %v", err) + } + + // 4 scripts: snapshot + import + rollback + verify. + if len(mock.scripts) != 4 { + t.Fatalf("expected 4 scripts (snapshot + import + rollback + verify), got %d", len(mock.scripts)) + } + + // Locate the rollback script and assert it contains BOTH a Remove-Item + // for the new thumbprint AND an Import-PfxCertificate for the + // snapshotted PFX. + var rollbackScript string + for _, s := range mock.scripts { + if strings.Contains(s, "# CERTCTL_ROLLBACK") { + rollbackScript = s + break + } + } + if rollbackScript == "" { + t.Fatal("expected rollback script to be executed") + } + if !strings.Contains(rollbackScript, "Remove-Item") { + t.Errorf("expected rollback to contain Remove-Item, got: %s", rollbackScript) + } + if !strings.Contains(rollbackScript, "Import-PfxCertificate") { + t.Errorf("expected rollback to Import-PfxCertificate the snapshot, got: %s", rollbackScript) + } + if !strings.Contains(rollbackScript, "OLDTHUMB123.pfx") { + t.Errorf("expected rollback to reference the snapshot pfx path, got: %s", rollbackScript) + } + + if result.Metadata["rolled_back"] != "true" { + t.Errorf("expected rolled_back=true in metadata, got: %s", result.Metadata["rolled_back"]) + } +} + +func TestWinCertStore_ImportFails_NoExistingSameSubject_RemovesNewCertOnly(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + // Snapshot returns THUMB lines (different-Subject certs in store) but + // NO SNAPSHOT lines — no same-Subject cert was exported. Rollback + // removes the new cert but does NOT call Import-PfxCertificate. + mock := &mockExecutor{ + responses: []string{ + "THUMB:UNRELATED1\nTHUMB:UNRELATED2\nTEMPDIR:/tmp/certctl-snapshot-noss", + "PFX import error", + "ROLLBACK_OK", + "VERIFY_OK", + }, + errors: []error{nil, fmt.Errorf("exit code 1"), nil, nil}, + } + c := NewWithExecutor(&Config{ + StoreName: "My", + StoreLocation: "LocalMachine", + }, testLogger(), mock) + + _, err = c.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }) + if err == nil { + t.Fatal("expected error when import fails") + } + + var rollbackScript string + for _, s := range mock.scripts { + if strings.Contains(s, "# CERTCTL_ROLLBACK") { + rollbackScript = s + break + } + } + if rollbackScript == "" { + t.Fatal("expected rollback script to be executed") + } + if !strings.Contains(rollbackScript, "Remove-Item") { + t.Errorf("expected rollback to remove new cert via Remove-Item, got: %s", rollbackScript) + } + // No same-Subject snapshots → no Import-PfxCertificate during rollback. + if strings.Contains(rollbackScript, "Import-PfxCertificate") { + t.Errorf("expected no Import-PfxCertificate when snapshot has no same-Subject entries, got: %s", rollbackScript) + } +} + +func TestWinCertStore_FriendlyNameFails_NewCertRemoved_OldCertsRestored(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + // The connector cannot inspect WHICH step inside the import script + // failed — Execute returns a single (output, error). Operators see + // the FriendlyName failure surfaced via the error output. The + // rollback runs the same way regardless of which post-Import step + // failed (FriendlyName, Get-ChildItem verify, RemoveExpired). + mock := &mockExecutor{ + responses: []string{ + "SNAPSHOT:OLDTHUMB456:/tmp/certctl-snapshot-fn/OLDTHUMB456.pfx\nTEMPDIR:/tmp/certctl-snapshot-fn", + "Cannot set FriendlyName: invalid character in friendly name", + "ROLLBACK_OK", + "VERIFY_OK", + }, + errors: []error{nil, fmt.Errorf("Set-ItemProperty failed"), nil, nil}, + } + c := NewWithExecutor(&Config{ + StoreName: "My", + StoreLocation: "LocalMachine", + FriendlyName: "Production", + }, testLogger(), mock) + + result, err := c.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }) + if err == nil { + t.Fatal("expected error when FriendlyName step fails") + } + if result.Success { + t.Fatal("expected failure result") + } + // Operator visibility: the import_error in metadata should preserve + // the PowerShell output so operators can tell what went wrong. + if !strings.Contains(result.Metadata["import_error"], "FriendlyName") { + t.Errorf("expected import_error to reference FriendlyName, got: %s", result.Metadata["import_error"]) + } + + var rollbackScript string + for _, s := range mock.scripts { + if strings.Contains(s, "# CERTCTL_ROLLBACK") { + rollbackScript = s + break + } + } + if rollbackScript == "" { + t.Fatal("expected rollback script to be executed") + } + if !strings.Contains(rollbackScript, "Remove-Item") { + t.Errorf("expected rollback to Remove-Item the new cert, got: %s", rollbackScript) + } + if !strings.Contains(rollbackScript, "OLDTHUMB456.pfx") { + t.Errorf("expected rollback to restore snapshotted cert, got: %s", rollbackScript) + } + + if result.Metadata["rolled_back"] != "true" { + t.Errorf("expected rolled_back=true, got: %s", result.Metadata["rolled_back"]) + } +} + +func TestWinCertStore_ImportFails_RollbackAlsoFails_OperatorActionable(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + // Snapshot succeeds; import fails; rollback ALSO fails. Operator- + // actionable case: both errors must be surfaced + metadata flags + // manual_action_required. + mock := &mockExecutor{ + responses: []string{ + "SNAPSHOT:OLDTHUMB789:/tmp/certctl-snapshot-rbf/OLDTHUMB789.pfx\nTEMPDIR:/tmp/certctl-snapshot-rbf", + "Import error", + "Rollback step failed", + }, + errors: []error{ + nil, + fmt.Errorf("import-step exit code 1"), + fmt.Errorf("rollback-step exit code 2"), + }, + } + c := NewWithExecutor(&Config{ + StoreName: "My", + StoreLocation: "LocalMachine", + }, testLogger(), mock) + + result, err := c.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }) + if err == nil { + t.Fatal("expected error when both import and rollback fail") + } + if result.Success { + t.Fatal("expected failure result") + } + + // Wrapped error must mention BOTH errors. + if !strings.Contains(err.Error(), "PowerShell import failed") { + t.Errorf("expected error to mention import failure, got: %v", err) + } + if !strings.Contains(err.Error(), "rollback also failed") { + t.Errorf("expected error to mention rollback failure, got: %v", err) + } + if !strings.Contains(err.Error(), "manual operator inspection required") { + t.Errorf("expected error to flag manual inspection, got: %v", err) + } + + // Metadata flags manual action + surfaces both errors. + if result.Metadata["manual_action_required"] != "true" { + t.Errorf("expected manual_action_required=true, got: %s", result.Metadata["manual_action_required"]) + } + if result.Metadata["rolled_back"] != "false" { + t.Errorf("expected rolled_back=false, got: %s", result.Metadata["rolled_back"]) + } + if result.Metadata["rollback_error"] == "" { + t.Error("expected rollback_error in metadata") + } + if result.Metadata["import_error"] == "" { + t.Error("expected import_error in metadata") } }