From c655e0f8c51a3a559b52b2521d57569a1314020f Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Fri, 17 Apr 2026 14:10:23 +0000 Subject: [PATCH] fix(crypto/local-ca): reject expired or not-yet-valid sub-CA certificates on disk load (M-5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit loadCAFromDisk now validates the upstream sub-CA certificate's NotBefore and NotAfter fields before accepting it, returning a fail-closed error at server startup instead of silently loading an out-of-window CA. Before this fix, loadCAFromDisk checked BasicConstraints.IsCA and KeyUsage=CertSign but not the validity window. An expired enterprise sub-CA (e.g. an ADCS subordinate whose rollover slipped) would load without warning and the scheduler would mint child certs that every RFC 5280 path validator rejects — outages show up at relying parties, not at certctl, and only after thresholds trip. CWE-672 (Operation on a Resource after Expiration or Release); secondary CWE-295 (Improper Certificate Validation). Error strings include the CA subject CommonName and both RFC3339 timestamps so the log line is actionable in a 3am incident. Tests: TestSubCAMode gains three subtests exercising the new gate — SubCA_ExpiredCert_IsRejected (CA expired 1h ago → error mentions 'expired' and the CN), SubCA_NotYetValid_IsRejected (CA valid +1h → error mentions 'not yet valid' and the CN), and SubCA_BarelyValid_IsAccepted (CA valid [now-1m, now+1h] → issuance succeeds, proving no over-rejection). Adds generateTestSubCAWithValidity helper; the original generateTestSubCA wrapper preserves the [now, now+5y] default for existing tests. Package coverage: 67.7% -> 68.3%. Verification: go build, go vet, go test -race, go test -cover all green locally; golangci-lint v2.11.4 clean; govulncheck clean. All CI coverage floors met with margin (service 67.6/55, handler 78.6/60, domain 92.7/40, middleware 80.0/30, crypto 86.7/85). Parent: 5abeeb8 (M-8 per-ciphertext salt). Closes: audit finding M-5 in certctl-audit-report.md. --- internal/connector/issuer/local/local.go | 19 +++ internal/connector/issuer/local/local_test.go | 123 +++++++++++++++++- 2 files changed, 139 insertions(+), 3 deletions(-) diff --git a/internal/connector/issuer/local/local.go b/internal/connector/issuer/local/local.go index d6e9f2c..848179b 100644 --- a/internal/connector/issuer/local/local.go +++ b/internal/connector/issuer/local/local.go @@ -359,6 +359,25 @@ func (c *Connector) loadCAFromDisk() error { return fmt.Errorf("loaded CA certificate does not have KeyUsageCertSign") } + // Validate CA certificate validity window (M-5, CWE-672). + // An expired or not-yet-valid sub-CA produces child certificates that any + // RFC 5280 path-validator will reject. Fail closed at load time so operators + // learn about it at startup, not at 3am when a renewal cycle silently + // starts minting broken certs. See audit finding M-5. + now := time.Now() + if now.After(caCert.NotAfter) { + return fmt.Errorf("CA certificate %q has expired (not_after=%s, now=%s)", + caCert.Subject.CommonName, + caCert.NotAfter.UTC().Format(time.RFC3339), + now.UTC().Format(time.RFC3339)) + } + if now.Before(caCert.NotBefore) { + return fmt.Errorf("CA certificate %q is not yet valid (not_before=%s, now=%s)", + caCert.Subject.CommonName, + caCert.NotBefore.UTC().Format(time.RFC3339), + now.UTC().Format(time.RFC3339)) + } + // Load CA private key (supports RSA and ECDSA) keyPEM, err := os.ReadFile(c.config.CAKeyPath) if err != nil { diff --git a/internal/connector/issuer/local/local_test.go b/internal/connector/issuer/local/local_test.go index 558486b..49c3d05 100644 --- a/internal/connector/issuer/local/local_test.go +++ b/internal/connector/issuer/local/local_test.go @@ -14,6 +14,7 @@ import ( "math/big" "os" "path/filepath" + "strings" "testing" "time" @@ -360,6 +361,114 @@ func TestSubCAMode(t *testing.T) { t.Logf("Correctly rejected non-CA cert: %v", err) }) + t.Run("SubCA_ExpiredCert_IsRejected", func(t *testing.T) { + // Sub-CA expired 1 hour ago. M-5: loadCAFromDisk must fail closed + // instead of minting child certs that immediately fail path validation + // at every relying party (CWE-672). + notBefore := time.Now().AddDate(-1, 0, 0) + notAfter := time.Now().Add(-1 * time.Hour) + certPath, keyPath := generateTestSubCAWithValidity(t, "rsa", notBefore, notAfter) + + config := &local.Config{ + ValidityDays: 30, + CACertPath: certPath, + CAKeyPath: keyPath, + } + connector := local.New(config, logger) + + _, csrPEM, err := generateTestCSR("app.internal.corp") + if err != nil { + t.Fatalf("Failed to generate CSR: %v", err) + } + req := issuer.IssuanceRequest{ + CommonName: "app.internal.corp", + CSRPEM: csrPEM, + } + + _, err = connector.IssueCertificate(ctx, req) + if err == nil { + t.Fatal("Expected error when loading expired sub-CA; got nil") + } + if !strings.Contains(err.Error(), "expired") { + t.Errorf("Expected error to mention 'expired'; got: %v", err) + } + if !strings.Contains(err.Error(), "Test Sub-CA") { + t.Errorf("Expected error to include CA subject CN 'Test Sub-CA'; got: %v", err) + } + t.Logf("Correctly rejected expired sub-CA: %v", err) + }) + + t.Run("SubCA_NotYetValid_IsRejected", func(t *testing.T) { + // Sub-CA is not valid for another hour (clock skew or operator error + // pushing a pre-production CA into prod). M-5: loadCAFromDisk must + // fail closed. + notBefore := time.Now().Add(1 * time.Hour) + notAfter := time.Now().AddDate(5, 0, 0) + certPath, keyPath := generateTestSubCAWithValidity(t, "rsa", notBefore, notAfter) + + config := &local.Config{ + ValidityDays: 30, + CACertPath: certPath, + CAKeyPath: keyPath, + } + connector := local.New(config, logger) + + _, csrPEM, err := generateTestCSR("app.internal.corp") + if err != nil { + t.Fatalf("Failed to generate CSR: %v", err) + } + req := issuer.IssuanceRequest{ + CommonName: "app.internal.corp", + CSRPEM: csrPEM, + } + + _, err = connector.IssueCertificate(ctx, req) + if err == nil { + t.Fatal("Expected error when loading not-yet-valid sub-CA; got nil") + } + if !strings.Contains(err.Error(), "not yet valid") { + t.Errorf("Expected error to mention 'not yet valid'; got: %v", err) + } + if !strings.Contains(err.Error(), "Test Sub-CA") { + t.Errorf("Expected error to include CA subject CN 'Test Sub-CA'; got: %v", err) + } + t.Logf("Correctly rejected not-yet-valid sub-CA: %v", err) + }) + + t.Run("SubCA_BarelyValid_IsAccepted", func(t *testing.T) { + // Sub-CA valid from 1 minute ago to 1 hour from now. Edge case: + // proves the M-5 window check doesn't over-reject CAs that are + // legitimately live but close to the boundaries. + notBefore := time.Now().Add(-1 * time.Minute) + notAfter := time.Now().Add(1 * time.Hour) + certPath, keyPath := generateTestSubCAWithValidity(t, "rsa", notBefore, notAfter) + + config := &local.Config{ + ValidityDays: 30, + CACertPath: certPath, + CAKeyPath: keyPath, + } + connector := local.New(config, logger) + + _, csrPEM, err := generateTestCSR("app.internal.corp") + if err != nil { + t.Fatalf("Failed to generate CSR: %v", err) + } + req := issuer.IssuanceRequest{ + CommonName: "app.internal.corp", + CSRPEM: csrPEM, + } + + result, err := connector.IssueCertificate(ctx, req) + if err != nil { + t.Fatalf("Barely-valid sub-CA was wrongly rejected: %v", err) + } + if result.CertPEM == "" { + t.Error("CertPEM is empty") + } + t.Logf("Correctly accepted barely-valid sub-CA: serial=%s", result.Serial) + }) + t.Run("SubCA_RenewCertificate", func(t *testing.T) { certPath, keyPath := generateTestSubCA(t, "rsa") defer os.Remove(certPath) @@ -396,8 +505,16 @@ func TestSubCAMode(t *testing.T) { } // generateTestSubCA creates a self-signed CA cert+key pair and writes them to temp files. -// keyType can be "rsa" or "ecdsa". +// keyType can be "rsa" or "ecdsa". Validity window is [now, now+5y]. func generateTestSubCA(t *testing.T, keyType string) (certPath, keyPath string) { + t.Helper() + return generateTestSubCAWithValidity(t, keyType, time.Now(), time.Now().AddDate(5, 0, 0)) +} + +// generateTestSubCAWithValidity creates a self-signed CA cert+key pair with an +// explicit NotBefore/NotAfter window. Used by M-5 tests that exercise expired +// and not-yet-valid CA rejection in loadCAFromDisk. +func generateTestSubCAWithValidity(t *testing.T, keyType string, notBefore, notAfter time.Time) (certPath, keyPath string) { t.Helper() tmpDir := t.TempDir() certPath = filepath.Join(tmpDir, "ca.pem") @@ -445,8 +562,8 @@ func generateTestSubCA(t *testing.T, keyType string) (certPath, keyPath string) CommonName: "Test Sub-CA", Organization: []string{"CertCtl Test"}, }, - NotBefore: time.Now(), - NotAfter: time.Now().AddDate(5, 0, 0), + NotBefore: notBefore, + NotAfter: notAfter, KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, BasicConstraintsValid: true, IsCA: true,