From 4142837cac4636dd446103ac94b423b6d8c45fe9 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sat, 2 May 2026 22:01:30 +0000 Subject: [PATCH] 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 08a86d3). Each probe runs at the top of DeployCertificate, BEFORE the destructive step, with a unique # CERTCTL_IDEM_PROBE PowerShell comment tag so test mocks match deterministically. IIS: Get-ChildItem Cert:\... + Get-WebBinding; matches when both the cert is in the store AND the active binding's certificateHash equals the new thumbprint. WinCertStore: Get-ChildItem Cert:\...\; matches when the cert exists in the configured store AND its NotAfter is still in the future. JavaKeystore: keytool -list -alias -v; matches when the parsed SHA-256 fingerprint equals sha256(certPEM_DER). On match: return Success=true with Metadata["idempotent"]="true", no destructive operation. On any error during the probe (network, parse, etc.): fall through to today's full deploy path. False negatives are safe; false positives are dangerous. Tests added (one positive + one negative per connector): - TestIIS_Idempotent_SkipsDeployWhenBindingMatches - TestIIS_Idempotent_DifferentBinding_FallsThroughToDeploy - TestWinCertStore_Idempotent_SkipsImportWhenCertInStore - TestWinCertStore_Idempotent_NotInStore_FallsThroughToDeploy - TestJKS_Idempotent_SkipsDeployWhenAliasMatches - TestJKS_Idempotent_DifferentAlias_FallsThroughToDeploy Verified locally: - gofmt clean across all three connectors. - Syntax-validated via gofmt. Audit reference: cowork/deployment-target-audit-2026-05-02-rerun/ RESULTS.md Top-10 fix #3. --- internal/connector/target/iis/iis.go | 90 +++++- internal/connector/target/iis/iis_test.go | 169 +++++++++- .../target/javakeystore/javakeystore.go | 96 ++++++ .../target/javakeystore/javakeystore_test.go | 290 ++++++++++++++---- .../target/wincertstore/wincertstore.go | 62 ++++ .../target/wincertstore/wincertstore_test.go | 197 +++++++++--- 6 files changed, 786 insertions(+), 118 deletions(-) diff --git a/internal/connector/target/iis/iis.go b/internal/connector/target/iis/iis.go index b24fdd0..79ceebb 100644 --- a/internal/connector/target/iis/iis.go +++ b/internal/connector/target/iis/iis.go @@ -254,6 +254,42 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy }, fmt.Errorf("%s", errMsg) } + // Bundle 10 / Top-10 fix #3: SHA-1 (Windows cert-store convention) + // idempotency short-circuit. If the configured site's active binding's + // certificateHash already matches the new thumbprint AND the cert exists + // in the store, skip the destructive Remove+Import cycle entirely. + // Conservative: any error during the probe falls through to today's full + // deploy path. False negatives are safe; false positives are dangerous. + thumbprint, err := certutil.ComputeThumbprint(request.CertPEM) + if err != nil { + errMsg := fmt.Sprintf("failed to compute certificate thumbprint: %v", err) + c.logger.Error("deployment failed", "error", err) + return &target.DeploymentResult{ + Success: false, + Message: errMsg, + DeployedAt: time.Now(), + }, fmt.Errorf("%s", errMsg) + } + + already, idemErr := c.isCertAlreadyDeployed(ctx, thumbprint) + if idemErr == nil && already { + c.logger.Info("IIS already has this cert bound; skipping deploy", + "thumbprint", thumbprint, "site", c.config.SiteName) + return &target.DeploymentResult{ + Success: true, + TargetAddress: fmt.Sprintf("%s (IIS: %s)", c.config.Hostname, c.config.SiteName), + DeploymentID: fmt.Sprintf("iis-idem-%d", time.Now().Unix()), + Message: "Cert already deployed and bound; idempotent skip", + DeployedAt: time.Now(), + Metadata: map[string]string{ + "thumbprint": thumbprint, + "site_name": c.config.SiteName, + "cert_store": c.config.CertStore, + "idempotent": "true", + }, + }, nil + } + // Bundle 5 (2026-05-02 deployment-target audit): pre-deploy snapshot // of the existing binding's SSL thumbprint so a binding-update failure // can roll back to the pre-deploy state. Empty oldThumbprint means @@ -298,19 +334,10 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy }, fmt.Errorf("%s", errMsg) } - // Step 2+3: Compute thumbprint and import PFX + // Step 2+3: Import PFX // In local mode: write PFX to temp file, import via file path // In WinRM mode: base64-encode PFX, decode on remote side to temp file, import, clean up - thumbprint, err := certutil.ComputeThumbprint(request.CertPEM) - if err != nil { - errMsg := fmt.Sprintf("failed to compute certificate thumbprint: %v", err) - c.logger.Error("deployment failed", "error", err) - return &target.DeploymentResult{ - Success: false, - Message: errMsg, - DeployedAt: time.Now(), - }, fmt.Errorf("%s", errMsg) - } + // (thumbprint already computed in the idempotency check above) c.logger.Debug("certificate thumbprint computed", "thumbprint", thumbprint) @@ -762,6 +789,47 @@ func (c *Connector) verifyRollback(ctx context.Context, oldThumbprint string) er return fmt.Errorf("rollback verification disagreed: %s", out) } +// isCertAlreadyDeployed checks if the given thumbprint is already deployed +// and bound to the configured site's active HTTPS binding. +// Returns (true, nil) iff the cert is in the store AND the binding's +// certificateHash matches the thumbprint. Returns (false, nil) on any +// mismatch or missing binding. Returns (false, error) only on executor errors +// — falls through to the full deploy path (conservative). +// +// Bundle 10 / Top-10 fix #3 of the 2026-05-02 deployment-target audit. +func (c *Connector) isCertAlreadyDeployed(ctx context.Context, thumbprint string) (bool, error) { + port := c.config.Port + if port == 0 { + port = 443 + } + + script := fmt.Sprintf( + "# CERTCTL_IDEM_PROBE\n"+ + "$cert = Get-ChildItem 'Cert:\\LocalMachine\\%s\\%s' -ErrorAction SilentlyContinue; "+ + "$binding = Get-WebBinding -Name '%s' -Protocol 'https' -Port %d -ErrorAction SilentlyContinue; "+ + "if ($cert -and $binding -and $binding.certificateHash -eq '%s') { Write-Output 'IDEM_MATCH' } else { Write-Output 'IDEM_MISS' }", + c.config.CertStore, thumbprint, + c.config.SiteName, port, + thumbprint, + ) + + output, err := c.executor.Execute(ctx, script) + if err != nil { + // Executor error: return false (conservative — fall through to full deploy) + c.logger.Debug("idempotency probe executor error", "error", err, "output", strings.TrimSpace(output)) + return false, nil + } + + out := strings.TrimSpace(output) + if out == "IDEM_MATCH" { + c.logger.Debug("idempotency probe matched", "thumbprint", thumbprint) + return true, nil + } + // "IDEM_MISS" or any other output + c.logger.Debug("idempotency probe missed", "output", out) + return false, nil +} + // NOTE: PFX creation, key parsing, thumbprint computation, and password generation // have been extracted to the shared certutil package (internal/connector/target/certutil) // for reuse by WinCertStore and JavaKeystore connectors. diff --git a/internal/connector/target/iis/iis_test.go b/internal/connector/target/iis/iis_test.go index af9c979..4f69690 100644 --- a/internal/connector/target/iis/iis_test.go +++ b/internal/connector/target/iis/iis_test.go @@ -316,25 +316,31 @@ func TestIISConnector_DeployCertificate_Success(t *testing.T) { t.Errorf("expected 40-char thumbprint, got %d", len(result.Metadata["thumbprint"])) } - // Bundle 5: snapshot script runs FIRST, then import, then binding. - // Three PowerShell commands total on the success path. - if len(executor.commands) != 3 { - t.Errorf("expected 3 PowerShell commands (snapshot, import, binding), got %d", len(executor.commands)) + // Bundle 10: idempotency probe runs FIRST (returns IDEM_MISS by default), + // then Bundle 5 snapshot, then import, then binding. + // Four PowerShell commands total on the success path. + if len(executor.commands) != 4 { + t.Errorf("expected 4 PowerShell commands (probe, snapshot, import, binding), got %d", len(executor.commands)) } - // First command should be the Bundle 5 snapshot. - if len(executor.commands) > 0 && !strings.Contains(executor.commands[0], "# CERTCTL_SNAPSHOT") { - t.Errorf("expected # CERTCTL_SNAPSHOT in first command, got: %s", executor.commands[0]) + // First command should be the Bundle 10 idempotency probe. + if len(executor.commands) > 0 && !strings.Contains(executor.commands[0], "# CERTCTL_IDEM_PROBE") { + t.Errorf("expected # CERTCTL_IDEM_PROBE in first command, got: %s", executor.commands[0]) } - // Second command should be PFX import. - if len(executor.commands) > 1 && !strings.Contains(executor.commands[1], "Import-PfxCertificate") { - t.Errorf("expected Import-PfxCertificate in second command, got: %s", executor.commands[1]) + // Second command should be the Bundle 5 snapshot. + if len(executor.commands) > 1 && !strings.Contains(executor.commands[1], "# CERTCTL_SNAPSHOT") { + t.Errorf("expected # CERTCTL_SNAPSHOT in second command, got: %s", executor.commands[1]) } - // Third command should be binding update. - if len(executor.commands) > 2 && !strings.Contains(executor.commands[2], "New-WebBinding") { - t.Errorf("expected New-WebBinding in third command, got: %s", executor.commands[2]) + // Third command should be PFX import. + if len(executor.commands) > 2 && !strings.Contains(executor.commands[2], "Import-PfxCertificate") { + t.Errorf("expected Import-PfxCertificate in third command, got: %s", executor.commands[2]) + } + + // Fourth command should be binding update. + if len(executor.commands) > 3 && !strings.Contains(executor.commands[3], "New-WebBinding") { + t.Errorf("expected New-WebBinding in fourth command, got: %s", executor.commands[3]) } // Verify metadata @@ -349,6 +355,120 @@ func TestIISConnector_DeployCertificate_Success(t *testing.T) { } } +func TestIIS_Idempotent_SkipsDeployWhenBindingMatches(t *testing.T) { + certPEM, keyPEM, chainPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("failed to generate test cert: %v", err) + } + + executor := newMockExecutor() + // Seed the probe to return IDEM_MATCH + executor.responses["# CERTCTL_IDEM_PROBE"] = mockResponse{output: "IDEM_MATCH\n", err: nil} + + cfg := &Config{ + Hostname: "web01.example.com", + SiteName: "Default Web Site", + CertStore: "My", + Port: 443, + IPAddress: "*", + } + + connector := NewWithExecutor(cfg, testLogger(), executor) + + result, err := connector.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + ChainPEM: chainPEM, + }) + if err != nil { + t.Fatalf("DeployCertificate failed: %v", err) + } + if !result.Success { + t.Fatalf("expected success, got: %s", result.Message) + } + + // Verify idempotent flag is set + if result.Metadata["idempotent"] != "true" { + t.Errorf("expected idempotent=true, got %s", result.Metadata["idempotent"]) + } + + // Only the probe should have run, no import/binding calls + if len(executor.commands) != 1 { + t.Errorf("expected 1 command (probe only), got %d", len(executor.commands)) + } + if !strings.Contains(executor.commands[0], "# CERTCTL_IDEM_PROBE") { + t.Errorf("expected probe command, got: %s", executor.commands[0]) + } + + // Verify no Import-PfxCertificate call + for i, cmd := range executor.commands { + if strings.Contains(cmd, "Import-PfxCertificate") { + t.Errorf("command %d should not contain Import-PfxCertificate (idempotent short-circuit): %s", i, cmd) + } + } +} + +func TestIIS_Idempotent_DifferentBinding_FallsThroughToDeploy(t *testing.T) { + certPEM, keyPEM, chainPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("failed to generate test cert: %v", err) + } + + executor := newMockExecutor() + executor.defaultOutput = "OK" + // Seed the probe to return IDEM_MISS + executor.responses["# CERTCTL_IDEM_PROBE"] = mockResponse{output: "IDEM_MISS\n", err: nil} + + cfg := &Config{ + Hostname: "web01.example.com", + SiteName: "Default Web Site", + CertStore: "My", + Port: 443, + IPAddress: "*", + } + + connector := NewWithExecutor(cfg, testLogger(), executor) + + result, err := connector.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + ChainPEM: chainPEM, + }) + if err != nil { + t.Fatalf("DeployCertificate failed: %v", err) + } + if !result.Success { + t.Fatalf("expected success, got: %s", result.Message) + } + + // Verify idempotent flag is NOT set + if result.Metadata["idempotent"] != "" { + t.Errorf("expected no idempotent flag, got %s", result.Metadata["idempotent"]) + } + + // Full flow: probe + snapshot + import + binding = 4 commands + if len(executor.commands) != 4 { + t.Errorf("expected 4 commands (probe, snapshot, import, binding), got %d", len(executor.commands)) + } + + // Verify probe ran first + if !strings.Contains(executor.commands[0], "# CERTCTL_IDEM_PROBE") { + t.Errorf("expected probe as first command, got: %s", executor.commands[0]) + } + + // Verify import happened + hasImport := false + for _, cmd := range executor.commands { + if strings.Contains(cmd, "Import-PfxCertificate") { + hasImport = true + break + } + } + if !hasImport { + t.Error("expected Import-PfxCertificate in commands") + } +} + func TestIISConnector_DeployCertificate_MissingKeyPEM(t *testing.T) { certPEM, _, chainPEM, err := generateTestCertAndKey() if err != nil { @@ -477,6 +597,11 @@ func TestIIS_BindingUpdateFails_RemovesNewCert_RebindsOld(t *testing.T) { } executor := newMockExecutor() + // Probe returns IDEM_MISS (cert not already deployed). + executor.responses["# CERTCTL_IDEM_PROBE"] = mockResponse{ + output: "IDEM_MISS\n", + err: nil, + } // Snapshot returns a pre-existing thumbprint (rollback target). executor.responses["# CERTCTL_SNAPSHOT"] = mockResponse{ output: "OLD_THUMBPRINT:abc123\n", @@ -568,6 +693,11 @@ func TestIIS_BindingUpdateFails_NoOldBinding_RemovesNewCertOnly(t *testing.T) { } executor := newMockExecutor() + // Probe returns IDEM_MISS (cert not already deployed). + executor.responses["# CERTCTL_IDEM_PROBE"] = mockResponse{ + output: "IDEM_MISS\n", + err: nil, + } // First-time deploy: snapshot finds no existing binding. executor.responses["# CERTCTL_SNAPSHOT"] = mockResponse{ output: "NO_OLD_BINDING\n", @@ -651,6 +781,11 @@ func TestIIS_BindingUpdateFails_RollbackAlsoFails_OperatorActionable(t *testing. } executor := newMockExecutor() + // Probe returns IDEM_MISS (cert not already deployed). + executor.responses["# CERTCTL_IDEM_PROBE"] = mockResponse{ + output: "IDEM_MISS\n", + err: nil, + } executor.responses["# CERTCTL_SNAPSHOT"] = mockResponse{ output: "OLD_THUMBPRINT:abc123\n", err: nil, @@ -748,11 +883,11 @@ func TestIISConnector_DeployCertificate_SNIEnabled(t *testing.T) { t.Fatalf("expected success, got: %s", result.Message) } - // Bundle 5: snapshot is commands[0], import is commands[1], binding is commands[2]. - if len(executor.commands) < 3 { - t.Fatal("expected at least 3 commands (snapshot, import, binding)") + // Bundle 10: probe is commands[0], Bundle 5: snapshot is commands[1], import is commands[2], binding is commands[3]. + if len(executor.commands) < 4 { + t.Fatal("expected at least 4 commands (probe, snapshot, import, binding)") } - bindingCmd := executor.commands[2] + bindingCmd := executor.commands[3] if !strings.Contains(bindingCmd, "-SslFlags 1") { t.Errorf("expected -SslFlags 1 for SNI, got: %s", bindingCmd) } diff --git a/internal/connector/target/javakeystore/javakeystore.go b/internal/connector/target/javakeystore/javakeystore.go index 5300edc..08d48e7 100644 --- a/internal/connector/target/javakeystore/javakeystore.go +++ b/internal/connector/target/javakeystore/javakeystore.go @@ -10,6 +10,8 @@ package javakeystore import ( "context" + "crypto/sha256" + "crypto/x509" "encoding/json" "fmt" "log/slog" @@ -211,6 +213,43 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy "alias", c.config.Alias, "type", c.config.KeystoreType) + // Bundle 10 / Top-10 fix #3: SHA-256 idempotency short-circuit. Only + // when the keystore exists (not first-time deploy). If the existing + // alias's SHA-256 fingerprint matches the new cert, skip the -delete + + // -importkeystore cycle entirely. Conservative: any error during the + // probe falls through to today's full deploy path. + if _, err := os.Stat(c.config.KeystorePath); err == nil { + // Keystore exists; try the probe + newCert, err := certutil.ParseCertificatePEM(request.CertPEM) + if err == nil { + // Compute SHA-256 of the new cert's DER-encoded bytes + sha256Hex := computeCertSHA256DERHex(newCert) + + already, idemErr := c.isAliasAlreadyDeployedWithThumbprint(ctx, sha256Hex) + if idemErr == nil && already { + c.logger.Info("JavaKeystore already has this cert; skipping deploy", + "keystore", c.config.KeystorePath, + "alias", c.config.Alias) + // Compute SHA-1 thumbprint for metadata compatibility + sha1Thumb, _ := certutil.ComputeThumbprint(request.CertPEM) + return &target.DeploymentResult{ + Success: true, + TargetAddress: fmt.Sprintf("jks:%s#%s", c.config.KeystorePath, c.config.Alias), + DeploymentID: fmt.Sprintf("jks-idem-%d", time.Now().Unix()), + Message: "Alias already deployed with matching cert; idempotent skip", + DeployedAt: time.Now(), + Metadata: map[string]string{ + "keystore_path": c.config.KeystorePath, + "alias": c.config.Alias, + "keystore_type": c.config.KeystoreType, + "thumbprint": sha1Thumb, + "idempotent": "true", + }, + }, nil + } + } + } + // Step 1: Convert PEM to temporary PKCS#12 file pfxPassword, err := certutil.GenerateRandomPassword(32) if err != nil { @@ -589,3 +628,60 @@ func (c *Connector) pruneBackups() { } } } + +// computeCertSHA256DERHex computes SHA-256 of cert's raw DER encoding as lowercase hex. +func computeCertSHA256DERHex(cert *x509.Certificate) string { + h := sha256.Sum256(cert.Raw) + return fmt.Sprintf("%x", h) +} + +// isAliasAlreadyDeployedWithThumbprint checks if the alias exists in the +// keystore and its SHA-256 fingerprint matches the given thumbprint. +// Runs `keytool -list -alias -v` and parses the output for the SHA-256 line. +// Returns (true, nil) iff the alias exists AND fingerprint matches. +// Returns (false, nil) on mismatch or alias missing. Returns (false, error) +// only on executor errors — falls through to the full deploy path (conservative). +// +// Bundle 10 / Top-10 fix #3 of the 2026-05-02 deployment-target audit. +func (c *Connector) isAliasAlreadyDeployedWithThumbprint(ctx context.Context, sha256Hex string) (bool, error) { + args := []string{ + "-list", + "-alias", c.config.Alias, + "-keystore", c.config.KeystorePath, + "-storepass", c.config.KeystorePassword, + "-storetype", c.config.KeystoreType, + "-v", + } + + output, err := c.executor.Execute(ctx, c.config.KeytoolPath, args...) + if err != nil { + // Alias missing, keystore missing, or executor error: treat as miss, fall through + c.logger.Debug("idempotency probe executor error or alias missing", + "error", err, + "output", output) + return false, nil + } + + // Parse output for SHA256 line. Real keytool output has format: + // "SHA256: AA:BB:CC:..." (colons every 2 chars, uppercase hex) + // or sometimes "SHA-256: ..." depending on JDK version + lines := strings.Split(output, "\n") + for _, line := range lines { + line = strings.TrimSpace(line) + if strings.HasPrefix(line, "SHA256:") || strings.HasPrefix(line, "SHA-256:") { + // Extract hex part: remove colons, convert to lowercase + parts := strings.SplitN(line, ":", 2) + if len(parts) == 2 { + hexWithColons := strings.TrimSpace(parts[1]) + hexNormalized := strings.ToLower(strings.ReplaceAll(hexWithColons, ":", "")) + if hexNormalized == sha256Hex { + c.logger.Debug("idempotency probe matched", "alias", c.config.Alias) + return true, nil + } + } + } + } + + c.logger.Debug("idempotency probe missed", "alias", c.config.Alias) + return false, nil +} diff --git a/internal/connector/target/javakeystore/javakeystore_test.go b/internal/connector/target/javakeystore/javakeystore_test.go index 8579604..3ec2cee 100644 --- a/internal/connector/target/javakeystore/javakeystore_test.go +++ b/internal/connector/target/javakeystore/javakeystore_test.go @@ -5,6 +5,7 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" + "crypto/sha256" "crypto/x509" "crypto/x509/pkix" "encoding/json" @@ -13,11 +14,13 @@ import ( "log/slog" "math/big" "os" + "path/filepath" "strings" "testing" "time" "github.com/shankar0123/certctl/internal/connector/target" + "github.com/shankar0123/certctl/internal/connector/target/certutil" ) func testLogger() *slog.Logger { @@ -604,6 +607,8 @@ func TestJKS_Snapshot_RunsBefore_Delete(t *testing.T) { mock := &mockExecutor{ responses: []mockResponse{ + // Top-10 fix #3 idempotency probe — alias missing → IDEM_MISS, fall through. + {Output: "", Err: fmt.Errorf("keytool exit 1: alias does not exist")}, {Output: "Imported keystore for alias ", Err: nil}, // -exportkeystore (snapshot) {Output: "", Err: nil}, // -delete (alias may exist) {Output: "Import command completed", Err: nil}, // -importkeystore (the actual deploy) @@ -627,42 +632,48 @@ func TestJKS_Snapshot_RunsBefore_Delete(t *testing.T) { t.Error("expected success=true") } - // 3 keytool calls: export (snapshot) → delete → import. The order is - // load-bearing: snapshot MUST run before delete, otherwise the delete - // destroys the very state the snapshot is meant to capture. - if len(mock.calls) != 3 { - t.Fatalf("expected 3 keytool calls (export + delete + import), got %d", len(mock.calls)) + // 4 keytool calls: probe → export (snapshot) → delete → import. The + // snapshot-before-delete ordering is load-bearing: the delete destroys + // the state the snapshot is meant to capture. + if len(mock.calls) != 4 { + t.Fatalf("expected 4 keytool calls (probe + export + delete + import), got %d", len(mock.calls)) } - // Call 0: -exportkeystore. - if mock.calls[0].Name != "keytool" { - t.Errorf("call 0: expected keytool, got %s", mock.calls[0].Name) - } + // Call 0: probe (-list -alias -v). args0 := strings.Join(mock.calls[0].Args, " ") - if !strings.Contains(args0, "-exportkeystore") { - t.Errorf("call 0: expected -exportkeystore in args, got: %s", args0) + if !strings.Contains(args0, "-list") { + t.Errorf("call 0: expected -list probe, got: %s", args0) } - if !strings.Contains(args0, "-srckeystore "+keystorePath) { - t.Errorf("call 0: expected -srckeystore %s, got: %s", keystorePath, args0) + + // Call 1: -exportkeystore. + if mock.calls[1].Name != "keytool" { + t.Errorf("call 1: expected keytool, got %s", mock.calls[1].Name) + } + args1 := strings.Join(mock.calls[1].Args, " ") + if !strings.Contains(args1, "-exportkeystore") { + t.Errorf("call 1: expected -exportkeystore in args, got: %s", args1) + } + if !strings.Contains(args1, "-srckeystore "+keystorePath) { + t.Errorf("call 1: expected -srckeystore %s, got: %s", keystorePath, args1) } // Backup path: /.certctl-bak..p12 - if !strings.Contains(args0, ".certctl-bak.") || !strings.Contains(args0, ".p12") { - t.Errorf("call 0: expected .certctl-bak.*.p12 backup path, got: %s", args0) + if !strings.Contains(args1, ".certctl-bak.") || !strings.Contains(args1, ".p12") { + t.Errorf("call 1: expected .certctl-bak.*.p12 backup path, got: %s", args1) } - // Call 1: -delete. - args1 := strings.Join(mock.calls[1].Args, " ") - if !strings.Contains(args1, "-delete") { - t.Errorf("call 1: expected -delete in args, got: %s", args1) - } - - // Call 2: -importkeystore (the deploy itself). + // Call 2: -delete. args2 := strings.Join(mock.calls[2].Args, " ") - if !strings.Contains(args2, "-importkeystore") { - t.Errorf("call 2: expected -importkeystore in args, got: %s", args2) + if !strings.Contains(args2, "-delete") { + t.Errorf("call 2: expected -delete in args, got: %s", args2) } - if !strings.Contains(args2, "-destkeystore "+keystorePath) { - t.Errorf("call 2: expected -destkeystore %s, got: %s", keystorePath, args2) + + // Call 3: -importkeystore (the deploy itself). + args3 := strings.Join(mock.calls[3].Args, " ") + if !strings.Contains(args3, "-importkeystore") { + t.Errorf("call 3: expected -importkeystore in args, got: %s", args3) + } + if !strings.Contains(args3, "-destkeystore "+keystorePath) { + t.Errorf("call 3: expected -destkeystore %s, got: %s", keystorePath, args3) } } @@ -729,11 +740,13 @@ func TestJKS_ImportFails_RollsBack(t *testing.T) { // rollback delete (best-effort) + rollback re-import from backup PFX. mock := &mockExecutor{ responses: []mockResponse{ - {Output: "Imported keystore for alias ", Err: nil}, // 0: -exportkeystore (snapshot) - {Output: "", Err: nil}, // 1: -delete (pre-import) - {Output: "keystore corruption error", Err: fmt.Errorf("exit 1")}, // 2: -importkeystore FAILS - {Output: "", Err: nil}, // 3: -delete (rollback step 1) - {Output: "Imported keystore for alias ", Err: nil}, // 4: -importkeystore (rollback step 2) + // Top-10 fix #3 idempotency probe — alias missing → fall through. + {Output: "", Err: fmt.Errorf("alias does not exist")}, + {Output: "Imported keystore for alias ", Err: nil}, // 1: -exportkeystore (snapshot) + {Output: "", Err: nil}, // 2: -delete (pre-import) + {Output: "keystore corruption error", Err: fmt.Errorf("exit 1")}, // 3: -importkeystore FAILS + {Output: "", Err: nil}, // 4: -delete (rollback step 1) + {Output: "Imported keystore for alias ", Err: nil}, // 5: -importkeystore (rollback step 2) }, } c := NewWithExecutor(&Config{ @@ -760,23 +773,24 @@ func TestJKS_ImportFails_RollsBack(t *testing.T) { t.Errorf("expected error to mention rollback from , got: %v", err) } - // 5 keytool calls: export, delete, import-fail, rollback-delete, - // rollback-import. Locate the rollback re-import call (call 4) and - // assert it references the backup path. - if len(mock.calls) != 5 { - t.Fatalf("expected 5 keytool calls (export, delete, import, rollback-delete, rollback-import), got %d", len(mock.calls)) + // 6 keytool calls with the new probe at index 0: + // probe, export, delete, import-fail, rollback-delete, rollback-import. + // Locate the rollback re-import call (now at index 5) and assert it + // references the backup path that the snapshot wrote. + if len(mock.calls) != 6 { + t.Fatalf("expected 6 keytool calls (probe, export, delete, import, rollback-delete, rollback-import), got %d", len(mock.calls)) } - rollbackImportArgs := strings.Join(mock.calls[4].Args, " ") + rollbackImportArgs := strings.Join(mock.calls[5].Args, " ") if !strings.Contains(rollbackImportArgs, "-importkeystore") { - t.Errorf("call 4: expected -importkeystore for rollback, got: %s", rollbackImportArgs) + t.Errorf("call 5: expected -importkeystore for rollback, got: %s", rollbackImportArgs) } if !strings.Contains(rollbackImportArgs, ".certctl-bak.") { - t.Errorf("call 4: expected backup path (.certctl-bak.*) as -srckeystore, got: %s", rollbackImportArgs) + t.Errorf("call 5: expected backup path (.certctl-bak.*) as -srckeystore, got: %s", rollbackImportArgs) } - // The same backup path that the snapshot wrote should be the source - // for the rollback re-import — verify by extracting both and comparing. - exportArgs := strings.Join(mock.calls[0].Args, " ") - for _, arg := range mock.calls[0].Args { + // The backup path that the snapshot wrote (call 1) must be the source + // for the rollback re-import (call 5). + exportArgs := strings.Join(mock.calls[1].Args, " ") + for _, arg := range mock.calls[1].Args { if strings.Contains(arg, ".certctl-bak.") && strings.HasSuffix(arg, ".p12") { if !strings.Contains(rollbackImportArgs, arg) { t.Errorf("rollback re-import did not reference snapshot backup %q\n export args: %s\n rollback args: %s", arg, exportArgs, rollbackImportArgs) @@ -797,17 +811,19 @@ func TestJKS_ImportFails_RollbackAlsoFails_OperatorActionable(t *testing.T) { t.Fatal(err) } - // Snapshot → delete → import-fail → rollback-delete → rollback-import - // ALSO fails. Operator-actionable case: BOTH errors AND the backup - // path must be in the wrapped error so operators can manually recover - // from the .p12 file on disk. + // Probe → snapshot → delete → import-fail → rollback-delete → + // rollback-import ALSO fails. Operator-actionable case: BOTH errors + // AND the backup path must be in the wrapped error so operators can + // manually recover from the .p12 file on disk. mock := &mockExecutor{ responses: []mockResponse{ - {Output: "Imported keystore for alias ", Err: nil}, // 0: snapshot - {Output: "", Err: nil}, // 1: pre-import delete - {Output: "import-step error", Err: fmt.Errorf("import exit 1")}, // 2: import FAILS - {Output: "", Err: nil}, // 3: rollback delete - {Output: "rollback-step error", Err: fmt.Errorf("rollback exit 2")}, // 4: rollback import FAILS + // Top-10 fix #3 idempotency probe — alias missing → fall through. + {Output: "", Err: fmt.Errorf("alias does not exist")}, + {Output: "Imported keystore for alias ", Err: nil}, // 1: snapshot + {Output: "", Err: nil}, // 2: pre-import delete + {Output: "import-step error", Err: fmt.Errorf("import exit 1")}, // 3: import FAILS + {Output: "", Err: nil}, // 4: rollback delete + {Output: "rollback-step error", Err: fmt.Errorf("rollback exit 2")}, // 5: rollback import FAILS }, } c := NewWithExecutor(&Config{ @@ -1117,3 +1133,173 @@ func TestJKS_Snapshot_AliasNotInKeystore_ProceedsCleanly(t *testing.T) { t.Error("expected success=true") } } + +func TestJKS_Idempotent_SkipsDeployWhenAliasMatches(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + tmpDir, err := os.MkdirTemp("", "jks-idem-test-*") + if err != nil { + t.Fatalf("create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + keystorePath := filepath.Join(tmpDir, "test.p12") + // Create a placeholder keystore file + if err := os.WriteFile(keystorePath, []byte("placeholder"), 0644); err != nil { + t.Fatalf("write keystore: %v", err) + } + + // Compute SHA-256 of the new cert's DER + newCert, _ := certutil.ParseCertificatePEM(certPEM) + sha256Hex := fmt.Sprintf("%x", sha256.Sum256(newCert.Raw)) + + // Format as keytool output: "SHA256: AA:BB:CC:..." + keytoolOutput := fmt.Sprintf("Alias name: server\n"+ + "Creation date: ...\n"+ + "Certificate fingerprints (SHA-256):\n"+ + "SHA256: %s\n", + formatSHA256WithColons(sha256Hex)) + + // Probe returns the matching output; no other calls should run. + mock := &mockExecutor{ + responses: []mockResponse{ + {Output: keytoolOutput, Err: nil}, // probe — match + }, + } + + c := NewWithExecutor(&Config{ + KeystorePath: keystorePath, + KeystorePassword: "password", + KeystoreType: "PKCS12", + Alias: "server", + }, testLogger(), mock) + + result, err := c.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }) + if err != nil { + t.Fatalf("deploy failed: %v", err) + } + if !result.Success { + t.Error("expected success=true") + } + + // Verify idempotent flag is set + if result.Metadata["idempotent"] != "true" { + t.Errorf("expected idempotent=true, got %s", result.Metadata["idempotent"]) + } + + // Only the probe should have run (1 keytool call). Subsequent keytool + // invocations would be -delete / -importkeystore — none of those should + // fire on idempotent skip. + if len(mock.calls) != 1 { + t.Errorf("expected 1 keytool call (probe only), got %d", len(mock.calls)) + } + if len(mock.calls) > 0 { + args := mock.calls[0].Args + hasList := false + for _, a := range args { + if a == "-list" { + hasList = true + break + } + } + if !hasList { + t.Errorf("expected first call to be `-list` probe, got args: %v", args) + } + } +} + +func TestJKS_Idempotent_DifferentAlias_FallsThroughToDeploy(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + tmpDir, err := os.MkdirTemp("", "jks-idem-test-*") + if err != nil { + t.Fatalf("create temp dir: %v", err) + } + defer os.RemoveAll(tmpDir) + + keystorePath := filepath.Join(tmpDir, "test.p12") + // Create a placeholder keystore file + if err := os.WriteFile(keystorePath, []byte("placeholder"), 0644); err != nil { + t.Fatalf("write keystore: %v", err) + } + + // Probe returns a DIFFERENT SHA-256 → IDEM_MISS → fall through to full + // snapshot+delete+importkeystore deploy path. Bundle 8 snapshot uses + // keytool -exportkeystore, which simulateExportSideEffect needs to + // fake on disk so post-deploy file checks see the backup. + differentFingerprint := "SHA256: FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF:FF\n" + + mock := &mockExecutor{ + responses: []mockResponse{ + {Output: differentFingerprint, Err: nil}, // probe — miss + {Output: "Keystore exported", Err: nil}, // snapshot -exportkeystore + {Output: "", Err: nil}, // -delete (best-effort) + {Output: "Import command completed", Err: nil}, // -importkeystore + }, + onCall: simulateExportSideEffect(t), + } + + c := NewWithExecutor(&Config{ + KeystorePath: keystorePath, + KeystorePassword: "password", + KeystoreType: "PKCS12", + Alias: "server", + }, testLogger(), mock) + + result, err := c.DeployCertificate(context.Background(), target.DeploymentRequest{ + CertPEM: certPEM, + KeyPEM: keyPEM, + }) + if err != nil { + t.Fatalf("deploy failed: %v", err) + } + if !result.Success { + t.Error("expected success=true") + } + + // Verify idempotent flag is NOT set (full deploy path ran) + if result.Metadata["idempotent"] != "" { + t.Errorf("expected no idempotent flag, got %q", result.Metadata["idempotent"]) + } + + // 4 keytool calls expected: probe, snapshot -exportkeystore, -delete, -importkeystore. + if len(mock.calls) != 4 { + t.Errorf("expected 4 keytool calls (probe + snapshot + delete + import), got %d", len(mock.calls)) + for i, c := range mock.calls { + t.Logf("call[%d] args=%v", i, c.Args) + } + } + // First call must be the -list probe. + if len(mock.calls) > 0 { + hasList := false + for _, a := range mock.calls[0].Args { + if a == "-list" { + hasList = true + break + } + } + if !hasList { + t.Errorf("expected first call to be `-list` probe, got args: %v", mock.calls[0].Args) + } + } +} + +func formatSHA256WithColons(hexStr string) string { + var result strings.Builder + for i := 0; i < len(hexStr); i += 2 { + if i > 0 { + result.WriteString(":") + } + result.WriteString(strings.ToUpper(hexStr[i : i+2])) + } + return result.String() +} diff --git a/internal/connector/target/wincertstore/wincertstore.go b/internal/connector/target/wincertstore/wincertstore.go index 9b8c69d..6638797 100644 --- a/internal/connector/target/wincertstore/wincertstore.go +++ b/internal/connector/target/wincertstore/wincertstore.go @@ -223,6 +223,36 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy } newSubject := newCert.Subject.String() + // Bundle 10 / Top-10 fix #3: SHA-1 idempotency short-circuit. If the + // cert is already in the store AND not expired, skip the destructive + // import cycle entirely. Conservative: any error during the probe falls + // through to today's full deploy path. False negatives are safe; false + // positives are dangerous. + thumbprintEarly, err := certutil.ComputeThumbprint(request.CertPEM) + if err != nil { + return nil, fmt.Errorf("compute thumbprint: %w", err) + } + + already, idemErr := c.isCertAlreadyInStore(ctx, thumbprintEarly) + if idemErr == nil && already { + c.logger.Info("WinCertStore already has this cert; skipping deploy", + "thumbprint", thumbprintEarly, + "store_name", c.config.StoreName) + return &target.DeploymentResult{ + Success: true, + TargetAddress: fmt.Sprintf("cert:\\%s\\%s", c.config.StoreLocation, c.config.StoreName), + DeploymentID: fmt.Sprintf("wincertstore-idem-%d", time.Now().Unix()), + Message: "Cert already in store and valid; idempotent skip", + DeployedAt: time.Now(), + Metadata: map[string]string{ + "thumbprint": thumbprintEarly, + "store_name": c.config.StoreName, + "store_location": c.config.StoreLocation, + "idempotent": "true", + }, + }, nil + } + // 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. @@ -640,3 +670,35 @@ func (c *Connector) cleanupSnapshot(ctx context.Context, state *snapshotState) e } return nil } + +// isCertAlreadyInStore checks if the given thumbprint is already in the +// configured certificate store and is still valid (NotAfter in future). +// Returns (true, nil) iff the cert is in the store AND not expired. +// Returns (false, nil) on any mismatch or missing cert. Returns (false, error) +// only on executor errors — falls through to the full deploy path (conservative). +// +// Bundle 10 / Top-10 fix #3 of the 2026-05-02 deployment-target audit. +func (c *Connector) isCertAlreadyInStore(ctx context.Context, thumbprint string) (bool, error) { + script := fmt.Sprintf( + "# CERTCTL_IDEM_PROBE\n"+ + "$cert = Get-ChildItem 'Cert:\\%s\\%s\\%s' -ErrorAction SilentlyContinue; "+ + "if ($cert -and $cert.NotAfter -gt (Get-Date)) { Write-Output 'IDEM_MATCH' } else { Write-Output 'IDEM_MISS' }", + c.config.StoreLocation, c.config.StoreName, thumbprint, + ) + + output, err := c.executor.Execute(ctx, script) + if err != nil { + // Executor error: return false (conservative — fall through to full deploy) + c.logger.Debug("idempotency probe executor error", "error", err, "output", output) + return false, nil + } + + out := strings.TrimSpace(output) + if out == "IDEM_MATCH" { + c.logger.Debug("idempotency probe matched", "thumbprint", thumbprint) + return true, nil + } + // "IDEM_MISS" or any other output + c.logger.Debug("idempotency probe missed", "output", out) + return false, nil +} diff --git a/internal/connector/target/wincertstore/wincertstore_test.go b/internal/connector/target/wincertstore/wincertstore_test.go index 7262335..94586c6 100644 --- a/internal/connector/target/wincertstore/wincertstore_test.go +++ b/internal/connector/target/wincertstore/wincertstore_test.go @@ -197,10 +197,12 @@ 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. + // Bundle 10: idempotency probe runs first (returns IDEM_MISS by default), + // then Bundle 7: success path runs 3 PowerShell scripts in order: + // probe → snapshot → import → cleanup. Seed responses for each. mock := &mockExecutor{ responses: []string{ + "IDEM_MISS", "TEMPDIR:/tmp/certctl-snapshot-abc", "SUCCESS:AABBCCDD", "CLEANUP_OK", @@ -228,22 +230,131 @@ func TestDeployCertificate_Success(t *testing.T) { t.Errorf("expected store_name metadata 'My', got: %s", result.Metadata["store_name"]) } - // 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)) + // Bundle 10: 4 scripts on success path (probe + snapshot + import + cleanup). + if len(mock.scripts) != 4 { + t.Fatalf("expected 4 script calls (probe + snapshot + import + cleanup), got %d", len(mock.scripts)) } - if !strings.Contains(mock.scripts[0], "# CERTCTL_SNAPSHOT") { - t.Errorf("expected # CERTCTL_SNAPSHOT in first script, got: %s", mock.scripts[0]) + if !strings.Contains(mock.scripts[0], "# CERTCTL_IDEM_PROBE") { + t.Errorf("expected # CERTCTL_IDEM_PROBE in first script, got: %s", mock.scripts[0]) } - importScript := mock.scripts[1] + if !strings.Contains(mock.scripts[1], "# CERTCTL_SNAPSHOT") { + t.Errorf("expected # CERTCTL_SNAPSHOT in second script, got: %s", mock.scripts[1]) + } + importScript := mock.scripts[2] if !strings.Contains(importScript, "Import-PfxCertificate") { - t.Error("expected Import-PfxCertificate in second script") + t.Error("expected Import-PfxCertificate in third script") } if !strings.Contains(importScript, "Cert:\\LocalMachine\\My") { - t.Error("expected correct cert store path in second script") + t.Error("expected correct cert store path in third script") } - if !strings.Contains(mock.scripts[2], "# CERTCTL_CLEANUP") { - t.Errorf("expected # CERTCTL_CLEANUP in third script, got: %s", mock.scripts[2]) + if !strings.Contains(mock.scripts[3], "# CERTCTL_CLEANUP") { + t.Errorf("expected # CERTCTL_CLEANUP in fourth script, got: %s", mock.scripts[3]) + } +} + +func TestWinCertStore_Idempotent_SkipsImportWhenCertInStore(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + // Probe returns IDEM_MATCH + mock := &mockExecutor{ + responses: []string{"IDEM_MATCH"}, + } + 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.Fatalf("deploy failed: %v", err) + } + if !result.Success { + t.Error("expected success=true") + } + + // Verify idempotent flag is set + if result.Metadata["idempotent"] != "true" { + t.Errorf("expected idempotent=true, got %s", result.Metadata["idempotent"]) + } + + // Only the probe should have run (1 script call) + if len(mock.scripts) != 1 { + t.Errorf("expected 1 script call (probe only), got %d", len(mock.scripts)) + } + if !strings.Contains(mock.scripts[0], "# CERTCTL_IDEM_PROBE") { + t.Errorf("expected probe script, got: %s", mock.scripts[0]) + } + + // Verify no Import-PfxCertificate call + for i, script := range mock.scripts { + if strings.Contains(script, "Import-PfxCertificate") { + t.Errorf("script %d should not contain Import-PfxCertificate (idempotent short-circuit): %s", i, script) + } + } +} + +func TestWinCertStore_Idempotent_NotInStore_FallsThroughToDeploy(t *testing.T) { + certPEM, keyPEM, err := generateTestCertAndKey() + if err != nil { + t.Fatalf("generate cert: %v", err) + } + + // Probe returns IDEM_MISS, then standard snapshot + import + cleanup responses + mock := &mockExecutor{ + responses: []string{ + "IDEM_MISS", + "TEMPDIR:/tmp/certctl-snapshot-def", + "SUCCESS:DDEEFFGG", + "CLEANUP_OK", + }, + } + 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.Fatalf("deploy failed: %v", err) + } + if !result.Success { + t.Error("expected success=true") + } + + // Verify idempotent flag is NOT set + if result.Metadata["idempotent"] != "" { + t.Errorf("expected no idempotent flag, got %s", result.Metadata["idempotent"]) + } + + // Full flow: probe + snapshot + import + cleanup = 4 scripts + if len(mock.scripts) != 4 { + t.Errorf("expected 4 script calls (probe, snapshot, import, cleanup), got %d", len(mock.scripts)) + } + + // Verify probe ran first + if !strings.Contains(mock.scripts[0], "# CERTCTL_IDEM_PROBE") { + t.Errorf("expected probe as first script, got: %s", mock.scripts[0]) + } + + // Verify import happened + hasImport := false + for _, script := range mock.scripts { + if strings.Contains(script, "Import-PfxCertificate") { + hasImport = true + break + } + } + if !hasImport { + t.Error("expected Import-PfxCertificate in scripts") } } @@ -279,16 +390,18 @@ 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. + // Bundle 10 / Top-10 fix #3 idempotency probe runs first → IDEM_MISS, + // fall through. Bundle 7: snapshot returns empty → import fails → + // rollback runs (and succeeds since snapshot was empty, only removes + // the new cert if it landed). mock := &mockExecutor{ responses: []string{ + "IDEM_MISS", "TEMPDIR:/tmp/certctl-snapshot-xyz", "Access denied", "ROLLBACK_OK", }, - errors: []error{nil, fmt.Errorf("exit code 1"), nil}, + errors: []error{nil, nil, fmt.Errorf("exit code 1"), nil}, } c := NewWithExecutor(&Config{}, testLogger(), mock) @@ -314,6 +427,7 @@ func TestDeployCertificate_WithFriendlyName(t *testing.T) { mock := &mockExecutor{ responses: []string{ + "IDEM_MISS", "TEMPDIR:/tmp/certctl-snapshot-fn", "SUCCESS:AABB", "CLEANUP_OK", @@ -331,11 +445,11 @@ func TestDeployCertificate_WithFriendlyName(t *testing.T) { if err != nil { t.Fatalf("deploy failed: %v", err) } - // 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)) + // Bundle 10: probe at [0], snapshot at [1], import at [2]. + if len(mock.scripts) < 3 { + t.Fatalf("expected at least 3 scripts (probe + snapshot + import), got %d", len(mock.scripts)) } - if !strings.Contains(mock.scripts[1], "FriendlyName") { + if !strings.Contains(mock.scripts[2], "FriendlyName") { t.Error("expected FriendlyName in import script") } } @@ -348,6 +462,7 @@ func TestDeployCertificate_WithRemoveExpired(t *testing.T) { mock := &mockExecutor{ responses: []string{ + "IDEM_MISS", "TEMPDIR:/tmp/certctl-snapshot-re", "SUCCESS:AABB", "CLEANUP_OK", @@ -365,11 +480,11 @@ func TestDeployCertificate_WithRemoveExpired(t *testing.T) { if err != nil { t.Fatalf("deploy failed: %v", err) } - // 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)) + // Bundle 10: probe at [0], snapshot at [1], import at [2]. + if len(mock.scripts) < 3 { + t.Fatalf("expected at least 3 scripts (probe + snapshot + import), got %d", len(mock.scripts)) } - if !strings.Contains(mock.scripts[1], "Remove-Item") { + if !strings.Contains(mock.scripts[2], "Remove-Item") { t.Error("expected Remove-Item for expired cert cleanup in import script") } } @@ -392,16 +507,17 @@ func TestWinCertStore_ImportFails_RemovesNewCert_RestoresOldFromSnapshot(t *test t.Fatalf("generate cert: %v", err) } - // Snapshot finds one same-Subject cert and exports it. - // Import fails. Rollback succeeds. Verify confirms. + // Probe → IDEM_MISS, fall through. Snapshot finds one same-Subject + // cert and exports it. Import fails. Rollback succeeds. Verify confirms. mock := &mockExecutor{ responses: []string{ + "IDEM_MISS", "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}, + errors: []error{nil, nil, fmt.Errorf("exit code 1"), nil, nil}, } c := NewWithExecutor(&Config{ StoreName: "My", @@ -428,9 +544,9 @@ func TestWinCertStore_ImportFails_RemovesNewCert_RestoresOldFromSnapshot(t *test 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)) + // 5 scripts: probe + snapshot + import + rollback + verify. + if len(mock.scripts) != 5 { + t.Fatalf("expected 5 scripts (probe + snapshot + import + rollback + verify), got %d", len(mock.scripts)) } // Locate the rollback script and assert it contains BOTH a Remove-Item @@ -467,17 +583,19 @@ func TestWinCertStore_ImportFails_NoExistingSameSubject_RemovesNewCertOnly(t *te 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. + // Probe → IDEM_MISS, fall through. 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{ + "IDEM_MISS", "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}, + errors: []error{nil, nil, fmt.Errorf("exit code 1"), nil, nil}, } c := NewWithExecutor(&Config{ StoreName: "My", @@ -524,12 +642,13 @@ func TestWinCertStore_FriendlyNameFails_NewCertRemoved_OldCertsRestored(t *testi // failed (FriendlyName, Get-ChildItem verify, RemoveExpired). mock := &mockExecutor{ responses: []string{ + "IDEM_MISS", "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}, + errors: []error{nil, nil, fmt.Errorf("Set-ItemProperty failed"), nil, nil}, } c := NewWithExecutor(&Config{ StoreName: "My", @@ -581,16 +700,18 @@ func TestWinCertStore_ImportFails_RollbackAlsoFails_OperatorActionable(t *testin 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. + // Probe → IDEM_MISS, fall through. Snapshot succeeds; import fails; + // rollback ALSO fails. Operator-actionable case: both errors must be + // surfaced + metadata flags manual_action_required. mock := &mockExecutor{ responses: []string{ + "IDEM_MISS", "SNAPSHOT:OLDTHUMB789:/tmp/certctl-snapshot-rbf/OLDTHUMB789.pfx\nTEMPDIR:/tmp/certctl-snapshot-rbf", "Import error", "Rollback step failed", }, errors: []error{ + nil, nil, fmt.Errorf("import-step exit code 1"), fmt.Errorf("rollback-step exit code 2"),