From 5949de4b173a331bf303c3b6e1c3021bf521ea6b Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sun, 9 Aug 2026 20:24:22 +0200 Subject: [PATCH 1/8] feat(cert-store): export/import of the certificate estate for disaster recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary The certificate estate (acme.state, certs/ incl. the ACME account key, dynamic-domains.state) was per-node disk with no supported backup path; losing the node meant re-issuing everything under the issuance rate limit. `kamal-proxy export certs ` snapshots it atomically — through the running proxy under the store's disk-write lock, or offline against a stopped data dir. `import certs --archive` restores it (refusing a non-empty store without --force), and `--verify` parses every certificate in an archive and reports domains + expiries without touching the store. To make the export lock real, all disk writes to the store now happen under the manager's stateMu: saveCertificate writes through the staged tmp+rename path shared with the importers, and removeCertificate holds the lock across file removal + state write. ## Test Coverage - cert_store_export_test.go: archive layout, 0600 mode, atomicity, empty store refusal, warnings, lock-holding ExportStore - cert_store_restore_test.go: round trip into a bootable manager, non-empty-store refusal, --force semantics, traversal/symlink/torn-pair rejection, verify reporting - commands_test.go / export_test.go: RPC handler and CLI wiring, flag group validation ## Verification - [x] gofmt -l internal/ cmd/ clean - [x] make test passes (go vet, golangci-lint 0 issues) - [x] go test -race ./internal/server/ passes Closes #90 --- README.md | 49 +++ internal/cmd/export.go | 94 ++++++ internal/cmd/export_test.go | 129 ++++++++ internal/cmd/import.go | 118 ++++++- internal/cmd/root.go | 1 + internal/server/cert_store_archive.go | 234 ++++++++++++++ internal/server/cert_store_export.go | 297 ++++++++++++++++++ internal/server/cert_store_export_test.go | 290 +++++++++++++++++ internal/server/cert_store_restore.go | 172 +++++++++++ internal/server/cert_store_restore_test.go | 344 +++++++++++++++++++++ internal/server/commands.go | 33 ++ internal/server/commands_test.go | 29 ++ internal/server/config.go | 10 + internal/server/san_cert_dynamic.go | 8 +- internal/server/san_cert_manager.go | 56 ++-- internal/server/traefik_import.go | 15 +- 16 files changed, 1825 insertions(+), 54 deletions(-) create mode 100644 internal/cmd/export.go create mode 100644 internal/cmd/export_test.go create mode 100644 internal/server/cert_store_archive.go create mode 100644 internal/server/cert_store_export.go create mode 100644 internal/server/cert_store_export_test.go create mode 100644 internal/server/cert_store_restore.go create mode 100644 internal/server/cert_store_restore_test.go diff --git a/README.md b/README.md index 6e6817d..fb4c14d 100644 --- a/README.md +++ b/README.md @@ -1179,6 +1179,55 @@ If you want a name approved at handshake time by your own application rather than at deploy time, that is what [on-demand TLS](#on-demand-tls) is for. Your endpoint answers `2xx` to approve, anything else to deny. +### Backing up and restoring certificates + +All certificate state lives on the proxy node's disk: the ACME account key, +every issued certificate with its private key, the domain-to-certificate +mappings, and the dynamic domain list. Losing that disk means re-issuing the +whole estate under the issuance rate limit (about 250 orders per 3 hours) — +for a large estate, hours of hard TLS failures. Export makes node loss a +restore instead of an outage: + +```bash +kamal-proxy export certs /backup/certs-$(date +%F).tar.gz +``` + +With the proxy running, the snapshot is taken through the proxy, under the +same lock the certificate managers use for writes, so a backup taken +mid-renewal is never torn. With no proxy reachable on its socket, the data +directory is read directly — only do that when the proxy is actually stopped. + +**The archive contains private keys** — every certificate key and the ACME +account key. It is written with mode `0600`; store and transfer it as the +secret it is. + +Backups are only as good as their last verification. `--verify` parses every +certificate in an archive and reports domains and expiries without touching +the store, so a cron job or CI can check each backup as it is taken: + +```bash +kamal-proxy import certs --archive /backup/certs-2026-08-09.tar.gz --verify +``` + +**Restore runbook** (new node, rebuilt host, or a volume mistake): + +1. Stop the proxy. +2. Restore the estate: `kamal-proxy import certs --archive /backup/certs-2026-08-09.tar.gz` + (add `--data-dir` if the proxy runs with one). The import refuses to + overwrite a non-empty certificate store unless you pass `--force`. +3. Start the proxy. +4. Verify: `kamal-proxy domains list` and the certificate expiry metrics + should show the restored estate; handshakes for restored domains serve + immediately, with no new ACME orders. + +Restores run offline against the data directory, sharing their writing path +with the Traefik `acme.json` importer (`import certs --traefik-acme`), so +there is one code path that knows how to populate the store correctly. + +A multi-node shared certificate store is deliberately not what this is: with +single-node TLS termination plus backups, losing the node is a restore, not +an outage. + ## Specifying `run` options with environment variables diff --git a/internal/cmd/export.go b/internal/cmd/export.go new file mode 100644 index 0000000..7e73feb --- /dev/null +++ b/internal/cmd/export.go @@ -0,0 +1,94 @@ +package cmd + +import ( + "fmt" + "net/rpc" + "path/filepath" + + "github.com/spf13/cobra" + + "github.com/basecamp/kamal-proxy/internal/server" +) + +type exportCommand struct { + cmd *cobra.Command +} + +func newExportCommand() *exportCommand { + exportCommand := &exportCommand{} + exportCommand.cmd = &cobra.Command{ + Use: "export", + Short: "Export proxy state for backup", + } + + exportCommand.cmd.AddCommand(newExportCertsCommand().cmd) + + return exportCommand +} + +// exportCertsCommand archives the certificate store for disaster recovery. +// Against a running proxy it exports over the RPC socket, under the same lock +// the certificate managers use for writes, so a backup taken mid-renewal is +// never torn. Without a reachable proxy it falls back to reading the data +// directory offline -- only safe when the proxy is actually stopped. +type exportCertsCommand struct { + cmd *cobra.Command +} + +func newExportCertsCommand() *exportCertsCommand { + exportCertsCommand := &exportCertsCommand{} + exportCertsCommand.cmd = &cobra.Command{ + Use: "certs [output-path]", + Short: "Export the certificate store to an archive for disaster recovery", + Long: "Export the certificate store -- ACME account key, issued certificates,\n" + + "domain mappings, and dynamic domain state -- to a gzipped tar archive.\n\n" + + "With the proxy running, the snapshot is taken through the proxy under its\n" + + "certificate write lock. With no proxy reachable on the socket, the data\n" + + "directory is read directly; only do that with the proxy stopped.\n\n" + + "The archive contains PRIVATE KEYS (certificate keys and the ACME account\n" + + "key). It is written with mode 0600; store and transfer it accordingly.", + RunE: exportCertsCommand.run, + Args: cobra.ExactArgs(1), + } + + exportCertsCommand.cmd.Flags().StringVar(&globalConfig.AlternateConfigDir, "data-dir", getEnvString("DATA_DIR", ""), "Directory for state and certificate storage (default $HOME/.config/kamal-proxy)") + + return exportCertsCommand +} + +func (c *exportCertsCommand) run(cmd *cobra.Command, args []string) error { + outputPath, err := filepath.Abs(args[0]) + if err != nil { + return fmt.Errorf("failed to resolve the output path: %w", err) + } + + summary, err := c.export(cmd, outputPath) + if err != nil { + return err + } + + for _, warning := range summary.Warnings { + fmt.Fprintf(cmd.ErrOrStderr(), "WARN %s\n", warning) + } + + fmt.Fprintf(cmd.OutOrStdout(), "Exported %d certificates (%d domains) to %s\n", + summary.Certificates, summary.Domains, outputPath) + + return nil +} + +// export snapshots through the running proxy when the socket answers, and +// falls back to reading the data directory offline when it does not. +func (c *exportCertsCommand) export(cmd *cobra.Command, outputPath string) (server.CertsExportSummary, error) { + var summary server.CertsExportSummary + + client, dialErr := rpc.Dial("unix", globalConfig.SocketPath()) + if dialErr == nil { + defer client.Close() + err := client.Call("kamal-proxy.CertsExport", server.CertsExportArgs{Path: outputPath}, &summary) + return summary, err + } + + fmt.Fprintln(cmd.ErrOrStderr(), "Proxy is not running; exporting offline from the data directory") + return server.ExportCertificateStore(globalConfig.CertStorePaths(), outputPath) +} diff --git a/internal/cmd/export_test.go b/internal/cmd/export_test.go new file mode 100644 index 0000000..2515231 --- /dev/null +++ b/internal/cmd/export_test.go @@ -0,0 +1,129 @@ +package cmd + +import ( + "bytes" + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/kamal-proxy/internal/server" +) + +// runExportCerts executes `export certs` with the given args and returns the +// combined output. The socket is pointed at nowhere so the command always +// falls back to the offline path. +func runExportCerts(t *testing.T, args ...string) (string, error) { + t.Helper() + + t.Setenv("KAMAL_PROXY_SOCKET", filepath.Join(t.TempDir(), "no-proxy.sock")) + + previousConfig := globalConfig + t.Cleanup(func() { + globalConfig = previousConfig + }) + globalConfig = server.Config{} + cmd := newExportCommand().cmd + + out := &bytes.Buffer{} + cmd.SetOut(out) + cmd.SetErr(out) + cmd.SetArgs(append([]string{"certs"}, args...)) + + err := cmd.Execute() + return out.String(), err +} + +// seedCertStore writes a minimal but valid store into dir. +func seedCertStore(t *testing.T, dir string) { + t.Helper() + + require.NoError(t, os.WriteFile(filepath.Join(dir, "acme.state"), + []byte(`{"certificates":{},"domain_map":{},"saved_at":"2026-08-09T00:00:00Z"}`), 0600)) +} + +func TestExportCertsCommand_RequiresAnOutputPath(t *testing.T) { + _, err := runExportCerts(t) + require.Error(t, err) +} + +func TestExportCertsCommand_ExportsOffline(t *testing.T) { + dir := t.TempDir() + seedCertStore(t, dir) + + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + out, err := runExportCerts(t, archivePath, "--data-dir", dir) + require.NoError(t, err) + + assert.Contains(t, out, "offline") + assert.Contains(t, out, "Exported 0 certificates (0 domains)") + assert.FileExists(t, archivePath) +} + +func TestExportCertsCommand_EmptyStoreFails(t *testing.T) { + _, err := runExportCerts(t, filepath.Join(t.TempDir(), "backup.tar.gz"), "--data-dir", t.TempDir()) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty") +} + +func TestImportCertsCommand_ArchiveRestoreRoundTrip(t *testing.T) { + source := t.TempDir() + seedCertStore(t, source) + + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + _, err := runExportCerts(t, archivePath, "--data-dir", source) + require.NoError(t, err) + + // Restore into an empty data dir. + target := t.TempDir() + out, err := runImportCerts(t, "--archive", archivePath, "--data-dir", target) + require.NoError(t, err) + assert.Contains(t, out, "Restored 0 certificates (0 domains)") + assert.FileExists(t, filepath.Join(target, "acme.state")) + + // A second restore refuses the now non-empty store... + _, err = runImportCerts(t, "--archive", archivePath, "--data-dir", target) + require.ErrorIs(t, err, server.ErrCertStoreNotEmpty) + + // ...unless forced. + _, err = runImportCerts(t, "--archive", archivePath, "--data-dir", target, "--force") + require.NoError(t, err) +} + +func TestImportCertsCommand_VerifyReportsWithoutWriting(t *testing.T) { + source := t.TempDir() + seedCertStore(t, source) + + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + _, err := runExportCerts(t, archivePath, "--data-dir", source) + require.NoError(t, err) + + target := t.TempDir() + out, err := runImportCerts(t, "--archive", archivePath, "--verify", "--data-dir", target) + require.NoError(t, err) + + assert.Contains(t, out, "Certificates: 0") + assert.NoFileExists(t, filepath.Join(target, "acme.state"), + "--verify must not touch the store") +} + +func TestImportCertsCommand_FlagValidation(t *testing.T) { + tests := []struct { + name string + args []string + }{ + {name: "archive and traefik-acme are exclusive", args: []string{"--archive", "a.tar.gz", "--traefik-acme", "acme.json"}}, + {name: "resolver applies only to traefik", args: []string{"--archive", "a.tar.gz", "--resolver", "le"}}, + {name: "verify requires archive", args: []string{"--traefik-acme", "acme.json", "--verify"}}, + {name: "verify and force are exclusive", args: []string{"--archive", "a.tar.gz", "--verify", "--force"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + _, err := runImportCerts(t, tt.args...) + require.Error(t, err) + }) + } +} diff --git a/internal/cmd/import.go b/internal/cmd/import.go index c17a8f7..feb4dac 100644 --- a/internal/cmd/import.go +++ b/internal/cmd/import.go @@ -1,7 +1,10 @@ package cmd import ( + "errors" "fmt" + "strings" + "time" "github.com/spf13/cobra" @@ -16,7 +19,7 @@ func newImportCommand() *importCommand { importCommand := &importCommand{} importCommand.cmd = &cobra.Command{ Use: "import", - Short: "Import externally issued certificates", + Short: "Import certificates into the certificate store", } importCommand.cmd.AddCommand(newImportCertsCommand().cmd) @@ -24,38 +27,63 @@ func newImportCommand() *importCommand { return importCommand } -// importCertsCommand seeds the certificate store from a Traefik acme.json. It -// runs offline against the data directory before the proxy's first boot -- no -// RPC socket, no running server -- so a fleet cut over from Traefik serves TLS -// immediately instead of re-issuing its whole estate. +// importCertsCommand seeds the certificate store from an external source: a +// Traefik acme.json (--traefik-acme) or a certificate store archive written by +// `export certs` (--archive). Both run offline against the data directory -- +// no RPC socket, no running server -- so restores follow the runbook: stop the +// proxy, import, start it again. type importCertsCommand struct { cmd *cobra.Command traefikAcmePath string resolver string + + archivePath string + force bool + verify bool } func newImportCertsCommand() *importCertsCommand { importCertsCommand := &importCertsCommand{} importCertsCommand.cmd = &cobra.Command{ Use: "certs", - Short: "Import certificates from a Traefik acme.json into the certificate store", + Short: "Import certificates from a Traefik acme.json, or restore an exported certificate store archive", RunE: importCertsCommand.run, Args: cobra.NoArgs, } - importCertsCommand.cmd.Flags().StringVar(&importCertsCommand.traefikAcmePath, "traefik-acme", "", "Path to the Traefik acme.json to import from (required)") - importCertsCommand.cmd.Flags().StringVar(&importCertsCommand.resolver, "resolver", "", "Import only this resolver's certificates (default all resolvers, last writer wins per domain)") - importCertsCommand.cmd.Flags().StringVar(&globalConfig.AlternateConfigDir, "data-dir", getEnvString("DATA_DIR", ""), "Directory for state and certificate storage (default $HOME/.config/kamal-proxy)") + flags := importCertsCommand.cmd.Flags() + flags.StringVar(&importCertsCommand.traefikAcmePath, "traefik-acme", "", "Path to the Traefik acme.json to import from") + flags.StringVar(&importCertsCommand.resolver, "resolver", "", "Import only this resolver's certificates (default all resolvers, last writer wins per domain)") + flags.StringVar(&importCertsCommand.archivePath, "archive", "", "Path to a certificate store archive written by `export certs`") + flags.BoolVar(&importCertsCommand.force, "force", false, "Overwrite a non-empty certificate store when restoring an archive") + flags.BoolVar(&importCertsCommand.verify, "verify", false, "Only verify the archive: parse every certificate and report domains and expiries, without touching the store") + flags.StringVar(&globalConfig.AlternateConfigDir, "data-dir", getEnvString("DATA_DIR", ""), "Directory for state and certificate storage (default $HOME/.config/kamal-proxy)") - if err := importCertsCommand.cmd.MarkFlagRequired("traefik-acme"); err != nil { - panic(err) - } + importCertsCommand.cmd.MarkFlagsOneRequired("traefik-acme", "archive") + importCertsCommand.cmd.MarkFlagsMutuallyExclusive("traefik-acme", "archive") + importCertsCommand.cmd.MarkFlagsMutuallyExclusive("archive", "resolver") + importCertsCommand.cmd.MarkFlagsMutuallyExclusive("verify", "force") return importCertsCommand } func (c *importCertsCommand) run(cmd *cobra.Command, args []string) error { + if c.verify { + if c.archivePath == "" { + return errors.New("--verify requires --archive") + } + return c.runVerify(cmd) + } + + if c.archivePath != "" { + return c.runRestore(cmd) + } + + return c.runTraefikImport(cmd) +} + +func (c *importCertsCommand) runTraefikImport(cmd *cobra.Command) error { // A fresh --data-dir must exist before the state file is written into it — // an import with zero certificates still writes state. if err := ensureDataDir(); err != nil { @@ -82,3 +110,69 @@ func (c *importCertsCommand) run(cmd *cobra.Command, args []string) error { return nil } + +func (c *importCertsCommand) runRestore(cmd *cobra.Command) error { + if err := ensureDataDir(); err != nil { + return err + } + + summary, err := server.RestoreCertificateStore(server.CertStoreRestoreOptions{ + ArchivePath: c.archivePath, + Paths: globalConfig.CertStorePaths(), + Force: c.force, + }) + if err != nil { + return err + } + + for _, warning := range summary.Warnings { + fmt.Fprintf(cmd.ErrOrStderr(), "WARN %s\n", warning) + } + + fmt.Fprintf(cmd.OutOrStdout(), "Restored %d certificates (%d domains)\nAccount key: %s\nDynamic domains state: %s\n", + summary.Certificates, summary.Domains, + restoredWord(summary.AccountKeyRestored), restoredWord(summary.DynamicDomainsRestored)) + + return nil +} + +func (c *importCertsCommand) runVerify(cmd *cobra.Command) error { + report, err := server.VerifyCertificateArchive(c.archivePath) + if err != nil { + return err + } + + out := cmd.OutOrStdout() + expired := 0 + for _, cert := range report.Certificates { + if cert.NotAfter.Before(time.Now()) { + expired++ + } + fmt.Fprintf(out, "%s expires %s %s\n", + cert.Identifier, cert.NotAfter.Format("2006-01-02"), strings.Join(cert.Domains, " ")) + } + + for _, warning := range report.Warnings { + fmt.Fprintf(cmd.ErrOrStderr(), "WARN %s\n", warning) + } + + fmt.Fprintf(out, "Certificates: %d (%d expired)\nDomain mappings: %d\nAccount key: %s\nDynamic domains state: %s\n", + len(report.Certificates), expired, report.DomainMappings, + presentWord(report.HasAccountKey), presentWord(report.HasDynamicDomains)) + + return nil +} + +func restoredWord(restored bool) string { + if restored { + return "restored" + } + return "not in archive" +} + +func presentWord(present bool) string { + if present { + return "present" + } + return "absent" +} diff --git a/internal/cmd/root.go b/internal/cmd/root.go index b0bc497..aa670f0 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -32,6 +32,7 @@ func Execute() { rootCmd.AddCommand(newCacheCommand().cmd) rootCmd.AddCommand(newDrainCommand().cmd) rootCmd.AddCommand(newImportCommand().cmd) + rootCmd.AddCommand(newExportCommand().cmd) rootCmd.AddCommand(newHoldCommand().cmd) err := rootCmd.Execute() diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go new file mode 100644 index 0000000..76b7e07 --- /dev/null +++ b/internal/server/cert_store_archive.go @@ -0,0 +1,234 @@ +package server + +import ( + "archive/tar" + "compress/gzip" + "crypto/tls" + "crypto/x509" + "encoding/json" + "errors" + "fmt" + "io" + "maps" + "os" + "path" + "slices" + "strings" +) + +// maxCertArchiveBytes caps how much an archive may decompress to. The whole +// estate of a 1,000-domain fleet is a few megabytes; anything near this limit +// is not a certificate backup. +const maxCertArchiveBytes = 512 << 20 + +// archiveCertPair is one certificate directory from an archive, parsed and +// validated. +type archiveCertPair struct { + certPEM []byte + keyPEM []byte + leaf *x509.Certificate +} + +// certStoreArchive is a fully read and validated certificate store archive. +// Reading never touches the store: verification and restore share this. +type certStoreArchive struct { + state managerState + hasState bool + + accountKey []byte + dynamicDomains []byte + + // certs is keyed by the certificate's directory name (the sanitized + // certificate identifier). + certs map[string]archiveCertPair + + warnings []string +} + +// readCertStoreArchive reads and validates an exported certificate store +// archive. Structural problems -- traversal-shaped or unexpected entry names, +// torn certificate pairs, an unparseable or inconsistent state file -- are +// errors: a backup that fails here cannot be trusted to restore. An expired +// certificate is not an error; a faithful backup of an expired certificate is +// still a backup. +func readCertStoreArchive(archivePath string) (certStoreArchive, error) { + archive := certStoreArchive{certs: map[string]archiveCertPair{}} + + file, err := os.Open(archivePath) + if err != nil { + return archive, fmt.Errorf("failed to open the archive: %w", err) + } + defer file.Close() + + gz, err := gzip.NewReader(file) + if err != nil { + return archive, fmt.Errorf("failed to read the archive %s: %w", archivePath, err) + } + defer gz.Close() + + rawCerts := map[string]map[string][]byte{} + entryCount := 0 + var totalBytes int64 + + tr := tar.NewReader(gz) + for { + header, err := tr.Next() + if err == io.EOF { + break + } + if err != nil { + return archive, fmt.Errorf("failed to read the archive %s: %w", archivePath, err) + } + + if header.Typeflag == tar.TypeDir { + continue + } + if header.Typeflag != tar.TypeReg { + return archive, fmt.Errorf("refusing archive entry %q: only regular files belong in a certificate archive", header.Name) + } + + totalBytes += header.Size + if totalBytes > maxCertArchiveBytes { + return archive, fmt.Errorf("refusing the archive %s: it decompresses beyond %d bytes", archivePath, int64(maxCertArchiveBytes)) + } + + data, err := io.ReadAll(io.LimitReader(tr, maxCertArchiveBytes)) + if err != nil { + return archive, fmt.Errorf("failed to read the archive entry %q: %w", header.Name, err) + } + entryCount++ + + if err := archive.placeEntry(header.Name, data, rawCerts); err != nil { + return archive, err + } + } + + if entryCount == 0 { + return archive, fmt.Errorf("the archive %s is empty", archivePath) + } + + if err := archive.assembleCertPairs(rawCerts); err != nil { + return archive, err + } + + if err := archive.validate(); err != nil { + return archive, err + } + + return archive, nil +} + +// placeEntry routes one archive entry to its slot, refusing any name the +// exporter would never write -- which is also what keeps a hostile archive +// from writing outside the store. +func (a *certStoreArchive) placeEntry(name string, data []byte, rawCerts map[string]map[string][]byte) error { + if name != path.Clean(name) || strings.HasPrefix(name, "/") || strings.HasPrefix(name, "..") { + return fmt.Errorf("refusing archive entry %q: not a certificate store path", name) + } + + switch name { + case archiveStateEntry: + if err := json.Unmarshal(data, &a.state); err != nil { + return fmt.Errorf("the archive's %s does not parse: %w", archiveStateEntry, err) + } + a.hasState = true + return nil + case archiveAccountKeyEntry: + a.accountKey = data + return nil + case archiveDynamicDomainsEntry: + a.dynamicDomains = data + return nil + } + + if rest, ok := strings.CutPrefix(name, archiveCertsPrefix); ok { + dir, base, found := strings.Cut(rest, "/") + if found && dir != "" && (base == "cert.pem" || base == "key.pem") && !strings.Contains(base, "/") { + if rawCerts[dir] == nil { + rawCerts[dir] = map[string][]byte{} + } + rawCerts[dir][base] = data + return nil + } + } + + return fmt.Errorf("unexpected archive entry %q: not part of a certificate store", name) +} + +// assembleCertPairs pairs and parses every certificate directory. Half a pair +// or an unparseable pair is a torn backup, not a skippable entry. +func (a *certStoreArchive) assembleCertPairs(rawCerts map[string]map[string][]byte) error { + for _, dir := range slices.Sorted(maps.Keys(rawCerts)) { + files := rawCerts[dir] + + for _, base := range []string{"cert.pem", "key.pem"} { + if _, ok := files[base]; !ok { + return fmt.Errorf("the archived certificate %s is missing %s", dir, base) + } + } + + certPEM, keyPEM := files["cert.pem"], files["key.pem"] + tlsCert, err := tls.X509KeyPair(certPEM, keyPEM) + if err != nil { + return fmt.Errorf("the archived certificate %s does not parse: %w", dir, err) + } + leaf, err := x509.ParseCertificate(tlsCert.Certificate[0]) + if err != nil { + return fmt.Errorf("the archived certificate %s has an invalid leaf: %w", dir, err) + } + + a.certs[dir] = archiveCertPair{certPEM: certPEM, keyPEM: keyPEM, leaf: leaf} + } + + return nil +} + +// validate cross-checks the state file against the archived certificates. +func (a *certStoreArchive) validate() error { + if !a.hasState { + if len(a.certs) > 0 { + return errors.New("the archive contains certificates but no acme.state; it cannot restore a working store") + } + return nil + } + + if err := validateManagerState(a.state); err != nil { + return fmt.Errorf("the archive's %s is not trustworthy: %w", archiveStateEntry, err) + } + + // A state-referenced certificate missing from the archive restores to the + // same place loadState puts a missing file: the domain re-orders. Warn, + // don't fail -- the export warned identically when it happened. + for _, id := range slices.Sorted(maps.Keys(a.state.Certificates)) { + if _, ok := a.certs[sanitizeFilename(id)]; !ok { + a.warnings = append(a.warnings, + fmt.Sprintf("certificate %s is referenced by the state file but missing from the archive; its domains will re-order after a restore", id)) + } + } + + return nil +} + +// validateManagerState checks the invariants a healthy manager always +// maintains: both maps present, no null certificate records, no domain mapped +// to a certificate that is not there. Shared by the Traefik importer (before +// merging into an existing state file) and the archive reader. +func validateManagerState(state managerState) error { + if state.Certificates == nil || state.DomainMap == nil { + return errors.New("it does not look like a certificate state file") + } + + for id, cert := range state.Certificates { + if cert == nil { + return fmt.Errorf("certificate %q is null", id) + } + } + + for domain, id := range state.DomainMap { + if _, ok := state.Certificates[id]; !ok { + return fmt.Errorf("domain %q references a missing certificate %q", domain, id) + } + } + + return nil +} diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go new file mode 100644 index 0000000..28747f1 --- /dev/null +++ b/internal/server/cert_store_export.go @@ -0,0 +1,297 @@ +package server + +import ( + "archive/tar" + "compress/gzip" + "encoding/json" + "errors" + "fmt" + "maps" + "os" + "path/filepath" + "slices" + "strings" + "time" +) + +// The certificate store archive mirrors the data directory layout, so a +// restore is a faithful extraction: acme.state and dynamic-domains.state at +// the root, the account key and one directory per certificate under certs/. +const ( + archiveStateEntry = "acme.state" + archiveDynamicDomainsEntry = "dynamic-domains.state" + archiveCertsPrefix = "certs/" + archiveAccountKeyEntry = archiveCertsPrefix + acmeUserFile + + acmeUserFile = "acme_user.json" +) + +// ErrCertStoreEmpty reports an export attempt against a store with nothing in +// it. Failing loudly beats a cron job faithfully archiving nothing. +var ErrCertStoreEmpty = errors.New("certificate store is empty; nothing to export") + +// CertStorePaths names the on-disk pieces of the certificate estate. +type CertStorePaths struct { + // CertsPath is the certificate cache directory, which also holds the ACME + // account key (Config.CertificatePath()). + CertsPath string + + // ACMEStatePath is the certificate manager's state file (Config.ACMEStatePath()). + ACMEStatePath string + + // DynamicDomainsStatePath is the dynamic domain manager's state file + // (Config.DynamicDomainsStatePath()). + DynamicDomainsStatePath string +} + +// CertsExportSummary reports what an export captured. Warnings flag pieces of +// the store that were skipped or inconsistent without aborting the export. +type CertsExportSummary struct { + Certificates int `json:"certificates"` + Domains int `json:"domains"` + Warnings []string `json:"warnings,omitempty"` +} + +// archiveFile is one file staged for the archive. +type archiveFile struct { + name string + data []byte + modTime time.Time +} + +// ExportStore writes a consistent snapshot of the certificate store while +// holding the store's disk-write lock, so no concurrent issuance, renewal, or +// removal tears the archive. +func (m *SANCertManager) ExportStore(paths CertStorePaths, outputPath string) (CertsExportSummary, error) { + m.stateMu.Lock() + defer m.stateMu.Unlock() + + return ExportCertificateStore(paths, outputPath) +} + +// ExportCertificateStore archives the certificate estate -- state file, +// certificates with their private keys, ACME account key, and dynamic domain +// state -- into a gzipped tarball written atomically with mode 0600. +// +// Callers running against a live proxy must hold the store's disk-write lock +// (SANCertManager.ExportStore does); running offline against a stopped data +// dir needs no lock. +func ExportCertificateStore(paths CertStorePaths, outputPath string) (CertsExportSummary, error) { + summary := CertsExportSummary{} + + state, files, err := collectStateEntry(paths.ACMEStatePath, &summary) + if err != nil { + return summary, err + } + + certFiles, err := collectCertsEntries(paths.CertsPath, state, &summary) + if err != nil { + return summary, err + } + files = append(files, certFiles...) + + if dynamic, ok := collectOptionalJSON(paths.DynamicDomainsStatePath, archiveDynamicDomainsEntry, &summary); ok { + files = append(files, dynamic) + } + + if len(files) == 0 { + return summary, ErrCertStoreEmpty + } + + slices.SortFunc(files, func(a, b archiveFile) int { + return strings.Compare(a.name, b.name) + }) + + if err := writeCertArchive(outputPath, files); err != nil { + return summary, err + } + + return summary, nil +} + +// collectStateEntry reads and validates acme.state. A store whose index is +// unreadable produces backups that cannot restore, so unlike every other file +// this one aborts the export when it exists but does not parse. +func collectStateEntry(path string, summary *CertsExportSummary) (managerState, []archiveFile, error) { + state := managerState{} + + data, modTime, err := readFileWithModTime(path) + if err != nil { + if os.IsNotExist(err) { + return state, nil, nil + } + return state, nil, fmt.Errorf("failed to read %s: %w", filepath.Base(path), err) + } + + if err := json.Unmarshal(data, &state); err != nil { + return state, nil, fmt.Errorf("refusing to export the unreadable state file %s: %w", path, err) + } + + summary.Certificates = len(state.Certificates) + summary.Domains = len(state.DomainMap) + + return state, []archiveFile{{name: archiveStateEntry, data: data, modTime: modTime}}, nil +} + +// collectCertsEntries walks the certificate cache directory, capturing the +// account key and every complete certificate pair, and warning about anything +// else it finds -- including certificates the state file references but the +// disk no longer holds. +func collectCertsEntries(certsPath string, state managerState, summary *CertsExportSummary) ([]archiveFile, error) { + files := []archiveFile{} + + entries, err := os.ReadDir(certsPath) + if err != nil { + if os.IsNotExist(err) { + warnMissingStateCerts(certsPath, state, summary) + return nil, nil + } + return nil, fmt.Errorf("failed to read the certificate directory %s: %w", certsPath, err) + } + + for _, entry := range entries { + name := entry.Name() + + switch { + case !entry.IsDir() && name == acmeUserFile: + if file, ok := collectOptionalJSON(filepath.Join(certsPath, name), archiveAccountKeyEntry, summary); ok { + files = append(files, file) + } + case entry.IsDir() && name == legacyHTTP01CacheDir: + summary.Warnings = append(summary.Warnings, + fmt.Sprintf("legacy %s cache is not exported: start the proxy once so it is adopted into the store first", legacyHTTP01CacheDir)) + case entry.IsDir(): + pair, ok := collectCertPair(certsPath, name, summary) + if !ok { + continue + } + files = append(files, pair...) + default: + summary.Warnings = append(summary.Warnings, fmt.Sprintf("not exported: unexpected file %s", filepath.Join("certs", name))) + } + } + + warnMissingStateCerts(certsPath, state, summary) + + return files, nil +} + +// collectCertPair captures one certificate directory's cert.pem and key.pem. +// A directory with only half the pair is skipped with a warning: restoring it +// would leave a certificate the manager cannot load. +func collectCertPair(certsPath, dir string, summary *CertsExportSummary) ([]archiveFile, bool) { + pair := make([]archiveFile, 0, 2) + + for _, name := range []string{"cert.pem", "key.pem"} { + data, modTime, err := readFileWithModTime(filepath.Join(certsPath, dir, name)) + if err != nil { + summary.Warnings = append(summary.Warnings, + fmt.Sprintf("not exported: certificate %s is missing %s", dir, name)) + return nil, false + } + pair = append(pair, archiveFile{name: archiveCertsPrefix + dir + "/" + name, data: data, modTime: modTime}) + } + + return pair, true +} + +// warnMissingStateCerts flags certificates the state file references that have +// no files on disk. The restore side treats them the same way loadState does: +// the domain falls through to ordinary provisioning. +func warnMissingStateCerts(certsPath string, state managerState, summary *CertsExportSummary) { + for _, id := range slices.Sorted(maps.Keys(state.Certificates)) { + if _, err := os.Stat(filepath.Join(certsPath, sanitizeFilename(id), "cert.pem")); err != nil { + summary.Warnings = append(summary.Warnings, + fmt.Sprintf("certificate %s is referenced by the state file but missing on disk; its domains will re-order after a restore", id)) + } + } +} + +// collectOptionalJSON captures a file that must be JSON to be worth restoring. +// A missing file is silently skipped; an unreadable or invalid one degrades to +// a warning, because both the account key and the dynamic domain list are +// rebuilt automatically by a booted proxy. +func collectOptionalJSON(path, entryName string, summary *CertsExportSummary) (archiveFile, bool) { + data, modTime, err := readFileWithModTime(path) + if err != nil { + if !os.IsNotExist(err) { + summary.Warnings = append(summary.Warnings, fmt.Sprintf("not exported: failed to read %s: %v", entryName, err)) + } + return archiveFile{}, false + } + + if !json.Valid(data) { + summary.Warnings = append(summary.Warnings, fmt.Sprintf("not exported: %s is not valid JSON", entryName)) + return archiveFile{}, false + } + + return archiveFile{name: entryName, data: data, modTime: modTime}, true +} + +func readFileWithModTime(path string) ([]byte, time.Time, error) { + info, err := os.Stat(path) + if err != nil { + return nil, time.Time{}, err + } + + data, err := os.ReadFile(path) + if err != nil { + return nil, time.Time{}, err + } + + return data, info.ModTime(), nil +} + +// writeCertArchive writes the staged files as a gzipped tarball, staged as a +// temp file and renamed into place so a partial write never looks like a valid +// backup. The archive holds private keys: it is created with mode 0600. +func writeCertArchive(outputPath string, files []archiveFile) error { + tmpPath := outputPath + ".tmp" + + file, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) + if err != nil { + return fmt.Errorf("failed to create the archive: %w", err) + } + + err = func() error { + gz := gzip.NewWriter(file) + tw := tar.NewWriter(gz) + + for _, entry := range files { + header := &tar.Header{ + Name: entry.name, + Mode: 0600, + Size: int64(len(entry.data)), + ModTime: entry.modTime, + } + if err := tw.WriteHeader(header); err != nil { + return err + } + if _, err := tw.Write(entry.data); err != nil { + return err + } + } + + if err := tw.Close(); err != nil { + return err + } + return gz.Close() + }() + if err != nil { + file.Close() + os.Remove(tmpPath) + return fmt.Errorf("failed to write the archive: %w", err) + } + + if err := file.Close(); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("failed to write the archive: %w", err) + } + + if err := os.Rename(tmpPath, outputPath); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("failed to finalize the archive: %w", err) + } + + return nil +} diff --git a/internal/server/cert_store_export_test.go b/internal/server/cert_store_export_test.go new file mode 100644 index 0000000..87f31a0 --- /dev/null +++ b/internal/server/cert_store_export_test.go @@ -0,0 +1,290 @@ +package server + +import ( + "archive/tar" + "compress/gzip" + "encoding/json" + "io" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testCertStorePaths returns store paths rooted in a fresh temp data dir. +func testCertStorePaths(t testing.TB) CertStorePaths { + t.Helper() + + dir := t.TempDir() + return CertStorePaths{ + CertsPath: filepath.Join(dir, "certs"), + ACMEStatePath: filepath.Join(dir, "acme.state"), + DynamicDomainsStatePath: filepath.Join(dir, "dynamic-domains.state"), + } +} + +// populateCertStore writes one certificate per domain set through the store's +// own write path, plus a matching acme.state, an account key file, and a +// dynamic domains state file. +func populateCertStore(t testing.TB, paths CertStorePaths, domainSets ...[]string) managerState { + t.Helper() + + state := managerState{ + Certificates: map[string]*ManagedCert{}, + DomainMap: map[string]string{}, + SavedAt: time.Now(), + } + + notAfter := time.Now().Add(60 * 24 * time.Hour) + for _, domains := range domainSets { + sorted := sortedCopy(domains) + resource := testCertResource(t, sorted, time.Now().Add(-time.Hour), notAfter) + + certID := sanCertID(sorted) + require.NoError(t, writeCertificateFiles(paths.CertsPath, certID, resource.Certificate, resource.PrivateKey)) + + state.Certificates[certID] = &ManagedCert{Identifier: certID, Domains: sorted, NotAfter: notAfter} + for _, domain := range sorted { + state.DomainMap[domain] = certID + } + } + + require.NoError(t, os.MkdirAll(paths.CertsPath, 0700)) + require.NoError(t, writeManagerStateFile(paths.ACMEStatePath, state)) + require.NoError(t, os.WriteFile(filepath.Join(paths.CertsPath, "acme_user.json"), + []byte(`{"email":"ops@example.com","key_pem":"dGVzdA=="}`), 0600)) + require.NoError(t, os.WriteFile(paths.DynamicDomainsStatePath, + []byte(`{"services":{},"quarantine":{},"saved_at":"2026-08-09T00:00:00Z"}`), 0600)) + + return state +} + +// readCertArchive extracts an exported archive into a name -> content map. +func readCertArchive(t testing.TB, path string) map[string][]byte { + t.Helper() + + file, err := os.Open(path) + require.NoError(t, err) + defer file.Close() + + gz, err := gzip.NewReader(file) + require.NoError(t, err) + defer gz.Close() + + entries := map[string][]byte{} + tr := tar.NewReader(gz) + for { + header, err := tr.Next() + if err == io.EOF { + break + } + require.NoError(t, err) + + data, err := io.ReadAll(tr) + require.NoError(t, err) + entries[header.Name] = data + } + + return entries +} + +func TestExportCertificateStore_ArchivesTheWholeEstate(t *testing.T) { + paths := testCertStorePaths(t) + state := populateCertStore(t, paths, []string{"example.com", "www.example.com"}, []string{"other.test"}) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + summary, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + + assert.Equal(t, 2, summary.Certificates) + assert.Equal(t, 3, summary.Domains) + assert.Empty(t, summary.Warnings) + + entries := readCertArchive(t, outputPath) + assert.Contains(t, entries, "acme.state") + assert.Contains(t, entries, "dynamic-domains.state") + assert.Contains(t, entries, "certs/acme_user.json") + for certID := range state.Certificates { + dir := "certs/" + sanitizeFilename(certID) + assert.Contains(t, entries, dir+"/cert.pem") + assert.Contains(t, entries, dir+"/key.pem") + + onDisk, err := os.ReadFile(filepath.Join(paths.CertsPath, sanitizeFilename(certID), "cert.pem")) + require.NoError(t, err) + assert.Equal(t, onDisk, entries[dir+"/cert.pem"]) + } + assert.Len(t, entries, 3+2*len(state.Certificates)) + + // The archive contains private keys: it must not be world-readable. + info, err := os.Stat(outputPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) + + // The staged write must not leave its temp file behind. + _, err = os.Stat(outputPath + ".tmp") + assert.True(t, os.IsNotExist(err)) +} + +func TestExportCertificateStore_EmptyStoreIsAnError(t *testing.T) { + paths := testCertStorePaths(t) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + _, err := ExportCertificateStore(paths, outputPath) + require.ErrorIs(t, err, ErrCertStoreEmpty) + + _, err = os.Stat(outputPath) + assert.True(t, os.IsNotExist(err), "an empty export must not write an archive") +} + +func TestExportCertificateStore_OptionalFilesMayBeMissing(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + require.NoError(t, os.Remove(filepath.Join(paths.CertsPath, "acme_user.json"))) + require.NoError(t, os.Remove(paths.DynamicDomainsStatePath)) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + summary, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + assert.Equal(t, 1, summary.Certificates) + + entries := readCertArchive(t, outputPath) + assert.NotContains(t, entries, "certs/acme_user.json") + assert.NotContains(t, entries, "dynamic-domains.state") +} + +func TestExportCertificateStore_UnparseableStateIsAnError(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + require.NoError(t, os.WriteFile(paths.ACMEStatePath, []byte("{torn"), 0600)) + + _, err := ExportCertificateStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) + require.Error(t, err) + assert.Contains(t, err.Error(), "acme.state") +} + +func TestExportCertificateStore_Warnings(t *testing.T) { + paths := testCertStorePaths(t) + state := populateCertStore(t, paths, []string{"example.com"}, []string{"gone.test"}) + + // A legacy autocert cache, a stray file, and a certificate directory + // missing from disk should each warn without failing the export. + require.NoError(t, os.MkdirAll(filepath.Join(paths.CertsPath, "http01"), 0700)) + require.NoError(t, os.WriteFile(filepath.Join(paths.CertsPath, "stray.txt"), []byte("x"), 0600)) + goneID := sanCertID([]string{"gone.test"}) + require.NoError(t, os.RemoveAll(filepath.Join(paths.CertsPath, sanitizeFilename(goneID)))) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + summary, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + + assert.Equal(t, len(state.Certificates), summary.Certificates) + require.Len(t, summary.Warnings, 3) + joined := "" + for _, warning := range summary.Warnings { + joined += warning + "\n" + } + assert.Contains(t, joined, "http01") + assert.Contains(t, joined, "stray.txt") + assert.Contains(t, joined, goneID) + + entries := readCertArchive(t, outputPath) + assert.NotContains(t, entries, "certs/stray.txt") +} + +func TestExportCertificateStore_OverwritesAnExistingArchive(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + require.NoError(t, os.WriteFile(outputPath, []byte("old backup"), 0644)) + + _, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + + entries := readCertArchive(t, outputPath) + assert.Contains(t, entries, "acme.state") + + info, err := os.Stat(outputPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) +} + +func TestSANCertManager_ExportStoreHoldsTheDiskLock(t *testing.T) { + manager := testSANCertManager(t) + + resource := testCertResource(t, []string{"example.com"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)) + _, err := manager.adoptCertificate(resource, []string{"example.com"}) + require.NoError(t, err) + + paths := CertStorePaths{ + CertsPath: manager.config.CachePath, + ACMEStatePath: manager.config.StatePath, + DynamicDomainsStatePath: filepath.Join(t.TempDir(), "dynamic-domains.state"), + } + + // Holding the disk-write lock must block the export until released. + manager.stateMu.Lock() + started := make(chan struct{}) + done := make(chan error, 1) + var summary CertsExportSummary + go func() { + close(started) + var exportErr error + summary, exportErr = manager.ExportStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) + done <- exportErr + }() + + <-started + select { + case <-done: + t.Fatal("export completed while the disk-write lock was held") + case <-time.After(50 * time.Millisecond): + } + + manager.stateMu.Unlock() + require.NoError(t, <-done) + assert.Equal(t, 1, summary.Certificates) + assert.Equal(t, 1, summary.Domains) +} + +func TestExportCertificateStore_StateWithoutMapsStillCounts(t *testing.T) { + paths := testCertStorePaths(t) + require.NoError(t, os.MkdirAll(paths.CertsPath, 0700)) + require.NoError(t, os.WriteFile(paths.ACMEStatePath, []byte(`{"saved_at":"2026-08-09T00:00:00Z"}`), 0600)) + + summary, err := ExportCertificateStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) + require.NoError(t, err) + assert.Equal(t, 0, summary.Certificates) + assert.Equal(t, 0, summary.Domains) +} + +func TestExportCertificateStore_SkipsInvalidOptionalFiles(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + require.NoError(t, os.WriteFile(filepath.Join(paths.CertsPath, "acme_user.json"), []byte("{torn"), 0600)) + require.NoError(t, os.WriteFile(paths.DynamicDomainsStatePath, []byte("{torn"), 0600)) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + summary, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + assert.Len(t, summary.Warnings, 2) + + entries := readCertArchive(t, outputPath) + assert.NotContains(t, entries, "certs/acme_user.json") + assert.NotContains(t, entries, "dynamic-domains.state") +} + +// Guard: the summary type crosses the RPC boundary, so a JSON round-trip (used +// by nothing today, but cheap) and exported fields matter. +func TestCertsExportSummary_RoundTrips(t *testing.T) { + summary := CertsExportSummary{Certificates: 2, Domains: 5, Warnings: []string{"w"}} + data, err := json.Marshal(summary) + require.NoError(t, err) + + var back CertsExportSummary + require.NoError(t, json.Unmarshal(data, &back)) + assert.Equal(t, summary, back) +} diff --git a/internal/server/cert_store_restore.go b/internal/server/cert_store_restore.go new file mode 100644 index 0000000..af8d1b0 --- /dev/null +++ b/internal/server/cert_store_restore.go @@ -0,0 +1,172 @@ +package server + +import ( + "errors" + "fmt" + "maps" + "os" + "path/filepath" + "slices" + "time" +) + +// ErrCertStoreNotEmpty reports a restore attempt over a store that already +// holds something. Overwriting live certificate state is an explicit decision, +// not a default. +var ErrCertStoreNotEmpty = errors.New("the certificate store is not empty") + +// CertStoreRestoreOptions configures an offline restore of a certificate store +// archive into a data directory. Like the Traefik import, it runs against a +// stopped proxy -- the runbook is stop, restore, start. +type CertStoreRestoreOptions struct { + // ArchivePath is the archive to restore from. + ArchivePath string + + // Paths is the target store (Config.CertStorePaths()). + Paths CertStorePaths + + // Force overwrites a non-empty store. Existing certificate directories the + // archive does not reference are left on disk but unreferenced by the + // restored state. + Force bool +} + +// CertsRestoreSummary reports what a restore wrote. +type CertsRestoreSummary struct { + Certificates int + Domains int + AccountKeyRestored bool + DynamicDomainsRestored bool + Warnings []string +} + +// ArchiveCertInfo describes one certificate found in an archive, read from the +// certificate itself rather than the state file. +type ArchiveCertInfo struct { + Identifier string + Domains []string + NotAfter time.Time +} + +// CertArchiveReport is what verification learned about an archive. +type CertArchiveReport struct { + Certificates []ArchiveCertInfo + DomainMappings int + HasAccountKey bool + HasDynamicDomains bool + Warnings []string +} + +// RestoreCertificateStore restores an exported archive into a data directory, +// writing certificates through the same staged path the importers and the live +// manager use. The state file is written last, so an interrupted restore never +// leaves a state file naming certificates that were not written yet. +func RestoreCertificateStore(opts CertStoreRestoreOptions) (CertsRestoreSummary, error) { + summary := CertsRestoreSummary{} + + archive, err := readCertStoreArchive(opts.ArchivePath) + if err != nil { + return summary, err + } + summary.Warnings = archive.warnings + + if !opts.Force { + if occupant := certStoreOccupant(opts.Paths); occupant != "" { + return summary, fmt.Errorf("%w: %s exists (use --force to overwrite)", ErrCertStoreNotEmpty, occupant) + } + } + + for _, dir := range slices.Sorted(maps.Keys(archive.certs)) { + pair := archive.certs[dir] + if err := writeCertificateFiles(opts.Paths.CertsPath, dir, pair.certPEM, pair.keyPEM); err != nil { + return summary, fmt.Errorf("failed to restore the certificate %s: %w", dir, err) + } + } + + if archive.accountKey != nil { + if err := os.MkdirAll(opts.Paths.CertsPath, 0700); err != nil { + return summary, fmt.Errorf("failed to create the certificate directory: %w", err) + } + if err := os.WriteFile(filepath.Join(opts.Paths.CertsPath, acmeUserFile), archive.accountKey, 0600); err != nil { + return summary, fmt.Errorf("failed to restore the ACME account key: %w", err) + } + summary.AccountKeyRestored = true + } + + if archive.dynamicDomains != nil { + if err := os.WriteFile(opts.Paths.DynamicDomainsStatePath, archive.dynamicDomains, 0600); err != nil { + return summary, fmt.Errorf("failed to restore the dynamic domains state: %w", err) + } + summary.DynamicDomainsRestored = true + } + + if archive.hasState { + if err := writeManagerStateFile(opts.Paths.ACMEStatePath, archive.state); err != nil { + return summary, fmt.Errorf("failed to restore the certificate state: %w", err) + } + summary.Certificates = len(archive.state.Certificates) + summary.Domains = len(archive.state.DomainMap) + } + + return summary, nil +} + +// VerifyCertificateArchive reads an archive the way a restore would -- full +// structural validation, every certificate parsed -- without touching any +// store, and reports what it holds. This is the CI/cron backup check. +func VerifyCertificateArchive(archivePath string) (CertArchiveReport, error) { + report := CertArchiveReport{} + + archive, err := readCertStoreArchive(archivePath) + if err != nil { + return report, err + } + + // Prefer the state file's identifier for a certificate; a directory the + // state does not name is reported by its directory name. + idByDir := map[string]string{} + for id := range archive.state.Certificates { + idByDir[sanitizeFilename(id)] = id + } + + for _, dir := range slices.Sorted(maps.Keys(archive.certs)) { + pair := archive.certs[dir] + + identifier := idByDir[dir] + if identifier == "" { + identifier = dir + } + + report.Certificates = append(report.Certificates, ArchiveCertInfo{ + Identifier: identifier, + Domains: sortedCopy(pair.leaf.DNSNames), + NotAfter: pair.leaf.NotAfter, + }) + } + + report.DomainMappings = len(archive.state.DomainMap) + report.HasAccountKey = archive.accountKey != nil + report.HasDynamicDomains = archive.dynamicDomains != nil + report.Warnings = archive.warnings + + return report, nil +} + +// certStoreOccupant names the first thing found occupying the target store, or +// "" when the store is empty. An existing but empty certs directory does not +// count. +func certStoreOccupant(paths CertStorePaths) string { + if _, err := os.Stat(paths.ACMEStatePath); err == nil { + return paths.ACMEStatePath + } + + if _, err := os.Stat(paths.DynamicDomainsStatePath); err == nil { + return paths.DynamicDomainsStatePath + } + + if entries, err := os.ReadDir(paths.CertsPath); err == nil && len(entries) > 0 { + return filepath.Join(paths.CertsPath, entries[0].Name()) + } + + return "" +} diff --git a/internal/server/cert_store_restore_test.go b/internal/server/cert_store_restore_test.go new file mode 100644 index 0000000..d298009 --- /dev/null +++ b/internal/server/cert_store_restore_test.go @@ -0,0 +1,344 @@ +package server + +import ( + "archive/tar" + "compress/gzip" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// exportedTestArchive populates a store and exports it, returning the archive +// path and the state that was captured. +func exportedTestArchive(t testing.TB, domainSets ...[]string) (string, managerState) { + t.Helper() + + paths := testCertStorePaths(t) + state := populateCertStore(t, paths, domainSets...) + + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + _, err := ExportCertificateStore(paths, archivePath) + require.NoError(t, err) + + return archivePath, state +} + +// writeTestArchive crafts a tar.gz with exactly the given entries, for +// exercising the reader against archives export would never produce. +func writeTestArchive(t testing.TB, path string, entries map[string][]byte) { + t.Helper() + + file, err := os.Create(path) + require.NoError(t, err) + defer file.Close() + + gz := gzip.NewWriter(file) + tw := tar.NewWriter(gz) + for name, data := range entries { + require.NoError(t, tw.WriteHeader(&tar.Header{Name: name, Mode: 0600, Size: int64(len(data))})) + _, err := tw.Write(data) + require.NoError(t, err) + } + require.NoError(t, tw.Close()) + require.NoError(t, gz.Close()) +} + +func TestRestoreCertificateStore_RoundTrip(t *testing.T) { + archivePath, exported := exportedTestArchive(t, []string{"example.com", "www.example.com"}, []string{"other.test"}) + + target := testCertStorePaths(t) + summary, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: target}) + require.NoError(t, err) + + assert.Equal(t, 2, summary.Certificates) + assert.Equal(t, 3, summary.Domains) + assert.True(t, summary.AccountKeyRestored) + assert.True(t, summary.DynamicDomainsRestored) + + // The restored store must boot: a manager pointed at it loads every + // certificate with its key pair. + manager, err := NewSANCertManager(SANCertManagerConfig{ + Email: "ops@example.com", + Directory: LetsEncryptStaging, + CachePath: target.CertsPath, + StatePath: target.ACMEStatePath, + }) + require.NoError(t, err) + require.NoError(t, manager.loadState()) + + require.Len(t, manager.certificates, len(exported.Certificates)) + for id, cert := range exported.Certificates { + restored := manager.certificates[id] + require.NotNil(t, restored, "certificate %s missing after restore", id) + assert.Equal(t, cert.Domains, restored.Domains) + assert.NotNil(t, restored.Certificate, "certificate %s did not load its key pair", id) + } + + // Account key and dynamic domain state come back byte-for-byte. + accountKey, err := os.ReadFile(filepath.Join(target.CertsPath, "acme_user.json")) + require.NoError(t, err) + assert.JSONEq(t, `{"email":"ops@example.com","key_pem":"dGVzdA=="}`, string(accountKey)) + + _, err = os.Stat(target.DynamicDomainsStatePath) + require.NoError(t, err) +} + +func TestRestoreCertificateStore_RefusesNonEmptyStore(t *testing.T) { + archivePath, _ := exportedTestArchive(t, []string{"example.com"}) + + tests := []struct { + name string + prepare func(t *testing.T, paths CertStorePaths) + }{ + { + name: "existing state file", + prepare: func(t *testing.T, paths CertStorePaths) { + require.NoError(t, writeManagerStateFile(paths.ACMEStatePath, managerState{ + Certificates: map[string]*ManagedCert{}, DomainMap: map[string]string{}, + })) + }, + }, + { + name: "existing certificate directory", + prepare: func(t *testing.T, paths CertStorePaths) { + require.NoError(t, writeCertificateFiles(paths.CertsPath, "san:existing", []byte("cert"), []byte("key"))) + }, + }, + { + name: "existing dynamic domains state", + prepare: func(t *testing.T, paths CertStorePaths) { + require.NoError(t, os.WriteFile(paths.DynamicDomainsStatePath, []byte("{}"), 0600)) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + paths := testCertStorePaths(t) + tt.prepare(t, paths) + + _, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: paths}) + require.ErrorIs(t, err, ErrCertStoreNotEmpty) + + // With Force the same restore proceeds. + _, err = RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: paths, Force: true}) + require.NoError(t, err) + }) + } +} + +func TestRestoreCertificateStore_EmptyCertsDirIsEmpty(t *testing.T) { + archivePath, _ := exportedTestArchive(t, []string{"example.com"}) + + paths := testCertStorePaths(t) + require.NoError(t, os.MkdirAll(paths.CertsPath, 0700)) + + _, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: paths}) + require.NoError(t, err, "an existing but empty certs directory is not a non-empty store") +} + +func TestRestoreCertificateStore_ForceKeepsUnreferencedCertDirs(t *testing.T) { + archivePath, _ := exportedTestArchive(t, []string{"example.com"}) + + paths := testCertStorePaths(t) + require.NoError(t, writeCertificateFiles(paths.CertsPath, "san:preexisting", []byte("cert"), []byte("key"))) + + _, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: paths, Force: true}) + require.NoError(t, err) + + // The old directory survives on disk, orphaned; the restored state does + // not reference it. + _, err = os.Stat(filepath.Join(paths.CertsPath, sanitizeFilename("san:preexisting"), "cert.pem")) + require.NoError(t, err) + + state := readImportedState(t, paths.ACMEStatePath) + assert.NotContains(t, state.Certificates, "san:preexisting") +} + +func TestRestoreCertificateStore_MissingArchive(t *testing.T) { + _, err := RestoreCertificateStore(CertStoreRestoreOptions{ + ArchivePath: filepath.Join(t.TempDir(), "nope.tar.gz"), + Paths: testCertStorePaths(t), + }) + require.Error(t, err) +} + +func TestVerifyCertificateArchive_ReportsDomainsAndExpiries(t *testing.T) { + // One live and one expired certificate: both report, neither fails + // verification -- a faithful backup of an expired cert is still a backup. + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com", "www.example.com"}) + + expiredNotAfter := time.Now().Add(-24 * time.Hour) + expired := testCertResource(t, []string{"expired.test"}, time.Now().Add(-48*time.Hour), expiredNotAfter) + expiredID := sanCertID([]string{"expired.test"}) + require.NoError(t, writeCertificateFiles(paths.CertsPath, expiredID, expired.Certificate, expired.PrivateKey)) + state := readImportedState(t, paths.ACMEStatePath) + state.Certificates[expiredID] = &ManagedCert{Identifier: expiredID, Domains: []string{"expired.test"}, NotAfter: expiredNotAfter} + state.DomainMap["expired.test"] = expiredID + require.NoError(t, writeManagerStateFile(paths.ACMEStatePath, state)) + + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + _, err := ExportCertificateStore(paths, archivePath) + require.NoError(t, err) + + report, err := VerifyCertificateArchive(archivePath) + require.NoError(t, err) + + require.Len(t, report.Certificates, 2) + byDomain := map[string]ArchiveCertInfo{} + for _, cert := range report.Certificates { + require.NotEmpty(t, cert.Domains) + byDomain[cert.Domains[0]] = cert + } + + live := byDomain["example.com"] + assert.Equal(t, []string{"example.com", "www.example.com"}, live.Domains) + assert.False(t, live.NotAfter.Before(time.Now())) + + gone := byDomain["expired.test"] + assert.True(t, gone.NotAfter.Before(time.Now())) + + assert.Equal(t, 3, report.DomainMappings) + assert.True(t, report.HasAccountKey) + assert.True(t, report.HasDynamicDomains) +} + +func TestVerifyCertificateArchive_RejectsBadArchives(t *testing.T) { + // A parseable pair to embed in otherwise-broken archives. + resource := testCertResource(t, []string{"example.com"}, time.Now().Add(-time.Hour), time.Now().Add(time.Hour)) + certID := sanitizeFilename(sanCertID([]string{"example.com"})) + validState := []byte(`{"certificates":{},"domain_map":{},"saved_at":"2026-08-09T00:00:00Z"}`) + + tests := []struct { + name string + entries map[string][]byte + errPart string + }{ + { + name: "path traversal", + entries: map[string][]byte{"../evil": []byte("x"), "acme.state": validState}, + errPart: "entry", + }, + { + name: "absolute path", + entries: map[string][]byte{"/etc/passwd": []byte("x"), "acme.state": validState}, + errPart: "entry", + }, + { + name: "traversal inside certs", + entries: map[string][]byte{"certs/../../evil/cert.pem": []byte("x"), "acme.state": validState}, + errPart: "entry", + }, + { + name: "unexpected entry", + entries: map[string][]byte{"extra.txt": []byte("x"), "acme.state": validState}, + errPart: "entry", + }, + { + name: "certificate missing its key", + entries: map[string][]byte{ + "acme.state": validState, + "certs/" + certID + "/cert.pem": resource.Certificate, + }, + errPart: "key.pem", + }, + { + name: "unparseable certificate pair", + entries: map[string][]byte{ + "acme.state": validState, + "certs/" + certID + "/cert.pem": []byte("not a cert"), + "certs/" + certID + "/key.pem": []byte("not a key"), + }, + errPart: certID, + }, + { + name: "certificates without a state file", + entries: map[string][]byte{ + "certs/" + certID + "/cert.pem": resource.Certificate, + "certs/" + certID + "/key.pem": resource.PrivateKey, + }, + errPart: "acme.state", + }, + { + name: "unparseable state", + entries: map[string][]byte{"acme.state": []byte("{torn")}, + errPart: "acme.state", + }, + { + name: "state with a dangling domain mapping", + entries: map[string][]byte{"acme.state": []byte(`{"certificates":{},"domain_map":{"a.test":"san:missing"},"saved_at":"2026-08-09T00:00:00Z"}`)}, + errPart: "a.test", + }, + { + name: "state with a null certificate record", + entries: map[string][]byte{"acme.state": []byte(`{"certificates":{"san:x":null},"domain_map":{},"saved_at":"2026-08-09T00:00:00Z"}`)}, + errPart: "null", + }, + { + name: "empty archive", + entries: map[string][]byte{}, + errPart: "empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, tt.entries) + + _, err := VerifyCertificateArchive(archivePath) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errPart) + + // Restore must refuse the same archives. + _, err = RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: testCertStorePaths(t)}) + require.Error(t, err) + }) + } +} + +func TestVerifyCertificateArchive_RejectsSymlinkEntries(t *testing.T) { + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + + file, err := os.Create(archivePath) + require.NoError(t, err) + gz := gzip.NewWriter(file) + tw := tar.NewWriter(gz) + require.NoError(t, tw.WriteHeader(&tar.Header{ + Name: "acme.state", Typeflag: tar.TypeSymlink, Linkname: "/etc/passwd", + })) + require.NoError(t, tw.Close()) + require.NoError(t, gz.Close()) + require.NoError(t, file.Close()) + + _, err = VerifyCertificateArchive(archivePath) + require.Error(t, err) +} + +func TestVerifyCertificateArchive_WarnsOnStateReferencingMissingCert(t *testing.T) { + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, map[string][]byte{ + "acme.state": []byte(`{ + "certificates":{"san:gone":{"identifier":"san:gone","domains":["gone.test"],"not_after":"2027-01-01T00:00:00Z"}}, + "domain_map":{"gone.test":"san:gone"}, + "saved_at":"2026-08-09T00:00:00Z"}`), + }) + + report, err := VerifyCertificateArchive(archivePath) + require.NoError(t, err) + require.Len(t, report.Warnings, 1) + assert.Contains(t, report.Warnings[0], "san:gone") +} + +func TestVerifyCertificateArchive_NotAnArchive(t *testing.T) { + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + require.NoError(t, os.WriteFile(archivePath, []byte("not gzip"), 0600)) + + _, err := VerifyCertificateArchive(archivePath) + require.Error(t, err) +} diff --git a/internal/server/commands.go b/internal/server/commands.go index 3a2bb79..b8fbee4 100644 --- a/internal/server/commands.go +++ b/internal/server/commands.go @@ -67,6 +67,13 @@ type RolloutStopArgs struct { Service string } +type CertsExportArgs struct { + // Path is where the archive is written, resolved server-side: the CLI + // absolutizes it before the call, and CLI and server share a filesystem + // (the socket is local by construction). + Path string +} + type CachePurgeArgs struct { Service string PathPrefix string @@ -223,6 +230,32 @@ func (h *CommandHandler) CacheStats(args CacheStatsArgs, reply *CacheStats) erro return nil } +// CertsExport archives the certificate store from inside the running proxy, +// under the store's disk-write lock, so a backup taken mid-renewal is never +// torn. Without a certificate manager the store has no writers, so the export +// runs directly. +func (h *CommandHandler) CertsExport(args CertsExportArgs, reply *CertsExportSummary) error { + if h.server == nil { + return errors.New("certificate export is not available") + } + + paths := h.server.config.CertStorePaths() + + var summary CertsExportSummary + var err error + if manager := h.router.SANCertManager(); manager != nil { + summary, err = manager.ExportStore(paths, args.Path) + } else { + summary, err = ExportCertificateStore(paths, args.Path) + } + if err != nil { + return err + } + + *reply = summary + return nil +} + func (h *CommandHandler) DomainsStatus(args bool, reply *DomainsStatusResponse) error { dynamicDomains := h.router.DynamicDomainManager() if dynamicDomains == nil { diff --git a/internal/server/commands_test.go b/internal/server/commands_test.go index 9052703..b0ce42f 100644 --- a/internal/server/commands_test.go +++ b/internal/server/commands_test.go @@ -2,6 +2,7 @@ package server import ( "net/http" + "path/filepath" "sync" "testing" @@ -111,3 +112,31 @@ func TestRouter_CacheStatsAndPurgeReadTheStoreUnderTheLock(t *testing.T) { } wg.Wait() } + +func TestCommandHandler_CertsExport(t *testing.T) { + router := testRouter(t) + handler := NewCommandHandler(router) + + // Without a running server there is no config to locate the store. + var summary CertsExportSummary + err := handler.CertsExport(CertsExportArgs{Path: filepath.Join(t.TempDir(), "backup.tar.gz")}, &summary) + require.ErrorContains(t, err, "not available") + + // With a server config pointing at a populated data dir, the export runs + // even without a certificate manager (a proxy started without --acme-email + // can still hold files worth backing up). + dataDir := t.TempDir() + handler.server = &Server{config: &Config{AlternateConfigDir: dataDir}} + paths := handler.server.config.CertStorePaths() + populateCertStore(t, paths, []string{"example.com"}) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + require.NoError(t, handler.CertsExport(CertsExportArgs{Path: outputPath}, &summary)) + assert.Equal(t, 1, summary.Certificates) + assert.FileExists(t, outputPath) + + // With a manager installed, the export goes through its disk-write lock. + router.SetSANCertManager(testSANCertManager(t)) + require.NoError(t, handler.CertsExport(CertsExportArgs{Path: outputPath}, &summary)) + assert.Equal(t, 1, summary.Certificates) +} diff --git a/internal/server/config.go b/internal/server/config.go index c079cd3..e7d60bc 100644 --- a/internal/server/config.go +++ b/internal/server/config.go @@ -148,6 +148,16 @@ func (c Config) DynamicDomainsStatePath() string { return path.Join(c.dataDirectory(), "dynamic-domains.state") } +// CertStorePaths names the pieces of the certificate estate for export and +// restore. +func (c Config) CertStorePaths() CertStorePaths { + return CertStorePaths{ + CertsPath: c.CertificatePath(), + ACMEStatePath: c.ACMEStatePath(), + DynamicDomainsStatePath: c.DynamicDomainsStatePath(), + } +} + func (c Config) DynamicRedirectsStatePath() string { return path.Join(c.dataDirectory(), "dynamic-redirects.state") } diff --git a/internal/server/san_cert_dynamic.go b/internal/server/san_cert_dynamic.go index d1abe7a..f6ccb35 100644 --- a/internal/server/san_cert_dynamic.go +++ b/internal/server/san_cert_dynamic.go @@ -174,13 +174,19 @@ func (m *SANCertManager) removeCertificate(certID string) { } m.mu.Unlock() + // File removal and the state write share the store's disk-write lock, so a + // concurrent export never captures a state file referencing a half-removed + // certificate directory. + m.stateMu.Lock() + defer m.stateMu.Unlock() + if m.config.CachePath != "" { if err := os.RemoveAll(filepath.Join(m.config.CachePath, sanitizeFilename(certID))); err != nil { slog.Warn("Failed to remove certificate files", "certificate", certID, "error", err) } } - if err := m.persistState(); err != nil { + if err := m.persistStateLocked(); err != nil { slog.Warn("Failed to save state", "error", err) } } diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index 6709fe7..810de77 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -87,8 +87,12 @@ type SANCertManagerConfig struct { // It batches up to 100 domains into a single certificate, // reducing the number of certificates and avoiding rate limits. type SANCertManager struct { - mu sync.RWMutex - stateMu sync.Mutex // serializes state-file snapshots and writes + mu sync.RWMutex + // stateMu serializes every disk write to the certificate store: state-file + // snapshots, certificate file writes, and certificate removals. Holding it + // yields a consistent on-disk snapshot, which is what the store exporter + // relies on. + stateMu sync.Mutex config SANCertManagerConfig // ACME clients. All are built on the SAME account (`acme_user.json`), so @@ -585,15 +589,17 @@ func (m *SANCertManager) adoptCertificate(resource *certificate.Resource, sorted } m.mu.Unlock() - // Save certificate to disk + // Save certificate and state under the store's disk-write lock, so a + // concurrent export never sees the certificate files and the state file + // mid-update. + m.stateMu.Lock() if err := m.saveCertificate(certID, resource); err != nil { slog.Warn("Failed to save certificate", "error", err) } - - // Persist state (called without lock held) - if err := m.persistState(); err != nil { + if err := m.persistStateLocked(); err != nil { slog.Warn("Failed to save state", "error", err) } + m.stateMu.Unlock() slog.Info("Certificate provisioned successfully", "identifier", certID, @@ -695,7 +701,7 @@ func (m *SANCertManager) GetStats() map[string]interface{} { // Persistence methods func (m *SANCertManager) loadOrCreateUser() (*acmeUser, error) { - userPath := filepath.Join(m.config.CachePath, "acme_user.json") + userPath := filepath.Join(m.config.CachePath, acmeUserFile) data, err := os.ReadFile(userPath) if err == nil { @@ -739,31 +745,19 @@ func (m *SANCertManager) saveUser() error { return err } - userPath := filepath.Join(m.config.CachePath, "acme_user.json") + userPath := filepath.Join(m.config.CachePath, acmeUserFile) return os.WriteFile(userPath, data, 0600) } +// saveCertificate stores a certificate pair via the same staged-write path the +// offline importers use, so a crash or concurrent reader never sees a torn +// pair. Callers must hold stateMu. func (m *SANCertManager) saveCertificate(certID string, resource *certificate.Resource) error { if m.config.CachePath == "" { return nil } - certDir := filepath.Join(m.config.CachePath, sanitizeFilename(certID)) - if err := os.MkdirAll(certDir, 0700); err != nil { - return err - } - - // Save certificate - if err := os.WriteFile(filepath.Join(certDir, "cert.pem"), resource.Certificate, 0600); err != nil { - return err - } - - // Save private key - if err := os.WriteFile(filepath.Join(certDir, "key.pem"), resource.PrivateKey, 0600); err != nil { - return err - } - - return nil + return writeCertificateFiles(m.config.CachePath, certID, resource.Certificate, resource.PrivateKey) } type managerState struct { @@ -816,16 +810,22 @@ func (m *SANCertManager) loadState() error { } func (m *SANCertManager) persistState() error { - if m.config.StatePath == "" { - return nil - } - // Serialize snapshot+write: issuer goroutines, the renewer, and // handshake-driven provisioning all persist concurrently, and interleaved // writes to the shared .tmp file would corrupt it. m.stateMu.Lock() defer m.stateMu.Unlock() + return m.persistStateLocked() +} + +// persistStateLocked snapshots and writes the state file. Callers must hold +// stateMu (and not m.mu, which it takes itself). +func (m *SANCertManager) persistStateLocked() error { + if m.config.StatePath == "" { + return nil + } + m.mu.RLock() certs := make(map[string]*ManagedCert, len(m.certificates)) for k, v := range m.certificates { diff --git a/internal/server/traefik_import.go b/internal/server/traefik_import.go index 42a8145..3ccf013 100644 --- a/internal/server/traefik_import.go +++ b/internal/server/traefik_import.go @@ -312,22 +312,11 @@ func loadStateForImport(path string) (managerState, error) { return managerState{}, fmt.Errorf("refusing to overwrite the unreadable certificate state %s: %w", path, err) } - if state.Certificates == nil || state.DomainMap == nil { - return managerState{}, fmt.Errorf("refusing to overwrite %s: it does not look like a certificate state file", path) - } - // A healthy manager never persists null records or dangling mappings // (removeCertificate unmaps domains in the same critical section), so // either one means the file is not trustworthy enough to merge into. - for id, cert := range state.Certificates { - if cert == nil { - return managerState{}, fmt.Errorf("refusing to overwrite %s: certificate %q is null", path, id) - } - } - for domain, id := range state.DomainMap { - if _, ok := state.Certificates[id]; !ok { - return managerState{}, fmt.Errorf("refusing to overwrite %s: domain %q references a missing certificate %q", path, domain, id) - } + if err := validateManagerState(state); err != nil { + return managerState{}, fmt.Errorf("refusing to overwrite %s: %w", path, err) } return state, nil From 9b705e60baa68718ba9f674f3df27aeec7dbe0d8 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sun, 9 Aug 2026 21:12:58 +0200 Subject: [PATCH 2/8] fix(cert-store): harden export/restore per review findings Addresses the cubic review on PR #95: - adoptCertificate/removeCertificate publish maps AND write files under one hold of stateMu, so a concurrent persist or export can never capture a state file naming a certificate whose pair is not on disk yet - export validates the state file (validateManagerState), parses every certificate pair before archiving (warn+skip on failure), refuses a store with certificates but no state, and rejects output paths inside the store - the archive is staged via os.CreateTemp (unique name, enforced 0600), fsynced before rename, with a best-effort directory sync after - archive reader: entry-count cap alongside the byte cap, certificate dirs must be in sanitized form, the account key must hold parseable key material (warn+drop otherwise), and every archived pair is cross-checked against its state record (domains coverage + expiry at second precision) - restore: account key and dynamic state written via staged rename; target dirs for state-referenced certificates absent from the archive are removed so they re-order instead of reviving the old pair - CLI: verify/force/traefik-acme are one exclusivity group, `certs ` help, and a clear error (not an unsafe offline fallback) when the running proxy predates the CertsExport RPC verb - README: runbook gains the redeploy step; verification via TLS handshake since `domains list` only covers dynamic domains ## Verification - [x] gofmt -l internal/ cmd/ clean; go vet; make lint (0 issues) - [x] make test passes (1928 tests); go test -race on the cert paths --- README.md | 11 +- internal/cmd/export.go | 10 +- internal/cmd/import.go | 11 +- internal/server/cert_store_archive.go | 85 +++++++++-- internal/server/cert_store_export.go | 84 ++++++++++- internal/server/cert_store_export_test.go | 97 +++++++++++- internal/server/cert_store_restore.go | 41 ++++- internal/server/cert_store_restore_test.go | 165 ++++++++++++++++++++- internal/server/commands_test.go | 11 +- internal/server/san_cert_dynamic.go | 12 +- internal/server/san_cert_manager.go | 10 +- 11 files changed, 484 insertions(+), 53 deletions(-) diff --git a/README.md b/README.md index fb4c14d..59b1ded 100644 --- a/README.md +++ b/README.md @@ -1215,10 +1215,13 @@ kamal-proxy import certs --archive /backup/certs-2026-08-09.tar.gz --verify 2. Restore the estate: `kamal-proxy import certs --archive /backup/certs-2026-08-09.tar.gz` (add `--data-dir` if the proxy runs with one). The import refuses to overwrite a non-empty certificate store unless you pass `--force`. -3. Start the proxy. -4. Verify: `kamal-proxy domains list` and the certificate expiry metrics - should show the restored estate; handshakes for restored domains serve - immediately, with no new ACME orders. +3. Start the proxy, then redeploy your TLS services (or restore the routing + state separately) — the archive holds certificates, not routes, and the + proxy refuses a TLS handshake for a host no service is deployed for. +4. Verify a restored static host with a TLS handshake; the certificate expiry + metrics should show the restored estate, with no new ACME orders. + (`kamal-proxy domains list` covers only dynamic `--tls-domains-source` + domains.) Restores run offline against the data directory, sharing their writing path with the Traefik `acme.json` importer (`import certs --traefik-acme`), so diff --git a/internal/cmd/export.go b/internal/cmd/export.go index 7e73feb..e62c1ef 100644 --- a/internal/cmd/export.go +++ b/internal/cmd/export.go @@ -4,6 +4,7 @@ import ( "fmt" "net/rpc" "path/filepath" + "strings" "github.com/spf13/cobra" @@ -38,7 +39,7 @@ type exportCertsCommand struct { func newExportCertsCommand() *exportCertsCommand { exportCertsCommand := &exportCertsCommand{} exportCertsCommand.cmd = &cobra.Command{ - Use: "certs [output-path]", + Use: "certs ", Short: "Export the certificate store to an archive for disaster recovery", Long: "Export the certificate store -- ACME account key, issued certificates,\n" + "domain mappings, and dynamic domain state -- to a gzipped tar archive.\n\n" + @@ -86,6 +87,13 @@ func (c *exportCertsCommand) export(cmd *cobra.Command, outputPath string) (serv if dialErr == nil { defer client.Close() err := client.Call("kamal-proxy.CertsExport", server.CertsExportArgs{Path: outputPath}, &summary) + if err != nil && strings.Contains(err.Error(), "can't find method") { + // A proxy is answering the socket but predates this command. Do + // NOT fall back to reading the data dir -- that proxy is live and + // writing, which is exactly the torn-snapshot case the RPC path + // exists to prevent. + return summary, fmt.Errorf("the running proxy does not support certificate export; upgrade it, or stop it and re-run for an offline export: %w", err) + } return summary, err } diff --git a/internal/cmd/import.go b/internal/cmd/import.go index feb4dac..a72cd1c 100644 --- a/internal/cmd/import.go +++ b/internal/cmd/import.go @@ -1,7 +1,6 @@ package cmd import ( - "errors" "fmt" "strings" "time" @@ -63,16 +62,18 @@ func newImportCertsCommand() *importCertsCommand { importCertsCommand.cmd.MarkFlagsOneRequired("traefik-acme", "archive") importCertsCommand.cmd.MarkFlagsMutuallyExclusive("traefik-acme", "archive") importCertsCommand.cmd.MarkFlagsMutuallyExclusive("archive", "resolver") - importCertsCommand.cmd.MarkFlagsMutuallyExclusive("verify", "force") + // --verify and --force are archive-only and meaningless for a Traefik + // import; grouping traefik-acme with them makes cobra reject those + // combinations while still allowing --archive with either. + importCertsCommand.cmd.MarkFlagsMutuallyExclusive("verify", "force", "traefik-acme") return importCertsCommand } func (c *importCertsCommand) run(cmd *cobra.Command, args []string) error { + // The flag groups guarantee --verify comes with --archive: one of + // traefik-acme/archive is required, and verify excludes traefik-acme. if c.verify { - if c.archivePath == "" { - return errors.New("--verify requires --archive") - } return c.runVerify(cmd) } diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go index 76b7e07..a388e7f 100644 --- a/internal/server/cert_store_archive.go +++ b/internal/server/cert_store_archive.go @@ -14,12 +14,20 @@ import ( "path" "slices" "strings" + "time" + + "github.com/go-acme/lego/v4/certcrypto" ) -// maxCertArchiveBytes caps how much an archive may decompress to. The whole -// estate of a 1,000-domain fleet is a few megabytes; anything near this limit -// is not a certificate backup. -const maxCertArchiveBytes = 512 << 20 +// maxCertArchiveBytes caps how much an archive may decompress to, and +// maxCertArchiveEntries caps how many entries it may hold (zero-length entries +// cost no payload bytes, so a byte cap alone would not bound the tar walk). +// The whole estate of a 1,000-domain fleet is a few megabytes across a few +// thousand entries; anything near these limits is not a certificate backup. +const ( + maxCertArchiveBytes = 512 << 20 + maxCertArchiveEntries = 100_000 +) // archiveCertPair is one certificate directory from an archive, parsed and // validated. @@ -80,6 +88,11 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { return archive, fmt.Errorf("failed to read the archive %s: %w", archivePath, err) } + entryCount++ + if entryCount > maxCertArchiveEntries { + return archive, fmt.Errorf("refusing the archive %s: more than %d entries", archivePath, maxCertArchiveEntries) + } + if header.Typeflag == tar.TypeDir { continue } @@ -96,7 +109,6 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { if err != nil { return archive, fmt.Errorf("failed to read the archive entry %q: %w", header.Name, err) } - entryCount++ if err := archive.placeEntry(header.Name, data, rawCerts); err != nil { return archive, err @@ -143,7 +155,11 @@ func (a *certStoreArchive) placeEntry(name string, data []byte, rawCerts map[str if rest, ok := strings.CutPrefix(name, archiveCertsPrefix); ok { dir, base, found := strings.Cut(rest, "/") - if found && dir != "" && (base == "cert.pem" || base == "key.pem") && !strings.Contains(base, "/") { + // The directory must already be in the sanitized form the exporter + // writes: two spellings that sanitize to the same on-disk path would + // otherwise silently overwrite each other during a restore. + if found && dir != "" && dir == sanitizeFilename(dir) && + (base == "cert.pem" || base == "key.pem") && !strings.Contains(base, "/") { if rawCerts[dir] == nil { rawCerts[dir] = map[string][]byte{} } @@ -183,8 +199,11 @@ func (a *certStoreArchive) assembleCertPairs(rawCerts map[string]map[string][]by return nil } -// validate cross-checks the state file against the archived certificates. +// validate cross-checks the state file against the archived certificates and +// discards an account key that could not carry the ACME identity forward. func (a *certStoreArchive) validate() error { + a.checkAccountKey() + if !a.hasState { if len(a.certs) > 0 { return errors.New("the archive contains certificates but no acme.state; it cannot restore a working store") @@ -196,19 +215,63 @@ func (a *certStoreArchive) validate() error { return fmt.Errorf("the archive's %s is not trustworthy: %w", archiveStateEntry, err) } - // A state-referenced certificate missing from the archive restores to the - // same place loadState puts a missing file: the domain re-orders. Warn, - // don't fail -- the export warned identically when it happened. for _, id := range slices.Sorted(maps.Keys(a.state.Certificates)) { - if _, ok := a.certs[sanitizeFilename(id)]; !ok { + record := a.state.Certificates[id] + + pair, ok := a.certs[sanitizeFilename(id)] + if !ok { + // A state-referenced certificate missing from the archive restores + // to the same place loadState puts a missing file: the domain + // re-orders. Warn, don't fail -- the export warned identically. a.warnings = append(a.warnings, fmt.Sprintf("certificate %s is referenced by the state file but missing from the archive; its domains will re-order after a restore", id)) + continue + } + + // The certificate must actually be what the state record says it is: + // restoring a record whose leaf names other hosts or expired earlier + // would have the manager serving the wrong certificate, or keeping it + // past its real expiry. + for _, domain := range record.Domains { + if !slices.Contains(pair.leaf.DNSNames, domain) { + return fmt.Errorf("the archived certificate %s does not cover %q, which its state record claims", id, domain) + } + } + // Compared at second precision: x509 validity has no sub-second field, + // while state metadata written from other sources may. + if !record.NotAfter.Truncate(time.Second).Equal(pair.leaf.NotAfter.Truncate(time.Second)) { + return fmt.Errorf("the archived certificate %s expires %s, but its state record says %s", + id, pair.leaf.NotAfter.Format(time.RFC3339), record.NotAfter.Format(time.RFC3339)) } } return nil } +// checkAccountKey drops an account key entry that does not hold usable key +// material, with a warning: restoring it would make the next boot silently +// register a fresh ACME account while the operator believes the identity was +// preserved. The estate's certificates still restore. +func (a *certStoreArchive) checkAccountKey() { + if a.accountKey == nil { + return + } + + var user acmeUser + if err := json.Unmarshal(a.accountKey, &user); err != nil { + a.warnings = append(a.warnings, + fmt.Sprintf("the archived ACME account key does not parse and will not be restored; the next boot will register a fresh account: %v", err)) + a.accountKey = nil + return + } + + if _, err := certcrypto.ParsePEMPrivateKey(user.KeyPEM); err != nil { + a.warnings = append(a.warnings, + fmt.Sprintf("the archived ACME account key holds no usable private key and will not be restored; the next boot will register a fresh account: %v", err)) + a.accountKey = nil + } +} + // validateManagerState checks the invariants a healthy manager always // maintains: both maps present, no null certificate records, no domain mapped // to a certificate that is not there. Shared by the Traefik importer (before diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go index 28747f1..c3d879b 100644 --- a/internal/server/cert_store_export.go +++ b/internal/server/cert_store_export.go @@ -3,6 +3,7 @@ package server import ( "archive/tar" "compress/gzip" + "crypto/tls" "encoding/json" "errors" "fmt" @@ -79,15 +80,27 @@ func (m *SANCertManager) ExportStore(paths CertStorePaths, outputPath string) (C func ExportCertificateStore(paths CertStorePaths, outputPath string) (CertsExportSummary, error) { summary := CertsExportSummary{} + if err := rejectOutputInsideStore(paths, outputPath); err != nil { + return summary, err + } + state, files, err := collectStateEntry(paths.ACMEStatePath, &summary) if err != nil { return summary, err } + hasState := len(files) > 0 certFiles, err := collectCertsEntries(paths.CertsPath, state, &summary) if err != nil { return summary, err } + + // Certificates without a state file cannot restore into a working store + // (the state is the estate's index), and the archive reader rejects that + // shape -- fail the backup now rather than hand over an unrestorable one. + if !hasState && len(certFiles) > 0 { + return summary, fmt.Errorf("the certificate store has certificates but no state file at %s; refusing to export an unrestorable archive", paths.ACMEStatePath) + } files = append(files, certFiles...) if dynamic, ok := collectOptionalJSON(paths.DynamicDomainsStatePath, archiveDynamicDomainsEntry, &summary); ok { @@ -127,6 +140,13 @@ func collectStateEntry(path string, summary *CertsExportSummary) (managerState, return state, nil, fmt.Errorf("refusing to export the unreadable state file %s: %w", path, err) } + // An inconsistent state file exports into an archive the verifier and the + // restore path reject; fail the backup while the operator can still fix + // the live store. + if err := validateManagerState(state); err != nil { + return state, nil, fmt.Errorf("refusing to export the state file %s: %w", path, err) + } + summary.Certificates = len(state.Certificates) summary.Domains = len(state.DomainMap) @@ -177,8 +197,10 @@ func collectCertsEntries(certsPath string, state managerState, summary *CertsExp } // collectCertPair captures one certificate directory's cert.pem and key.pem. -// A directory with only half the pair is skipped with a warning: restoring it -// would leave a certificate the manager cannot load. +// A directory with only half the pair, or a pair that does not parse, is +// skipped with a warning: the strict archive reader would reject the whole +// archive over it, and a certificate the manager cannot load is not worth +// failing the backup for. func collectCertPair(certsPath, dir string, summary *CertsExportSummary) ([]archiveFile, bool) { pair := make([]archiveFile, 0, 2) @@ -192,9 +214,39 @@ func collectCertPair(certsPath, dir string, summary *CertsExportSummary) ([]arch pair = append(pair, archiveFile{name: archiveCertsPrefix + dir + "/" + name, data: data, modTime: modTime}) } + if _, err := tls.X509KeyPair(pair[0].data, pair[1].data); err != nil { + summary.Warnings = append(summary.Warnings, + fmt.Sprintf("not exported: certificate %s does not parse: %v", dir, err)) + return nil, false + } + return pair, true } +// rejectOutputInsideStore refuses an output path that would overwrite part of +// the store being exported -- writing the archive over acme.state completes +// the export and then destroys the live state it archived. +func rejectOutputInsideStore(paths CertStorePaths, outputPath string) error { + output, err := filepath.Abs(outputPath) + if err != nil { + return fmt.Errorf("failed to resolve the output path: %w", err) + } + + for _, statePath := range []string{paths.ACMEStatePath, paths.DynamicDomainsStatePath} { + if abs, err := filepath.Abs(statePath); err == nil && abs == output { + return fmt.Errorf("refusing to write the archive over the store's own %s", filepath.Base(statePath)) + } + } + + if certsAbs, err := filepath.Abs(paths.CertsPath); err == nil { + if output == certsAbs || strings.HasPrefix(output, certsAbs+string(filepath.Separator)) { + return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + } + } + + return nil +} + // warnMissingStateCerts flags certificates the state file references that have // no files on disk. The restore side treats them the same way loadState does: // the domain falls through to ordinary provisioning. @@ -243,17 +295,23 @@ func readFileWithModTime(path string) ([]byte, time.Time, error) { } // writeCertArchive writes the staged files as a gzipped tarball, staged as a -// temp file and renamed into place so a partial write never looks like a valid -// backup. The archive holds private keys: it is created with mode 0600. +// uniquely named same-directory temp file and renamed into place so a partial +// write never looks like a valid backup and a pre-planted path cannot redirect +// the write. The archive holds private keys: the temp file is created 0600 by +// CreateTemp and chmodded to be certain. It is fsynced before the rename -- +// this is a disaster-recovery artifact, "written" has to mean "on disk". func writeCertArchive(outputPath string, files []archiveFile) error { - tmpPath := outputPath + ".tmp" - - file, err := os.OpenFile(tmpPath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) + file, err := os.CreateTemp(filepath.Dir(outputPath), filepath.Base(outputPath)+".*.tmp") if err != nil { return fmt.Errorf("failed to create the archive: %w", err) } + tmpPath := file.Name() err = func() error { + if err := file.Chmod(0600); err != nil { + return err + } + gz := gzip.NewWriter(file) tw := tar.NewWriter(gz) @@ -275,7 +333,10 @@ func writeCertArchive(outputPath string, files []archiveFile) error { if err := tw.Close(); err != nil { return err } - return gz.Close() + if err := gz.Close(); err != nil { + return err + } + return file.Sync() }() if err != nil { file.Close() @@ -293,5 +354,12 @@ func writeCertArchive(outputPath string, files []archiveFile) error { return fmt.Errorf("failed to finalize the archive: %w", err) } + // Best-effort directory sync so the rename itself survives power loss; + // not every filesystem supports it, and the archive is already durable. + if dir, err := os.Open(filepath.Dir(outputPath)); err == nil { + _ = dir.Sync() + dir.Close() + } + return nil } diff --git a/internal/server/cert_store_export_test.go b/internal/server/cert_store_export_test.go index 87f31a0..ff7b0e0 100644 --- a/internal/server/cert_store_export_test.go +++ b/internal/server/cert_store_export_test.go @@ -3,6 +3,9 @@ package server import ( "archive/tar" "compress/gzip" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" "encoding/json" "io" "os" @@ -10,6 +13,7 @@ import ( "testing" "time" + "github.com/go-acme/lego/v4/certcrypto" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -55,13 +59,26 @@ func populateCertStore(t testing.TB, paths CertStorePaths, domainSets ...[]strin require.NoError(t, os.MkdirAll(paths.CertsPath, 0700)) require.NoError(t, writeManagerStateFile(paths.ACMEStatePath, state)) require.NoError(t, os.WriteFile(filepath.Join(paths.CertsPath, "acme_user.json"), - []byte(`{"email":"ops@example.com","key_pem":"dGVzdA=="}`), 0600)) + testAccountKeyJSON(t), 0600)) require.NoError(t, os.WriteFile(paths.DynamicDomainsStatePath, []byte(`{"services":{},"quarantine":{},"saved_at":"2026-08-09T00:00:00Z"}`), 0600)) return state } +// testAccountKeyJSON builds an acme_user.json with real key material, the way +// saveUser writes it. +func testAccountKeyJSON(t testing.TB) []byte { + t.Helper() + + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + data, err := json.Marshal(acmeUser{Email: "ops@example.com", KeyPEM: certcrypto.PEMEncode(key)}) + require.NoError(t, err) + return data +} + // readCertArchive extracts an exported archive into a name -> content map. func readCertArchive(t testing.TB, path string) map[string][]byte { t.Helper() @@ -250,15 +267,81 @@ func TestSANCertManager_ExportStoreHoldsTheDiskLock(t *testing.T) { assert.Equal(t, 1, summary.Domains) } -func TestExportCertificateStore_StateWithoutMapsStillCounts(t *testing.T) { +func TestExportCertificateStore_InvalidStateIsAnError(t *testing.T) { + tests := []struct { + name string + state string + }{ + // A state without its maps would export into an archive the reader + // rejects, so the export fails instead of producing it. + {name: "missing maps", state: `{"saved_at":"2026-08-09T00:00:00Z"}`}, + {name: "dangling domain mapping", state: `{"certificates":{},"domain_map":{"a.test":"san:missing"},"saved_at":"2026-08-09T00:00:00Z"}`}, + {name: "null certificate record", state: `{"certificates":{"san:x":null},"domain_map":{},"saved_at":"2026-08-09T00:00:00Z"}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + paths := testCertStorePaths(t) + require.NoError(t, os.MkdirAll(paths.CertsPath, 0700)) + require.NoError(t, os.WriteFile(paths.ACMEStatePath, []byte(tt.state), 0600)) + + _, err := ExportCertificateStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) + require.Error(t, err) + }) + } +} + +func TestExportCertificateStore_CertsWithoutStateIsAnError(t *testing.T) { paths := testCertStorePaths(t) - require.NoError(t, os.MkdirAll(paths.CertsPath, 0700)) - require.NoError(t, os.WriteFile(paths.ACMEStatePath, []byte(`{"saved_at":"2026-08-09T00:00:00Z"}`), 0600)) + populateCertStore(t, paths, []string{"example.com"}) + require.NoError(t, os.Remove(paths.ACMEStatePath)) + + _, err := ExportCertificateStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) + require.Error(t, err) + assert.Contains(t, err.Error(), "no state file") +} + +func TestExportCertificateStore_SkipsUnparseablePairs(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}, []string{"broken.test"}) + + brokenID := sanitizeFilename(sanCertID([]string{"broken.test"})) + require.NoError(t, os.WriteFile(filepath.Join(paths.CertsPath, brokenID, "cert.pem"), []byte("not a cert"), 0600)) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + summary, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + + // Both the skip and the resulting state/disk mismatch warn. + require.NotEmpty(t, summary.Warnings) + joined := "" + for _, warning := range summary.Warnings { + joined += warning + "\n" + } + assert.Contains(t, joined, "does not parse") + + entries := readCertArchive(t, outputPath) + assert.NotContains(t, entries, "certs/"+brokenID+"/cert.pem") + assert.NotContains(t, entries, "certs/"+brokenID+"/key.pem") +} + +func TestExportCertificateStore_RejectsOutputInsideTheStore(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) - summary, err := ExportCertificateStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) + for _, outputPath := range []string{ + paths.ACMEStatePath, + paths.DynamicDomainsStatePath, + filepath.Join(paths.CertsPath, "backup.tar.gz"), + } { + _, err := ExportCertificateStore(paths, outputPath) + require.Error(t, err, "output path %s is inside the store", outputPath) + assert.Contains(t, err.Error(), "refusing") + } + + // The store must be untouched afterwards. + _, err := ExportCertificateStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) require.NoError(t, err) - assert.Equal(t, 0, summary.Certificates) - assert.Equal(t, 0, summary.Domains) } func TestExportCertificateStore_SkipsInvalidOptionalFiles(t *testing.T) { diff --git a/internal/server/cert_store_restore.go b/internal/server/cert_store_restore.go index af8d1b0..ba923dc 100644 --- a/internal/server/cert_store_restore.go +++ b/internal/server/cert_store_restore.go @@ -83,18 +83,26 @@ func RestoreCertificateStore(opts CertStoreRestoreOptions) (CertsRestoreSummary, } } + // A forced restore over an existing store must not let a leftover target + // directory answer for a state-referenced certificate the archive itself + // does not hold -- the "will re-order" warning would instead silently + // revive whatever pair the old store had under that identifier. + if err := removeStaleCertDirs(opts.Paths.CertsPath, archive); err != nil { + return summary, err + } + if archive.accountKey != nil { if err := os.MkdirAll(opts.Paths.CertsPath, 0700); err != nil { return summary, fmt.Errorf("failed to create the certificate directory: %w", err) } - if err := os.WriteFile(filepath.Join(opts.Paths.CertsPath, acmeUserFile), archive.accountKey, 0600); err != nil { + if err := writeFileStaged(filepath.Join(opts.Paths.CertsPath, acmeUserFile), archive.accountKey); err != nil { return summary, fmt.Errorf("failed to restore the ACME account key: %w", err) } summary.AccountKeyRestored = true } if archive.dynamicDomains != nil { - if err := os.WriteFile(opts.Paths.DynamicDomainsStatePath, archive.dynamicDomains, 0600); err != nil { + if err := writeFileStaged(opts.Paths.DynamicDomainsStatePath, archive.dynamicDomains); err != nil { return summary, fmt.Errorf("failed to restore the dynamic domains state: %w", err) } summary.DynamicDomainsRestored = true @@ -152,6 +160,35 @@ func VerifyCertificateArchive(archivePath string) (CertArchiveReport, error) { return report, nil } +// removeStaleCertDirs deletes target directories for certificates the +// restored state references but the archive does not hold, so those domains +// actually re-order instead of serving whatever the old store left behind. +func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { + for _, id := range slices.Sorted(maps.Keys(archive.state.Certificates)) { + dir := sanitizeFilename(id) + if _, ok := archive.certs[dir]; ok { + continue + } + + if err := os.RemoveAll(filepath.Join(certsPath, dir)); err != nil { + return fmt.Errorf("failed to remove the stale certificate directory for %s: %w", id, err) + } + } + + return nil +} + +// writeFileStaged writes a file through a same-directory temp file and a +// rename, so an interrupted restore never leaves the target truncated. +func writeFileStaged(path string, data []byte) error { + tmpPath := path + ".tmp" + if err := os.WriteFile(tmpPath, data, 0600); err != nil { + return err + } + + return os.Rename(tmpPath, path) +} + // certStoreOccupant names the first thing found occupying the target store, or // "" when the store is empty. An existing but empty certs directory does not // count. diff --git a/internal/server/cert_store_restore_test.go b/internal/server/cert_store_restore_test.go index d298009..4bc879a 100644 --- a/internal/server/cert_store_restore_test.go +++ b/internal/server/cert_store_restore_test.go @@ -3,6 +3,9 @@ package server import ( "archive/tar" "compress/gzip" + "crypto/tls" + "crypto/x509" + "encoding/json" "os" "path/filepath" "testing" @@ -13,8 +16,13 @@ import ( ) // exportedTestArchive populates a store and exports it, returning the archive -// path and the state that was captured. +// path, the state that was captured, and the source store's paths. func exportedTestArchive(t testing.TB, domainSets ...[]string) (string, managerState) { + archivePath, state, _ := exportedTestArchiveWithSource(t, domainSets...) + return archivePath, state +} + +func exportedTestArchiveWithSource(t testing.TB, domainSets ...[]string) (string, managerState, CertStorePaths) { t.Helper() paths := testCertStorePaths(t) @@ -24,7 +32,7 @@ func exportedTestArchive(t testing.TB, domainSets ...[]string) (string, managerS _, err := ExportCertificateStore(paths, archivePath) require.NoError(t, err) - return archivePath, state + return archivePath, state, paths } // writeTestArchive crafts a tar.gz with exactly the given entries, for @@ -48,7 +56,7 @@ func writeTestArchive(t testing.TB, path string, entries map[string][]byte) { } func TestRestoreCertificateStore_RoundTrip(t *testing.T) { - archivePath, exported := exportedTestArchive(t, []string{"example.com", "www.example.com"}, []string{"other.test"}) + archivePath, exported, source := exportedTestArchiveWithSource(t, []string{"example.com", "www.example.com"}, []string{"other.test"}) target := testCertStorePaths(t) summary, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: target}) @@ -81,7 +89,9 @@ func TestRestoreCertificateStore_RoundTrip(t *testing.T) { // Account key and dynamic domain state come back byte-for-byte. accountKey, err := os.ReadFile(filepath.Join(target.CertsPath, "acme_user.json")) require.NoError(t, err) - assert.JSONEq(t, `{"email":"ops@example.com","key_pem":"dGVzdA=="}`, string(accountKey)) + sourceKey, err := os.ReadFile(filepath.Join(source.CertsPath, "acme_user.json")) + require.NoError(t, err) + assert.Equal(t, sourceKey, accountKey) _, err = os.Stat(target.DynamicDomainsStatePath) require.NoError(t, err) @@ -342,3 +352,150 @@ func TestVerifyCertificateArchive_NotAnArchive(t *testing.T) { _, err := VerifyCertificateArchive(archivePath) require.Error(t, err) } + +// stateJSON marshals a managerState for hand-built archives. +func stateJSON(t testing.TB, state managerState) []byte { + t.Helper() + + data, err := json.Marshal(state) + require.NoError(t, err) + return data +} + +// resourceLeaf parses the leaf certificate of a test resource. +func resourceLeaf(t testing.TB, certPEM, keyPEM []byte) *x509.Certificate { + t.Helper() + + tlsCert, err := tls.X509KeyPair(certPEM, keyPEM) + require.NoError(t, err) + leaf, err := x509.ParseCertificate(tlsCert.Certificate[0]) + require.NoError(t, err) + return leaf +} + +func TestRestoreCertificateStore_ForceRemovesStaleStateReferencedDirs(t *testing.T) { + // The archive's state references a certificate whose files the archive + // does not hold. The target store has a leftover directory under that same + // identifier: restoring must remove it, or the "will re-order" warning + // would silently revive the old pair instead. + staleID := sanCertID([]string{"stale.test"}) + notAfter := time.Now().Add(30 * 24 * time.Hour) + + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, map[string][]byte{ + "acme.state": stateJSON(t, managerState{ + Certificates: map[string]*ManagedCert{ + staleID: {Identifier: staleID, Domains: []string{"stale.test"}, NotAfter: notAfter}, + }, + DomainMap: map[string]string{"stale.test": staleID}, + SavedAt: time.Now(), + }), + }) + + paths := testCertStorePaths(t) + old := testCertResource(t, []string{"stale.test"}, time.Now().Add(-time.Hour), notAfter) + require.NoError(t, writeCertificateFiles(paths.CertsPath, staleID, old.Certificate, old.PrivateKey)) + + summary, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: paths, Force: true}) + require.NoError(t, err) + require.NotEmpty(t, summary.Warnings) + + assert.NoDirExists(t, filepath.Join(paths.CertsPath, sanitizeFilename(staleID)), + "the stale directory must not answer for a certificate the archive does not hold") +} + +func TestReadCertStoreArchive_RejectsUnsanitizedDirNames(t *testing.T) { + // "san:x" sanitizes to "san_x": two spellings, one on-disk path. Only the + // sanitized form the exporter writes is accepted. + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, map[string][]byte{ + "acme.state": []byte(`{"certificates":{},"domain_map":{},"saved_at":"2026-08-09T00:00:00Z"}`), + "certs/san:x/cert.pem": []byte("x"), + "certs/san:x/key.pem": []byte("x"), + }) + + _, err := VerifyCertificateArchive(archivePath) + require.Error(t, err) + assert.Contains(t, err.Error(), "unexpected archive entry") +} + +func TestVerifyCertificateArchive_DropsInvalidAccountKey(t *testing.T) { + tests := []struct { + name string + key []byte + }{ + {name: "not JSON at all is rejected at export time, valid JSON without key material is not", key: []byte(`{"email":"ops@example.com"}`)}, + {name: "garbage key material", key: []byte(`{"email":"ops@example.com","key_pem":"bm90IGEga2V5"}`)}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, map[string][]byte{ + "acme.state": []byte(`{"certificates":{},"domain_map":{},"saved_at":"2026-08-09T00:00:00Z"}`), + "certs/acme_user.json": tt.key, + }) + + report, err := VerifyCertificateArchive(archivePath) + require.NoError(t, err) + assert.False(t, report.HasAccountKey) + require.NotEmpty(t, report.Warnings) + + // The restore skips it rather than planting a broken identity. + summary, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: testCertStorePaths(t)}) + require.NoError(t, err) + assert.False(t, summary.AccountKeyRestored) + }) + } +} + +func TestVerifyCertificateArchive_RejectsLeafDisagreeingWithState(t *testing.T) { + resource := testCertResource(t, []string{"example.com"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)) + leaf := resourceLeaf(t, resource.Certificate, resource.PrivateKey) + certID := sanCertID([]string{"example.com"}) + dir := sanitizeFilename(certID) + + tests := []struct { + name string + record *ManagedCert + errPart string + }{ + { + name: "state claims a domain the leaf does not cover", + record: &ManagedCert{Identifier: certID, Domains: []string{"evil.test", "example.com"}, NotAfter: leaf.NotAfter}, + errPart: "evil.test", + }, + { + name: "state expiry disagrees with the leaf", + record: &ManagedCert{Identifier: certID, Domains: []string{"example.com"}, NotAfter: leaf.NotAfter.Add(90 * 24 * time.Hour)}, + errPart: "expires", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + domainMap := map[string]string{} + for _, domain := range tt.record.Domains { + domainMap[domain] = certID + } + + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, map[string][]byte{ + "acme.state": stateJSON(t, managerState{ + Certificates: map[string]*ManagedCert{certID: tt.record}, + DomainMap: domainMap, + SavedAt: time.Now(), + }), + "certs/" + dir + "/cert.pem": resource.Certificate, + "certs/" + dir + "/key.pem": resource.PrivateKey, + }) + + _, err := VerifyCertificateArchive(archivePath) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errPart) + + _, err = RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: testCertStorePaths(t)}) + require.Error(t, err) + }) + } +} diff --git a/internal/server/commands_test.go b/internal/server/commands_test.go index b0ce42f..231047b 100644 --- a/internal/server/commands_test.go +++ b/internal/server/commands_test.go @@ -133,7 +133,16 @@ func TestCommandHandler_CertsExport(t *testing.T) { outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") require.NoError(t, handler.CertsExport(CertsExportArgs{Path: outputPath}, &summary)) assert.Equal(t, 1, summary.Certificates) - assert.FileExists(t, outputPath) + + // The summary counts come from acme.state; prove the tarball itself holds + // the estate, not just that a file appeared. + entries := readCertArchive(t, outputPath) + certDir := "certs/" + sanitizeFilename(sanCertID([]string{"example.com"})) + assert.Contains(t, entries, "acme.state") + assert.Contains(t, entries, "certs/acme_user.json") + assert.Contains(t, entries, "dynamic-domains.state") + assert.Contains(t, entries, certDir+"/cert.pem") + assert.Contains(t, entries, certDir+"/key.pem") // With a manager installed, the export goes through its disk-write lock. router.SetSANCertManager(testSANCertManager(t)) diff --git a/internal/server/san_cert_dynamic.go b/internal/server/san_cert_dynamic.go index f6ccb35..ac740f9 100644 --- a/internal/server/san_cert_dynamic.go +++ b/internal/server/san_cert_dynamic.go @@ -165,6 +165,12 @@ func (m *SANCertManager) ManagedCertificates() []*ManagedCert { // removeCertificate drops a certificate from the maps, deletes its cached // files, and persists. Domains still mapped to it are unmapped. func (m *SANCertManager) removeCertificate(certID string) { + // Map changes, file removal, and the state write all happen under one hold + // of the store's disk-write lock, so a concurrent persist or export never + // captures the removal half-applied. + m.stateMu.Lock() + defer m.stateMu.Unlock() + m.mu.Lock() delete(m.certificates, certID) for domain, id := range m.domainToCert { @@ -174,12 +180,6 @@ func (m *SANCertManager) removeCertificate(certID string) { } m.mu.Unlock() - // File removal and the state write share the store's disk-write lock, so a - // concurrent export never captures a state file referencing a half-removed - // certificate directory. - m.stateMu.Lock() - defer m.stateMu.Unlock() - if m.config.CachePath != "" { if err := os.RemoveAll(filepath.Join(m.config.CachePath, sanitizeFilename(certID))); err != nil { slog.Warn("Failed to remove certificate files", "certificate", certID, "error", err) diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index 810de77..e6edc41 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -582,6 +582,12 @@ func (m *SANCertManager) adoptCertificate(resource *certificate.Resource, sorted Certificate: &tlsCert, } + // The maps are published and the files written under one hold of the + // store's disk-write lock: if the in-memory maps changed hands first, + // another goroutine's persist could snapshot a state file naming this + // certificate before its files exist, and an export taken at that moment + // would archive the incomplete pair. + m.stateMu.Lock() m.mu.Lock() m.certificates[certID] = managed for _, d := range sortedDomains { @@ -589,10 +595,6 @@ func (m *SANCertManager) adoptCertificate(resource *certificate.Resource, sorted } m.mu.Unlock() - // Save certificate and state under the store's disk-write lock, so a - // concurrent export never sees the certificate files and the state file - // mid-update. - m.stateMu.Lock() if err := m.saveCertificate(certID, resource); err != nil { slog.Warn("Failed to save certificate", "error", err) } From 6e479f1041e595ebe92f4ebb8dc477bae42237e8 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sun, 9 Aug 2026 21:52:32 +0200 Subject: [PATCH 3/8] =?UTF-8?q?fix(cert-store):=20second=20review=20round?= =?UTF-8?q?=20=E2=80=94=20deadlock,=20unsafe=20identifiers,=20self-verifyi?= =?UTF-8?q?ng=20exports?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic's round-2 findings on PR #95: - Initialize no longer holds the manager lock across the legacy cache import: adoption takes the store's locks itself, so the first boot after an upgrade deadlocked whenever certs/http01/ held a certificate. This bug predates the PR (adoptCertificate always took m.mu); the new regression test drives real Initialize against a stub ACME directory. - validateManagerState (shared by export, archive reader, and the Traefik importer) now rejects: identifiers that are path-special after sanitization (a crafted ".." id could make restore's RemoveAll escape the store), identifiers colliding on one sanitized directory, Identifier/map-key mismatches, and domains mapped to certificates that do not cover them (wildcard-aware via identifiersCover) - export: the staged archive is read back through the strict reader before the rename, so a published backup is restorable by construction; the store-path guard resolves symlinks on both sides; an account-key-only store exports; directory-sync failures surface (only genuinely unsupported filesystems are excused) - reader: the decompression cap wraps the whole gzip stream (PAX/GNU metadata counted, not just payloads); emptiness is decided by regular files, not directory headers; the account key must hold an ECDSA key, mirroring loadOrCreateUser - restore: stale-dir removal runs after the state commit, so a restore that fails mid-way leaves the old store's files intact - CLI: the outdated-proxy detection matches the exact rpc lookup error - README: routing-state restore ordered before proxy startup ## Verification - [x] gofmt clean; go vet; make lint (0 issues) - [x] full suite x3 green (1939 tests); go test -race on internal/server --- README.md | 11 +- internal/cmd/export.go | 2 +- internal/server/cert_store_archive.go | 101 +++++++++++++++---- internal/server/cert_store_export.go | 69 +++++++++++-- internal/server/cert_store_export_test.go | 38 +++++++ internal/server/cert_store_restore.go | 26 +++-- internal/server/cert_store_restore_test.go | 111 +++++++++++++++++++++ internal/server/san_cert_manager.go | 40 +++++--- internal/server/san_cert_manager_test.go | 80 +++++++++++++++ 9 files changed, 423 insertions(+), 55 deletions(-) diff --git a/README.md b/README.md index 59b1ded..58e885f 100644 --- a/README.md +++ b/README.md @@ -1215,10 +1215,13 @@ kamal-proxy import certs --archive /backup/certs-2026-08-09.tar.gz --verify 2. Restore the estate: `kamal-proxy import certs --archive /backup/certs-2026-08-09.tar.gz` (add `--data-dir` if the proxy runs with one). The import refuses to overwrite a non-empty certificate store unless you pass `--force`. -3. Start the proxy, then redeploy your TLS services (or restore the routing - state separately) — the archive holds certificates, not routes, and the - proxy refuses a TLS handshake for a host no service is deployed for. -4. Verify a restored static host with a TLS handshake; the certificate expiry +3. If you keep a backup of the routing state (`kamal-proxy.state`), restore + it now, while the proxy is still stopped — the proxy saves routing state + on changes, so a copy restored after startup would be overwritten. +4. Start the proxy. If no routing state was restored, redeploy your TLS + services — the archive holds certificates, not routes, and the proxy + refuses a TLS handshake for a host no service is deployed for. +5. Verify a restored static host with a TLS handshake; the certificate expiry metrics should show the restored estate, with no new ACME orders. (`kamal-proxy domains list` covers only dynamic `--tls-domains-source` domains.) diff --git a/internal/cmd/export.go b/internal/cmd/export.go index e62c1ef..d106df6 100644 --- a/internal/cmd/export.go +++ b/internal/cmd/export.go @@ -87,7 +87,7 @@ func (c *exportCertsCommand) export(cmd *cobra.Command, outputPath string) (serv if dialErr == nil { defer client.Close() err := client.Call("kamal-proxy.CertsExport", server.CertsExportArgs{Path: outputPath}, &summary) - if err != nil && strings.Contains(err.Error(), "can't find method") { + if err != nil && strings.HasPrefix(err.Error(), "rpc: can't find method kamal-proxy.CertsExport") { // A proxy is answering the socket but predates this command. Do // NOT fall back to reading the data dir -- that proxy is live and // writing, which is exactly the torn-snapshot case the RPC path diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go index a388e7f..be2d112 100644 --- a/internal/server/cert_store_archive.go +++ b/internal/server/cert_store_archive.go @@ -3,6 +3,7 @@ package server import ( "archive/tar" "compress/gzip" + "crypto/ecdsa" "crypto/tls" "crypto/x509" "encoding/json" @@ -29,6 +30,30 @@ const ( maxCertArchiveEntries = 100_000 ) +// errCertArchiveTooLarge marks the decompressed-size cap being hit mid-read. +var errCertArchiveTooLarge = errors.New("certificate archive decompresses beyond the size limit") + +// cappedReader bounds how many bytes may be read through it, failing with +// errCertArchiveTooLarge instead of a bare EOF so the caller can tell a +// too-large archive from a truncated one. +type cappedReader struct { + reader io.Reader + remaining int64 +} + +func (c *cappedReader) Read(p []byte) (int, error) { + if c.remaining <= 0 { + return 0, errCertArchiveTooLarge + } + if int64(len(p)) > c.remaining { + p = p[:c.remaining] + } + + n, err := c.reader.Read(p) + c.remaining -= int64(n) + return n, err +} + // archiveCertPair is one certificate directory from an archive, parsed and // validated. type archiveCertPair struct { @@ -75,16 +100,23 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { defer gz.Close() rawCerts := map[string]map[string][]byte{} - entryCount := 0 - var totalBytes int64 + entryCount, fileCount := 0, 0 + + // The cap sits around the whole decompressed gzip stream, not just entry + // payloads: PAX and GNU metadata records are consumed inside Next() and + // would otherwise be free decompression work for a hostile archive. + capped := &cappedReader{reader: gz, remaining: maxCertArchiveBytes} - tr := tar.NewReader(gz) + tr := tar.NewReader(capped) for { header, err := tr.Next() if err == io.EOF { break } if err != nil { + if errors.Is(err, errCertArchiveTooLarge) || capped.remaining <= 0 { + return archive, fmt.Errorf("refusing the archive %s: it decompresses beyond %d bytes", archivePath, int64(maxCertArchiveBytes)) + } return archive, fmt.Errorf("failed to read the archive %s: %w", archivePath, err) } @@ -99,14 +131,13 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { if header.Typeflag != tar.TypeReg { return archive, fmt.Errorf("refusing archive entry %q: only regular files belong in a certificate archive", header.Name) } + fileCount++ - totalBytes += header.Size - if totalBytes > maxCertArchiveBytes { - return archive, fmt.Errorf("refusing the archive %s: it decompresses beyond %d bytes", archivePath, int64(maxCertArchiveBytes)) - } - - data, err := io.ReadAll(io.LimitReader(tr, maxCertArchiveBytes)) + data, err := io.ReadAll(tr) if err != nil { + if errors.Is(err, errCertArchiveTooLarge) { + return archive, fmt.Errorf("refusing the archive %s: it decompresses beyond %d bytes", archivePath, int64(maxCertArchiveBytes)) + } return archive, fmt.Errorf("failed to read the archive entry %q: %w", header.Name, err) } @@ -115,7 +146,9 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { } } - if entryCount == 0 { + // Directory headers alone do not make an archive: emptiness is decided by + // regular files, while the entry cap above counts every header. + if fileCount == 0 { return archive, fmt.Errorf("the archive %s is empty", archivePath) } @@ -265,32 +298,64 @@ func (a *certStoreArchive) checkAccountKey() { return } - if _, err := certcrypto.ParsePEMPrivateKey(user.KeyPEM); err != nil { - a.warnings = append(a.warnings, - fmt.Sprintf("the archived ACME account key holds no usable private key and will not be restored; the next boot will register a fresh account: %v", err)) - a.accountKey = nil + // Mirror loadOrCreateUser exactly: it only accepts an ECDSA key, so any + // other key type would be silently discarded at boot and a fresh account + // registered -- the very outcome this check exists to make loud. + key, err := certcrypto.ParsePEMPrivateKey(user.KeyPEM) + if err == nil { + if _, ok := key.(*ecdsa.PrivateKey); ok { + return + } + err = errors.New("the key is not an ECDSA key") } + + a.warnings = append(a.warnings, + fmt.Sprintf("the archived ACME account key holds no usable private key and will not be restored; the next boot will register a fresh account: %v", err)) + a.accountKey = nil } // validateManagerState checks the invariants a healthy manager always -// maintains: both maps present, no null certificate records, no domain mapped -// to a certificate that is not there. Shared by the Traefik importer (before -// merging into an existing state file) and the archive reader. +// maintains: both maps present; no null certificate records; identifiers that +// are safe as directory names, unique after sanitization, and consistent with +// their map key; and every domain mapped to a certificate that exists and +// actually covers it. Shared by the Traefik importer (before merging into an +// existing state file) and the archive reader. func validateManagerState(state managerState) error { if state.Certificates == nil || state.DomainMap == nil { return errors.New("it does not look like a certificate state file") } + dirs := map[string]string{} for id, cert := range state.Certificates { if cert == nil { return fmt.Errorf("certificate %q is null", id) } + if cert.Identifier != id { + return fmt.Errorf("certificate %q carries the mismatched identifier %q", id, cert.Identifier) + } + + // The sanitized identifier becomes an on-disk directory under the + // certificate path: path-special names would escape it, and two + // identifiers sharing one sanitized form would overwrite (or delete) + // each other's files. + dir := sanitizeFilename(id) + if dir == "" || dir == "." || dir == ".." { + return fmt.Errorf("certificate %q does not name a safe storage directory", id) + } + if earlier, ok := dirs[dir]; ok { + return fmt.Errorf("certificates %q and %q collide on the storage directory %q", earlier, id, dir) + } + dirs[dir] = id } for domain, id := range state.DomainMap { - if _, ok := state.Certificates[id]; !ok { + cert, ok := state.Certificates[id] + if !ok { return fmt.Errorf("domain %q references a missing certificate %q", domain, id) } + if !identifiersCover(cert.Domains, domain) { + return fmt.Errorf("domain %q is mapped to certificate %q, which does not cover it", domain, id) + } } return nil diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go index c3d879b..378984f 100644 --- a/internal/server/cert_store_export.go +++ b/internal/server/cert_store_export.go @@ -12,6 +12,7 @@ import ( "path/filepath" "slices" "strings" + "syscall" "time" ) @@ -98,7 +99,12 @@ func ExportCertificateStore(paths CertStorePaths, outputPath string) (CertsExpor // Certificates without a state file cannot restore into a working store // (the state is the estate's index), and the archive reader rejects that // shape -- fail the backup now rather than hand over an unrestorable one. - if !hasState && len(certFiles) > 0 { + // The account key alone is not a certificate: a fresh estate that has only + // registered an account still gets its backup. + certsWithoutState := slices.ContainsFunc(certFiles, func(file archiveFile) bool { + return file.name != archiveAccountKeyEntry + }) + if !hasState && certsWithoutState { return summary, fmt.Errorf("the certificate store has certificates but no state file at %s; refusing to export an unrestorable archive", paths.ACMEStatePath) } files = append(files, certFiles...) @@ -225,21 +231,23 @@ func collectCertPair(certsPath, dir string, summary *CertsExportSummary) ([]arch // rejectOutputInsideStore refuses an output path that would overwrite part of // the store being exported -- writing the archive over acme.state completes -// the export and then destroys the live state it archived. +// the export and then destroys the live state it archived. Paths are compared +// after resolving symlinks, so a link into the store cannot slip past the +// guard. func rejectOutputInsideStore(paths CertStorePaths, outputPath string) error { - output, err := filepath.Abs(outputPath) + output, err := resolveForComparison(outputPath) if err != nil { return fmt.Errorf("failed to resolve the output path: %w", err) } for _, statePath := range []string{paths.ACMEStatePath, paths.DynamicDomainsStatePath} { - if abs, err := filepath.Abs(statePath); err == nil && abs == output { + if resolved, err := resolveForComparison(statePath); err == nil && resolved == output { return fmt.Errorf("refusing to write the archive over the store's own %s", filepath.Base(statePath)) } } - if certsAbs, err := filepath.Abs(paths.CertsPath); err == nil { - if output == certsAbs || strings.HasPrefix(output, certsAbs+string(filepath.Separator)) { + if certsResolved, err := resolveForComparison(paths.CertsPath); err == nil { + if output == certsResolved || strings.HasPrefix(output, certsResolved+string(filepath.Separator)) { return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) } } @@ -247,6 +255,30 @@ func rejectOutputInsideStore(paths CertStorePaths, outputPath string) error { return nil } +// resolveForComparison absolutizes a path and resolves the symlinks in every +// component that exists: the path itself when it does, otherwise its deepest +// existing ancestor, with the non-existing remainder rejoined. +func resolveForComparison(path string) (string, error) { + abs, err := filepath.Abs(path) + if err != nil { + return "", err + } + + remainder := "" + for current := abs; ; { + if resolved, err := filepath.EvalSymlinks(current); err == nil { + return filepath.Join(resolved, remainder), nil + } + + parent := filepath.Dir(current) + if parent == current { + return abs, nil + } + remainder = filepath.Join(filepath.Base(current), remainder) + current = parent + } +} + // warnMissingStateCerts flags certificates the state file references that have // no files on disk. The restore side treats them the same way loadState does: // the domain falls through to ordinary provisioning. @@ -349,16 +381,31 @@ func writeCertArchive(outputPath string, files []archiveFile) error { return fmt.Errorf("failed to write the archive: %w", err) } + // Read the staged archive back through the same strict reader verify and + // restore use, so a published export is restorable by construction -- any + // disagreement between what was collected and what the reader accepts + // fails the backup here, not in a disaster. + if _, err := readCertStoreArchive(tmpPath); err != nil { + os.Remove(tmpPath) + return fmt.Errorf("the staged archive failed verification: %w", err) + } + if err := os.Rename(tmpPath, outputPath); err != nil { os.Remove(tmpPath) return fmt.Errorf("failed to finalize the archive: %w", err) } - // Best-effort directory sync so the rename itself survives power loss; - // not every filesystem supports it, and the archive is already durable. - if dir, err := os.Open(filepath.Dir(outputPath)); err == nil { - _ = dir.Sync() - dir.Close() + // Sync the directory so the rename itself survives power loss. Only a + // filesystem that genuinely does not support syncing a directory is + // excused; a real failure means the backup's existence is not durable, + // which a disaster-recovery artifact cannot shrug off. + dir, err := os.Open(filepath.Dir(outputPath)) + if err != nil { + return fmt.Errorf("failed to sync the archive's directory: %w", err) + } + defer dir.Close() + if err := dir.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) && !errors.Is(err, syscall.EINVAL) { + return fmt.Errorf("failed to sync the archive's directory: %w", err) } return nil diff --git a/internal/server/cert_store_export_test.go b/internal/server/cert_store_export_test.go index ff7b0e0..58a0cb3 100644 --- a/internal/server/cert_store_export_test.go +++ b/internal/server/cert_store_export_test.go @@ -371,3 +371,41 @@ func TestCertsExportSummary_RoundTrips(t *testing.T) { require.NoError(t, json.Unmarshal(data, &back)) assert.Equal(t, summary, back) } + +func TestExportCertificateStore_AccountKeyOnlyStoreExports(t *testing.T) { + // A fresh estate that has only registered its ACME account is still worth + // backing up, and must not trip the certificates-without-state refusal. + paths := testCertStorePaths(t) + require.NoError(t, os.MkdirAll(paths.CertsPath, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(paths.CertsPath, "acme_user.json"), testAccountKeyJSON(t), 0600)) + + outputPath := filepath.Join(t.TempDir(), "backup.tar.gz") + summary, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + assert.Equal(t, 0, summary.Certificates) + + report, err := VerifyCertificateArchive(outputPath) + require.NoError(t, err) + assert.True(t, report.HasAccountKey) +} + +func TestExportCertificateStore_RejectsSymlinkedOutputIntoTheStore(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + + // A symlink pointing at the live state file must not slip past the guard. + linkPath := filepath.Join(t.TempDir(), "innocent.tar.gz") + require.NoError(t, os.Symlink(paths.ACMEStatePath, linkPath)) + + _, err := ExportCertificateStore(paths, linkPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing") + + // A symlinked directory into the store is refused too. + linkDir := filepath.Join(t.TempDir(), "linkdir") + require.NoError(t, os.Symlink(paths.CertsPath, linkDir)) + + _, err = ExportCertificateStore(paths, filepath.Join(linkDir, "backup.tar.gz")) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing") +} diff --git a/internal/server/cert_store_restore.go b/internal/server/cert_store_restore.go index ba923dc..960e398 100644 --- a/internal/server/cert_store_restore.go +++ b/internal/server/cert_store_restore.go @@ -83,14 +83,6 @@ func RestoreCertificateStore(opts CertStoreRestoreOptions) (CertsRestoreSummary, } } - // A forced restore over an existing store must not let a leftover target - // directory answer for a state-referenced certificate the archive itself - // does not hold -- the "will re-order" warning would instead silently - // revive whatever pair the old store had under that identifier. - if err := removeStaleCertDirs(opts.Paths.CertsPath, archive); err != nil { - return summary, err - } - if archive.accountKey != nil { if err := os.MkdirAll(opts.Paths.CertsPath, 0700); err != nil { return summary, fmt.Errorf("failed to create the certificate directory: %w", err) @@ -116,6 +108,17 @@ func RestoreCertificateStore(opts CertStoreRestoreOptions) (CertsRestoreSummary, summary.Domains = len(archive.state.DomainMap) } + // A forced restore over an existing store must not let a leftover target + // directory answer for a state-referenced certificate the archive itself + // does not hold -- the "will re-order" warning would instead silently + // revive whatever pair the old store had under that identifier. This runs + // after the state commit on purpose: a restore that fails mid-way leaves + // the OLD store's files intact rather than an old state file pointing at + // deleted directories. + if err := removeStaleCertDirs(opts.Paths.CertsPath, archive); err != nil { + return summary, err + } + return summary, nil } @@ -170,6 +173,13 @@ func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { continue } + // validateManagerState already rejects identifiers that do not name a + // safe directory; this is the last line of defense in front of an + // os.RemoveAll that must never resolve outside certsPath. + if dir == "" || dir == "." || dir == ".." { + return fmt.Errorf("refusing to remove the unsafe certificate directory for %q", id) + } + if err := os.RemoveAll(filepath.Join(certsPath, dir)); err != nil { return fmt.Errorf("failed to remove the stale certificate directory for %s: %w", id, err) } diff --git a/internal/server/cert_store_restore_test.go b/internal/server/cert_store_restore_test.go index 4bc879a..a189715 100644 --- a/internal/server/cert_store_restore_test.go +++ b/internal/server/cert_store_restore_test.go @@ -2,15 +2,21 @@ package server import ( "archive/tar" + "bytes" "compress/gzip" + "crypto/rand" + "crypto/rsa" "crypto/tls" "crypto/x509" "encoding/json" + "io" "os" "path/filepath" "testing" "time" + "github.com/go-acme/lego/v4/certcrypto" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -426,6 +432,7 @@ func TestVerifyCertificateArchive_DropsInvalidAccountKey(t *testing.T) { }{ {name: "not JSON at all is rejected at export time, valid JSON without key material is not", key: []byte(`{"email":"ops@example.com"}`)}, {name: "garbage key material", key: []byte(`{"email":"ops@example.com","key_pem":"bm90IGEga2V5"}`)}, + {name: "non-ECDSA key, which loadOrCreateUser would silently discard", key: testRSAAccountKeyJSON(t)}, } for _, tt := range tests { @@ -499,3 +506,107 @@ func TestVerifyCertificateArchive_RejectsLeafDisagreeingWithState(t *testing.T) }) } } + +func TestVerifyCertificateArchive_RejectsCorruptStateRecords(t *testing.T) { + tests := []struct { + name string + state managerState + errPart string + }{ + { + name: "path-special identifier", + state: managerState{ + Certificates: map[string]*ManagedCert{ + "..": {Identifier: "..", Domains: []string{"a.test"}, NotAfter: time.Now()}, + }, + DomainMap: map[string]string{"a.test": ".."}, + }, + errPart: "safe storage directory", + }, + { + name: "identifier disagrees with its map key", + state: managerState{ + Certificates: map[string]*ManagedCert{ + "san:a": {Identifier: "san:b", Domains: []string{"a.test"}, NotAfter: time.Now()}, + }, + DomainMap: map[string]string{"a.test": "san:a"}, + }, + errPart: "mismatched identifier", + }, + { + name: "identifiers colliding after sanitization", + state: managerState{ + Certificates: map[string]*ManagedCert{ + "san:x": {Identifier: "san:x", Domains: []string{"a.test"}, NotAfter: time.Now()}, + "san_x": {Identifier: "san_x", Domains: []string{"b.test"}, NotAfter: time.Now()}, + }, + DomainMap: map[string]string{"a.test": "san:x", "b.test": "san_x"}, + }, + errPart: "collide", + }, + { + name: "domain mapped to a certificate that does not cover it", + state: managerState{ + Certificates: map[string]*ManagedCert{ + "san:a": {Identifier: "san:a", Domains: []string{"bar.test"}, NotAfter: time.Now()}, + }, + DomainMap: map[string]string{"foo.test": "san:a"}, + }, + errPart: "does not cover", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, map[string][]byte{ + "acme.state": stateJSON(t, tt.state), + }) + + _, err := VerifyCertificateArchive(archivePath) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.errPart) + + // The same archive must never reach a restore's RemoveAll. + _, err = RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: testCertStorePaths(t)}) + require.Error(t, err) + }) + } +} + +func TestVerifyCertificateArchive_DirectoryOnlyArchiveIsEmpty(t *testing.T) { + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + + file, err := os.Create(archivePath) + require.NoError(t, err) + gz := gzip.NewWriter(file) + tw := tar.NewWriter(gz) + require.NoError(t, tw.WriteHeader(&tar.Header{Name: "certs/", Typeflag: tar.TypeDir, Mode: 0700})) + require.NoError(t, tw.Close()) + require.NoError(t, gz.Close()) + require.NoError(t, file.Close()) + + _, err = VerifyCertificateArchive(archivePath) + require.Error(t, err) + assert.Contains(t, err.Error(), "empty") +} + +func TestCappedReader_FailsBeyondTheLimit(t *testing.T) { + capped := &cappedReader{reader: bytes.NewReader(make([]byte, 100)), remaining: 10} + + _, err := io.ReadAll(capped) + require.ErrorIs(t, err, errCertArchiveTooLarge) +} + +// testRSAAccountKeyJSON builds an acme_user.json holding a valid RSA key -- +// parseable, but not the ECDSA key loadOrCreateUser requires. +func testRSAAccountKeyJSON(t testing.TB) []byte { + t.Helper() + + key, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + data, err := json.Marshal(acmeUser{Email: "ops@example.com", KeyPEM: certcrypto.PEMEncode(key)}) + require.NoError(t, err) + return data +} diff --git a/internal/server/san_cert_manager.go b/internal/server/san_cert_manager.go index e6edc41..a7f7479 100644 --- a/internal/server/san_cert_manager.go +++ b/internal/server/san_cert_manager.go @@ -216,6 +216,33 @@ func NewSANCertManager(config SANCertManagerConfig) (*SANCertManager, error) { // Initialize sets up the ACME client and loads persisted state func (m *SANCertManager) Initialize(ctx context.Context) error { + if err := m.initializeClients(); err != nil { + return err + } + + // Runs with no locks held: adoption takes the store's own locks, and + // calling it from inside the initialization critical section would + // deadlock on m.mu the moment the legacy cache holds a certificate. + m.importLegacyHTTP01Cache() + + m.mu.Lock() + m.ready = true + m.mu.Unlock() + + slog.Info("SAN certificate manager initialized", + "email", m.config.Email, + "directory", m.config.Directory, + "dns_provider", m.config.DNSProvider, + "prefer_wildcard", m.config.PreferWildcard, + "http_fallback", m.config.HTTPFallback, + ) + + return nil +} + +// initializeClients builds the ACME clients and loads persisted state, under +// the manager lock. +func (m *SANCertManager) initializeClients() error { m.mu.Lock() defer m.mu.Unlock() @@ -273,19 +300,6 @@ func (m *SANCertManager) Initialize(ctx context.Context) error { slog.Warn("Failed to load certificate state", "error", err) } - // Adopt anything the deleted certificate registry left behind, so an - // upgrade does not re-order certificates the proxy already holds. - m.importLegacyHTTP01Cache() - - m.ready = true - slog.Info("SAN certificate manager initialized", - "email", m.config.Email, - "directory", m.config.Directory, - "dns_provider", m.config.DNSProvider, - "prefer_wildcard", m.config.PreferWildcard, - "http_fallback", m.config.HTTPFallback, - ) - return nil } diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index 46fe7c3..f86bb68 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -1,13 +1,22 @@ package server import ( + "context" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" "crypto/tls" + "encoding/json" + "encoding/pem" "net/http" "net/http/httptest" + "os" "path/filepath" "testing" "time" + "github.com/go-acme/lego/v4/certcrypto" + "github.com/go-acme/lego/v4/registration" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -376,3 +385,74 @@ func TestSanitizeFilename(t *testing.T) { }) } } + +// TestSANCertManager_InitializeAdoptsLegacyCacheWithoutDeadlock is the +// regression test for Initialize holding the manager lock across the legacy +// cache import: adoption takes the store's locks itself, so calling it from +// inside the initialization critical section deadlocked the first boot after +// an upgrade whenever the legacy cache held a certificate. +func TestSANCertManager_InitializeAdoptsLegacyCacheWithoutDeadlock(t *testing.T) { + // A stub ACME directory: Initialize fetches it when building the lego + // client, and with a pre-registered account on disk that is the only + // network round trip. lego insists on HTTPS, so the stub serves TLS and + // its certificate is trusted via LEGO_CA_CERTIFICATES. + var directory *httptest.Server + directory = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + "newNonce": directory.URL + "/nonce", + "newAccount": directory.URL + "/account", + "newOrder": directory.URL + "/order", + "revokeCert": directory.URL + "/revoke", + "keyChange": directory.URL + "/keychange", + })) + })) + defer directory.Close() + + dir := t.TempDir() + + caPath := filepath.Join(dir, "stub-ca.pem") + require.NoError(t, os.WriteFile(caPath, + pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE", Bytes: directory.Certificate().Raw}), 0600)) + t.Setenv("LEGO_CA_CERTIFICATES", caPath) + cachePath := filepath.Join(dir, "certs") + + // A registered account on disk, so Initialize skips ACME registration. + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + user, err := json.Marshal(acmeUser{ + Email: "ops@example.com", + KeyPEM: certcrypto.PEMEncode(key), + Registration: ®istration.Resource{URI: directory.URL + "/account/1"}, + }) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(cachePath, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(cachePath, acmeUserFile), user, 0600)) + + // A legacy autocert cache entry: key PEM followed by the chain. + resource := testCertResource(t, []string{"legacy.test"}, time.Now().Add(-time.Hour), time.Now().Add(60*24*time.Hour)) + legacyDir := filepath.Join(cachePath, legacyHTTP01CacheDir) + require.NoError(t, os.MkdirAll(legacyDir, 0700)) + require.NoError(t, os.WriteFile(filepath.Join(legacyDir, "legacy.test"), + append(append([]byte{}, resource.PrivateKey...), resource.Certificate...), 0600)) + + manager, err := NewSANCertManager(SANCertManagerConfig{ + Email: "ops@example.com", + Directory: directory.URL, + CachePath: cachePath, + StatePath: filepath.Join(dir, "acme.state"), + }) + require.NoError(t, err) + + done := make(chan error, 1) + go func() { done <- manager.Initialize(context.Background()) }() + + select { + case err := <-done: + require.NoError(t, err) + case <-time.After(15 * time.Second): + t.Fatal("Initialize deadlocked while adopting the legacy certificate cache") + } + + assert.True(t, manager.HasCertificate("legacy.test"), "the legacy certificate was not adopted") +} From 8bc16b5ad3fddc1ed7156d0942f6bc60f62fbe1d Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sun, 9 Aug 2026 22:31:51 +0200 Subject: [PATCH 4/8] =?UTF-8?q?fix(cert-store):=20third=20review=20round?= =?UTF-8?q?=20=E2=80=94=20write-path=20identity,=20metadata=20caps,=20exac?= =?UTF-8?q?t=20leaf=20matching?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic's round-3 findings on PR #95: - writeFileStaged (account key, dynamic state) stages through a unique os.CreateTemp file with enforced 0600 and fsync, closing the pre-existing-.tmp permission/symlink hole writeCertArchive already had closed for the archive itself - the export path guard returns the symlink-resolved output path and the write uses that same path (no check-to-write divergence through a swapped parent symlink), and adds os.SameFile identity checks against the store paths and the output's existing ancestors, which also holds on case-insensitive filesystems - the archive reader bounds tar header work: bytes consumed inside Next() -- including PAX/GNU metadata records the entry counter never sees -- are capped at 64MB - leaf-vs-state comparison is now strict set equality in both directions; extra leaf SANs absent from the state record are rejected, since every state writer copies the leaf's DNS names exactly - export surfaces the staged-archive verification's warnings (minus the missing-pair class already reported from the disk side), so an unrestorable ACME account key is loud at backup time - clarified a misleading account-key test case name ## Verification - [x] gofmt clean; go vet; make lint (0 issues) - [x] make test green; go test -race on the cert-store tests --- internal/server/cert_store_archive.go | 30 +++++-- internal/server/cert_store_export.go | 96 ++++++++++++++++------ internal/server/cert_store_export_test.go | 21 +++++ internal/server/cert_store_restore.go | 38 +++++++-- internal/server/cert_store_restore_test.go | 2 +- 5 files changed, 150 insertions(+), 37 deletions(-) diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go index be2d112..1b3b3ca 100644 --- a/internal/server/cert_store_archive.go +++ b/internal/server/cert_store_archive.go @@ -28,6 +28,13 @@ import ( const ( maxCertArchiveBytes = 512 << 20 maxCertArchiveEntries = 100_000 + + // maxCertArchiveHeaderBytes bounds the decompressed bytes spent on tar + // headers and their PAX/GNU metadata records, which archive/tar consumes + // inside Next() before the entry counter can run. 100k plain headers cost + // ~51MB, so the cap leaves legitimate archives room while a hostile chain + // of metadata records runs out of budget. + maxCertArchiveHeaderBytes = 64 << 20 ) // errCertArchiveTooLarge marks the decompressed-size cap being hit mid-read. @@ -108,7 +115,9 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { capped := &cappedReader{reader: gz, remaining: maxCertArchiveBytes} tr := tar.NewReader(capped) + var headerBytes int64 for { + beforeHeader := capped.remaining header, err := tr.Next() if err == io.EOF { break @@ -120,6 +129,13 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { return archive, fmt.Errorf("failed to read the archive %s: %w", archivePath, err) } + // Everything Next() consumed is header work -- including PAX/GNU + // metadata records the entry counter below never sees. + headerBytes += beforeHeader - capped.remaining + if headerBytes > maxCertArchiveHeaderBytes { + return archive, fmt.Errorf("refusing the archive %s: more than %d bytes of tar headers", archivePath, int64(maxCertArchiveHeaderBytes)) + } + entryCount++ if entryCount > maxCertArchiveEntries { return archive, fmt.Errorf("refusing the archive %s: more than %d entries", archivePath, maxCertArchiveEntries) @@ -262,13 +278,13 @@ func (a *certStoreArchive) validate() error { } // The certificate must actually be what the state record says it is: - // restoring a record whose leaf names other hosts or expired earlier - // would have the manager serving the wrong certificate, or keeping it - // past its real expiry. - for _, domain := range record.Domains { - if !slices.Contains(pair.leaf.DNSNames, domain) { - return fmt.Errorf("the archived certificate %s does not cover %q, which its state record claims", id, domain) - } + // restoring a record whose leaf names disagree -- in either direction + // -- would have the manager serving the wrong certificate, and every + // writer of state records copies the leaf's DNS names exactly, so the + // sets must match, not merely overlap. + if !slices.Equal(sortedCopy(record.Domains), sortedCopy(pair.leaf.DNSNames)) { + return fmt.Errorf("the archived certificate %s names %v, but its state record claims %v", + id, pair.leaf.DNSNames, record.Domains) } // Compared at second precision: x509 validity has no sub-second field, // while state metadata written from other sources may. diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go index 378984f..bb7b3a8 100644 --- a/internal/server/cert_store_export.go +++ b/internal/server/cert_store_export.go @@ -81,7 +81,11 @@ func (m *SANCertManager) ExportStore(paths CertStorePaths, outputPath string) (C func ExportCertificateStore(paths CertStorePaths, outputPath string) (CertsExportSummary, error) { summary := CertsExportSummary{} - if err := rejectOutputInsideStore(paths, outputPath); err != nil { + // The write below uses this same resolved path, so the validated path and + // the written path cannot diverge through a parent symlink swapped after + // the check. + outputPath, err := safeOutputPath(paths, outputPath) + if err != nil { return summary, err } @@ -121,10 +125,21 @@ func ExportCertificateStore(paths CertStorePaths, outputPath string) (CertsExpor return strings.Compare(a.name, b.name) }) - if err := writeCertArchive(outputPath, files); err != nil { + readerWarnings, err := writeCertArchive(outputPath, files) + if err != nil { return summary, err } + // The staged-archive verification sees things the collection pass cannot + // -- an account key the reader would refuse to restore, for one. Its + // missing-certificate warnings are skipped: each of those was already + // reported above from the disk side. + for _, warning := range readerWarnings { + if !strings.Contains(warning, "missing from the archive") { + summary.Warnings = append(summary.Warnings, warning) + } + } + return summary, nil } @@ -229,30 +244,61 @@ func collectCertPair(certsPath, dir string, summary *CertsExportSummary) ([]arch return pair, true } -// rejectOutputInsideStore refuses an output path that would overwrite part of -// the store being exported -- writing the archive over acme.state completes -// the export and then destroys the live state it archived. Paths are compared -// after resolving symlinks, so a link into the store cannot slip past the -// guard. -func rejectOutputInsideStore(paths CertStorePaths, outputPath string) error { +// safeOutputPath refuses an output path that would overwrite part of the +// store being exported -- writing the archive over acme.state completes the +// export and then destroys the live state it archived -- and returns the +// symlink-resolved path the caller must write to, so the validated path and +// the written path are one and the same. +// +// Two layers: string comparison on resolved paths, then filesystem identity +// (os.SameFile) against the output's existing ancestors, which also holds on +// case-insensitive filesystems where two spellings name one file. +func safeOutputPath(paths CertStorePaths, outputPath string) (string, error) { output, err := resolveForComparison(outputPath) if err != nil { - return fmt.Errorf("failed to resolve the output path: %w", err) + return "", fmt.Errorf("failed to resolve the output path: %w", err) } for _, statePath := range []string{paths.ACMEStatePath, paths.DynamicDomainsStatePath} { if resolved, err := resolveForComparison(statePath); err == nil && resolved == output { - return fmt.Errorf("refusing to write the archive over the store's own %s", filepath.Base(statePath)) + return "", fmt.Errorf("refusing to write the archive over the store's own %s", filepath.Base(statePath)) + } + if sameExistingFile(statePath, output) { + return "", fmt.Errorf("refusing to write the archive over the store's own %s", filepath.Base(statePath)) } } - if certsResolved, err := resolveForComparison(paths.CertsPath); err == nil { - if output == certsResolved || strings.HasPrefix(output, certsResolved+string(filepath.Separator)) { - return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + certsResolved, err := resolveForComparison(paths.CertsPath) + if err == nil && (output == certsResolved || strings.HasPrefix(output, certsResolved+string(filepath.Separator))) { + return "", fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + } + if certsInfo, err := os.Stat(paths.CertsPath); err == nil { + for current := output; ; { + if info, err := os.Stat(current); err == nil && os.SameFile(certsInfo, info) { + return "", fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + } + parent := filepath.Dir(current) + if parent == current { + break + } + current = parent } } - return nil + return output, nil +} + +// sameExistingFile reports whether two paths name the same existing file. +func sameExistingFile(a, b string) bool { + infoA, err := os.Stat(a) + if err != nil { + return false + } + infoB, err := os.Stat(b) + if err != nil { + return false + } + return os.SameFile(infoA, infoB) } // resolveForComparison absolutizes a path and resolves the symlinks in every @@ -332,10 +378,11 @@ func readFileWithModTime(path string) ([]byte, time.Time, error) { // the write. The archive holds private keys: the temp file is created 0600 by // CreateTemp and chmodded to be certain. It is fsynced before the rename -- // this is a disaster-recovery artifact, "written" has to mean "on disk". -func writeCertArchive(outputPath string, files []archiveFile) error { +// It returns the warnings the staged-archive verification produced. +func writeCertArchive(outputPath string, files []archiveFile) ([]string, error) { file, err := os.CreateTemp(filepath.Dir(outputPath), filepath.Base(outputPath)+".*.tmp") if err != nil { - return fmt.Errorf("failed to create the archive: %w", err) + return nil, fmt.Errorf("failed to create the archive: %w", err) } tmpPath := file.Name() @@ -373,26 +420,27 @@ func writeCertArchive(outputPath string, files []archiveFile) error { if err != nil { file.Close() os.Remove(tmpPath) - return fmt.Errorf("failed to write the archive: %w", err) + return nil, fmt.Errorf("failed to write the archive: %w", err) } if err := file.Close(); err != nil { os.Remove(tmpPath) - return fmt.Errorf("failed to write the archive: %w", err) + return nil, fmt.Errorf("failed to write the archive: %w", err) } // Read the staged archive back through the same strict reader verify and // restore use, so a published export is restorable by construction -- any // disagreement between what was collected and what the reader accepts // fails the backup here, not in a disaster. - if _, err := readCertStoreArchive(tmpPath); err != nil { + staged, err := readCertStoreArchive(tmpPath) + if err != nil { os.Remove(tmpPath) - return fmt.Errorf("the staged archive failed verification: %w", err) + return nil, fmt.Errorf("the staged archive failed verification: %w", err) } if err := os.Rename(tmpPath, outputPath); err != nil { os.Remove(tmpPath) - return fmt.Errorf("failed to finalize the archive: %w", err) + return nil, fmt.Errorf("failed to finalize the archive: %w", err) } // Sync the directory so the rename itself survives power loss. Only a @@ -401,12 +449,12 @@ func writeCertArchive(outputPath string, files []archiveFile) error { // which a disaster-recovery artifact cannot shrug off. dir, err := os.Open(filepath.Dir(outputPath)) if err != nil { - return fmt.Errorf("failed to sync the archive's directory: %w", err) + return nil, fmt.Errorf("failed to sync the archive's directory: %w", err) } defer dir.Close() if err := dir.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) && !errors.Is(err, syscall.EINVAL) { - return fmt.Errorf("failed to sync the archive's directory: %w", err) + return nil, fmt.Errorf("failed to sync the archive's directory: %w", err) } - return nil + return staged.warnings, nil } diff --git a/internal/server/cert_store_export_test.go b/internal/server/cert_store_export_test.go index 58a0cb3..6931d60 100644 --- a/internal/server/cert_store_export_test.go +++ b/internal/server/cert_store_export_test.go @@ -409,3 +409,24 @@ func TestExportCertificateStore_RejectsSymlinkedOutputIntoTheStore(t *testing.T) require.Error(t, err) assert.Contains(t, err.Error(), "refusing") } + +func TestExportCertificateStore_SurfacesUnrestorableAccountKeyWarning(t *testing.T) { + // Valid JSON passes the collection pass, but the staged-archive + // verification knows the reader would refuse to restore it -- the export + // summary must say so, or the operator learns about the lost ACME + // identity during a disaster instead of when the backup was taken. + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + require.NoError(t, os.WriteFile(filepath.Join(paths.CertsPath, "acme_user.json"), + []byte(`{"email":"ops@example.com"}`), 0600)) + + summary, err := ExportCertificateStore(paths, filepath.Join(t.TempDir(), "backup.tar.gz")) + require.NoError(t, err) + + require.NotEmpty(t, summary.Warnings) + joined := "" + for _, warning := range summary.Warnings { + joined += warning + "\n" + } + assert.Contains(t, joined, "account key") +} diff --git a/internal/server/cert_store_restore.go b/internal/server/cert_store_restore.go index 960e398..f083998 100644 --- a/internal/server/cert_store_restore.go +++ b/internal/server/cert_store_restore.go @@ -188,15 +188,43 @@ func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { return nil } -// writeFileStaged writes a file through a same-directory temp file and a -// rename, so an interrupted restore never leaves the target truncated. +// writeFileStaged writes a file through a uniquely named same-directory temp +// file and a rename, so an interrupted restore never leaves the target +// truncated, a pre-planted path cannot redirect the write, and a pre-existing +// temp file cannot lend the private key its old permissions. func writeFileStaged(path string, data []byte) error { - tmpPath := path + ".tmp" - if err := os.WriteFile(tmpPath, data, 0600); err != nil { + file, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*.tmp") + if err != nil { + return err + } + tmpPath := file.Name() + + err = func() error { + if err := file.Chmod(0600); err != nil { + return err + } + if _, err := file.Write(data); err != nil { + return err + } + return file.Sync() + }() + if err != nil { + file.Close() + os.Remove(tmpPath) return err } - return os.Rename(tmpPath, path) + if err := file.Close(); err != nil { + os.Remove(tmpPath) + return err + } + + if err := os.Rename(tmpPath, path); err != nil { + os.Remove(tmpPath) + return err + } + + return nil } // certStoreOccupant names the first thing found occupying the target store, or diff --git a/internal/server/cert_store_restore_test.go b/internal/server/cert_store_restore_test.go index a189715..0e0b09c 100644 --- a/internal/server/cert_store_restore_test.go +++ b/internal/server/cert_store_restore_test.go @@ -430,7 +430,7 @@ func TestVerifyCertificateArchive_DropsInvalidAccountKey(t *testing.T) { name string key []byte }{ - {name: "not JSON at all is rejected at export time, valid JSON without key material is not", key: []byte(`{"email":"ops@example.com"}`)}, + {name: "valid JSON without key material", key: []byte(`{"email":"ops@example.com"}`)}, {name: "garbage key material", key: []byte(`{"email":"ops@example.com","key_pem":"bm90IGEga2V5"}`)}, {name: "non-ECDSA key, which loadOrCreateUser would silently discard", key: testRSAAccountKeyJSON(t)}, } From 8e2d367cc179ababa14bfebe7d1d7ac4f24b2bfb Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Sun, 9 Aug 2026 23:13:47 +0200 Subject: [PATCH 5/8] =?UTF-8?q?fix(cert-store):=20fourth=20review=20round?= =?UTF-8?q?=20=E2=80=94=20pinned=20write=20directory,=20full-stream=20vali?= =?UTF-8?q?dation,=20durable=20restores?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic's round-4 findings on PR #95: - the archive writer pins the output directory as an os.Root handle for the whole create-verify-rename-sync sequence, re-validated by filesystem identity after pinning, so a parent component swapped between the path check and the write cannot redirect the archive into the store; the staged archive is also verified through the pinned handle rather than a re-resolved path - the reader drains the capped gzip stream after tar EOF, so the gzip trailer (checksum included) must parse and fit the byte cap — a corrupt or oversized backup can no longer verify successfully; regression test flips the trailer - restore durability: writeFileStaged syncs the containing directory after its rename, and stale-directory removals are committed with a directory sync, so a restore that reported success survives power loss - reader warnings are structured (kind + text) so the export's missing-certificate suppression keys on the class, not a substring - staging temp files use short fixed patterns (.kamal-proxy-cert-export-*, .kamal-proxy-restore-*), so a near-limit destination basename cannot push the temp name past the filesystem's component length (test with a 240-char basename) - the Initialize regression test's directory stub reports encode errors with t.Error instead of require from the server goroutine ## Verification - [x] gofmt clean; go vet; make lint (0 issues) - [x] make test green (1942 tests); go test -race on the cert-store tests --- internal/server/cert_store_archive.go | 77 +++++++++-- internal/server/cert_store_export.go | 152 +++++++++++++++++---- internal/server/cert_store_export_test.go | 20 ++- internal/server/cert_store_restore.go | 40 +++++- internal/server/cert_store_restore_test.go | 15 ++ internal/server/san_cert_manager_test.go | 8 +- 6 files changed, 263 insertions(+), 49 deletions(-) diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go index 1b3b3ca..4c7c169 100644 --- a/internal/server/cert_store_archive.go +++ b/internal/server/cert_store_archive.go @@ -69,6 +69,23 @@ type archiveCertPair struct { leaf *x509.Certificate } +// certArchiveWarningKind classifies a reader warning, so callers can act on a +// class of warning without being coupled to its human-readable text. +type certArchiveWarningKind int + +const ( + // warnMissingCertificate: the state references a certificate whose files + // the archive does not hold; its domains re-order after a restore. + warnMissingCertificate certArchiveWarningKind = iota + // warnAccountKey: the account key entry is unusable and will not restore. + warnAccountKey +) + +type certArchiveWarning struct { + kind certArchiveWarningKind + text string +} + // certStoreArchive is a fully read and validated certificate store archive. // Reading never touches the store: verification and restore share this. type certStoreArchive struct { @@ -82,7 +99,20 @@ type certStoreArchive struct { // certificate identifier). certs map[string]archiveCertPair - warnings []string + warnings []certArchiveWarning +} + +// warningTexts flattens the warnings for reporting. +func (a *certStoreArchive) warningTexts() []string { + if len(a.warnings) == 0 { + return nil + } + + texts := make([]string, 0, len(a.warnings)) + for _, warning := range a.warnings { + texts = append(texts, warning.text) + } + return texts } // readCertStoreArchive reads and validates an exported certificate store @@ -92,15 +122,23 @@ type certStoreArchive struct { // certificate is not an error; a faithful backup of an expired certificate is // still a backup. func readCertStoreArchive(archivePath string) (certStoreArchive, error) { - archive := certStoreArchive{certs: map[string]archiveCertPair{}} - file, err := os.Open(archivePath) if err != nil { - return archive, fmt.Errorf("failed to open the archive: %w", err) + return certStoreArchive{}, fmt.Errorf("failed to open the archive: %w", err) } defer file.Close() - gz, err := gzip.NewReader(file) + return readCertStoreArchiveFrom(file, archivePath) +} + +// readCertStoreArchiveFrom is readCertStoreArchive over an already-open +// source; archivePath only labels error messages. The exporter uses it to +// verify its staged archive through the file handle it wrote, rather than +// re-opening a path. +func readCertStoreArchiveFrom(source io.Reader, archivePath string) (certStoreArchive, error) { + archive := certStoreArchive{certs: map[string]archiveCertPair{}} + + gz, err := gzip.NewReader(source) if err != nil { return archive, fmt.Errorf("failed to read the archive %s: %w", archivePath, err) } @@ -162,6 +200,17 @@ func readCertStoreArchive(archivePath string) (certStoreArchive, error) { } } + // Drain the rest of the stream: the tar reader stops at its end-of-archive + // marker, but the gzip trailer -- its checksum included -- still has to + // parse and fit the cap, otherwise a corrupt or oversized backup could + // verify successfully. + if _, err := io.Copy(io.Discard, capped); err != nil { + if errors.Is(err, errCertArchiveTooLarge) { + return archive, fmt.Errorf("refusing the archive %s: it decompresses beyond %d bytes", archivePath, int64(maxCertArchiveBytes)) + } + return archive, fmt.Errorf("failed to read the archive %s: %w", archivePath, err) + } + // Directory headers alone do not make an archive: emptiness is decided by // regular files, while the entry cap above counts every header. if fileCount == 0 { @@ -272,8 +321,10 @@ func (a *certStoreArchive) validate() error { // A state-referenced certificate missing from the archive restores // to the same place loadState puts a missing file: the domain // re-orders. Warn, don't fail -- the export warned identically. - a.warnings = append(a.warnings, - fmt.Sprintf("certificate %s is referenced by the state file but missing from the archive; its domains will re-order after a restore", id)) + a.warnings = append(a.warnings, certArchiveWarning{ + kind: warnMissingCertificate, + text: fmt.Sprintf("certificate %s is referenced by the state file but missing from the archive; its domains will re-order after a restore", id), + }) continue } @@ -308,8 +359,10 @@ func (a *certStoreArchive) checkAccountKey() { var user acmeUser if err := json.Unmarshal(a.accountKey, &user); err != nil { - a.warnings = append(a.warnings, - fmt.Sprintf("the archived ACME account key does not parse and will not be restored; the next boot will register a fresh account: %v", err)) + a.warnings = append(a.warnings, certArchiveWarning{ + kind: warnAccountKey, + text: fmt.Sprintf("the archived ACME account key does not parse and will not be restored; the next boot will register a fresh account: %v", err), + }) a.accountKey = nil return } @@ -325,8 +378,10 @@ func (a *certStoreArchive) checkAccountKey() { err = errors.New("the key is not an ECDSA key") } - a.warnings = append(a.warnings, - fmt.Sprintf("the archived ACME account key holds no usable private key and will not be restored; the next boot will register a fresh account: %v", err)) + a.warnings = append(a.warnings, certArchiveWarning{ + kind: warnAccountKey, + text: fmt.Sprintf("the archived ACME account key holds no usable private key and will not be restored; the next boot will register a fresh account: %v", err), + }) a.accountKey = nil } diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go index bb7b3a8..b79bca9 100644 --- a/internal/server/cert_store_export.go +++ b/internal/server/cert_store_export.go @@ -3,10 +3,13 @@ package server import ( "archive/tar" "compress/gzip" + "crypto/rand" "crypto/tls" + "encoding/hex" "encoding/json" "errors" "fmt" + "io/fs" "maps" "os" "path/filepath" @@ -125,7 +128,7 @@ func ExportCertificateStore(paths CertStorePaths, outputPath string) (CertsExpor return strings.Compare(a.name, b.name) }) - readerWarnings, err := writeCertArchive(outputPath, files) + readerWarnings, err := writeCertArchive(outputPath, paths, files) if err != nil { return summary, err } @@ -135,8 +138,8 @@ func ExportCertificateStore(paths CertStorePaths, outputPath string) (CertsExpor // missing-certificate warnings are skipped: each of those was already // reported above from the disk side. for _, warning := range readerWarnings { - if !strings.Contains(warning, "missing from the archive") { - summary.Warnings = append(summary.Warnings, warning) + if warning.kind != warnMissingCertificate { + summary.Warnings = append(summary.Warnings, warning.text) } } @@ -372,19 +375,34 @@ func readFileWithModTime(path string) ([]byte, time.Time, error) { return data, info.ModTime(), nil } -// writeCertArchive writes the staged files as a gzipped tarball, staged as a -// uniquely named same-directory temp file and renamed into place so a partial -// write never looks like a valid backup and a pre-planted path cannot redirect -// the write. The archive holds private keys: the temp file is created 0600 by -// CreateTemp and chmodded to be certain. It is fsynced before the rename -- -// this is a disaster-recovery artifact, "written" has to mean "on disk". -// It returns the warnings the staged-archive verification produced. -func writeCertArchive(outputPath string, files []archiveFile) ([]string, error) { - file, err := os.CreateTemp(filepath.Dir(outputPath), filepath.Base(outputPath)+".*.tmp") +// writeCertArchive writes the staged files as a gzipped tarball into the +// output path's directory, which is pinned as an os.Root handle for the whole +// create-verify-rename-sync sequence -- re-validated by identity after +// pinning, so a parent component swapped between the path check and the write +// cannot redirect the archive into the store. The temp file has a short fixed +// name pattern (a long destination basename must not push the temp name past +// the filesystem's component limit), is created 0600, and is fsynced before +// the rename -- this is a disaster-recovery artifact, "written" has to mean +// "on disk". It returns the warnings the staged-archive verification +// produced. +func writeCertArchive(outputPath string, paths CertStorePaths, files []archiveFile) ([]certArchiveWarning, error) { + root, err := os.OpenRoot(filepath.Dir(outputPath)) + if err != nil { + return nil, fmt.Errorf("failed to open the output directory: %w", err) + } + defer root.Close() + + base := filepath.Base(outputPath) + if err := rejectPinnedRootInsideStore(root, base, paths); err != nil { + return nil, err + } + + const tmpPattern = ".kamal-proxy-cert-export-*.tmp" + file, err := createTempInRoot(root, tmpPattern) if err != nil { return nil, fmt.Errorf("failed to create the archive: %w", err) } - tmpPath := file.Name() + tmpName := filepath.Base(file.Name()) err = func() error { if err := file.Chmod(0600); err != nil { @@ -419,42 +437,118 @@ func writeCertArchive(outputPath string, files []archiveFile) ([]string, error) }() if err != nil { file.Close() - os.Remove(tmpPath) + root.Remove(tmpName) return nil, fmt.Errorf("failed to write the archive: %w", err) } if err := file.Close(); err != nil { - os.Remove(tmpPath) + root.Remove(tmpName) return nil, fmt.Errorf("failed to write the archive: %w", err) } // Read the staged archive back through the same strict reader verify and - // restore use, so a published export is restorable by construction -- any - // disagreement between what was collected and what the reader accepts - // fails the backup here, not in a disaster. - staged, err := readCertStoreArchive(tmpPath) + // restore use -- via the pinned root, not a re-resolved path -- so a + // published export is restorable by construction. + staged, err := verifyStagedArchive(root, tmpName) if err != nil { - os.Remove(tmpPath) + root.Remove(tmpName) return nil, fmt.Errorf("the staged archive failed verification: %w", err) } - if err := os.Rename(tmpPath, outputPath); err != nil { - os.Remove(tmpPath) + if err := root.Rename(tmpName, base); err != nil { + root.Remove(tmpName) return nil, fmt.Errorf("failed to finalize the archive: %w", err) } - // Sync the directory so the rename itself survives power loss. Only a - // filesystem that genuinely does not support syncing a directory is + // Sync the pinned directory so the rename itself survives power loss. Only + // a filesystem that genuinely does not support syncing a directory is // excused; a real failure means the backup's existence is not durable, // which a disaster-recovery artifact cannot shrug off. - dir, err := os.Open(filepath.Dir(outputPath)) - if err != nil { + if err := syncRootDir(root); err != nil { return nil, fmt.Errorf("failed to sync the archive's directory: %w", err) } + + return staged.warnings, nil +} + +// rejectPinnedRootInsideStore re-validates the already-opened output directory +// by filesystem identity: the handle, not a pathname, is what the writes go +// through, so this check cannot be raced by swapping path components. +func rejectPinnedRootInsideStore(root *os.Root, base string, paths CertStorePaths) error { + dir, err := root.Open(".") + if err != nil { + return fmt.Errorf("failed to inspect the output directory: %w", err) + } defer dir.Close() - if err := dir.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) && !errors.Is(err, syscall.EINVAL) { - return nil, fmt.Errorf("failed to sync the archive's directory: %w", err) + + rootInfo, err := dir.Stat() + if err != nil { + return fmt.Errorf("failed to inspect the output directory: %w", err) } - return staged.warnings, nil + if certsInfo, err := os.Stat(paths.CertsPath); err == nil && os.SameFile(certsInfo, rootInfo) { + return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + } + + if targetInfo, err := root.Stat(base); err == nil { + for _, statePath := range []string{paths.ACMEStatePath, paths.DynamicDomainsStatePath} { + if stateInfo, err := os.Stat(statePath); err == nil && os.SameFile(stateInfo, targetInfo) { + return fmt.Errorf("refusing to write the archive over the store's own %s", filepath.Base(statePath)) + } + } + } + + return nil +} + +// createTempInRoot is os.CreateTemp confined to an os.Root: a uniquely named +// file created with O_EXCL and mode 0600 inside the pinned directory. +func createTempInRoot(root *os.Root, pattern string) (*os.File, error) { + prefix, suffix, _ := strings.Cut(pattern, "*") + + for range 10 { + random := make([]byte, 8) + if _, err := rand.Read(random); err != nil { + return nil, err + } + + name := prefix + hex.EncodeToString(random) + suffix + file, err := root.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0600) + if errors.Is(err, fs.ErrExist) { + continue + } + if err != nil { + return nil, err + } + return file, nil + } + + return nil, errors.New("could not create a unique temporary file") +} + +// verifyStagedArchive runs the strict archive reader over the staged file, +// opened through the pinned root. +func verifyStagedArchive(root *os.Root, tmpName string) (certStoreArchive, error) { + staged, err := root.Open(tmpName) + if err != nil { + return certStoreArchive{}, err + } + defer staged.Close() + + return readCertStoreArchiveFrom(staged, "staged archive") +} + +// syncRootDir fsyncs the pinned directory; only a filesystem that cannot sync +// a directory is excused. +func syncRootDir(root *os.Root) error { + dir, err := root.Open(".") + if err != nil { + return err + } + defer dir.Close() + + if err := dir.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) && !errors.Is(err, syscall.EINVAL) { + return err + } + return nil } diff --git a/internal/server/cert_store_export_test.go b/internal/server/cert_store_export_test.go index 6931d60..d988875 100644 --- a/internal/server/cert_store_export_test.go +++ b/internal/server/cert_store_export_test.go @@ -10,6 +10,7 @@ import ( "io" "os" "path/filepath" + "strings" "testing" "time" @@ -141,8 +142,9 @@ func TestExportCertificateStore_ArchivesTheWholeEstate(t *testing.T) { assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) // The staged write must not leave its temp file behind. - _, err = os.Stat(outputPath + ".tmp") - assert.True(t, os.IsNotExist(err)) + leftovers, err := filepath.Glob(filepath.Join(filepath.Dir(outputPath), ".kamal-proxy-cert-export-*")) + require.NoError(t, err) + assert.Empty(t, leftovers) } func TestExportCertificateStore_EmptyStoreIsAnError(t *testing.T) { @@ -430,3 +432,17 @@ func TestExportCertificateStore_SurfacesUnrestorableAccountKeyWarning(t *testing } assert.Contains(t, joined, "account key") } + +func TestExportCertificateStore_LongOutputBasename(t *testing.T) { + // A destination near the filesystem's 255-byte component limit must not + // fail because the staging file appends to its name. + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + + longName := strings.Repeat("b", 240) + ".tar.gz" + outputPath := filepath.Join(t.TempDir(), longName) + + _, err := ExportCertificateStore(paths, outputPath) + require.NoError(t, err) + assert.FileExists(t, outputPath) +} diff --git a/internal/server/cert_store_restore.go b/internal/server/cert_store_restore.go index f083998..b645241 100644 --- a/internal/server/cert_store_restore.go +++ b/internal/server/cert_store_restore.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "slices" + "syscall" "time" ) @@ -68,7 +69,7 @@ func RestoreCertificateStore(opts CertStoreRestoreOptions) (CertsRestoreSummary, if err != nil { return summary, err } - summary.Warnings = archive.warnings + summary.Warnings = archive.warningTexts() if !opts.Force { if occupant := certStoreOccupant(opts.Paths); occupant != "" { @@ -158,7 +159,7 @@ func VerifyCertificateArchive(archivePath string) (CertArchiveReport, error) { report.DomainMappings = len(archive.state.DomainMap) report.HasAccountKey = archive.accountKey != nil report.HasDynamicDomains = archive.dynamicDomains != nil - report.Warnings = archive.warnings + report.Warnings = archive.warningTexts() return report, nil } @@ -167,6 +168,7 @@ func VerifyCertificateArchive(archivePath string) (CertArchiveReport, error) { // restored state references but the archive does not hold, so those domains // actually re-order instead of serving whatever the old store left behind. func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { + removed := 0 for _, id := range slices.Sorted(maps.Keys(archive.state.Certificates)) { dir := sanitizeFilename(id) if _, ok := archive.certs[dir]; ok { @@ -183,17 +185,45 @@ func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { if err := os.RemoveAll(filepath.Join(certsPath, dir)); err != nil { return fmt.Errorf("failed to remove the stale certificate directory for %s: %w", id, err) } + removed++ + } + + // Commit the unlinks: without a directory sync, a crash after the restore + // reported success could resurrect a stale directory and undo the + // "missing certificate re-orders" behavior the warnings promised. + if removed > 0 { + if err := syncDir(certsPath); err != nil { + return fmt.Errorf("failed to sync the certificate directory: %w", err) + } } return nil } +// syncDir fsyncs a directory so renames and unlinks inside it survive power +// loss; only a filesystem that cannot sync a directory is excused. +func syncDir(path string) error { + dir, err := os.Open(path) + if err != nil { + return err + } + defer dir.Close() + + if err := dir.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) && !errors.Is(err, syscall.EINVAL) { + return err + } + return nil +} + // writeFileStaged writes a file through a uniquely named same-directory temp // file and a rename, so an interrupted restore never leaves the target // truncated, a pre-planted path cannot redirect the write, and a pre-existing -// temp file cannot lend the private key its old permissions. +// temp file cannot lend the private key its old permissions. The temp pattern +// is short and fixed so a near-limit destination basename cannot push it past +// the filesystem's component length. The directory is synced after the +// rename, so a restore that reported success survives power loss. func writeFileStaged(path string, data []byte) error { - file, err := os.CreateTemp(filepath.Dir(path), filepath.Base(path)+".*.tmp") + file, err := os.CreateTemp(filepath.Dir(path), ".kamal-proxy-restore-*.tmp") if err != nil { return err } @@ -224,7 +254,7 @@ func writeFileStaged(path string, data []byte) error { return err } - return nil + return syncDir(filepath.Dir(path)) } // certStoreOccupant names the first thing found occupying the target store, or diff --git a/internal/server/cert_store_restore_test.go b/internal/server/cert_store_restore_test.go index 0e0b09c..db9a835 100644 --- a/internal/server/cert_store_restore_test.go +++ b/internal/server/cert_store_restore_test.go @@ -610,3 +610,18 @@ func testRSAAccountKeyJSON(t testing.TB) []byte { require.NoError(t, err) return data } + +func TestVerifyCertificateArchive_RejectsCorruptGzipTrailer(t *testing.T) { + // The tar reader stops at the end-of-archive marker, before the gzip + // trailer; a verifier that never drains the stream would bless a backup + // whose checksum no longer matches its contents. + archivePath, _ := exportedTestArchive(t, []string{"example.com"}) + + data, err := os.ReadFile(archivePath) + require.NoError(t, err) + data[len(data)-1] ^= 0xff // corrupt the gzip trailer (ISIZE) + require.NoError(t, os.WriteFile(archivePath, data, 0600)) + + _, err = VerifyCertificateArchive(archivePath) + require.Error(t, err) +} diff --git a/internal/server/san_cert_manager_test.go b/internal/server/san_cert_manager_test.go index f86bb68..8eb3cb4 100644 --- a/internal/server/san_cert_manager_test.go +++ b/internal/server/san_cert_manager_test.go @@ -399,13 +399,17 @@ func TestSANCertManager_InitializeAdoptsLegacyCacheWithoutDeadlock(t *testing.T) var directory *httptest.Server directory = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - require.NoError(t, json.NewEncoder(w).Encode(map[string]any{ + // t.Error, not require: this runs on the server's goroutine, where + // FailNow would kill the handler instead of failing the test. + if err := json.NewEncoder(w).Encode(map[string]any{ "newNonce": directory.URL + "/nonce", "newAccount": directory.URL + "/account", "newOrder": directory.URL + "/order", "revokeCert": directory.URL + "/revoke", "keyChange": directory.URL + "/keychange", - })) + }); err != nil { + t.Error(err) + } })) defer directory.Close() From fad9ce77f99ad41bf63cfb902600b088e210c645 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 07:15:49 +0200 Subject: [PATCH 6/8] =?UTF-8?q?fix(cert-store):=20fifth=20review=20round?= =?UTF-8?q?=20=E2=80=94=20subdirectory=20containment,=20boundary=20and=20d?= =?UTF-8?q?egenerate-store=20edges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic's round-5 findings on PR #95: - the pinned-root identity check also walks the output directory's existing ancestors against the certificate directory, so a swap that lands the root on a subdirectory inside the store is caught, not just the store directory itself - removeStaleCertDirs only counts directories that actually existed (RemoveAll succeeds silently on missing paths), so restoring a degenerate archive into a store with no certificate directory no longer fails post-commit on syncing a directory that was never there (regression test added) - cappedReader probes the underlying reader at the boundary, so an archive decompressing to exactly the limit is accepted while one more byte still trips the cap (test added) - directory-sync errno policy lives in one syncOpenDir helper shared by the pathname-opening restore paths and the pinned-root export path ## Verification - [x] gofmt clean; go vet; make lint (0 issues) - [x] make test green (1944 tests); go test -race on the cert-store tests --- internal/server/cert_store_archive.go | 17 +++++++++- internal/server/cert_store_export.go | 38 ++++++++++++++-------- internal/server/cert_store_restore.go | 20 ++++++++++-- internal/server/cert_store_restore_test.go | 31 ++++++++++++++++++ 4 files changed, 90 insertions(+), 16 deletions(-) diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go index 4c7c169..f3ca634 100644 --- a/internal/server/cert_store_archive.go +++ b/internal/server/cert_store_archive.go @@ -42,7 +42,9 @@ var errCertArchiveTooLarge = errors.New("certificate archive decompresses beyond // cappedReader bounds how many bytes may be read through it, failing with // errCertArchiveTooLarge instead of a bare EOF so the caller can tell a -// too-large archive from a truncated one. +// too-large archive from a truncated one. A stream that ends exactly at the +// limit is not over it: at the boundary the underlying reader is probed, and +// only actual further data trips the cap. type cappedReader struct { reader io.Reader remaining int64 @@ -50,6 +52,19 @@ type cappedReader struct { func (c *cappedReader) Read(p []byte) (int, error) { if c.remaining <= 0 { + var probe [1]byte + for range 3 { + n, err := c.reader.Read(probe[:]) + if n > 0 { + return 0, errCertArchiveTooLarge + } + if err == io.EOF { + return 0, io.EOF + } + if err != nil { + return 0, err + } + } return 0, errCertArchiveTooLarge } if int64(len(p)) > c.remaining { diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go index b79bca9..dcc4ab0 100644 --- a/internal/server/cert_store_export.go +++ b/internal/server/cert_store_export.go @@ -15,7 +15,6 @@ import ( "path/filepath" "slices" "strings" - "syscall" "time" ) @@ -393,7 +392,7 @@ func writeCertArchive(outputPath string, paths CertStorePaths, files []archiveFi defer root.Close() base := filepath.Base(outputPath) - if err := rejectPinnedRootInsideStore(root, base, paths); err != nil { + if err := rejectPinnedRootInsideStore(root, filepath.Dir(outputPath), base, paths); err != nil { return nil, err } @@ -472,9 +471,12 @@ func writeCertArchive(outputPath string, paths CertStorePaths, files []archiveFi } // rejectPinnedRootInsideStore re-validates the already-opened output directory -// by filesystem identity: the handle, not a pathname, is what the writes go -// through, so this check cannot be raced by swapping path components. -func rejectPinnedRootInsideStore(root *os.Root, base string, paths CertStorePaths) error { +// by filesystem identity -- the handle, not a pathname, is what the writes go +// through. The pinned directory itself is compared against the certificate +// directory, and so is every existing ancestor of its path, so a swap that +// lands the root on a subdirectory inside the store is caught too, not just +// the store directory itself. +func rejectPinnedRootInsideStore(root *os.Root, rootDir, base string, paths CertStorePaths) error { dir, err := root.Open(".") if err != nil { return fmt.Errorf("failed to inspect the output directory: %w", err) @@ -486,8 +488,21 @@ func rejectPinnedRootInsideStore(root *os.Root, base string, paths CertStorePath return fmt.Errorf("failed to inspect the output directory: %w", err) } - if certsInfo, err := os.Stat(paths.CertsPath); err == nil && os.SameFile(certsInfo, rootInfo) { - return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + if certsInfo, err := os.Stat(paths.CertsPath); err == nil { + if os.SameFile(certsInfo, rootInfo) { + return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + } + + for current := rootDir; ; { + if info, err := os.Stat(current); err == nil && os.SameFile(certsInfo, info) { + return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) + } + parent := filepath.Dir(current) + if parent == current { + break + } + current = parent + } } if targetInfo, err := root.Stat(base); err == nil { @@ -538,8 +553,8 @@ func verifyStagedArchive(root *os.Root, tmpName string) (certStoreArchive, error return readCertStoreArchiveFrom(staged, "staged archive") } -// syncRootDir fsyncs the pinned directory; only a filesystem that cannot sync -// a directory is excused. +// syncRootDir fsyncs the pinned directory, with syncOpenDir deciding which +// failures are excusable. func syncRootDir(root *os.Root) error { dir, err := root.Open(".") if err != nil { @@ -547,8 +562,5 @@ func syncRootDir(root *os.Root) error { } defer dir.Close() - if err := dir.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) && !errors.Is(err, syscall.EINVAL) { - return err - } - return nil + return syncOpenDir(dir) } diff --git a/internal/server/cert_store_restore.go b/internal/server/cert_store_restore.go index b645241..b52ed79 100644 --- a/internal/server/cert_store_restore.go +++ b/internal/server/cert_store_restore.go @@ -182,7 +182,15 @@ func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { return fmt.Errorf("refusing to remove the unsafe certificate directory for %q", id) } - if err := os.RemoveAll(filepath.Join(certsPath, dir)); err != nil { + // Only a directory that actually existed counts as a removal: RemoveAll + // succeeds silently on a missing path, and a restore into a store with + // no certificate directory at all must not then try to sync it. + stalePath := filepath.Join(certsPath, dir) + if _, err := os.Lstat(stalePath); err != nil { + continue + } + + if err := os.RemoveAll(stalePath); err != nil { return fmt.Errorf("failed to remove the stale certificate directory for %s: %w", id, err) } removed++ @@ -201,7 +209,7 @@ func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { } // syncDir fsyncs a directory so renames and unlinks inside it survive power -// loss; only a filesystem that cannot sync a directory is excused. +// loss. func syncDir(path string) error { dir, err := os.Open(path) if err != nil { @@ -209,6 +217,14 @@ func syncDir(path string) error { } defer dir.Close() + return syncOpenDir(dir) +} + +// syncOpenDir fsyncs an already-opened directory; only a filesystem that +// cannot sync a directory is excused. The one place the excusable errnos are +// decided, shared by the pathname-opening restore paths and the pinned-root +// export path. +func syncOpenDir(dir *os.File) error { if err := dir.Sync(); err != nil && !errors.Is(err, syscall.ENOTSUP) && !errors.Is(err, syscall.EINVAL) { return err } diff --git a/internal/server/cert_store_restore_test.go b/internal/server/cert_store_restore_test.go index db9a835..949956a 100644 --- a/internal/server/cert_store_restore_test.go +++ b/internal/server/cert_store_restore_test.go @@ -625,3 +625,34 @@ func TestVerifyCertificateArchive_RejectsCorruptGzipTrailer(t *testing.T) { _, err = VerifyCertificateArchive(archivePath) require.Error(t, err) } + +func TestCappedReader_ExactlyAtTheLimitIsNotOversized(t *testing.T) { + capped := &cappedReader{reader: bytes.NewReader(make([]byte, 10)), remaining: 10} + + data, err := io.ReadAll(capped) + require.NoError(t, err, "a stream ending exactly at the limit is within it") + assert.Len(t, data, 10) +} + +func TestRestoreCertificateStore_DegenerateArchiveIntoFreshStore(t *testing.T) { + // A state referencing certificates the archive does not hold (tolerated + // with a re-order warning) restored into a store with no certificate + // directory at all: nothing to remove, nothing to sync, no error. + archivePath := filepath.Join(t.TempDir(), "backup.tar.gz") + writeTestArchive(t, archivePath, map[string][]byte{ + "acme.state": []byte(`{ + "certificates":{"san:gone":{"identifier":"san:gone","domains":["gone.test"],"not_after":"2027-01-01T00:00:00Z"}}, + "domain_map":{"gone.test":"san:gone"}, + "saved_at":"2026-08-09T00:00:00Z"}`), + }) + + paths := testCertStorePaths(t) + summary, err := RestoreCertificateStore(CertStoreRestoreOptions{ArchivePath: archivePath, Paths: paths}) + require.NoError(t, err) + require.NotEmpty(t, summary.Warnings) + assert.Equal(t, 1, summary.Certificates) + + // The state landed even though the certificate directory never existed. + state := readImportedState(t, paths.ACMEStatePath) + assert.Contains(t, state.Certificates, "san:gone") +} From e22de8c9d3d0c8810c04959c89cf58002178ab88 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 07:30:25 +0200 Subject: [PATCH 7/8] =?UTF-8?q?fix(cert-store):=20sixth=20review=20round?= =?UTF-8?q?=20=E2=80=94=20pinned-tree=20containment,=20strict=20inspection?= =?UTF-8?q?=20errors,=20reader-contract=20edge?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic's round-6 findings on PR #95: - containment for the pinned output directory is decided by walking the certificate tree through its OWN pinned root and comparing directory identities against the output handle -- neither side of the comparison is a re-resolvable pathname anymore, so restoring the swapped pathname after pinning no longer defeats the check - removeStaleCertDirs treats only os.ErrNotExist as skippable; any other Lstat failure (EACCES, EIO, ENOTDIR) fails the restore instead of silently retaining a stale pair - cappedReader at the boundary preserves the io.Reader contract: zero-length reads return (0, nil), a legal (0, nil) from the wrapped reader passes through for the caller to retry, and the cap error fires only on actual excess bytes ## Verification - [x] gofmt clean; go vet; make lint (0 issues) - [x] make test green (1944+ tests); go test -race on the cert-store tests --- internal/server/cert_store_archive.go | 23 ++++---- internal/server/cert_store_export.go | 68 ++++++++++++++++------- internal/server/cert_store_export_test.go | 26 +++++++++ internal/server/cert_store_restore.go | 9 ++- 4 files changed, 91 insertions(+), 35 deletions(-) diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go index f3ca634..4e14267 100644 --- a/internal/server/cert_store_archive.go +++ b/internal/server/cert_store_archive.go @@ -52,20 +52,19 @@ type cappedReader struct { func (c *cappedReader) Read(p []byte) (int, error) { if c.remaining <= 0 { + // Preserve the io.Reader contract at the boundary: zero-length reads + // stay (0, nil), and the cap error is reserved for actual excess data + // -- a legal (0, nil) from the underlying reader passes through for + // the caller to retry. + if len(p) == 0 { + return 0, nil + } var probe [1]byte - for range 3 { - n, err := c.reader.Read(probe[:]) - if n > 0 { - return 0, errCertArchiveTooLarge - } - if err == io.EOF { - return 0, io.EOF - } - if err != nil { - return 0, err - } + n, err := c.reader.Read(probe[:]) + if n > 0 { + return 0, errCertArchiveTooLarge } - return 0, errCertArchiveTooLarge + return 0, err } if int64(len(p)) > c.remaining { p = p[:c.remaining] diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go index dcc4ab0..ece9577 100644 --- a/internal/server/cert_store_export.go +++ b/internal/server/cert_store_export.go @@ -392,7 +392,7 @@ func writeCertArchive(outputPath string, paths CertStorePaths, files []archiveFi defer root.Close() base := filepath.Base(outputPath) - if err := rejectPinnedRootInsideStore(root, filepath.Dir(outputPath), base, paths); err != nil { + if err := rejectPinnedRootInsideStore(root, base, paths); err != nil { return nil, err } @@ -472,11 +472,11 @@ func writeCertArchive(outputPath string, paths CertStorePaths, files []archiveFi // rejectPinnedRootInsideStore re-validates the already-opened output directory // by filesystem identity -- the handle, not a pathname, is what the writes go -// through. The pinned directory itself is compared against the certificate -// directory, and so is every existing ancestor of its path, so a swap that -// lands the root on a subdirectory inside the store is caught too, not just -// the store directory itself. -func rejectPinnedRootInsideStore(root *os.Root, rootDir, base string, paths CertStorePaths) error { +// through. Containment is decided by walking the certificate tree through its +// own pinned root and comparing every directory's identity against the output +// handle, so neither side of the comparison can be swapped out from under the +// check by re-resolving a pathname. +func rejectPinnedRootInsideStore(root *os.Root, base string, paths CertStorePaths) error { dir, err := root.Open(".") if err != nil { return fmt.Errorf("failed to inspect the output directory: %w", err) @@ -488,21 +488,10 @@ func rejectPinnedRootInsideStore(root *os.Root, rootDir, base string, paths Cert return fmt.Errorf("failed to inspect the output directory: %w", err) } - if certsInfo, err := os.Stat(paths.CertsPath); err == nil { - if os.SameFile(certsInfo, rootInfo) { - return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) - } - - for current := rootDir; ; { - if info, err := os.Stat(current); err == nil && os.SameFile(certsInfo, info) { - return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) - } - parent := filepath.Dir(current) - if parent == current { - break - } - current = parent - } + if inside, err := dirInsidePinnedTree(paths.CertsPath, rootInfo); err != nil { + return err + } else if inside { + return fmt.Errorf("refusing to write the archive inside the certificate directory %s", paths.CertsPath) } if targetInfo, err := root.Stat(base); err == nil { @@ -516,6 +505,43 @@ func rejectPinnedRootInsideStore(root *os.Root, rootDir, base string, paths Cert return nil } +// dirInsidePinnedTree reports whether target names the given tree's own +// directory or any directory inside it, comparing identities collected +// through the tree's pinned root. A tree that does not exist contains +// nothing. +func dirInsidePinnedTree(treePath string, target os.FileInfo) (bool, error) { + treeRoot, err := os.OpenRoot(treePath) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("failed to inspect the certificate directory: %w", err) + } + defer treeRoot.Close() + + inside := false + walkErr := fs.WalkDir(treeRoot.FS(), ".", func(name string, entry fs.DirEntry, err error) error { + if err != nil || !entry.IsDir() { + return nil + } + + info, err := entry.Info() + if err != nil { + return nil + } + if os.SameFile(info, target) { + inside = true + return fs.SkipAll + } + return nil + }) + if walkErr != nil { + return false, fmt.Errorf("failed to inspect the certificate directory: %w", walkErr) + } + + return inside, nil +} + // createTempInRoot is os.CreateTemp confined to an os.Root: a uniquely named // file created with O_EXCL and mode 0600 inside the pinned directory. func createTempInRoot(root *os.Root, pattern string) (*os.File, error) { diff --git a/internal/server/cert_store_export_test.go b/internal/server/cert_store_export_test.go index d988875..b728183 100644 --- a/internal/server/cert_store_export_test.go +++ b/internal/server/cert_store_export_test.go @@ -446,3 +446,29 @@ func TestExportCertificateStore_LongOutputBasename(t *testing.T) { require.NoError(t, err) assert.FileExists(t, outputPath) } + +func TestDirInsidePinnedTree(t *testing.T) { + paths := testCertStorePaths(t) + populateCertStore(t, paths, []string{"example.com"}) + subDir := filepath.Join(paths.CertsPath, sanitizeFilename(sanCertID([]string{"example.com"}))) + + for _, target := range []string{paths.CertsPath, subDir} { + info, err := os.Stat(target) + require.NoError(t, err) + + inside, err := dirInsidePinnedTree(paths.CertsPath, info) + require.NoError(t, err) + assert.True(t, inside, "%s is inside the certificate tree", target) + } + + outsideInfo, err := os.Stat(t.TempDir()) + require.NoError(t, err) + inside, err := dirInsidePinnedTree(paths.CertsPath, outsideInfo) + require.NoError(t, err) + assert.False(t, inside) + + // A tree that does not exist contains nothing. + inside, err = dirInsidePinnedTree(filepath.Join(t.TempDir(), "nope"), outsideInfo) + require.NoError(t, err) + assert.False(t, inside) +} diff --git a/internal/server/cert_store_restore.go b/internal/server/cert_store_restore.go index b52ed79..761eb87 100644 --- a/internal/server/cert_store_restore.go +++ b/internal/server/cert_store_restore.go @@ -184,10 +184,15 @@ func removeStaleCertDirs(certsPath string, archive certStoreArchive) error { // Only a directory that actually existed counts as a removal: RemoveAll // succeeds silently on a missing path, and a restore into a store with - // no certificate directory at all must not then try to sync it. + // no certificate directory at all must not then try to sync it. Only + // a genuinely missing path is skippable -- any other inspection + // failure could silently retain a stale pair. stalePath := filepath.Join(certsPath, dir) if _, err := os.Lstat(stalePath); err != nil { - continue + if errors.Is(err, os.ErrNotExist) { + continue + } + return fmt.Errorf("failed to inspect the stale certificate directory for %s: %w", id, err) } if err := os.RemoveAll(stalePath); err != nil { From bd05e74704fe61fb64c0160e15695a3cf67c8da5 Mon Sep 17 00:00:00 2001 From: mhenrixon Date: Mon, 10 Aug 2026 07:39:19 +0200 Subject: [PATCH 8/8] =?UTF-8?q?fix(cert-store):=20seventh=20review=20round?= =?UTF-8?q?=20=E2=80=94=20fail-closed=20containment=20walk,=20uniform=20ze?= =?UTF-8?q?ro-length=20reads?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses cubic's round-7 findings on PR #95: - dirInsidePinnedTree fails closed: a certificate subtree that cannot be read or statted aborts the export instead of being silently skipped by the containment comparison - cappedReader treats zero-length reads as no-ops on both sides of the byte budget, not just after it is exhausted ## Verification - [x] gofmt clean; go vet; make lint (0 issues) - [x] make test green --- internal/server/cert_store_archive.go | 16 +++++++++------- internal/server/cert_store_export.go | 10 ++++++++-- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/internal/server/cert_store_archive.go b/internal/server/cert_store_archive.go index 4e14267..125a2d3 100644 --- a/internal/server/cert_store_archive.go +++ b/internal/server/cert_store_archive.go @@ -51,14 +51,16 @@ type cappedReader struct { } func (c *cappedReader) Read(p []byte) (int, error) { + // A zero-length read is a no-op regardless of the budget, so its + // behavior cannot differ on either side of the boundary. + if len(p) == 0 { + return 0, nil + } + if c.remaining <= 0 { - // Preserve the io.Reader contract at the boundary: zero-length reads - // stay (0, nil), and the cap error is reserved for actual excess data - // -- a legal (0, nil) from the underlying reader passes through for - // the caller to retry. - if len(p) == 0 { - return 0, nil - } + // Preserve the io.Reader contract at the boundary: the cap error is + // reserved for actual excess data -- a legal (0, nil) from the + // underlying reader passes through for the caller to retry. var probe [1]byte n, err := c.reader.Read(probe[:]) if n > 0 { diff --git a/internal/server/cert_store_export.go b/internal/server/cert_store_export.go index ece9577..c1d5633 100644 --- a/internal/server/cert_store_export.go +++ b/internal/server/cert_store_export.go @@ -521,13 +521,19 @@ func dirInsidePinnedTree(treePath string, target os.FileInfo) (bool, error) { inside := false walkErr := fs.WalkDir(treeRoot.FS(), ".", func(name string, entry fs.DirEntry, err error) error { - if err != nil || !entry.IsDir() { + // Fail closed: a subtree that cannot be read or statted is a subtree + // that was not compared, and containment must not silently pass over + // it. + if err != nil { + return err + } + if !entry.IsDir() { return nil } info, err := entry.Info() if err != nil { - return nil + return err } if os.SameFile(info, target) { inside = true