feat(cert-store): certificate store export/import for disaster recovery - #95
Merged
Conversation
…r recovery ## 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 <out.tar.gz>` 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
|
We've triggered an ultrareview automatically — This adds certificate-store export/restore handling private keys, a new RPC verb, and moves certificate file writes under stateMu — a missed bug could corrupt or leak backups, tear archives, or deadlock issuance, so it deserves a deep multi-pass review.. I'll post findings when complete. An ultrareview is cubic's deepest review, catching hard-to-find bugs in the most critical PRs. It runs a longer, multi-pass analysis using cubic's most capable review models, and typically takes around 30 minutes. It consumes your team's reviewed-lines allowance at 3× the standard rate. Automated ultrareviews are disabled by default. We triggered this run as part of your trial. Want cubic to do this for every high-risk PR? Enable auto-ultrareview in your settings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ultrareview completed in 36m 38s
All reported issues were addressed across 16 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
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 <output-path>` 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
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
…self-verifying exports 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
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…aps, exact leaf matching 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
There was a problem hiding this comment.
All reported issues were addressed across 12 files (changes from recent commits).
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…tream validation, durable restores 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
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
…ary and degenerate-store edges 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
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
… inspection errors, reader-contract edge 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
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Confidence score: 2/5
- In
internal/server/cert_store_export.go, the containment check fails open when a certificate-tree directory can’t be read or statted, so export can continue without validating that subtree against the pinned output directory, which creates a real path-containment bypass risk — make these filesystem errors fail closed by propagating them sorejectContainsblocks the export. - In
internal/server/cert_store_archive.go(Read), zero-length reads can still hit the wrapped reader whileremaining > 0and surface unexpected downstream errors, which can make capped-reader behavior inconsistent — move the zero-length guard to the top ofReadto keep zero-byte reads a strict no-op.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/server/cert_store_archive.go">
<violation number="1" location="internal/server/cert_store_archive.go:59">
P3: Zero-length reads are only no-ops after the byte budget is exhausted; while `remaining > 0`, they still reach the wrapped reader and may return its error. Moving this guard to the top of `Read` would make the capped reader's zero-length behavior consistent.
(Based on your team's feedback about capped-reader boundary semantics.)</violation>
</file>
<file name="internal/server/cert_store_export.go">
<violation number="1" location="internal/server/cert_store_export.go:524">
P1: Containment fails open when any certificate-tree directory cannot be read or statted, allowing export to proceed without comparing that subtree against the pinned output directory. Propagating both errors makes `rejectPinnedRootInsideStore` abort before writing.
(Based on your team's feedback about pinned certificate-tree containment.)</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
… uniform zero-length reads 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
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Requires human review: Auto-approval blocked by 2 unresolved issues from previous reviews.
Re-trigger cubic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
kamal-proxy export certs <output.tar.gz>— atomic, mode-0600 tar.gz of the whole certificate estate:acme.state,certs/(including the ACME account key),dynamic-domains.state. Runs through the running proxy over the RPC socket (newCertsExportverb) so the snapshot is taken under the store's disk-write lock; falls back to reading the data dir offline when no proxy answers the socket (internal/cmd/export.go,internal/server/cert_store_export.go).kamal-proxy import certs --archive <archive>— offline restore, refusing a non-empty store without--force. Shares its writing path (writeCertificateFiles,writeManagerStateFile) with the Traefik importer, and writes the state file last so an interrupted restore never names certificates that were not written (internal/server/cert_store_restore.go).--verify— parses every certificate in an archive and reports domains + expiries without touching the store, for CI/cron backup checks. Hostile archives (path traversal, symlink entries, torn pairs, inconsistent state) are rejected by a shared strict reader (internal/server/cert_store_archive.go).saveCertificatenow writes through the staged tmp+rename path, andadoptCertificate/removeCertificateholdstateMuacross their cert-file + state writes.stateMuis now the store's disk-write lock, full stop.Closes #90
Test plan
SANCertManagerboots on the restored dir and loads every pairTestSANCertManager_ExportStoreHoldsTheDiskLock)--force; verify never writes; traversal/symlink/torn archives rejected on both verify and restoregofmtclean,make test,go vet,make lint(0 issues),go test -race ./internal/server/Deviations & judgment calls
acme_user.jsonlives INSIDEcerts/(CachePath), not beside it — the archive layout mirrors the data dir (certs/acme_user.json), so restore is a faithful extraction.saveCertificatewrote cert.pem/key.pem non-atomically and outsidestateMu, andremoveCertificatedeleted cert dirs outside it. "Export under the managers' write lock" is only real if those file ops move understateMu— that refactor is part of this PR.--verifyhangs offimport certs --archive X --verifyrather than a separate verb — it reads an archive exactly the way import would, minus the writes.--forceoverwrites from the archive but does NOT delete existing cert dirs absent from it; the replacedacme.statesimply no longer references them (orphans are inert). Conservative vs. rm -rf.acme.stateaborts an export (it is the estate's index; a backup that cannot restore is worse than no backup). A corrupt account key or dynamic-domains file degrades to a warning, because a booted proxy rebuilds both automatically.KAMAL_PROXY_SOCKETwould therefore export offline — documented in the command's long help.certs/http01/autocert cache is not exported; export warns to boot the proxy once so it is adopted into the store first.Summary by cubic
Adds export/import/verify for the certificate store to enable disaster recovery with durable, self-verified backups and restores. Fixes a deadlock on first boot when adopting a legacy cache. Closes #90.
New Features
kamal-proxy export certs <out.tar.gz>: atomic, fsynced, mode-0600 snapshot via RPCCertsExportunder the disk-write lock; falls back to offline when no proxy; rejects outputs inside the store; pins the output directory and self-verifies the staged archive through the pinned handle; supports account-key–only stores.kamal-proxy import certs --archive <archive>: offline restore (refuses non-empty stores unless--force), writes via staged tmp+rename and commits state last;--verifyparses and cross-checks without writes.Refactors
stateMu, with parent directories synced after renames;saveCertificatestages via tmp+rename;removeCertificateholds the lock across file removal and state write; temp names use short fixed patterns to avoid component-length overflows.validateManagerState: caps bytes/entries including tar-header work and drains the full gzip stream (trailer counted) so oversized/corrupt archives cannot verify; rejects traversal/symlinks/torn pairs; enforces sanitized/non-colliding identifiers and identifier/map-key consistency; requires an ECDSA account key; demands exact leaf-vs-state SAN equality; warnings are structured; zero-length reads are handled uniformly before and after the byte cap.--forceremoves stale state-referenced dirs and syncs directory updates after the state commit; only existing dirs are counted, and degenerate stores (no cert dir) don’t fail post-commit.os.ErrNotExist(otherlstaterrors fail the restore).Written for commit bd05e74. Summary will update on new commits.