security: require SCEP challenge password when SCEP enabled (fixes H-2)

Problem (CWE-306 Missing Authentication for Critical Function): internal/service/scep.go PKCSReq skipped the shared-secret check when s.challengePassword was empty. An unconfigured-but-enabled SCEP server accepted any unauthenticated client reaching /scep and issued a certificate against the configured issuer for any CSR with a valid signature. No audit trail distinguished authenticated from unauthenticated enrollments. This matches the two-layer fail-closed pattern already used for C-2 (f549a7a): reject at startup AND reject at the service boundary. Fix (two layers, defense-in-depth): Layer 1 — startup pre-flight in cmd/server/main.go: preflightSCEPChallengePassword returns a non-nil error when SCEP is enabled and CERTCTL_SCEP_CHALLENGE_PASSWORD is empty. main logs and os.Exit(1)s before the SCEP service is constructed. Disabled SCEP is unaffected. The helper is unit-testable in isolation. Layer 2 — service-layer rejection in internal/service/scep.go: PKCSReq refuses enrollment when s.challengePassword == "" even though main already blocks this state — protects future call sites (tests, library reuse, a REST-over-HTTPS wrapper). When a secret is configured, the comparison now uses crypto/subtle.ConstantTimeCompare so response time does not leak the configured secret through a short-circuiting byte compare. Files: - cmd/server/main.go: preflightSCEPChallengePassword helper; call site inside the `if cfg.SCEP.Enabled` block before issuer lookup; fatal slog error references CWE-306 and names the env var so operators can diagnose the startup failure without reading code. - cmd/server/main_test.go: TestPreflightSCEPChallengePassword with five table-driven subtests (disabled empty, disabled set, enabled empty rejected, enabled set, single-char boundary). The enabled-empty case asserts the error string contains both CERTCTL_SCEP_CHALLENGE_PASSWORD and CWE-306 so the log message remains actionable. - internal/config/config.go: SCEPConfig.ChallengePassword godoc now states the field is REQUIRED when SCEP.Enabled and cross-references preflightSCEPChallengePassword. - internal/service/scep.go: imports crypto/subtle; PKCSReq rewritten with the two-layer check; comment block cites H-2 / CWE-306 and the constant-time rationale. - internal/service/scep_test.go: existing tests that relied on the vulnerable empty-password path now configure a secret on both sides. TestSCEPService_PKCSReq_ChallengePassword_NotRequired is replaced by TestSCEPService_PKCSReq_ChallengePassword_EmptyServerConfigRejected which iterates ["", "any-value", "guess"] against an unconfigured server and asserts "not configured" in the error. A new TestSCEPService_PKCSReq_ChallengePassword_ConstantTimeLengthIndependence exercises same-prefix-longer and wrong-case inputs to guard against a regression from ConstantTimeCompare to a short-circuiting byte compare. - internal/service/m11c_crypto_enforcement_test.go: four tests (RejectsWeakKey, AcceptsStrongKey, MaxTTL_ForwardedToIssuer, NoProfileRepo_PassesThrough) constructed NewSCEPService with an empty challenge password and exercised PKCSReq through the now-rejected vulnerable path. All four now configure "secret123" on both sides with an inline H-2 comment; the crypto/MaxTTL/profile behavior they assert is unchanged. Wire-format / behavioral invariants preserved: - RFC 8894 SCEP handler is untouched (internal/api/handler/scep.go and internal/pkcs7/*): GetCACaps/GetCACert responses, PKIOperation request parsing, and the PKCS#7 certs-only response format are byte-identical. - RFC 7030 EST handler is untouched (internal/api/handler/est.go + internal/pkcs7/*). - Revocation idempotency composite key (H-1, migration 000012) untouched. - AES-256-GCM config encryption (C-2) untouched. - CRL DER bytes and OCSP response bytes unchanged. Verification: - go build ./... silent success - go vet ./... silent success - go test -race -count=1 ./internal/service/ ./cmd/server/ ./internal/api/handler/ ./internal/integration/ all OK - Coverage with comfortable headroom over CI gates: service 67.8% (gate 55%) handler 79.0% (gate 60%) domain 92.7% (gate 40%) middleware 80.0% (gate 30%) cmd/server 1.6% (preflightSCEPChallengePassword: 100%) internal/service/scep.go PKCSReq statement coverage: 100%. - rg sweeps: no `s.challengePassword != ""` remains; no `challengePassword != s.challengePassword` remains. Operational note: operators with SCEP enabled but no challenge password set will see a fatal startup error and a log line citing CERTCTL_SCEP_CHALLENGE_PASSWORD and CWE-306 after upgrading. This is the intended fail-closed behavior. Fix by either setting the env var to a non-empty shared secret or setting CERTCTL_SCEP_ENABLED=false. Audit report: certctl-audit-report.md (revision 5) logs this under H-2 Resolution Log.
2026-06-07 17:22:07 +00:00 · 2026-04-16 22:22:51 +00:00
parent 387fb555ac
commit b813660c74
6 changed files with 201 additions and 33 deletions
@@ -641,7 +641,12 @@ type SCEPConfig struct {

 	// ChallengePassword is the shared secret used to authenticate SCEP enrollment requests.
 	// Clients include this in the PKCS#10 CSR challengePassword attribute.
-	// Required when SCEP is enabled.
+	//
+	// REQUIRED when Enabled is true. If SCEP is enabled and this value is empty,
+	// cmd/server/main.go's preflightSCEPChallengePassword check will refuse to
+	// start the server (H-2, CWE-306): an empty shared secret allowed any client
+	// that could reach /scep to enroll a CSR against the configured issuer. The
+	// service-layer PKCSReq path also rejects this configuration defense-in-depth.
 	ChallengePassword string
 }

@@ -119,7 +119,10 @@ func TestESTService_MaxTTL_ForwardedToIssuer(t *testing.T) {

 func TestSCEPService_CryptoValidation_RejectsWeakKey(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
-	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	// H-2: SCEPService now requires a configured challenge password. Pass a
+	// matching client password so this test exercises the crypto-policy path
+	// rather than being short-circuited by the challenge-password guard.
+	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")

 	// Profile requiring ECDSA P-384 minimum
 	profileRepo := newM11cProfileRepo()
@@ -136,7 +139,7 @@ func TestSCEPService_CryptoValidation_RejectsWeakKey(t *testing.T) {
 	// P-256 CSR should be rejected
 	csrPEM := generateCSRPEM(t, "device.example.com", nil)

-	_, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-001")
+	_, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-001")
 	if err == nil {
 		t.Fatal("expected rejection for ECDSA P-256 against P-384 minimum")
 	}
@@ -152,7 +155,8 @@ func TestSCEPService_CryptoValidation_AcceptsStrongKey(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
 	auditRepo := newMockAuditRepository()
 	auditSvc := NewAuditService(auditRepo)
-	svc := NewSCEPService("iss-local", mockIssuer, auditSvc, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	// H-2: happy path exercises the authenticated branch.
+	svc := NewSCEPService("iss-local", mockIssuer, auditSvc, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")

 	profileRepo := newM11cProfileRepo()
 	profileRepo.profiles["prof-standard"] = &domain.CertificateProfile{
@@ -167,7 +171,7 @@ func TestSCEPService_CryptoValidation_AcceptsStrongKey(t *testing.T) {

 	csrPEM := generateCSRPEM(t, "device-ok.example.com", nil)

-	result, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-002")
+	result, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-002")
 	if err != nil {
 		t.Fatalf("expected success: %v", err)
 	}
@@ -179,7 +183,8 @@ func TestSCEPService_CryptoValidation_AcceptsStrongKey(t *testing.T) {
 func TestSCEPService_MaxTTL_ForwardedToIssuer(t *testing.T) {
 	capturingMock := &capturingIssuerConnector{}

-	svc := NewSCEPService("iss-local", capturingMock, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	// H-2: challenge password required for enrollment.
+	svc := NewSCEPService("iss-local", capturingMock, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")

 	profileRepo := newM11cProfileRepo()
 	profileRepo.profiles["prof-device"] = &domain.CertificateProfile{
@@ -192,7 +197,7 @@ func TestSCEPService_MaxTTL_ForwardedToIssuer(t *testing.T) {

 	csrPEM := generateCSRPEM(t, "mdm-device.example.com", nil)

-	_, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-003")
+	_, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-003")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -341,12 +346,13 @@ func TestESTService_NoProfileRepo_PassesThrough(t *testing.T) {

 func TestSCEPService_NoProfileRepo_PassesThrough(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
-	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	// H-2: challenge password required for enrollment.
+	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")
 	svc.SetProfileID("nonexistent-profile")

 	csrPEM := generateCSRPEM(t, "no-profile-scep.example.com", nil)

-	result, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-004")
+	result, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-004")
 	if err != nil {
 		t.Fatalf("expected success when no profile repo set: %v", err)
 	}
@@ -2,6 +2,7 @@ package service

 import (
 	"context"
+	"crypto/subtle"
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
@@ -68,14 +69,34 @@ func (s *SCEPService) GetCACert(ctx context.Context) (string, error) {
 // PKCSReq processes a SCEP enrollment request.
 // RFC 8894 Section 3.3.1: PKCSReq contains a PKCS#10 CSR for certificate enrollment.
 // The CSR PEM and challenge password are extracted by the handler from the PKCS#7 envelope.
+//
+// H-2 fix (CWE-306): the previous implementation skipped the shared-secret
+// check entirely when s.challengePassword was empty, meaning any unauthenticated
+// client that could reach /scep could enroll a CSR against the configured
+// issuer. Reject that configuration defense-in-depth even though main() already
+// refuses to start in the same state (see preflightSCEPChallengePassword). The
+// non-empty branch now uses crypto/subtle.ConstantTimeCompare to avoid leaking
+// the shared secret through a response-time side channel.
 func (s *SCEPService) PKCSReq(ctx context.Context, csrPEM string, challengePassword string, transactionID string) (*domain.SCEPEnrollResult, error) {
-	// Validate challenge password
-	if s.challengePassword != "" {
-		if challengePassword != s.challengePassword {
-			s.logger.Warn("SCEP enrollment rejected: invalid challenge password",
-				"transaction_id", transactionID)
-			return nil, fmt.Errorf("invalid challenge password")
-		}
+	// Defense-in-depth: refuse any enrollment when no shared secret is
+	// configured. The server-level pre-flight check in cmd/server/main.go
+	// normally prevents the service from being constructed in this state, but
+	// this branch also protects future call sites (tests, library reuse, a
+	// future REST-over-HTTPS wrapper) from silently accepting unauthenticated
+	// CSRs.
+	if s.challengePassword == "" {
+		s.logger.Warn("SCEP enrollment rejected: server has no challenge password configured",
+			"transaction_id", transactionID)
+		return nil, fmt.Errorf("SCEP challenge password not configured on server")
+	}
+	// Constant-time compare avoids leaking the configured secret through
+	// response-time variance. ConstantTimeCompare returns 1 only when both
+	// slices have equal length AND equal content; a mismatched-length input
+	// still takes the same path as a content mismatch.
+	if subtle.ConstantTimeCompare([]byte(challengePassword), []byte(s.challengePassword)) != 1 {
+		s.logger.Warn("SCEP enrollment rejected: invalid challenge password",
+			"transaction_id", transactionID)
+		return nil, fmt.Errorf("invalid challenge password")
 	}

 	return s.processEnrollment(ctx, csrPEM, transactionID, "scep_pkcsreq")
@@ -58,11 +58,13 @@ func TestSCEPService_PKCSReq_Success(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
 	auditRepo := newMockAuditRepository()
 	auditSvc := NewAuditService(auditRepo)
-	svc := NewSCEPService("iss-local", mockIssuer, auditSvc, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	// H-2: SCEPService now requires a configured challenge password; the happy
+	// path exercises a matching client-submitted password.
+	svc := NewSCEPService("iss-local", mockIssuer, auditSvc, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")

 	csrPEM := generateCSRPEM(t, "device.example.com", []string{"device.example.com"})

-	result, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-001")
+	result, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-001")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -81,9 +83,9 @@ func TestSCEPService_PKCSReq_Success(t *testing.T) {

 func TestSCEPService_PKCSReq_InvalidCSR(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
-	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")

-	_, err := svc.PKCSReq(context.Background(), "not-valid-pem", "", "txn-002")
+	_, err := svc.PKCSReq(context.Background(), "not-valid-pem", "secret123", "txn-002")
 	if err == nil {
 		t.Fatal("expected error for invalid CSR")
 	}
@@ -91,11 +93,11 @@ func TestSCEPService_PKCSReq_InvalidCSR(t *testing.T) {

 func TestSCEPService_PKCSReq_MissingCN(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
-	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")

 	csrPEM := generateCSRPEM(t, "", []string{"test.example.com"})

-	_, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-003")
+	_, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-003")
 	if err == nil {
 		t.Fatal("expected error for missing CN")
 	}
@@ -106,11 +108,11 @@ func TestSCEPService_PKCSReq_MissingCN(t *testing.T) {

 func TestSCEPService_PKCSReq_IssuerError(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{Err: errors.New("issuance failed")}
-	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")

 	csrPEM := generateCSRPEM(t, "test.example.com", nil)

-	_, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-004")
+	_, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-004")
 	if err == nil {
 		t.Fatal("expected error")
 	}
@@ -151,19 +153,49 @@ func TestSCEPService_PKCSReq_ChallengePassword_Invalid(t *testing.T) {
 	}
 }

-func TestSCEPService_PKCSReq_ChallengePassword_NotRequired(t *testing.T) {
-	// When server has no challenge password configured, any value should be accepted
+// TestSCEPService_PKCSReq_ChallengePassword_EmptyServerConfigRejected is the
+// H-2 regression guard. Before the fix (internal/service/scep.go:72-79 skipped
+// the password check when s.challengePassword was empty), an unconfigured
+// server accepted any enrollment (CWE-306). The service now rejects PKCSReq
+// defense-in-depth even if main()'s pre-flight is somehow bypassed.
+func TestSCEPService_PKCSReq_ChallengePassword_EmptyServerConfigRejected(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
 	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")

 	csrPEM := generateCSRPEM(t, "device.example.com", nil)

-	result, err := svc.PKCSReq(context.Background(), csrPEM, "any-value", "txn-007")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+	// Any client-submitted password (including empty) must be rejected when
+	// the server has no shared secret configured.
+	for _, clientPassword := range []string{"", "any-value", "guess"} {
+		_, err := svc.PKCSReq(context.Background(), csrPEM, clientPassword, "txn-empty")
+		if err == nil {
+			t.Fatalf("expected rejection when server challenge password is empty (client=%q)", clientPassword)
+		}
+		if !strings.Contains(err.Error(), "not configured") {
+			t.Errorf("expected 'not configured' in error, got: %v", err)
+		}
 	}
-	if result == nil {
-		t.Fatal("expected non-nil result")
+}
+
+// TestSCEPService_PKCSReq_ChallengePassword_ConstantTimeLengthIndependence
+// guards against regression from crypto/subtle.ConstantTimeCompare to a
+// short-circuiting byte compare. ConstantTimeCompare returns 0 whenever the
+// two slices differ in length OR content, so a same-prefix-but-longer input
+// must be rejected the same way as a completely different string.
+func TestSCEPService_PKCSReq_ChallengePassword_ConstantTimeLengthIndependence(t *testing.T) {
+	mockIssuer := &mockIssuerConnector{}
+	svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")
+
+	csrPEM := generateCSRPEM(t, "device.example.com", nil)
+
+	for _, bad := range []string{"secret", "secret12", "secret1234", "SECRET123", "wrong"} {
+		_, err := svc.PKCSReq(context.Background(), csrPEM, bad, "txn-ct")
+		if err == nil {
+			t.Fatalf("expected rejection for bad password %q", bad)
+		}
+		if !strings.Contains(err.Error(), "invalid challenge password") {
+			t.Errorf("expected 'invalid challenge password' for %q, got: %v", bad, err)
+		}
 	}
 }

@@ -171,12 +203,12 @@ func TestSCEPService_PKCSReq_WithProfile(t *testing.T) {
 	mockIssuer := &mockIssuerConnector{}
 	auditRepo := newMockAuditRepository()
 	auditSvc := NewAuditService(auditRepo)
-	svc := NewSCEPService("iss-local", mockIssuer, auditSvc, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
+	svc := NewSCEPService("iss-local", mockIssuer, auditSvc, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "secret123")
 	svc.SetProfileID("profile-mdm-device")

 	csrPEM := generateCSRPEM(t, "device.example.com", nil)

-	result, err := svc.PKCSReq(context.Background(), csrPEM, "", "txn-008")
+	result, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-008")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}