mirror of
https://github.com/shankar0123/certctl.git
synced 2026-06-13 01:18:51 +00:00
fix(crypto/local-ca): reject expired or not-yet-valid sub-CA certificates on disk load (M-5)
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: 76d383b (M-8 per-ciphertext salt).
Closes: audit finding M-5 in certctl-audit-report.md.
This commit is contained in:
@@ -359,6 +359,25 @@ func (c *Connector) loadCAFromDisk() error {
|
|||||||
return fmt.Errorf("loaded CA certificate does not have KeyUsageCertSign")
|
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)
|
// Load CA private key (supports RSA and ECDSA)
|
||||||
keyPEM, err := os.ReadFile(c.config.CAKeyPath)
|
keyPEM, err := os.ReadFile(c.config.CAKeyPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@@ -14,6 +14,7 @@ import (
|
|||||||
"math/big"
|
"math/big"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
@@ -360,6 +361,114 @@ func TestSubCAMode(t *testing.T) {
|
|||||||
t.Logf("Correctly rejected non-CA cert: %v", err)
|
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) {
|
t.Run("SubCA_RenewCertificate", func(t *testing.T) {
|
||||||
certPath, keyPath := generateTestSubCA(t, "rsa")
|
certPath, keyPath := generateTestSubCA(t, "rsa")
|
||||||
defer os.Remove(certPath)
|
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.
|
// 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) {
|
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()
|
t.Helper()
|
||||||
tmpDir := t.TempDir()
|
tmpDir := t.TempDir()
|
||||||
certPath = filepath.Join(tmpDir, "ca.pem")
|
certPath = filepath.Join(tmpDir, "ca.pem")
|
||||||
@@ -445,8 +562,8 @@ func generateTestSubCA(t *testing.T, keyType string) (certPath, keyPath string)
|
|||||||
CommonName: "Test Sub-CA",
|
CommonName: "Test Sub-CA",
|
||||||
Organization: []string{"CertCtl Test"},
|
Organization: []string{"CertCtl Test"},
|
||||||
},
|
},
|
||||||
NotBefore: time.Now(),
|
NotBefore: notBefore,
|
||||||
NotAfter: time.Now().AddDate(5, 0, 0),
|
NotAfter: notAfter,
|
||||||
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
|
KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
|
||||||
BasicConstraintsValid: true,
|
BasicConstraintsValid: true,
|
||||||
IsCA: true,
|
IsCA: true,
|
||||||
|
|||||||
Reference in New Issue
Block a user