diff --git a/internal/cert/cert.go b/internal/cert/cert.go index 47bd9be..0ab3c3d 100644 --- a/internal/cert/cert.go +++ b/internal/cert/cert.go @@ -5,15 +5,18 @@ import ( "crypto/ecdsa" "crypto/elliptic" "crypto/rand" + "crypto/sha256" "crypto/tls" "crypto/x509" "crypto/x509/pkix" + "encoding/hex" "encoding/pem" "fmt" "math/big" "net" "os" "path/filepath" + "sort" "strings" "sync/atomic" "time" @@ -41,20 +44,21 @@ import ( // // Priority Logic (when CertMode is empty): // -// 1. "http" (if auto_tls is true) -// 2. "content" (if both CertContent and KeyContent are provided) -// 3. "file" (if both CertFile and KeyFile paths are provided) -// 4. "none" (default) +// 1. "http" (if auto_tls is true) +// 2. "content" (if both CertContent and KeyContent are provided) +// 3. "file" (if both CertFile and KeyFile paths are provided) +// 4. "none" (default) type Manager struct { cfg config.CertConfig - // Cert material stored atomically so the ACME renewal goroutine can - // swap it without racing with TLSCert() readers. + // Atomic so the ACME renewal goroutine can swap without racing readers. mat atomic.Pointer[certMaterial] - magic *certmagic.Config - renewed atomic.Bool - acmeStarted bool + magic *certmagic.Config + renewed atomic.Bool + acmeStarted bool + acmeFingerprint string + acmeCancel context.CancelFunc } // certMaterial is an immutable snapshot of PEM-encoded cert + key. @@ -131,14 +135,22 @@ func (m *Manager) loadPersistedPEM() bool { } // Reconfigure applies a new cert configuration at runtime (e.g. from panel push). -// For ACME modes that are already running this is a no-op — a process restart -// is required to change ACME settings. // Returns true when the PEM material changed (caller should restart kernel). func (m *Manager) Reconfigure(ctx context.Context, newCfg config.CertConfig) (bool, error) { if newCfg.CertDir == "" { newCfg.CertDir = m.cfg.CertDir } + // If ACME is running and the new config materially differs (or switches + // away from ACME), tear down the old certmagic instance first. + if m.acmeStarted { + newFp := acmeFingerprint(newCfg) + newMode := resolveModeFor(newCfg) + if newMode != "http" && newMode != "dns" || newFp != m.acmeFingerprint { + m.tearDownACME() + } + } + oldTLS := m.TLSCert() m.cfg = newCfg @@ -150,6 +162,65 @@ func (m *Manager) Reconfigure(ctx context.Context, newCfg config.CertConfig) (bo return !pemEqual(oldTLS, newTLS), nil } +// tearDownACME cancels the running certmagic background goroutines and clears +// related state so a fresh ACME init can run. +func (m *Manager) tearDownACME() { + if m.acmeCancel != nil { + m.acmeCancel() + m.acmeCancel = nil + } + m.magic = nil + m.acmeStarted = false + m.acmeFingerprint = "" + m.mat.Store(nil) +} + +// acmeFingerprint produces a stable string capturing every config field that +// requires re-running ACME when changed. +func acmeFingerprint(cfg config.CertConfig) string { + keys := make([]string, 0, len(cfg.DNSEnv)) + for k := range cfg.DNSEnv { + keys = append(keys, k) + } + sort.Strings(keys) + var b strings.Builder + b.WriteString(strings.ToLower(strings.TrimSpace(cfg.CertMode))) + b.WriteByte('|') + b.WriteString(strings.TrimSpace(cfg.Domain)) + b.WriteByte('|') + b.WriteString(strings.TrimSpace(cfg.Email)) + b.WriteByte('|') + b.WriteString(strings.TrimSpace(cfg.DNSProvider)) + b.WriteByte('|') + for _, k := range keys { + b.WriteString(k) + b.WriteByte('=') + b.WriteString(cfg.DNSEnv[k]) + b.WriteByte(';') + } + sum := sha256.Sum256([]byte(b.String())) + return hex.EncodeToString(sum[:]) +} + +// resolveModeFor mirrors (*Manager).resolveMode() for an arbitrary cfg, used +// by Reconfigure to decide tear-down without mutating manager state. +func resolveModeFor(cfg config.CertConfig) string { + mode := strings.ToLower(strings.TrimSpace(cfg.CertMode)) + if mode != "" { + return mode + } + if cfg.AutoTLS { + return "http" + } + if cfg.CertContent != "" && cfg.KeyContent != "" { + return "content" + } + if cfg.CertFile != "" && cfg.KeyFile != "" { + return "file" + } + return "none" +} + // resolveMode returns the effective cert mode, handling backward compat for auto_tls. func (m *Manager) resolveMode() string { mode := strings.ToLower(strings.TrimSpace(m.cfg.CertMode)) @@ -320,10 +391,8 @@ func (m *Manager) startContent() error { // ─── Mode: http / dns (ACME) ────────────────────────────────────────────── func (m *Manager) startACME(ctx context.Context, dnsSolver *certmagic.DNS01Solver) error { - // Guard: certmagic's background goroutines must only be started once. - // A process restart is required to change ACME settings at runtime. + // Idempotent for unchanged config: Reconfigure already tore down stale ACME state. if m.acmeStarted { - nlog.Core().Info("cert: ACME already active, skipping re-init (restart to change ACME settings)") return nil } @@ -347,9 +416,23 @@ func (m *Manager) startACME(ctx context.Context, dnsSolver *certmagic.DNS01Solve magic = certmagic.New(cache, certmagic.Config{ Storage: storage, OnEvent: func(evtCtx context.Context, event string, data map[string]any) error { - if event == "cert_obtained" { - m.loadCertFromStorage(evtCtx, storage, data) + // Only react to renewals; initial load happens explicitly after ObtainCertSync. + if event != "cert_obtained" { + return nil } + if renewed, _ := data["renewal"].(bool); !renewed { + return nil + } + issuerKey, _ := data["issuer"].(string) + if issuerKey == "" { + return nil + } + if err := m.loadPEMFromStorage(evtCtx, storage, issuerKey, m.cfg.Domain); err != nil { + nlog.Core().Error("failed to reload cert after renewal", "error", err) + return nil + } + m.renewed.Store(true) + nlog.Core().Info("TLS certificate reloaded after renewal", "domain", m.cfg.Domain) return nil }, }) @@ -379,19 +462,27 @@ func (m *Manager) startACME(ctx context.Context, dnsSolver *certmagic.DNS01Solve } m.magic = magic - if err := magic.ObtainCertSync(ctx, m.cfg.Domain); err != nil { + // Derived ctx so Reconfigure can cancel ACME background goroutines. + acmeCtx, acmeCancel := context.WithCancel(ctx) + + if err := magic.ObtainCertSync(acmeCtx, m.cfg.Domain); err != nil { + acmeCancel() return fmt.Errorf("obtain certificate: %w", err) } - // The cert_obtained event callback should have loaded PEM. Verify. - if !m.HasCert() { - return fmt.Errorf("ACME obtained certificate but PEM was not loaded; check storage") + issuerKey := magic.Issuers[0].IssuerKey() + if err := m.loadPEMFromStorage(acmeCtx, storage, issuerKey, m.cfg.Domain); err != nil { + acmeCancel() + return fmt.Errorf("load cert from storage: %w", err) } - if err := magic.ManageAsync(ctx, []string{m.cfg.Domain}); err != nil { + if err := magic.ManageAsync(acmeCtx, []string{m.cfg.Domain}); err != nil { + acmeCancel() return fmt.Errorf("start cert manager: %w", err) } + m.acmeCancel = acmeCancel + m.acmeFingerprint = acmeFingerprint(m.cfg) m.acmeStarted = true return nil } @@ -433,31 +524,26 @@ func (m *Manager) newDNSProvider() (certmagic.DNSProvider, error) { // ─── Helpers ─────────────────────────────────────────────────────────────── -// loadCertFromStorage reads ACME cert/key from certmagic's storage into memory. -func (m *Manager) loadCertFromStorage(ctx context.Context, storage certmagic.Storage, data map[string]any) { - certKey, _ := data["certificate_path"].(string) - keyKey, _ := data["private_key_path"].(string) - if certKey == "" || keyKey == "" { - nlog.Core().Warn("cert_obtained event missing storage paths", "data", data) - return +// loadPEMFromStorage reads cert + key for domain from certmagic storage into +// the in-memory cache. Single entry point for both initial load and renewal refresh. +func (m *Manager) loadPEMFromStorage(ctx context.Context, storage certmagic.Storage, issuerKey, domain string) error { + if issuerKey == "" || domain == "" { + return fmt.Errorf("loadPEMFromStorage: issuerKey and domain are required") } + certKey := certmagic.StorageKeys.SiteCert(issuerKey, domain) + keyKey := certmagic.StorageKeys.SitePrivateKey(issuerKey, domain) certPEM, err := storage.Load(ctx, certKey) if err != nil { - nlog.Core().Error("failed to load cert from ACME storage", "error", err) - return + return fmt.Errorf("load cert %q: %w", certKey, err) } keyPEM, err := storage.Load(ctx, keyKey) if err != nil { - nlog.Core().Error("failed to load key from ACME storage", "error", err) - return + return fmt.Errorf("load key %q: %w", keyKey, err) + } + if err := validateKeyPair(certPEM, keyPEM); err != nil { + return fmt.Errorf("invalid cert/key pair on disk: %w", err) } - m.storePEM(certPEM, keyPEM) - - renewal, _ := data["renewal"].(bool) - nlog.Core().Info("TLS certificate loaded from ACME storage", "domain", m.cfg.Domain, "renewal", renewal) - if renewal { - m.renewed.Store(true) - } + return nil } diff --git a/internal/cert/cert_test.go b/internal/cert/cert_test.go new file mode 100644 index 0000000..68bc5bc --- /dev/null +++ b/internal/cert/cert_test.go @@ -0,0 +1,236 @@ +package cert + +import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/pem" + "math/big" + "path/filepath" + "testing" + "time" + + "github.com/caddyserver/certmagic" + + "github.com/cedar2025/xboard-node/internal/config" +) + +// generateSelfSignedPair returns a fresh self-signed certificate and matching +// private key in PEM form, suitable for storing as a libdns/certmagic site +// resource in tests. +func generateSelfSignedPair(t *testing.T, domain string) (certPEM, keyPEM []byte) { + t.Helper() + priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("gen key: %v", err) + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: domain}, + DNSNames: []string{domain}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(24 * time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &priv.PublicKey, priv) + if err != nil { + t.Fatalf("create cert: %v", err) + } + certPEM = pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: der}) + keyDER, err := x509.MarshalECPrivateKey(priv) + if err != nil { + t.Fatalf("marshal key: %v", err) + } + keyPEM = pem.EncodeToMemory(&pem.Block{Type: "EC PRIVATE KEY", Bytes: keyDER}) + return +} + +// TestLoadPEMFromStorage_RestartScenario simulates the warm-restart bug fix: +// certmagic.ObtainCertSync returns nil without firing cert_obtained when a +// valid cert already exists in storage. The manager must still populate its +// in-memory PEM cache by reading storage explicitly. +func TestLoadPEMFromStorage_RestartScenario(t *testing.T) { + dir := t.TempDir() + storage := &certmagic.FileStorage{Path: dir} + const domain = "example.test" + const issuerKey = "test-issuer" + + certPEM, keyPEM := generateSelfSignedPair(t, domain) + + ctx := context.Background() + if err := storage.Store(ctx, certmagic.StorageKeys.SiteCert(issuerKey, domain), certPEM); err != nil { + t.Fatalf("seed cert: %v", err) + } + if err := storage.Store(ctx, certmagic.StorageKeys.SitePrivateKey(issuerKey, domain), keyPEM); err != nil { + t.Fatalf("seed key: %v", err) + } + + m := NewManager(config.CertConfig{}) + if m.HasCert() { + t.Fatal("manager should start with no cert in memory") + } + + if err := m.loadPEMFromStorage(ctx, storage, issuerKey, domain); err != nil { + t.Fatalf("loadPEMFromStorage: %v", err) + } + if !m.HasCert() { + t.Fatal("expected HasCert() == true after explicit load") + } + got := m.TLSCert() + if string(got.CertPEM) != string(certPEM) { + t.Errorf("certPEM mismatch") + } + if string(got.KeyPEM) != string(keyPEM) { + t.Errorf("keyPEM mismatch") + } +} + +// TestLoadPEMFromStorage_RenewalRefresh confirms that calling load a second +// time with newer bytes on disk swaps the in-memory cache atomically — this +// is the renewal hot-reload path triggered from the cert_obtained event. +func TestLoadPEMFromStorage_RenewalRefresh(t *testing.T) { + dir := t.TempDir() + storage := &certmagic.FileStorage{Path: dir} + const domain = "example.test" + const issuerKey = "test-issuer" + ctx := context.Background() + + certV1, keyV1 := generateSelfSignedPair(t, domain) + if err := storage.Store(ctx, certmagic.StorageKeys.SiteCert(issuerKey, domain), certV1); err != nil { + t.Fatalf("seed v1 cert: %v", err) + } + if err := storage.Store(ctx, certmagic.StorageKeys.SitePrivateKey(issuerKey, domain), keyV1); err != nil { + t.Fatalf("seed v1 key: %v", err) + } + + m := NewManager(config.CertConfig{}) + if err := m.loadPEMFromStorage(ctx, storage, issuerKey, domain); err != nil { + t.Fatalf("initial load: %v", err) + } + + certV2, keyV2 := generateSelfSignedPair(t, domain) + if err := storage.Store(ctx, certmagic.StorageKeys.SiteCert(issuerKey, domain), certV2); err != nil { + t.Fatalf("seed v2 cert: %v", err) + } + if err := storage.Store(ctx, certmagic.StorageKeys.SitePrivateKey(issuerKey, domain), keyV2); err != nil { + t.Fatalf("seed v2 key: %v", err) + } + + if err := m.loadPEMFromStorage(ctx, storage, issuerKey, domain); err != nil { + t.Fatalf("refresh load: %v", err) + } + got := m.TLSCert() + if string(got.CertPEM) != string(certV2) { + t.Error("expected v2 cert after refresh") + } + if string(got.KeyPEM) != string(keyV2) { + t.Error("expected v2 key after refresh") + } +} + +// TestLoadPEMFromStorage_MissingFiles ensures we surface a clear error when +// storage doesn't have the expected keys (defensive — should not happen in +// practice because ObtainCertSync guarantees presence on success). +func TestLoadPEMFromStorage_MissingFiles(t *testing.T) { + dir := t.TempDir() + storage := &certmagic.FileStorage{Path: dir} + m := NewManager(config.CertConfig{}) + err := m.loadPEMFromStorage(context.Background(), storage, "test-issuer", "example.test") + if err == nil { + t.Fatal("expected error when storage is empty") + } +} + +// TestLoadPEMFromStorage_RejectsMismatchedPair guards against silently loading +// a cert and key that do not pair (e.g. partial write or external tampering). +func TestLoadPEMFromStorage_RejectsMismatchedPair(t *testing.T) { + dir := t.TempDir() + storage := &certmagic.FileStorage{Path: dir} + const domain = "example.test" + const issuerKey = "test-issuer" + ctx := context.Background() + + certA, _ := generateSelfSignedPair(t, domain) + _, keyB := generateSelfSignedPair(t, domain) + if err := storage.Store(ctx, certmagic.StorageKeys.SiteCert(issuerKey, domain), certA); err != nil { + t.Fatalf("seed cert: %v", err) + } + if err := storage.Store(ctx, certmagic.StorageKeys.SitePrivateKey(issuerKey, domain), keyB); err != nil { + t.Fatalf("seed key: %v", err) + } + + m := NewManager(config.CertConfig{}) + err := m.loadPEMFromStorage(ctx, storage, issuerKey, domain) + if err == nil { + t.Fatal("expected error for mismatched cert/key pair") + } + if m.HasCert() { + t.Error("manager should not store invalid pair") + } + _ = filepath.Join(dir, "stub") +} + +// TestACMEFingerprint covers the config-equality logic that decides whether +// Reconfigure must tear down a running certmagic instance. +func TestACMEFingerprint(t *testing.T) { + base := config.CertConfig{ + CertMode: "dns", + Domain: "a.example.com", + Email: "ops@example.com", + DNSProvider: "cloudflare", + DNSEnv: map[string]string{"CLOUDFLARE_API_TOKEN": "tok"}, + } + + cases := []struct { + name string + mutate func(*config.CertConfig) + differs bool + }{ + {"same", func(*config.CertConfig) {}, false}, + {"domain", func(c *config.CertConfig) { c.Domain = "b.example.com" }, true}, + {"mode", func(c *config.CertConfig) { c.CertMode = "http" }, true}, + {"email", func(c *config.CertConfig) { c.Email = "other@example.com" }, true}, + {"provider", func(c *config.CertConfig) { c.DNSProvider = "alidns" }, true}, + {"env_value", func(c *config.CertConfig) { c.DNSEnv = map[string]string{"CLOUDFLARE_API_TOKEN": "other"} }, true}, + {"env_key_only", func(c *config.CertConfig) { c.DNSEnv = map[string]string{"OTHER": "tok"} }, true}, + } + + baseFp := acmeFingerprint(base) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := base + if tc.mutate != nil { + // deep-copy DNSEnv before mutation to avoid sharing + cfg.DNSEnv = map[string]string{} + for k, v := range base.DNSEnv { + cfg.DNSEnv[k] = v + } + tc.mutate(&cfg) + } + got := acmeFingerprint(cfg) + if (got != baseFp) != tc.differs { + t.Errorf("differs=%v but got==base=%v", tc.differs, got == baseFp) + } + }) + } +} + +// TestACMEFingerprintEnvOrderStable ensures map iteration order does not +// affect the fingerprint (must be sorted internally). +func TestACMEFingerprintEnvOrderStable(t *testing.T) { + a := config.CertConfig{ + CertMode: "dns", Domain: "x", DNSProvider: "p", + DNSEnv: map[string]string{"A": "1", "B": "2", "C": "3"}, + } + b := config.CertConfig{ + CertMode: "dns", Domain: "x", DNSProvider: "p", + DNSEnv: map[string]string{"C": "3", "A": "1", "B": "2"}, + } + if acmeFingerprint(a) != acmeFingerprint(b) { + t.Fatal("fingerprint not order-independent") + } +}