fix(cert): reload PEM on warm restart and rebuild ACME on config change

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)
+//  1. "http" (if auto_tls is true)
-//	2. "content" (if both CertContent and KeyContent are provided)
+//  2. "content" (if both CertContent and KeyContent are provided)
-//	3. "file" (if both CertFile and KeyFile paths are provided)
+//  3. "file" (if both CertFile and KeyFile paths are provided)
-//	4. "none" (default)
+//  4. "none" (default)
 type Manager struct {
 	cfg config.CertConfig
 
-	// Cert material stored atomically so the ACME renewal goroutine can
+	// Atomic so the ACME renewal goroutine can swap without racing readers.
-	// swap it without racing with TLSCert() readers.
 	mat atomic.Pointer[certMaterial]
 
-	magic       *certmagic.Config
+	magic           *certmagic.Config
-	renewed     atomic.Bool
+	renewed         atomic.Bool
-	acmeStarted 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.
+	// Idempotent for unchanged config: Reconfigure already tore down stale ACME state.
-	// A process restart is required to change ACME settings at runtime.
 	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" {
+			// Only react to renewals; initial load happens explicitly after ObtainCertSync.
-				m.loadCertFromStorage(evtCtx, storage, data)
+			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.
+	issuerKey := magic.Issuers[0].IssuerKey()
-	if !m.HasCert() {
+	if err := m.loadPEMFromStorage(acmeCtx, storage, issuerKey, m.cfg.Domain); err != nil {
-		return fmt.Errorf("ACME obtained certificate but PEM was not loaded; check storage")
+		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.
+// loadPEMFromStorage reads cert + key for domain from certmagic storage into
-func (m *Manager) loadCertFromStorage(ctx context.Context, storage certmagic.Storage, data map[string]any) {
+// the in-memory cache. Single entry point for both initial load and renewal refresh.
-	certKey, _ := data["certificate_path"].(string)
+func (m *Manager) loadPEMFromStorage(ctx context.Context, storage certmagic.Storage, issuerKey, domain string) error {
-	keyKey, _ := data["private_key_path"].(string)
+	if issuerKey == "" || domain == "" {
-	if certKey == "" || keyKey == "" {
+		return fmt.Errorf("loadPEMFromStorage: issuerKey and domain are required")
-		nlog.Core().Warn("cert_obtained event missing storage paths", "data", data)
-		return
 	}
+	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 fmt.Errorf("load cert %q: %w", certKey, err)
-		return
 	}
 	keyPEM, err := storage.Load(ctx, keyKey)
 	if err != nil {
-		nlog.Core().Error("failed to load key from ACME storage", "error", err)
+		return fmt.Errorf("load key %q: %w", keyKey, err)
-		return
+	}
+	if err := validateKeyPair(certPEM, keyPEM); err != nil {
+		return fmt.Errorf("invalid cert/key pair on disk: %w", err)
 	}
-
 	m.storePEM(certPEM, keyPEM)
-
+	return nil
-	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)
-	}
 }

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")
+	}
+}