diff --git a/cmd/server/main.go b/cmd/server/main.go index fa1c08e..51e73b6 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -483,6 +483,24 @@ func main() { // Register SCEP (RFC 8894) handlers if enabled if cfg.SCEP.Enabled { + // H-2 fix: fail closed at startup when SCEP is enabled without a + // challenge password configured. Previously the service-layer guard + // at internal/service/scep.go:72-79 skipped the password check when + // s.challengePassword == "", meaning any client that could reach the + // /scep endpoint could enroll an arbitrary CSR against the configured + // issuer (CWE-306, missing authentication for a critical function). + // Refuse to start instead: the operator must set + // CERTCTL_SCEP_CHALLENGE_PASSWORD (or disable SCEP) before the control + // plane can boot. + if err := preflightSCEPChallengePassword(cfg.SCEP.Enabled, cfg.SCEP.ChallengePassword); err != nil { + logger.Error( + "startup refused: SCEP is enabled but CERTCTL_SCEP_CHALLENGE_PASSWORD is not set "+ + "(would allow unauthenticated certificate enrollment, CWE-306). "+ + "Set a non-empty challenge password or disable SCEP before restarting.", + "error", err, + ) + os.Exit(1) + } issuerConn, ok := issuerRegistry.Get(cfg.SCEP.IssuerID) if !ok { logger.Error("SCEP issuer not found in registry", "issuer_id", cfg.SCEP.IssuerID) @@ -683,3 +701,23 @@ func main() { logger.Info("certctl server stopped") } +// preflightSCEPChallengePassword enforces the H-2 fix: if SCEP is enabled, a +// non-empty challenge password MUST be configured. Returns a non-nil error +// otherwise so the caller can refuse to start the control plane (CWE-306, +// missing authentication for a critical function). +// +// This helper is extracted so the check can be unit tested without booting +// the full server. The caller (main) is responsible for translating the +// returned error into a structured log line and os.Exit(1). +func preflightSCEPChallengePassword(enabled bool, challengePassword string) error { + if !enabled { + return nil + } + if challengePassword == "" { + return fmt.Errorf("SCEP enabled but CERTCTL_SCEP_CHALLENGE_PASSWORD is empty: " + + "SCEP enrollment would accept any client (CWE-306); " + + "configure a non-empty shared secret or set CERTCTL_SCEP_ENABLED=false") + } + return nil +} + diff --git a/cmd/server/main_test.go b/cmd/server/main_test.go index 56ad92c..4bfb801 100644 --- a/cmd/server/main_test.go +++ b/cmd/server/main_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" "github.com/shankar0123/certctl/internal/api/middleware" @@ -538,3 +539,68 @@ func TestMain_ContextPropagation(t *testing.T) { t.Logf("Context value may not be propagated (status %d), this may be expected", w.Code) } } + +// TestPreflightSCEPChallengePassword is the H-2 regression guard for the +// startup pre-flight check. The helper MUST return a non-nil error whenever +// SCEP is enabled with an empty challenge password — that configuration +// previously allowed unauthenticated certificate enrollment (CWE-306). +// Disabled-SCEP and configured-password cases must pass cleanly. +func TestPreflightSCEPChallengePassword(t *testing.T) { + tests := []struct { + name string + enabled bool + challengePassword string + wantErr bool + wantErrSubstring string + }{ + { + name: "disabled_empty_password_ok", + enabled: false, + challengePassword: "", + wantErr: false, + }, + { + name: "disabled_with_password_ok", + enabled: false, + challengePassword: "leftover-value", + wantErr: false, + }, + { + name: "enabled_empty_password_rejected", + enabled: true, + challengePassword: "", + wantErr: true, + wantErrSubstring: "CERTCTL_SCEP_CHALLENGE_PASSWORD", + }, + { + name: "enabled_with_password_ok", + enabled: true, + challengePassword: "hunter2", + wantErr: false, + }, + { + name: "enabled_single_char_password_ok", + enabled: true, + challengePassword: "x", + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := preflightSCEPChallengePassword(tt.enabled, tt.challengePassword) + if tt.wantErr { + if err == nil { + t.Fatalf("expected error, got nil") + } + if tt.wantErrSubstring != "" && !strings.Contains(err.Error(), tt.wantErrSubstring) { + t.Errorf("expected error to mention %q, got: %v", tt.wantErrSubstring, err) + } + if !strings.Contains(err.Error(), "CWE-306") { + t.Errorf("expected error to cite CWE-306 for traceability, got: %v", err) + } + } else if err != nil { + t.Errorf("expected no error, got: %v", err) + } + }) + } +} diff --git a/internal/config/config.go b/internal/config/config.go index d98ef1b..217f8dc 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -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 } diff --git a/internal/service/m11c_crypto_enforcement_test.go b/internal/service/m11c_crypto_enforcement_test.go index cffa882..cb5121b 100644 --- a/internal/service/m11c_crypto_enforcement_test.go +++ b/internal/service/m11c_crypto_enforcement_test.go @@ -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) } diff --git a/internal/service/scep.go b/internal/service/scep.go index 48dec04..2e8d4b9 100644 --- a/internal/service/scep.go +++ b/internal/service/scep.go @@ -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") diff --git a/internal/service/scep_test.go b/internal/service/scep_test.go index f99784f..dbd3d10 100644 --- a/internal/service/scep_test.go +++ b/internal/service/scep_test.go @@ -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) }