diff --git a/api/openapi.yaml b/api/openapi.yaml index 65140e8..a205b54 100644 --- a/api/openapi.yaml +++ b/api/openapi.yaml @@ -3461,6 +3461,7 @@ components: - RequiredMetadata - AllowedEnvironments - RenewalLeadTime + - CertificateLifetime PolicySeverity: type: string diff --git a/cmd/server/main.go b/cmd/server/main.go index 4c75059..fcd87bf 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -145,6 +145,7 @@ func main() { // Initialize services (following the dependency graph) auditService := service.NewAuditService(auditRepo) policyService := service.NewPolicyService(policyRepo, auditService) + policyService.SetCertRepo(certificateRepo) // D-008: CertificateLifetime arm needs CertificateVersion.NotBefore/NotAfter certificateService := service.NewCertificateService(certificateRepo, policyService, auditService) notifierRegistry := make(map[string]service.Notifier) diff --git a/internal/api/handler/validation.go b/internal/api/handler/validation.go index bc87eeb..d52d41a 100644 --- a/internal/api/handler/validation.go +++ b/internal/api/handler/validation.go @@ -71,10 +71,11 @@ func ValidatePolicyType(policyType interface{}) error { "RequiredMetadata": true, "AllowedEnvironments": true, "RenewalLeadTime": true, + "CertificateLifetime": true, } typeStr := fmt.Sprintf("%v", policyType) if !validTypes[typeStr] { - return ValidationError{Field: "type", Message: "type must be one of: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime"} + return ValidationError{Field: "type", Message: "type must be one of: AllowedIssuers, AllowedDomains, RequiredMetadata, AllowedEnvironments, RenewalLeadTime, CertificateLifetime"} } return nil } diff --git a/internal/domain/policy.go b/internal/domain/policy.go index 312a24c..af0dd08 100644 --- a/internal/domain/policy.go +++ b/internal/domain/policy.go @@ -21,11 +21,12 @@ type PolicyRule struct { type PolicyType string const ( - PolicyTypeAllowedIssuers PolicyType = "AllowedIssuers" - PolicyTypeAllowedDomains PolicyType = "AllowedDomains" - PolicyTypeRequiredMetadata PolicyType = "RequiredMetadata" - PolicyTypeAllowedEnvironments PolicyType = "AllowedEnvironments" - PolicyTypeRenewalLeadTime PolicyType = "RenewalLeadTime" + PolicyTypeAllowedIssuers PolicyType = "AllowedIssuers" + PolicyTypeAllowedDomains PolicyType = "AllowedDomains" + PolicyTypeRequiredMetadata PolicyType = "RequiredMetadata" + PolicyTypeAllowedEnvironments PolicyType = "AllowedEnvironments" + PolicyTypeRenewalLeadTime PolicyType = "RenewalLeadTime" + PolicyTypeCertificateLifetime PolicyType = "CertificateLifetime" ) // PolicyViolation records an instance of a certificate violating a policy rule. diff --git a/internal/service/policy.go b/internal/service/policy.go index e411791..e090308 100644 --- a/internal/service/policy.go +++ b/internal/service/policy.go @@ -2,8 +2,10 @@ package service import ( "context" + "encoding/json" "fmt" "log/slog" + "strings" "time" "github.com/shankar0123/certctl/internal/domain" @@ -14,6 +16,11 @@ import ( type PolicyService struct { policyRepo repository.PolicyRepository auditService *AuditService + // certRepo is optional and only required by the CertificateLifetime rule + // arm, which must read NotBefore/NotAfter from the latest CertificateVersion. + // Wire via SetCertRepo after construction; rules other than + // CertificateLifetime operate without it. + certRepo repository.CertificateRepository } // NewPolicyService creates a new policy service. @@ -27,6 +34,16 @@ func NewPolicyService( } } +// SetCertRepo wires the certificate repository needed for the CertificateLifetime +// rule arm. Kept as a setter (not a constructor parameter) so the ~36 existing +// NewPolicyService call sites don't churn for a single new arm's dependency. +// Safe to call before or after construction; evaluateRule checks for nil and +// returns an error if a CertificateLifetime rule fires without a wired repo +// (the caller at ValidateCertificate logs and continues). +func (s *PolicyService) SetCertRepo(r repository.CertificateRepository) { + s.certRepo = r +} + // ValidateCertificate runs all enabled policy rules against a certificate. func (s *PolicyService) ValidateCertificate(ctx context.Context, cert *domain.ManagedCertificate) ([]*domain.PolicyViolation, error) { rules, err := s.policyRepo.ListRules(ctx) @@ -43,7 +60,7 @@ func (s *PolicyService) ValidateCertificate(ctx context.Context, cert *domain.Ma } // Evaluate rule against certificate - v, err := s.evaluateRule(rule, cert) + v, err := s.evaluateRule(ctx, rule, cert) if err != nil { slog.Error("failed to evaluate rule", "rule_id", rule.ID, "error", err) continue @@ -58,73 +75,163 @@ func (s *PolicyService) ValidateCertificate(ctx context.Context, cert *domain.Ma } // evaluateRule checks if a certificate violates a single policy rule. -func (s *PolicyService) evaluateRule(rule *domain.PolicyRule, cert *domain.ManagedCertificate) (*domain.PolicyViolation, error) { +// +// D-008 closes the engine loop by: +// 1. Consuming rule.Severity on every violation (the pre-D-008 engine +// hardcoded PolicySeverityWarning, which silently defeated the D-006 +// per-rule severity column). +// 2. Parsing rule.Config per-arm so rules carry real thresholds / allowlists +// instead of the pre-D-008 "metadata absent" placeholders. Empty/null +// Config preserves the pre-D-008 missing-field behavior as a +// backward-compat invariant — a rule without config still fires on the +// absent-field shape but using its configured severity. +// 3. Adding the CertificateLifetime arm, which reads NotBefore/NotAfter from +// the latest CertificateVersion (injected via SetCertRepo). Required +// because ManagedCertificate tracks ExpiresAt but not issuance date. +// +// Bad-config failure mode: json.Unmarshal error returns (nil, error) shaped +// as `invalid config for rule (type=): `; the caller at +// ValidateCertificate logs and continues so one malformed rule doesn't fail +// the entire pass. +func (s *PolicyService) evaluateRule(ctx context.Context, rule *domain.PolicyRule, cert *domain.ManagedCertificate) (*domain.PolicyViolation, error) { switch rule.Type { case domain.PolicyTypeAllowedIssuers: - // Restrict to specific issuers - // Note: In a production implementation, we would parse rule.Config to extract parameters + // Config: {"allowed_issuer_ids": ["iss-a", "iss-b"]} + // Empty config = fire only on absent IssuerID (backward-compat). + var cfg struct { + AllowedIssuerIDs []string `json:"allowed_issuer_ids"` + } + if len(rule.Config) > 0 { + if err := json.Unmarshal(rule.Config, &cfg); err != nil { + return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err) + } + } if cert.IssuerID == "" { - return &domain.PolicyViolation{ - ID: generateID("violation"), - RuleID: rule.ID, - CertificateID: cert.ID, - Severity: domain.PolicySeverityWarning, - Message: "certificate has no issuer assigned", - CreatedAt: time.Now(), - }, nil + return s.violation(rule, cert, "certificate has no issuer assigned"), nil + } + if len(cfg.AllowedIssuerIDs) > 0 && !containsString(cfg.AllowedIssuerIDs, cert.IssuerID) { + return s.violation(rule, cert, fmt.Sprintf("issuer %q is not in the allowed list", cert.IssuerID)), nil } case domain.PolicyTypeAllowedDomains: - // Ensure certificate domains are in allowed list + // Config: {"allowed_domains": ["example.com", "*.internal.example.com"]} + // Wildcards are literal prefix matches (*.foo matches anything ending + // in .foo). Empty config = fire only on zero SANs (backward-compat). + var cfg struct { + AllowedDomains []string `json:"allowed_domains"` + } + if len(rule.Config) > 0 { + if err := json.Unmarshal(rule.Config, &cfg); err != nil { + return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err) + } + } if len(cert.SANs) == 0 { - return &domain.PolicyViolation{ - ID: generateID("violation"), - RuleID: rule.ID, - CertificateID: cert.ID, - Severity: domain.PolicySeverityWarning, - Message: "certificate has no subject alternative names", - CreatedAt: time.Now(), - }, nil + return s.violation(rule, cert, "certificate has no subject alternative names"), nil + } + if len(cfg.AllowedDomains) > 0 { + for _, san := range cert.SANs { + if !domainAllowed(san, cfg.AllowedDomains) { + return s.violation(rule, cert, fmt.Sprintf("SAN %q is not in the allowed domain list", san)), nil + } + } } case domain.PolicyTypeRequiredMetadata: - // Ensure certificate has required metadata/tags + // Config: {"required_keys": ["owner", "cost-center"]} + // Empty config = fire only on zero tags (backward-compat). + var cfg struct { + RequiredKeys []string `json:"required_keys"` + } + if len(rule.Config) > 0 { + if err := json.Unmarshal(rule.Config, &cfg); err != nil { + return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err) + } + } if len(cert.Tags) == 0 { - return &domain.PolicyViolation{ - ID: generateID("violation"), - RuleID: rule.ID, - CertificateID: cert.ID, - Severity: domain.PolicySeverityWarning, - Message: "certificate has no tags or metadata", - CreatedAt: time.Now(), - }, nil + return s.violation(rule, cert, "certificate has no tags or metadata"), nil + } + for _, key := range cfg.RequiredKeys { + if _, ok := cert.Tags[key]; !ok { + return s.violation(rule, cert, fmt.Sprintf("certificate is missing required metadata key %q", key)), nil + } } case domain.PolicyTypeAllowedEnvironments: - // Restrict to specific environments + // Config: {"allowed": ["prod", "staging"]} + // Empty config = fire only on empty Environment (backward-compat). + var cfg struct { + Allowed []string `json:"allowed"` + } + if len(rule.Config) > 0 { + if err := json.Unmarshal(rule.Config, &cfg); err != nil { + return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err) + } + } if cert.Environment == "" { - return &domain.PolicyViolation{ - ID: generateID("violation"), - RuleID: rule.ID, - CertificateID: cert.ID, - Severity: domain.PolicySeverityWarning, - Message: "certificate has no environment assigned", - CreatedAt: time.Now(), - }, nil + return s.violation(rule, cert, "certificate has no environment assigned"), nil + } + if len(cfg.Allowed) > 0 && !containsString(cfg.Allowed, cert.Environment) { + return s.violation(rule, cert, fmt.Sprintf("environment %q is not in the allowed list", cert.Environment)), nil } case domain.PolicyTypeRenewalLeadTime: - // Ensure renewal begins before certificate expires + // Config: {"lead_time_days": 30} + // Fires when remaining validity drops below lead_time_days and the + // cert is not already expired. Empty/zero config falls back to the + // pre-D-008 hardcoded 30-day threshold for backward compatibility. + var cfg struct { + LeadTimeDays int `json:"lead_time_days"` + } + if len(rule.Config) > 0 { + if err := json.Unmarshal(rule.Config, &cfg); err != nil { + return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err) + } + } + leadDays := cfg.LeadTimeDays + if leadDays <= 0 { + leadDays = 30 + } daysUntilExpiry := time.Until(cert.ExpiresAt).Hours() / 24 - if daysUntilExpiry < 30 && daysUntilExpiry > 0 { - return &domain.PolicyViolation{ - ID: generateID("violation"), - RuleID: rule.ID, - CertificateID: cert.ID, - Severity: domain.PolicySeverityWarning, - Message: fmt.Sprintf("certificate expires in %.1f days, plan renewal soon", daysUntilExpiry), - CreatedAt: time.Now(), - }, nil + if daysUntilExpiry < float64(leadDays) && daysUntilExpiry > 0 { + return s.violation(rule, cert, fmt.Sprintf("certificate expires in %.1f days, plan renewal soon (policy lead time: %d days)", daysUntilExpiry, leadDays)), nil + } + + case domain.PolicyTypeCertificateLifetime: + // Config: {"max_days": 397} + // Reads NotBefore/NotAfter from the latest CertificateVersion via the + // injected certRepo. ManagedCertificate exposes ExpiresAt but not the + // issuance date, so lifetime math requires the version record. + // + // If certRepo wasn't wired (test misconfiguration / early boot), + // returns an error so the caller logs it — better a loud failure + // than silently ignoring the rule. If GetLatestVersion errors (e.g., + // the cert hasn't been issued yet), we skip the check — a cert with + // no version has no lifetime to measure, matching the missing-field + // backward-compat pattern used by the other arms. + if s.certRepo == nil { + return nil, fmt.Errorf("CertificateLifetime rule %s requires cert repository (not wired via SetCertRepo)", rule.ID) + } + var cfg struct { + MaxDays int `json:"max_days"` + } + if len(rule.Config) > 0 { + if err := json.Unmarshal(rule.Config, &cfg); err != nil { + return nil, fmt.Errorf("invalid config for rule %s (type=%s): %w", rule.ID, rule.Type, err) + } + } + if cfg.MaxDays <= 0 { + // No threshold configured — nothing meaningful to enforce. + return nil, nil + } + version, err := s.certRepo.GetLatestVersion(ctx, cert.ID) + if err != nil { + // No version yet — nothing to measure. Not an engine error; + // the cert simply hasn't been issued. + return nil, nil + } + lifetimeDays := version.NotAfter.Sub(version.NotBefore).Hours() / 24 + if lifetimeDays > float64(cfg.MaxDays) { + return s.violation(rule, cert, fmt.Sprintf("certificate lifetime is %.1f days, exceeds policy max of %d days", lifetimeDays, cfg.MaxDays)), nil } default: @@ -134,6 +241,56 @@ func (s *PolicyService) evaluateRule(rule *domain.PolicyRule, cert *domain.Manag return nil, nil } +// violation constructs a PolicyViolation carrying the rule's configured +// severity. Centralizing the build eliminates the pre-D-008 bug where each +// arm independently stamped PolicySeverityWarning on its violation. +func (s *PolicyService) violation(rule *domain.PolicyRule, cert *domain.ManagedCertificate, message string) *domain.PolicyViolation { + return &domain.PolicyViolation{ + ID: generateID("violation"), + RuleID: rule.ID, + CertificateID: cert.ID, + Severity: rule.Severity, + Message: message, + CreatedAt: time.Now(), + } +} + +// containsString reports whether needle is present in haystack. +func containsString(haystack []string, needle string) bool { + for _, s := range haystack { + if s == needle { + return true + } + } + return false +} + +// domainAllowed reports whether a SAN (hostname) matches any of the allowed +// domain patterns. Patterns may be exact matches or `*.example.com` wildcards +// (the wildcard consumes a single label: `*.foo.com` matches `bar.foo.com` +// but not `baz.bar.foo.com`, mirroring X.509 SAN wildcard semantics). +func domainAllowed(san string, allowed []string) bool { + san = strings.ToLower(strings.TrimSpace(san)) + for _, pattern := range allowed { + pattern = strings.ToLower(strings.TrimSpace(pattern)) + if pattern == san { + return true + } + if strings.HasPrefix(pattern, "*.") { + suffix := pattern[1:] // ".foo.com" + if strings.HasSuffix(san, suffix) { + // Ensure wildcard consumes exactly one label — reject + // sub-subdomains. + head := strings.TrimSuffix(san, suffix) + if head != "" && !strings.Contains(head, ".") { + return true + } + } + } + } + return false +} + // CreateRule stores a new policy rule. func (s *PolicyService) CreateRule(ctx context.Context, rule *domain.PolicyRule, actor string) error { if rule.ID == "" { diff --git a/internal/service/policy_test.go b/internal/service/policy_test.go index a368f04..2626876 100644 --- a/internal/service/policy_test.go +++ b/internal/service/policy_test.go @@ -3,6 +3,7 @@ package service import ( "context" "encoding/json" + "strings" "testing" "time" @@ -420,3 +421,536 @@ func TestCreatePolicy(t *testing.T) { t.Errorf("expected 1 rule in repo, got %d", len(policyRepo.Rules)) } } + +// ============================================================================ +// D-008 regression tests +// +// These pin the behavior that closes the D-006 loop: +// 1. evaluateRule copies rule.Severity onto every violation (pre-D-008 the +// engine hardcoded Warning regardless of the rule's configured severity). +// 2. evaluateRule parses rule.Config per-arm so rules enforce real thresholds +// and allowlists (pre-D-008 the configs were ignored; rules fired only on +// the missing-field shape). +// 3. An empty/zero Config preserves the pre-D-008 missing-field violation +// (backward-compat invariant). +// 4. Malformed Config returns an error; the caller logs and skips the rule +// instead of producing a zero-value violation. +// 5. CertificateLifetime (new 6th arm) reads NotBefore/NotAfter from the +// latest CertificateVersion via the cert repo wired with SetCertRepo. +// ============================================================================ + +// mkRule is a tiny constructor used by the D-008 tests to keep the table rows +// readable. Every rule is enabled; test-specific fields layer on top. +func mkRule(id string, t domain.PolicyType, sev domain.PolicySeverity, cfg string) *domain.PolicyRule { + return &domain.PolicyRule{ + ID: id, + Name: id, + Type: t, + Config: json.RawMessage(cfg), + Enabled: true, + Severity: sev, + } +} + +// evalCert is a minimal cert used by the arms that don't look at much beyond +// the shape of the field they're testing. Tests shadow fields as needed. +func evalCert() *domain.ManagedCertificate { + return &domain.ManagedCertificate{ + ID: "cert-001", + CommonName: "example.com", + Status: domain.CertificateStatusActive, + ExpiresAt: time.Now().AddDate(1, 0, 0), + } +} + +// TestEvaluateRule_SeverityPassThrough pins invariant #1 — every arm stamps +// rule.Severity onto the violation. The pre-D-008 bug was that arms +// independently hardcoded PolicySeverityWarning. We test each arm with a +// severity that isn't the legacy default so a regression would be visible. +func TestEvaluateRule_SeverityPassThrough(t *testing.T) { + ctx := context.Background() + + // Cert shaped to fail every non-empty-config check via the backward-compat + // missing-field path. Each row picks a severity intentionally ≠ Warning to + // make a stray hardcoded default obvious. + cases := []struct { + name string + rule *domain.PolicyRule + cert *domain.ManagedCertificate + setupFn func(svc *PolicyService) + expected domain.PolicySeverity + }{ + { + name: "AllowedIssuers Critical via missing IssuerID", + rule: mkRule("r-ai", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityCritical, ""), + cert: func() *domain.ManagedCertificate { + c := evalCert() + c.IssuerID = "" + return c + }(), + expected: domain.PolicySeverityCritical, + }, + { + name: "AllowedDomains Error via empty SANs", + rule: mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityError, ""), + cert: func() *domain.ManagedCertificate { + c := evalCert() + c.SANs = nil + return c + }(), + expected: domain.PolicySeverityError, + }, + { + name: "RequiredMetadata Critical via empty Tags", + rule: mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityCritical, ""), + cert: func() *domain.ManagedCertificate { + c := evalCert() + c.Tags = nil + return c + }(), + expected: domain.PolicySeverityCritical, + }, + { + name: "AllowedEnvironments Warning via empty Environment", + rule: mkRule("r-ae", domain.PolicyTypeAllowedEnvironments, domain.PolicySeverityWarning, ""), + cert: func() *domain.ManagedCertificate { + c := evalCert() + c.Environment = "" + return c + }(), + expected: domain.PolicySeverityWarning, + }, + { + name: "RenewalLeadTime Critical via short remaining validity", + rule: mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityCritical, `{"lead_time_days": 60}`), + cert: func() *domain.ManagedCertificate { + c := evalCert() + c.ExpiresAt = time.Now().AddDate(0, 0, 30) // 30d remaining < 60d lead + return c + }(), + expected: domain.PolicySeverityCritical, + }, + { + name: "CertificateLifetime Error via 365d span vs 90d max", + rule: mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError, `{"max_days": 90}`), + cert: evalCert(), + setupFn: func(svc *PolicyService) { + // Seed a version with 365d lifetime on the same cert ID used + // by evalCert(). + cr := &mockCertRepo{ + Certs: map[string]*domain.ManagedCertificate{}, + Versions: map[string][]*domain.CertificateVersion{}, + } + now := time.Now() + cr.Versions["cert-001"] = []*domain.CertificateVersion{{ + ID: "ver-001", + CertificateID: "cert-001", + NotBefore: now.AddDate(0, 0, -10), + NotAfter: now.AddDate(1, 0, -10), // ~365d lifetime + }} + svc.SetCertRepo(cr) + }, + expected: domain.PolicySeverityError, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + policyRepo := &mockPolicyRepo{ + Rules: map[string]*domain.PolicyRule{tc.rule.ID: tc.rule}, + Violations: []*domain.PolicyViolation{}, + } + auditService := NewAuditService(&mockAuditRepo{}) + svc := NewPolicyService(policyRepo, auditService) + if tc.setupFn != nil { + tc.setupFn(svc) + } + + violations, err := svc.ValidateCertificate(ctx, tc.cert) + if err != nil { + t.Fatalf("ValidateCertificate failed: %v", err) + } + if len(violations) != 1 { + t.Fatalf("expected 1 violation, got %d", len(violations)) + } + if violations[0].Severity != tc.expected { + t.Errorf("expected severity %q, got %q", tc.expected, violations[0].Severity) + } + if violations[0].RuleID != tc.rule.ID { + t.Errorf("expected rule ID %q, got %q", tc.rule.ID, violations[0].RuleID) + } + }) + } +} + +// TestEvaluateRule_ConfigConsumed pins invariant #2 — non-empty Config drives +// arm behavior (allowlists, thresholds, keys). Each subtest supplies a config +// that the cert would satisfy under the backward-compat missing-field path +// but violates under the config-aware path. A regression to the pre-D-008 +// "config silently dropped" behavior would make these pass with 0 violations. +func TestEvaluateRule_ConfigConsumed(t *testing.T) { + ctx := context.Background() + + t.Run("AllowedIssuers rejects issuer not in allowlist", func(t *testing.T) { + rule := mkRule("r-ai", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityWarning, + `{"allowed_issuer_ids": ["iss-acme"]}`) + cert := evalCert() + cert.IssuerID = "iss-wrong" + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Fatalf("expected 1 violation for disallowed issuer, got %d", len(violations)) + } + if !strings.Contains(violations[0].Message, "iss-wrong") { + t.Errorf("expected message to mention issuer ID, got %q", violations[0].Message) + } + }) + + t.Run("AllowedIssuers accepts issuer in allowlist", func(t *testing.T) { + rule := mkRule("r-ai", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityWarning, + `{"allowed_issuer_ids": ["iss-acme"]}`) + cert := evalCert() + cert.IssuerID = "iss-acme" + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 0 { + t.Errorf("expected 0 violations for allowed issuer, got %d", len(violations)) + } + }) + + t.Run("AllowedDomains rejects SAN outside allowlist", func(t *testing.T) { + rule := mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityWarning, + `{"allowed_domains": ["*.foo.com"]}`) + cert := evalCert() + cert.SANs = []string{"bar.elsewhere.com"} + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Fatalf("expected 1 violation for disallowed SAN, got %d", len(violations)) + } + }) + + t.Run("AllowedDomains wildcard matches single-label subdomain", func(t *testing.T) { + rule := mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityWarning, + `{"allowed_domains": ["*.foo.com"]}`) + cert := evalCert() + cert.SANs = []string{"bar.foo.com"} + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 0 { + t.Errorf("expected 0 violations for single-label wildcard match, got %d", len(violations)) + } + }) + + t.Run("AllowedDomains wildcard rejects multi-label subdomain", func(t *testing.T) { + // X.509 wildcard semantics: *.foo consumes exactly one label. + rule := mkRule("r-ad", domain.PolicyTypeAllowedDomains, domain.PolicySeverityWarning, + `{"allowed_domains": ["*.foo.com"]}`) + cert := evalCert() + cert.SANs = []string{"baz.bar.foo.com"} + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Errorf("expected 1 violation for multi-label wildcard (X.509 semantics), got %d", len(violations)) + } + }) + + t.Run("RequiredMetadata rejects missing key", func(t *testing.T) { + rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityWarning, + `{"required_keys": ["owner"]}`) + cert := evalCert() + cert.Tags = map[string]string{"team": "platform"} + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Fatalf("expected 1 violation for missing owner key, got %d", len(violations)) + } + if !strings.Contains(violations[0].Message, "owner") { + t.Errorf("expected message to mention the missing key, got %q", violations[0].Message) + } + }) + + t.Run("RequiredMetadata accepts all required keys present", func(t *testing.T) { + rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityWarning, + `{"required_keys": ["owner"]}`) + cert := evalCert() + cert.Tags = map[string]string{"owner": "alice"} + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 0 { + t.Errorf("expected 0 violations when all required keys present, got %d", len(violations)) + } + }) + + t.Run("AllowedEnvironments rejects env outside allowlist", func(t *testing.T) { + rule := mkRule("r-ae", domain.PolicyTypeAllowedEnvironments, domain.PolicySeverityWarning, + `{"allowed": ["production", "staging"]}`) + cert := evalCert() + cert.Environment = "wild-west" + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Fatalf("expected 1 violation for disallowed env, got %d", len(violations)) + } + }) + + t.Run("RenewalLeadTime fires when remaining < configured lead", func(t *testing.T) { + rule := mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityWarning, + `{"lead_time_days": 60}`) + cert := evalCert() + cert.ExpiresAt = time.Now().AddDate(0, 0, 30) // 30d < 60d lead + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Fatalf("expected 1 violation for 30d remaining vs 60d lead, got %d", len(violations)) + } + }) + + t.Run("RenewalLeadTime quiet when remaining > configured lead", func(t *testing.T) { + rule := mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityWarning, + `{"lead_time_days": 14}`) + cert := evalCert() + cert.ExpiresAt = time.Now().AddDate(0, 0, 60) // 60d > 14d lead + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 0 { + t.Errorf("expected 0 violations when plenty of runway remains, got %d", len(violations)) + } + }) + + t.Run("CertificateLifetime fires when lifetime exceeds max", func(t *testing.T) { + rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityWarning, + `{"max_days": 90}`) + cert := evalCert() + now := time.Now() + + certRepo := &mockCertRepo{ + Certs: map[string]*domain.ManagedCertificate{}, + Versions: map[string][]*domain.CertificateVersion{}, + } + certRepo.Versions["cert-001"] = []*domain.CertificateVersion{{ + ID: "ver-001", + CertificateID: "cert-001", + NotBefore: now.AddDate(0, 0, -1), + NotAfter: now.AddDate(1, 0, -1), // ~365d > 90d + }} + + violations := runEval(ctx, t, rule, cert, certRepo) + if len(violations) != 1 { + t.Fatalf("expected 1 violation for 365d lifetime vs 90d max, got %d", len(violations)) + } + if !strings.Contains(violations[0].Message, "90 days") { + t.Errorf("expected message to mention max_days threshold, got %q", violations[0].Message) + } + }) + + t.Run("CertificateLifetime quiet when lifetime within max", func(t *testing.T) { + rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityWarning, + `{"max_days": 90}`) + cert := evalCert() + now := time.Now() + + certRepo := &mockCertRepo{ + Certs: map[string]*domain.ManagedCertificate{}, + Versions: map[string][]*domain.CertificateVersion{}, + } + certRepo.Versions["cert-001"] = []*domain.CertificateVersion{{ + ID: "ver-001", + CertificateID: "cert-001", + NotBefore: now.AddDate(0, 0, -10), + NotAfter: now.AddDate(0, 0, 60), // 70d lifetime < 90d + }} + + violations := runEval(ctx, t, rule, cert, certRepo) + if len(violations) != 0 { + t.Errorf("expected 0 violations for 70d lifetime under 90d max, got %d", len(violations)) + } + }) +} + +// TestEvaluateRule_EmptyConfig_BackCompat pins invariant #3 — a rule with no +// Config (e.g., a legacy row from a pre-D-008 migration) still fires on the +// pre-D-008 missing-field shape using its configured severity. This is how +// we let existing deployments migrate without a schema rewrite. +func TestEvaluateRule_EmptyConfig_BackCompat(t *testing.T) { + ctx := context.Background() + + t.Run("RequiredMetadata fires on zero tags", func(t *testing.T) { + rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityError, "") + cert := evalCert() + cert.Tags = nil + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Fatalf("expected 1 backcompat violation, got %d", len(violations)) + } + if violations[0].Severity != domain.PolicySeverityError { + t.Errorf("expected severity Error (passed through from rule), got %q", violations[0].Severity) + } + }) + + t.Run("RequiredMetadata quiet when any tags present under empty config", func(t *testing.T) { + // Empty config means "only fire on missing-field shape" — so a cert + // with any tags (even not what a human would call meaningful) passes. + rule := mkRule("r-rm", domain.PolicyTypeRequiredMetadata, domain.PolicySeverityError, "") + cert := evalCert() + cert.Tags = map[string]string{"arbitrary": "value"} + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 0 { + t.Errorf("expected 0 violations under backcompat shape w/ tags set, got %d", len(violations)) + } + }) + + t.Run("RenewalLeadTime uses 30d default under empty/zero config", func(t *testing.T) { + rule := mkRule("r-rl", domain.PolicyTypeRenewalLeadTime, domain.PolicySeverityWarning, "") + cert := evalCert() + cert.ExpiresAt = time.Now().AddDate(0, 0, 15) // 15d < 30d default + + violations := runEval(ctx, t, rule, cert, nil) + if len(violations) != 1 { + t.Errorf("expected 1 violation under 30d backcompat default, got %d", len(violations)) + } + }) +} + +// TestEvaluateRule_BadConfig_SkipsRule pins invariant #4 — malformed JSON in +// Config returns an error from evaluateRule, which ValidateCertificate logs +// and swallows. The pass continues; no zero-value violation is emitted. +// Co-located rules still fire normally. +func TestEvaluateRule_BadConfig_SkipsRule(t *testing.T) { + ctx := context.Background() + + // Rule 1 has malformed JSON — should log+skip. + // Rule 2 is a healthy AllowedIssuers rule that should still emit its + // violation on the missing-IssuerID cert. If the bad rule poisoned the + // loop, we'd see 0 or 2 violations instead of exactly 1. + badRule := mkRule("r-bad", domain.PolicyTypeAllowedIssuers, domain.PolicySeverityError, + `{"allowed_issuer_ids": [`) // unterminated JSON + goodRule := mkRule("r-good", domain.PolicyTypeAllowedEnvironments, domain.PolicySeverityWarning, "") + + policyRepo := &mockPolicyRepo{ + Rules: map[string]*domain.PolicyRule{ + badRule.ID: badRule, + goodRule.ID: goodRule, + }, + Violations: []*domain.PolicyViolation{}, + } + auditService := NewAuditService(&mockAuditRepo{}) + svc := NewPolicyService(policyRepo, auditService) + + cert := evalCert() + cert.IssuerID = "" // would trigger the bad rule if it wasn't skipped + cert.Environment = "" // triggers goodRule via missing-field backcompat + + violations, err := svc.ValidateCertificate(ctx, cert) + if err != nil { + t.Fatalf("ValidateCertificate should swallow rule-eval errors, got %v", err) + } + if len(violations) != 1 { + t.Fatalf("expected exactly 1 violation (bad rule skipped, good rule fires), got %d", len(violations)) + } + if violations[0].RuleID != goodRule.ID { + t.Errorf("expected violation from r-good, got %q", violations[0].RuleID) + } +} + +// TestEvaluateRule_CertificateLifetime_RepoScenarios pins the setter-injection +// pattern for the 6th arm. SetCertRepo wires the dependency; without it the +// arm errors (logged+skipped by the caller). With it but no version present, +// the arm silently returns nil (matching the missing-field backcompat shape). +func TestEvaluateRule_CertificateLifetime_RepoScenarios(t *testing.T) { + ctx := context.Background() + + t.Run("repo not wired logs and skips", func(t *testing.T) { + rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError, + `{"max_days": 90}`) + policyRepo := &mockPolicyRepo{ + Rules: map[string]*domain.PolicyRule{rule.ID: rule}, + Violations: []*domain.PolicyViolation{}, + } + svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{})) + // deliberately do NOT call SetCertRepo + + violations, err := svc.ValidateCertificate(ctx, evalCert()) + if err != nil { + t.Fatalf("ValidateCertificate should swallow the nil-repo error, got %v", err) + } + if len(violations) != 0 { + t.Errorf("expected 0 violations when repo unwired (rule skipped), got %d", len(violations)) + } + }) + + t.Run("version missing silently skips", func(t *testing.T) { + rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError, + `{"max_days": 90}`) + policyRepo := &mockPolicyRepo{ + Rules: map[string]*domain.PolicyRule{rule.ID: rule}, + Violations: []*domain.PolicyViolation{}, + } + svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{})) + // Empty Versions map — GetLatestVersion returns errNotFound, arm skips. + svc.SetCertRepo(&mockCertRepo{ + Certs: map[string]*domain.ManagedCertificate{}, + Versions: map[string][]*domain.CertificateVersion{}, + }) + + violations, err := svc.ValidateCertificate(ctx, evalCert()) + if err != nil { + t.Fatalf("ValidateCertificate failed: %v", err) + } + if len(violations) != 0 { + t.Errorf("expected 0 violations when no version exists (nothing to measure), got %d", len(violations)) + } + }) + + t.Run("max_days zero/absent means no enforcement", func(t *testing.T) { + // Even with a version, max_days=0 is a no-op (matches the + // no-threshold-configured guard in the arm). + rule := mkRule("r-cl", domain.PolicyTypeCertificateLifetime, domain.PolicySeverityError, "") + policyRepo := &mockPolicyRepo{ + Rules: map[string]*domain.PolicyRule{rule.ID: rule}, + Violations: []*domain.PolicyViolation{}, + } + svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{})) + now := time.Now() + svc.SetCertRepo(&mockCertRepo{ + Certs: map[string]*domain.ManagedCertificate{}, + Versions: map[string][]*domain.CertificateVersion{ + "cert-001": {{ + CertificateID: "cert-001", + NotBefore: now.AddDate(0, 0, -1), + NotAfter: now.AddDate(10, 0, 0), // 10 years — huge but unchecked + }}, + }, + }) + + violations, err := svc.ValidateCertificate(ctx, evalCert()) + if err != nil { + t.Fatalf("ValidateCertificate failed: %v", err) + } + if len(violations) != 0 { + t.Errorf("expected 0 violations when max_days absent (no enforcement), got %d", len(violations)) + } + }) +} + +// runEval is a test helper that exercises ValidateCertificate against a +// single-rule configuration and returns the violation slice. Optionally +// wires a cert repo for the CertificateLifetime arm. +func runEval(ctx context.Context, t *testing.T, rule *domain.PolicyRule, cert *domain.ManagedCertificate, certRepo *mockCertRepo) []*domain.PolicyViolation { + t.Helper() + policyRepo := &mockPolicyRepo{ + Rules: map[string]*domain.PolicyRule{rule.ID: rule}, + Violations: []*domain.PolicyViolation{}, + } + svc := NewPolicyService(policyRepo, NewAuditService(&mockAuditRepo{})) + if certRepo != nil { + svc.SetCertRepo(certRepo) + } + violations, err := svc.ValidateCertificate(ctx, cert) + if err != nil { + t.Fatalf("ValidateCertificate failed: %v", err) + } + return violations +} diff --git a/migrations/000014_policy_violation_severity_check.down.sql b/migrations/000014_policy_violation_severity_check.down.sql new file mode 100644 index 0000000..6488c39 --- /dev/null +++ b/migrations/000014_policy_violation_severity_check.down.sql @@ -0,0 +1,9 @@ +-- Rollback migration 000014: drop the policy_violations severity CHECK. +-- +-- Drops the named CHECK constraint added by the up migration. The severity +-- column itself stays (it predates this migration — see 000001 line 183), +-- so any application code that reads/writes the column continues to work. +-- Only the DB-level enforcement of the TitleCase allowlist is removed. + +ALTER TABLE policy_violations + DROP CONSTRAINT IF EXISTS policy_violations_severity_check; diff --git a/migrations/000014_policy_violation_severity_check.up.sql b/migrations/000014_policy_violation_severity_check.up.sql new file mode 100644 index 0000000..dfc4c0d --- /dev/null +++ b/migrations/000014_policy_violation_severity_check.up.sql @@ -0,0 +1,29 @@ +-- Migration 000014: CHECK constraint on policy_violations.severity +-- +-- Sibling to migration 000013, which added severity + CHECK to policy_rules. +-- policy_violations has carried a severity column since the initial schema +-- (000001, line 183) but without any CHECK. The engine used to hardcode +-- `Warning` on every violation regardless of the triggering rule's severity +-- (see pre-D-008 internal/service/policy.go:evaluateRule), so the column +-- value was uniform by accident of implementation, not by constraint. +-- +-- D-008 rewrites evaluateRule to copy rule.Severity into the violation. The +-- engine now writes values drawn from the application-layer PolicySeverity +-- allowlist, but nothing at the DB level prevents a future caller — or a +-- bypassed write from a migration or psql session — from inserting casing +-- drift ('warning', 'ERROR', etc.) and re-opening the same class of bug +-- that D-005 and D-006 closed. This constraint is the defense-in-depth +-- complement to the handler validator. +-- +-- Pre-existing seed_demo.sql rows use lowercase severity values. D-008 +-- updates those in the same commit so this migration can apply cleanly +-- against both a fresh install and an upgraded install that has already +-- seeded the demo data. +-- +-- Named constraint (policy_violations_severity_check) so the down migration +-- can DROP it by name without ambiguity; un-named CHECK constraints use +-- a synthesized PostgreSQL name that varies by environment. + +ALTER TABLE policy_violations + ADD CONSTRAINT policy_violations_severity_check + CHECK (severity IN ('Warning', 'Error', 'Critical')); diff --git a/migrations/seed.sql b/migrations/seed.sql index 06d5a63..50d7cc1 100644 --- a/migrations/seed.sql +++ b/migrations/seed.sql @@ -12,19 +12,25 @@ VALUES ( '[30, 14, 7, 0]'::jsonb ) ON CONFLICT (id) DO NOTHING; --- Policy rules: Require owner assignment --- Severity differentiated per rule to demonstrate the field means something --- (D-006). The backend CHECK constraint (migration 000013) enforces the --- TitleCase allowlist Warning/Error/Critical. Type-value drift --- (ownership/environment/lifetime/renewal_window vs. the engine's TitleCase --- canonicals) is tracked separately in d/D-008 and intentionally left --- unchanged in this commit. +-- Policy rules: Require owner assignment, bound environments, cap lifetime, +-- and enforce a renewal lead-time. +-- +-- Severity is differentiated per rule (D-006) and the types are now the +-- TitleCase canonicals the engine actually recognizes (D-008). Pre-D-008 the +-- types were lowercase strings (`ownership`, `environment`, `lifetime`, +-- `renewal_window`) that the engine silently dropped through to its +-- default-case error path — the rules looked alive in the GUI but did not +-- enforce anything. The backend CHECK constraint (migration 000013) enforces +-- the TitleCase severity allowlist Warning/Error/Critical. Configs are also +-- reshaped to match the D-008 per-arm schemas so the rules actually exercise +-- the config-consuming paths instead of falling back to the missing-field +-- placeholders. INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-require-owner', 'require-owner', - 'ownership', - '{"requirement": "owner_id must be set"}'::jsonb, + 'RequiredMetadata', + '{"required_keys": ["owner"]}'::jsonb, true, 'Warning' ) ON CONFLICT (id) DO NOTHING; @@ -34,7 +40,7 @@ INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-allowed-environments', 'allowed-environments', - 'environment', + 'AllowedEnvironments', '{"allowed": ["production", "staging", "development"]}'::jsonb, true, 'Error' @@ -45,19 +51,19 @@ INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-max-certificate-lifetime', 'max-certificate-lifetime', - 'lifetime', + 'CertificateLifetime', '{"max_days": 90}'::jsonb, true, 'Critical' ) ON CONFLICT (id) DO NOTHING; --- Policy rules: Minimum renewal window +-- Policy rules: Minimum renewal window (renew at least 14 days before expiry) INSERT INTO policy_rules (id, name, type, config, enabled, severity) VALUES ( 'pr-min-renewal-window', 'min-renewal-window', - 'renewal_window', - '{"min_days": 14}'::jsonb, + 'RenewalLeadTime', + '{"lead_time_days": 14}'::jsonb, true, 'Warning' ) ON CONFLICT (id) DO NOTHING; diff --git a/migrations/seed_demo.sql b/migrations/seed_demo.sql index e1e0399..3b6239b 100644 --- a/migrations/seed_demo.sql +++ b/migrations/seed_demo.sql @@ -478,13 +478,20 @@ ON CONFLICT (id) DO NOTHING; -- ============================================================ -- 13. Policy Violations -- ============================================================ +-- D-008: severity values rewritten to TitleCase canonicals (Warning/Error/Critical). +-- Pre-D-008 these rows used lowercase strings ('critical', 'error', 'warning'). Those +-- values were silently tolerated by the pre-D-008 engine, which hardcoded 'Warning' +-- on every new violation regardless of the triggering rule's severity. D-008 rewires +-- evaluateRule to copy rule.Severity into the violation AND migration 000014 adds a +-- CHECK constraint enforcing the TitleCase allowlist at the DB level. Both paths now +-- round-trip correctly against these demo rows. INSERT INTO policy_violations (id, certificate_id, rule_id, message, severity, created_at) VALUES - ('pv-001', 'mc-legacy-prod', 'pr-max-certificate-lifetime', 'Certificate has expired and exceeds maximum lifetime policy', 'critical', NOW() - INTERVAL '3 days'), - ('pv-002', 'mc-old-api', 'pr-max-certificate-lifetime', 'Certificate expired 15 days ago', 'critical', NOW() - INTERVAL '15 days'), - ('pv-003', 'mc-vpn-prod', 'pr-min-renewal-window', 'Renewal failed within minimum renewal window', 'error', NOW() - INTERVAL '3 days'), - ('pv-004', 'mc-mail-prod', 'pr-min-renewal-window', 'Certificate expiring in 5 days, below 14-day minimum window','warning', NOW() - INTERVAL '20 minutes'), - ('pv-005', 'mc-wiki-prod', 'pr-max-certificate-lifetime', 'Certificate expired 7 days ago', 'critical', NOW() - INTERVAL '7 days'), - ('pv-006', 'mc-compromised', 'pr-min-renewal-window', 'Certificate revoked due to key compromise', 'critical', NOW() - INTERVAL '14 days') + ('pv-001', 'mc-legacy-prod', 'pr-max-certificate-lifetime', 'Certificate has expired and exceeds maximum lifetime policy', 'Critical', NOW() - INTERVAL '3 days'), + ('pv-002', 'mc-old-api', 'pr-max-certificate-lifetime', 'Certificate expired 15 days ago', 'Critical', NOW() - INTERVAL '15 days'), + ('pv-003', 'mc-vpn-prod', 'pr-min-renewal-window', 'Renewal failed within minimum renewal window', 'Error', NOW() - INTERVAL '3 days'), + ('pv-004', 'mc-mail-prod', 'pr-min-renewal-window', 'Certificate expiring in 5 days, below 14-day minimum window','Warning', NOW() - INTERVAL '20 minutes'), + ('pv-005', 'mc-wiki-prod', 'pr-max-certificate-lifetime', 'Certificate expired 7 days ago', 'Critical', NOW() - INTERVAL '7 days'), + ('pv-006', 'mc-compromised', 'pr-min-renewal-window', 'Certificate revoked due to key compromise', 'Critical', NOW() - INTERVAL '14 days') ON CONFLICT (id) DO NOTHING; -- ============================================================ diff --git a/web/src/api/types.test.ts b/web/src/api/types.test.ts index 957305f..8ac0af5 100644 --- a/web/src/api/types.test.ts +++ b/web/src/api/types.test.ts @@ -34,6 +34,7 @@ describe('POLICY_TYPES', () => { 'RequiredMetadata', 'AllowedEnvironments', 'RenewalLeadTime', + 'CertificateLifetime', ]); }); diff --git a/web/src/api/types.ts b/web/src/api/types.ts index c58cb70..2fbd216 100644 --- a/web/src/api/types.ts +++ b/web/src/api/types.ts @@ -126,6 +126,7 @@ export const POLICY_TYPES = [ 'RequiredMetadata', 'AllowedEnvironments', 'RenewalLeadTime', + 'CertificateLifetime', ] as const; export type PolicyType = (typeof POLICY_TYPES)[number];