caddy: fix duration metric + file-mode PEM validate + api-mode idempotency

Closes Bundle 9 of the 2026-05-02 deployment-target coverage audit (see cowork/deployment-target-audit-2026-05-02/RESULTS.md). Three small independent fixes that share one connector file: 1. Duration metric (caddy.go L176). Pre-fix: "duration_ms": fmt.Sprintf("%d", time.Since(time.Now()).Milliseconds()) This always returned ~0ms because time.Now() was called twice — the second call captured a baseline immediately before time.Since computed the delta. The intended baseline is `startTime` declared at L113 and threaded through deployViaFile correctly. Post-fix: "duration_ms": fmt.Sprintf("%d", time.Since(startTime).Milliseconds()) deployViaAPI's signature evolves to take startTime time.Time so the api-mode path uses the same baseline as the file-mode path. 2. File-mode ValidateDeployment now validates PEM syntax. Pre-fix (caddy.go L266-293) checked file existence only via os.Stat. A cert file containing garbage bytes passed validation; Caddy's file-watcher silently failed to load it; operators saw "validation green" + "TLS handshake fails" with no obvious connection. Post-fix: after the os.Stat checks succeed, os.ReadFile + parse the first PEM block as an x509 cert via the shared certutil.ParseCertificatePEM helper. Failure surfaces as Valid=false with a clear "not valid PEM/x509" message. 3. API-mode idempotency short-circuit. Pre-fix, every deploy POSTed to /config/apps/tls/certificates/load even when the active cert was already what we wanted to deploy. Caddy reloads TLS state on every POST, briefly bumping CPU and possibly disrupting connections in flight. Post-fix: idempotencySkipPOST runs a GET first, parses the response (handles BOTH the array-of-objects and single-object shapes Caddy admin can return), SHA-256 compares the entry's `cert` field to the deploy payload's cert bytes, and skips the POST when match. Result.Metadata["idempotent"]="true" surfaces the no-op. Conservative: any GET failure (network, non-200, parse error, no matching entry, hash mismatch) silently falls through to the POST, preserving today's behavior. Idempotency is a fast path, not a correctness boundary — false negatives are safe; false positives are dangerous. Tests added to caddy_test.go (6 new tests, ~290 LOC): - TestCaddy_API_DurationMetric_NonZero (httptest server with a 10ms sleep in the POST handler; asserts duration_ms parses as int >= 5). - TestCaddy_ValidateDeployment_FileMode_MalformedPEM_Rejected (writes garbage to cert.pem; asserts Valid=false with PEM/x509 in message). - TestCaddy_ValidateDeployment_FileMode_ValidPEM_Accepted (writes a real ECDSA P-256 self-signed cert; asserts Valid=true). - TestCaddy_API_Idempotent_SkipsPOSTWhenCertHashMatches (GET response contains the same cert as the deploy payload; POST counter remains 0; metadata.idempotent=true; exactly 1 GET probe ran). - TestCaddy_API_Idempotent_RunsPOSTWhenCertHashDiffers (GET response contains a DIFFERENT cert; POST counter is 1; idempotent absent). - TestCaddy_API_Idempotent_GETFails_FallsThroughToPOST (GET returns 500; POST still runs; deploy succeeds; idempotent absent). Two existing tests updated to match the new contracts: - TestCaddyConnector_DeployViaAPI_Success: mock handler now serves BOTH GET (returns "[]" so the comparison falls through) and POST (the original 200-OK path). The dispatch is a method-switch inside the path-match branch. - TestCaddyConnector_ValidateDeployment_Success: the placeholder cert "MIIC..." used to pass the old existence-only check; post-Fix-2 it fails the PEM-parse check. Test now uses generateTestCertAndKey to produce a real self-signed ECDSA P-256 cert. generateTestCertAndKey helper added to the test file — same pattern the javakeystore + wincertstore tests use, kept local because the caddy package has no other test in the certutil family that would make a shared helper cleaner. Verified locally: - gofmt -l ./internal/connector/target/caddy/ clean - go vet ./internal/connector/target/caddy/ clean - go build ./cmd/agent/... clean (factory wiring unchanged) - go test -race -count=1 ./internal/connector/target/caddy/ green (16 tests total: 11 pre-existing including the two updated + 6 new) Audit reference: cowork/deployment-target-audit-2026-05-02/RESULTS.md Bundle 9.
2026-06-07 20:01:31 +00:00 · 2026-05-02 19:13:18 +00:00
parent 87e0009d97
commit 8cda860026
2 changed files with 538 additions and 24 deletions
@@ -3,6 +3,7 @@ package caddy
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -13,6 +14,7 @@ import (
 	"time"

 	"github.com/shankar0123/certctl/internal/connector/target"
+	"github.com/shankar0123/certctl/internal/connector/target/certutil"
 	"github.com/shankar0123/certctl/internal/deploy"
 )

@@ -114,7 +116,7 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy

 	// Try API mode if configured
 	if c.config.Mode == "api" {
-		result, err := c.deployViaAPI(ctx, request)
+		result, err := c.deployViaAPI(ctx, request, startTime)
 		if err == nil {
 			c.logger.Info("certificate deployed to Caddy via API",
 				"duration", time.Since(startTime).String())
@@ -128,7 +130,15 @@ func (c *Connector) DeployCertificate(ctx context.Context, request target.Deploy
 }

 // deployViaAPI deploys a certificate using Caddy's admin API.
-func (c *Connector) deployViaAPI(ctx context.Context, request target.DeploymentRequest) (*target.DeploymentResult, error) {
+//
+// Bundle 9 of the 2026-05-02 deployment-target audit:
+//   - Fix 1: duration_ms metric uses the caller-passed startTime (was
+//     time.Since(time.Now()) which always rounded to ~0ms).
+//   - Fix 3: SHA-256 idempotency short-circuit — GET the current loaded
+//     certs and compare to the deploy payload's cert bytes; skip the POST
+//     when they match. Conservative: any GET failure (network, non-200,
+//     parse error, hash mismatch) falls through to today's POST behavior.
+func (c *Connector) deployViaAPI(ctx context.Context, request target.DeploymentRequest, startTime time.Time) (*target.DeploymentResult, error) {
 	c.logger.Debug("attempting API deployment", "url", c.config.AdminAPI)

 	// Build the certificate payload with combined cert and chain
@@ -137,13 +147,41 @@ func (c *Connector) deployViaAPI(ctx context.Context, request target.DeploymentR
 		certData += request.ChainPEM + "\n"
 	}

+	apiURL := c.config.AdminAPI + "/config/apps/tls/certificates/load"
+
+	// Bundle 9 Fix 3: idempotency short-circuit. GET the current loaded
+	// certs and SHA-256 compare to the deploy payload — if they match,
+	// skip the POST and return success with metadata.idempotent=true.
+	// Caddy's response shape varies between operators / config versions;
+	// handle BOTH the array-of-objects and single-object shapes. Any
+	// failure during the GET (network, non-200, JSON parse error, hash
+	// mismatch) silently falls through to the POST below — false
+	// negatives are safe; false positives are dangerous.
+	deployHash := sha256.Sum256([]byte(certData))
+	if c.idempotencySkipPOST(ctx, apiURL, deployHash) {
+		c.logger.Debug("Caddy already has this certificate loaded; skipping POST",
+			"admin_url", c.config.AdminAPI)
+		return &target.DeploymentResult{
+			Success:       true,
+			TargetAddress: c.config.AdminAPI,
+			DeploymentID:  fmt.Sprintf("caddy-api-idem-%d", time.Now().Unix()),
+			Message:       "Caddy already has this certificate loaded (idempotent skip)",
+			DeployedAt:    time.Now(),
+			Metadata: map[string]string{
+				"method":      "api",
+				"admin_url":   c.config.AdminAPI,
+				"idempotent":  "true",
+				"duration_ms": fmt.Sprintf("%d", time.Since(startTime).Milliseconds()),
+			},
+		}, nil
+	}
+
 	payload := map[string]string{
 		"cert": certData,
 		"key":  request.KeyPEM,
 	}

 	bodyBytes, _ := json.Marshal(payload)
-	apiURL := c.config.AdminAPI + "/config/apps/tls/certificates/load"

 	req, err := http.NewRequestWithContext(ctx, "POST", apiURL, bytes.NewReader(bodyBytes))
 	if err != nil {
@@ -173,11 +211,65 @@ func (c *Connector) deployViaAPI(ctx context.Context, request target.DeploymentR
 		Metadata: map[string]string{
 			"method":      "api",
 			"admin_url":   c.config.AdminAPI,
-			"duration_ms": fmt.Sprintf("%d", time.Since(time.Now()).Milliseconds()),
+			"duration_ms": fmt.Sprintf("%d", time.Since(startTime).Milliseconds()),
 		},
 	}, nil
 }

+// idempotencySkipPOST returns true iff GET-ing the Caddy admin API at the
+// same path returns a cert whose SHA-256 matches the deploy payload's
+// SHA-256. The function is best-effort: it returns false (= "fall through
+// to the POST") on any error along the way (network, non-200, JSON parse,
+// no matching entry, hash mismatch).
+//
+// Caddy's response shape can be either:
+//   - a single object: {"cert": "...", "key": "..."}
+//   - an array of objects: [{"cert": "...", "key": "..."}, ...]
+//
+// We try both shapes; the first that decodes AND has a matching hash wins.
+//
+// Bundle 9 Fix 3 of the 2026-05-02 deployment-target audit.
+func (c *Connector) idempotencySkipPOST(ctx context.Context, apiURL string, deployHash [32]byte) bool {
+	getReq, err := http.NewRequestWithContext(ctx, "GET", apiURL, nil)
+	if err != nil {
+		return false
+	}
+	getReq.Header.Set("Accept", "application/json")
+	resp, err := c.client.Do(getReq)
+	if err != nil {
+		return false
+	}
+	defer resp.Body.Close()
+	if resp.StatusCode != http.StatusOK {
+		return false
+	}
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return false
+	}
+	// Try array-of-objects shape first.
+	var asArray []map[string]string
+	if err := json.Unmarshal(body, &asArray); err == nil {
+		for _, entry := range asArray {
+			if entry["cert"] == "" {
+				continue
+			}
+			if sha256.Sum256([]byte(entry["cert"])) == deployHash {
+				return true
+			}
+		}
+		return false
+	}
+	// Fall back to single-object shape.
+	var asObject map[string]string
+	if err := json.Unmarshal(body, &asObject); err == nil {
+		if asObject["cert"] != "" && sha256.Sum256([]byte(asObject["cert"])) == deployHash {
+			return true
+		}
+	}
+	return false
+}
+
 // deployViaFile deploys a certificate by writing files to the cert directory.
 func (c *Connector) deployViaFile(ctx context.Context, request target.DeploymentRequest, startTime time.Time) (*target.DeploymentResult, error) {
 	c.logger.Debug("deploying via file mode", "cert_dir", c.config.CertDir)
@@ -262,7 +354,15 @@ func (c *Connector) ValidateDeployment(ctx context.Context, request target.Valid

 	startTime := time.Now()

-	// For file mode, verify files exist
+	// For file mode, verify files exist AND the cert is valid PEM/x509.
+	//
+	// Bundle 9 Fix 2 of the 2026-05-02 deployment-target audit: pre-fix,
+	// this branch checked file existence only (os.Stat). A cert file that
+	// existed but contained malformed bytes passed validation; Caddy's
+	// file-watcher silently failed to load it; operators saw "validation
+	// green" + "TLS handshake fails" with no obvious connection. Post-fix,
+	// the cert file is read and parsed via certutil.ParseCertificatePEM so
+	// only files that Caddy can actually load pass validation.
 	if c.config.Mode == "file" || c.config.CertDir != "" {
 		certPath := filepath.Join(c.config.CertDir, c.config.CertFile)
 		keyPath := filepath.Join(c.config.CertDir, c.config.KeyFile)
@@ -290,6 +390,34 @@ func (c *Connector) ValidateDeployment(ctx context.Context, request target.Valid
 				ValidatedAt:   time.Now(),
 			}, fmt.Errorf("%s", errMsg)
 		}
+
+		// Bundle 9 Fix 2: PEM-parse the cert file. A malformed cert that
+		// Caddy's watcher would silently reject must fail validation here
+		// instead of bubbling up as a confusing "TLS handshake fails"
+		// later.
+		certBytes, err := os.ReadFile(certPath)
+		if err != nil {
+			errMsg := fmt.Sprintf("failed to read certificate file %s: %v", certPath, err)
+			c.logger.Error("validation failed", "error", err)
+			return &target.ValidationResult{
+				Valid:         false,
+				Serial:        request.Serial,
+				TargetAddress: certPath,
+				Message:       errMsg,
+				ValidatedAt:   time.Now(),
+			}, fmt.Errorf("%s", errMsg)
+		}
+		if _, err := certutil.ParseCertificatePEM(string(certBytes)); err != nil {
+			errMsg := fmt.Sprintf("certificate file %s is not valid PEM/x509: %v", certPath, err)
+			c.logger.Error("validation failed", "error", err)
+			return &target.ValidationResult{
+				Valid:         false,
+				Serial:        request.Serial,
+				TargetAddress: certPath,
+				Message:       errMsg,
+				ValidatedAt:   time.Now(),
+			}, fmt.Errorf("%s", errMsg)
+		}
 	}

 	validationDuration := time.Since(startTime)
@@ -2,20 +2,64 @@ package caddy_test

 import (
 	"context"
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
+	"crypto/x509"
+	"crypto/x509/pkix"
 	"encoding/json"
+	"encoding/pem"
+	"fmt"
 	"io"
 	"log/slog"
+	"math/big"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strconv"
 	"strings"
+	"sync/atomic"
 	"testing"
+	"time"

 	"github.com/shankar0123/certctl/internal/connector/target"
 	"github.com/shankar0123/certctl/internal/connector/target/caddy"
 )

+// generateTestCertAndKey creates a self-signed cert + ECDSA key for tests
+// that exercise the file-mode PEM-validation path added in Bundle 9 (the
+// 2026-05-02 deployment-target audit). Pre-Bundle-9 the placeholder
+// "-----BEGIN CERTIFICATE-----\nMIIC...\n-----END CERTIFICATE-----" was
+// enough because ValidateDeployment only checked file existence; Fix 2
+// of Bundle 9 PEM-parses the file via certutil.ParseCertificatePEM, so
+// real test certs are required wherever the test deploys-then-validates.
+func generateTestCertAndKey(t *testing.T) (string, string) {
+	t.Helper()
+	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	if err != nil {
+		t.Fatalf("generate key: %v", err)
+	}
+	template := &x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject:      pkix.Name{CommonName: "caddy-test.example.com"},
+		NotBefore:    time.Now().Add(-1 * time.Hour),
+		NotAfter:     time.Now().Add(24 * time.Hour),
+		KeyUsage:     x509.KeyUsageDigitalSignature,
+	}
+	certDER, err := x509.CreateCertificate(rand.Reader, template, template, &key.PublicKey, key)
+	if err != nil {
+		t.Fatalf("create cert: %v", err)
+	}
+	certPEM := string(pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: certDER}))
+	keyDER, err := x509.MarshalPKCS8PrivateKey(key)
+	if err != nil {
+		t.Fatalf("marshal key: %v", err)
+	}
+	keyPEM := string(pem.EncodeToMemory(&pem.Block{Type: "PRIVATE KEY", Bytes: keyDER}))
+	return certPEM, keyPEM
+}
+
 func TestCaddyConnector_ValidateConfig_Success(t *testing.T) {
 	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
 	ctx := context.Background()
@@ -107,24 +151,37 @@ func TestCaddyConnector_DeployViaAPI_Success(t *testing.T) {
 	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
 	ctx := context.Background()

-	// Create a mock Caddy admin API server
+	// Create a mock Caddy admin API server.
+	//
+	// Bundle 9 Fix 3 of the 2026-05-02 deployment-target audit added an
+	// idempotency short-circuit: the connector now GETs the load endpoint
+	// first to compare the active cert hash with the deploy payload. The
+	// GET returns an empty array so the comparison falls through to the
+	// POST (which is what this test exercises).
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if strings.Contains(r.URL.Path, "/config/apps/tls/certificates/load") {
-			// Verify POST request with JSON body
-			if r.Method != "POST" {
-				t.Fatalf("expected POST, got %s", r.Method)
+			switch r.Method {
+			case "GET":
+				// Idempotency probe — return empty so the connector falls
+				// through to the POST path that this test asserts on.
+				w.Header().Set("Content-Type", "application/json")
+				w.WriteHeader(http.StatusOK)
+				w.Write([]byte("[]"))
+				return
+			case "POST":
+				body, _ := io.ReadAll(r.Body)
+				var payload map[string]string
+				json.Unmarshal(body, &payload)
+				if payload["cert"] == "" {
+					t.Fatal("cert field missing in payload")
+				}
+				if payload["key"] == "" {
+					t.Fatal("key field missing in payload")
+				}
+				w.WriteHeader(http.StatusOK)
+				return
 			}
-			body, _ := io.ReadAll(r.Body)
-			var payload map[string]string
-			json.Unmarshal(body, &payload)
-			if payload["cert"] == "" {
-				t.Fatal("cert field missing in payload")
-			}
-			if payload["key"] == "" {
-				t.Fatal("key field missing in payload")
-			}
-			w.WriteHeader(http.StatusOK)
-			return
+			t.Fatalf("unexpected method: %s", r.Method)
 		}
 		w.WriteHeader(http.StatusNotFound)
 	}))
@@ -298,11 +355,13 @@ func TestCaddyConnector_ValidateDeployment_Success(t *testing.T) {
 	rawConfig, _ := json.Marshal(cfg)
 	_ = connector.ValidateConfig(ctx, rawConfig)

-	// Deploy a certificate
+	// Bundle 9 Fix 2: ValidateDeployment now PEM-parses the cert file, so
+	// the deploy-then-validate flow needs a real test cert (placeholder
+	// "MIIC..." would fail the new PEM-parse check).
+	certPEM, keyPEM := generateTestCertAndKey(t)
 	deployRequest := target.DeploymentRequest{
-		CertPEM:  "-----BEGIN CERTIFICATE-----\nMIIC...\n-----END CERTIFICATE-----",
-		KeyPEM:   "-----BEGIN PRIVATE KEY-----\nMIIE...\n-----END PRIVATE KEY-----",
-		ChainPEM: "-----BEGIN CERTIFICATE-----\nMIIC...\n-----END CERTIFICATE-----",
+		CertPEM: certPEM,
+		KeyPEM:  keyPEM,
 	}
 	connector.DeployCertificate(ctx, deployRequest)

@@ -396,3 +455,330 @@ func TestCaddyConnector_APIMode_NoChain(t *testing.T) {
 		t.Fatalf("deployment should succeed, got: %s", result.Message)
 	}
 }
+
+// --- Bundle 9: duration metric + file-mode PEM validate + api-mode idempotency ---
+//
+// Six tests pin the three independent fixes added in Bundle 9 of the
+// 2026-05-02 deployment-target audit:
+//   - Fix 1 (duration metric L176): TestCaddy_API_DurationMetric_NonZero.
+//   - Fix 2 (file-mode PEM validate):
+//     TestCaddy_ValidateDeployment_FileMode_MalformedPEM_Rejected,
+//     TestCaddy_ValidateDeployment_FileMode_ValidPEM_Accepted.
+//   - Fix 3 (api-mode idempotency short-circuit):
+//     TestCaddy_API_Idempotent_SkipsPOSTWhenCertHashMatches,
+//     TestCaddy_API_Idempotent_RunsPOSTWhenCertHashDiffers,
+//     TestCaddy_API_Idempotent_GETFails_FallsThroughToPOST.
+
+func TestCaddy_API_DurationMetric_NonZero(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
+	ctx := context.Background()
+
+	// Bundle 9 Fix 1: pre-fix the api-mode duration_ms metric was
+	// computed as time.Since(time.Now()).Milliseconds() which always
+	// rounded to ~0ms. Post-fix it uses the startTime captured in
+	// DeployCertificate. Add a small artificial delay in the handler so
+	// the asserted duration_ms is unambiguously non-zero.
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !strings.Contains(r.URL.Path, "/config/apps/tls/certificates/load") {
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		switch r.Method {
+		case "GET":
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusOK)
+			w.Write([]byte("[]"))
+		case "POST":
+			// Simulate a slow Caddy admin reload — 10ms is enough to
+			// produce a measurable duration_ms.
+			time.Sleep(10 * time.Millisecond)
+			w.WriteHeader(http.StatusOK)
+		default:
+			w.WriteHeader(http.StatusMethodNotAllowed)
+		}
+	}))
+	defer server.Close()
+
+	cfg := caddy.Config{AdminAPI: server.URL, Mode: "api"}
+	connector := caddy.New(&cfg, logger)
+	rawConfig, _ := json.Marshal(cfg)
+	_ = connector.ValidateConfig(ctx, rawConfig)
+
+	certPEM, keyPEM := generateTestCertAndKey(t)
+	result, err := connector.DeployCertificate(ctx, target.DeploymentRequest{
+		CertPEM: certPEM,
+		KeyPEM:  keyPEM,
+	})
+	if err != nil {
+		t.Fatalf("deploy failed: %v", err)
+	}
+	if !result.Success {
+		t.Fatalf("expected success, got: %s", result.Message)
+	}
+
+	// duration_ms must parse as int >= 5 (we slept 10ms in the handler;
+	// allow some headroom for clock granularity on slow CI hosts).
+	durationStr := result.Metadata["duration_ms"]
+	if durationStr == "" {
+		t.Fatal("expected duration_ms in metadata")
+	}
+	durationMs, err := strconv.Atoi(durationStr)
+	if err != nil {
+		t.Fatalf("duration_ms is not int-parseable: %q (%v)", durationStr, err)
+	}
+	if durationMs < 5 {
+		t.Errorf("duration_ms = %d, expected >= 5 (handler slept 10ms; pre-Bundle-9 bug rounded this to 0)", durationMs)
+	}
+}
+
+func TestCaddy_ValidateDeployment_FileMode_MalformedPEM_Rejected(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
+	ctx := context.Background()
+
+	// Bundle 9 Fix 2: ValidateDeployment now PEM-parses the cert file
+	// (was only os.Stat existence check). A cert file containing garbage
+	// passes existence-check but fails PEM-decode → Valid=false.
+	tmpDir := t.TempDir()
+	certPath := filepath.Join(tmpDir, "cert.pem")
+	keyPath := filepath.Join(tmpDir, "key.pem")
+	if err := os.WriteFile(certPath, []byte("this is not a PEM cert"), 0644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(keyPath, []byte("this is not a key either"), 0600); err != nil {
+		t.Fatal(err)
+	}
+
+	cfg := caddy.Config{
+		AdminAPI: "http://localhost:2019",
+		CertDir:  tmpDir,
+		CertFile: "cert.pem",
+		KeyFile:  "key.pem",
+		Mode:     "file",
+	}
+	connector := caddy.New(&cfg, logger)
+	rawConfig, _ := json.Marshal(cfg)
+	_ = connector.ValidateConfig(ctx, rawConfig)
+
+	result, err := connector.ValidateDeployment(ctx, target.ValidationRequest{
+		CertificateID: "mc-test",
+		Serial:        "0xDEADBEEF",
+	})
+	if err == nil {
+		t.Fatal("expected error when cert file is malformed PEM")
+	}
+	if result.Valid {
+		t.Fatal("expected Valid=false for malformed PEM")
+	}
+	// Error message must reference the PEM/x509 failure so operators see
+	// what's wrong rather than a confusing downstream symptom.
+	if !strings.Contains(result.Message, "PEM") && !strings.Contains(result.Message, "x509") {
+		t.Errorf("expected error message to mention PEM/x509, got: %s", result.Message)
+	}
+}
+
+func TestCaddy_ValidateDeployment_FileMode_ValidPEM_Accepted(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
+	ctx := context.Background()
+
+	// Bundle 9 Fix 2: a real test cert + key passes both the os.Stat
+	// existence check and the new certutil.ParseCertificatePEM check.
+	tmpDir := t.TempDir()
+	certPath := filepath.Join(tmpDir, "cert.pem")
+	keyPath := filepath.Join(tmpDir, "key.pem")
+	certPEM, keyPEM := generateTestCertAndKey(t)
+	if err := os.WriteFile(certPath, []byte(certPEM), 0644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(keyPath, []byte(keyPEM), 0600); err != nil {
+		t.Fatal(err)
+	}
+
+	cfg := caddy.Config{
+		AdminAPI: "http://localhost:2019",
+		CertDir:  tmpDir,
+		CertFile: "cert.pem",
+		KeyFile:  "key.pem",
+		Mode:     "file",
+	}
+	connector := caddy.New(&cfg, logger)
+	rawConfig, _ := json.Marshal(cfg)
+	_ = connector.ValidateConfig(ctx, rawConfig)
+
+	result, err := connector.ValidateDeployment(ctx, target.ValidationRequest{
+		CertificateID: "mc-test",
+		Serial:        "1",
+	})
+	if err != nil {
+		t.Fatalf("ValidateDeployment failed unexpectedly: %v (msg: %s)", err, result.Message)
+	}
+	if !result.Valid {
+		t.Errorf("expected Valid=true for a real PEM cert, got: %s", result.Message)
+	}
+}
+
+func TestCaddy_API_Idempotent_SkipsPOSTWhenCertHashMatches(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
+	ctx := context.Background()
+
+	// Bundle 9 Fix 3: the connector GETs first, hashes the active cert,
+	// and skips the POST when SHA-256 matches. Build the deploy payload
+	// (cert + trailing newline; no chain in this test) and seed the mock
+	// GET response with an identical cert string so the hash matches and
+	// the POST counter stays at 0.
+	certPEM, keyPEM := generateTestCertAndKey(t)
+	expectedCertField := certPEM + "\n" // matches deployViaAPI's "request.CertPEM + \"\\n\"" build step
+
+	var postCount, getCount atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !strings.Contains(r.URL.Path, "/config/apps/tls/certificates/load") {
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		switch r.Method {
+		case "GET":
+			getCount.Add(1)
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusOK)
+			payload, _ := json.Marshal([]map[string]string{
+				{"cert": expectedCertField, "key": keyPEM},
+			})
+			w.Write(payload)
+		case "POST":
+			postCount.Add(1)
+			w.WriteHeader(http.StatusOK)
+		default:
+			w.WriteHeader(http.StatusMethodNotAllowed)
+		}
+	}))
+	defer server.Close()
+
+	cfg := caddy.Config{AdminAPI: server.URL, Mode: "api"}
+	connector := caddy.New(&cfg, logger)
+	rawConfig, _ := json.Marshal(cfg)
+	_ = connector.ValidateConfig(ctx, rawConfig)
+
+	result, err := connector.DeployCertificate(ctx, target.DeploymentRequest{
+		CertPEM: certPEM,
+		KeyPEM:  keyPEM,
+	})
+	if err != nil {
+		t.Fatalf("deploy failed: %v", err)
+	}
+	if !result.Success {
+		t.Fatalf("expected success, got: %s", result.Message)
+	}
+	if got := postCount.Load(); got != 0 {
+		t.Errorf("expected 0 POST calls (idempotent skip), got %d", got)
+	}
+	if got := getCount.Load(); got != 1 {
+		t.Errorf("expected exactly 1 GET call (idempotency probe), got %d", got)
+	}
+	if result.Metadata["idempotent"] != "true" {
+		t.Errorf("expected metadata.idempotent=true on the skip path, got: %q", result.Metadata["idempotent"])
+	}
+}
+
+func TestCaddy_API_Idempotent_RunsPOSTWhenCertHashDiffers(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
+	ctx := context.Background()
+
+	// Bundle 9 Fix 3: when the GET response's cert hash differs from
+	// the deploy payload, fall through to the POST. metadata.idempotent
+	// must NOT be set on the POST path (only on the skip path).
+	certPEM, keyPEM := generateTestCertAndKey(t)
+	differentCert, _ := generateTestCertAndKey(t) // a DIFFERENT cert in the GET response
+
+	var postCount atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !strings.Contains(r.URL.Path, "/config/apps/tls/certificates/load") {
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		switch r.Method {
+		case "GET":
+			w.Header().Set("Content-Type", "application/json")
+			w.WriteHeader(http.StatusOK)
+			payload, _ := json.Marshal([]map[string]string{
+				{"cert": differentCert, "key": "different-key"},
+			})
+			w.Write(payload)
+		case "POST":
+			postCount.Add(1)
+			w.WriteHeader(http.StatusOK)
+		}
+	}))
+	defer server.Close()
+
+	cfg := caddy.Config{AdminAPI: server.URL, Mode: "api"}
+	connector := caddy.New(&cfg, logger)
+	rawConfig, _ := json.Marshal(cfg)
+	_ = connector.ValidateConfig(ctx, rawConfig)
+
+	result, err := connector.DeployCertificate(ctx, target.DeploymentRequest{
+		CertPEM: certPEM,
+		KeyPEM:  keyPEM,
+	})
+	if err != nil {
+		t.Fatalf("deploy failed: %v", err)
+	}
+	if !result.Success {
+		t.Fatalf("expected success, got: %s", result.Message)
+	}
+	if got := postCount.Load(); got != 1 {
+		t.Errorf("expected exactly 1 POST call (cert-hash mismatch fell through), got %d", got)
+	}
+	if result.Metadata["idempotent"] == "true" {
+		t.Error("expected metadata.idempotent absent or false on the non-idempotent POST path")
+	}
+}
+
+func TestCaddy_API_Idempotent_GETFails_FallsThroughToPOST(t *testing.T) {
+	logger := slog.New(slog.NewTextHandler(os.Stdout, &slog.HandlerOptions{Level: slog.LevelDebug}))
+	ctx := context.Background()
+
+	// Bundle 9 Fix 3: idempotency is best-effort. A GET that returns 500
+	// (or 404, or a malformed JSON body, or a network error) silently
+	// falls through to the POST so deploys never get blocked by a
+	// misbehaving admin endpoint.
+	certPEM, keyPEM := generateTestCertAndKey(t)
+
+	var postCount atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !strings.Contains(r.URL.Path, "/config/apps/tls/certificates/load") {
+			w.WriteHeader(http.StatusNotFound)
+			return
+		}
+		switch r.Method {
+		case "GET":
+			// Return 500 — connector should fall through to POST.
+			w.WriteHeader(http.StatusInternalServerError)
+			fmt.Fprint(w, "internal error")
+		case "POST":
+			postCount.Add(1)
+			w.WriteHeader(http.StatusOK)
+		}
+	}))
+	defer server.Close()
+
+	cfg := caddy.Config{AdminAPI: server.URL, Mode: "api"}
+	connector := caddy.New(&cfg, logger)
+	rawConfig, _ := json.Marshal(cfg)
+	_ = connector.ValidateConfig(ctx, rawConfig)
+
+	result, err := connector.DeployCertificate(ctx, target.DeploymentRequest{
+		CertPEM: certPEM,
+		KeyPEM:  keyPEM,
+	})
+	if err != nil {
+		t.Fatalf("deploy failed: %v", err)
+	}
+	if !result.Success {
+		t.Fatalf("expected success, got: %s", result.Message)
+	}
+	if got := postCount.Load(); got != 1 {
+		t.Errorf("expected exactly 1 POST call (GET failure fell through), got %d", got)
+	}
+	if result.Metadata["idempotent"] == "true" {
+		t.Error("expected metadata.idempotent absent on the fallthrough POST path")
+	}
+}