Files
certctl/internal/service/scep_test.go
T
shankar0123 b813660c74 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-04-16 22:22:51 +00:00

228 lines
8.4 KiB
Go

package service
import (
"context"
"errors"
"log/slog"
"os"
"strings"
"testing"
)
func TestSCEPService_GetCACaps(t *testing.T) {
mockIssuer := &mockIssuerConnector{}
svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
caps := svc.GetCACaps(context.Background())
if caps == "" {
t.Error("expected non-empty capabilities")
}
if !strings.Contains(caps, "POSTPKIOperation") {
t.Errorf("expected POSTPKIOperation in caps, got: %s", caps)
}
if !strings.Contains(caps, "SHA-256") {
t.Errorf("expected SHA-256 in caps, got: %s", caps)
}
if !strings.Contains(caps, "SCEPStandard") {
t.Errorf("expected SCEPStandard in caps, got: %s", caps)
}
}
func TestSCEPService_GetCACert_Success(t *testing.T) {
mockIssuer := &mockIssuerConnector{}
svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
caPEM, err := svc.GetCACert(context.Background())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if caPEM == "" {
t.Error("expected non-empty CA PEM")
}
}
func TestSCEPService_GetCACert_IssuerError(t *testing.T) {
mockIssuer := &mockIssuerConnector{Err: errors.New("CA unavailable")}
svc := NewSCEPService("iss-local", mockIssuer, nil, slog.New(slog.NewTextHandler(os.Stderr, &slog.HandlerOptions{Level: slog.LevelError})), "")
_, err := svc.GetCACert(context.Background())
if err == nil {
t.Fatal("expected error")
}
if !strings.Contains(err.Error(), "CA unavailable") {
t.Errorf("expected error to contain 'CA unavailable', got: %v", err)
}
}
func TestSCEPService_PKCSReq_Success(t *testing.T) {
mockIssuer := &mockIssuerConnector{}
auditRepo := newMockAuditRepository()
auditSvc := NewAuditService(auditRepo)
// 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, "secret123", "txn-001")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if result == nil {
t.Fatal("expected non-nil result")
}
if result.CertPEM == "" {
t.Error("expected non-empty CertPEM")
}
// Verify audit event was recorded
if len(auditRepo.Events) == 0 {
t.Error("expected audit event to be recorded")
}
}
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})), "secret123")
_, err := svc.PKCSReq(context.Background(), "not-valid-pem", "secret123", "txn-002")
if err == nil {
t.Fatal("expected error for invalid CSR")
}
}
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})), "secret123")
csrPEM := generateCSRPEM(t, "", []string{"test.example.com"})
_, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-003")
if err == nil {
t.Fatal("expected error for missing CN")
}
if !strings.Contains(err.Error(), "Common Name") {
t.Errorf("expected 'Common Name' in error, got: %v", err)
}
}
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})), "secret123")
csrPEM := generateCSRPEM(t, "test.example.com", nil)
_, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-004")
if err == nil {
t.Fatal("expected error")
}
if !strings.Contains(err.Error(), "issuance failed") {
t.Errorf("expected 'issuance failed', got: %v", err)
}
}
func TestSCEPService_PKCSReq_ChallengePassword_Valid(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})), "secret123")
csrPEM := generateCSRPEM(t, "mdm-device.example.com", nil)
result, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-005")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if result == nil {
t.Fatal("expected non-nil result")
}
}
func TestSCEPService_PKCSReq_ChallengePassword_Invalid(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, "mdm-device.example.com", nil)
_, err := svc.PKCSReq(context.Background(), csrPEM, "wrong-password", "txn-006")
if err == nil {
t.Fatal("expected error for invalid challenge password")
}
if !strings.Contains(err.Error(), "challenge password") {
t.Errorf("expected 'challenge password' in error, got: %v", err)
}
}
// 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)
// 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)
}
}
}
// 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)
}
}
}
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})), "secret123")
svc.SetProfileID("profile-mdm-device")
csrPEM := generateCSRPEM(t, "device.example.com", nil)
result, err := svc.PKCSReq(context.Background(), csrPEM, "secret123", "txn-008")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if result == nil {
t.Fatal("expected non-nil result")
}
// Verify audit event includes profile_id
if len(auditRepo.Events) == 0 {
t.Fatal("expected audit event")
}
lastEvent := auditRepo.Events[len(auditRepo.Events)-1]
if lastEvent.Details == nil {
t.Fatal("expected audit details")
}
}