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 8cda860). 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:\...\<thumbprint>; 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.
2026-06-14 10:08:51 +00:00 · 2026-05-02 22:01:30 +00:00
parent 514bf9050c
commit 6d3d861acc
6 changed files with 786 additions and 118 deletions
@@ -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.
@@ -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)
 	}