feat(san-cert): import certificates from a Traefik acme.json - #91
Merged
Conversation
## Summary
Offline `kamal-proxy import certs --traefik-acme /path/acme.json` seeds the
SAN certificate store from a Traefik estate before first boot, so a cutover
serves TLS immediately instead of hard-failing handshakes while re-issuing
every certificate under the issuance rate limit. Imported certs land in the
manager's own layout (certs/<id>/{cert,key}.pem + merged acme.state, atomic
tmp+rename) and become ordinary managed certificates the renewer rotates.
Semantics: skip expired/not-yet-valid entries; a domain already mapped in an
existing state file keeps its cert when it outlives the imported one; without
--resolver all resolver blocks import in sorted order, last writer wins per
domain with a warning; wildcard imports warn that renewal needs DNS-01;
per-entry parse failures are counted, never fatal. The ACME account is not
imported.
Closes #87
## Test Coverage
- TestImportTraefikCertificates_*: happy path, state loads into the
SANCertManager, expired/not-yet-valid skips, longer-lived incumbent kept,
shorter-lived incumbent replaced on disk and in state, resolver selection,
cross-resolver last-writer-wins, wildcard warning, per-entry parse
failures, missing/unparseable input, corrupt existing state not clobbered
- TestImportCertsCommand_*: required flag, data-dir wiring, summary output,
error exits
## Verification
- [x] gofmt -l internal/ cmd/ clean
- [x] make test passes
- [x] go vet ./... clean
- [x] go test -race ./internal/server/ clean
- [x] make lint (golangci-lint) 0 issues
|
We've triggered an ultrareview automatically — This import introduces a new offline path that writes certificates and merges domain mappings into acme.state, so a subtle parsing, expiry, or atomicity bug could corrupt the SAN store or break TLS handshakes across a fleet cutover.. 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. |
📝 WalkthroughWalkthroughAdds an offline ChangesTraefik certificate import
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant ImportCerts
participant ImportTraefikCertificates
participant TraefikAcmeJSON
participant SANCertificateStore
participant AcmeState
Operator->>ImportCerts: provide ACME path and options
ImportCerts->>ImportTraefikCertificates: invoke import
ImportTraefikCertificates->>TraefikAcmeJSON: read resolver certificates
ImportTraefikCertificates->>SANCertificateStore: write valid certificates
ImportTraefikCertificates->>AcmeState: persist domain mappings
ImportTraefikCertificates-->>ImportCerts: return counts and warnings
ImportCerts-->>Operator: print import summary
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/server/traefik_import.go (1)
101-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePersist
imp.stateinstead of relying on shared map aliasing.
impreceives a copy ofstate. The import works only because the two copies share the sameCertificatesandDomainMapmaps. If a later change reassigns either map insidetraefikImport, the write at Line 122 silently drops those entries. Hold a pointer, or writeimp.state.♻️ Proposed change
- state.SavedAt = time.Now() - if err := writeManagerStateFile(opts.StatePath, state); err != nil { + imp.state.SavedAt = time.Now() + if err := writeManagerStateFile(opts.StatePath, imp.state); err != nil { return summary, fmt.Errorf("failed to write certificate state: %w", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/server/traefik_import.go` around lines 101 - 124, Update the traefikImport initialization and final persistence flow to retain and write the mutated import state directly rather than the original state copy. Ensure imp.state is a pointer or otherwise use imp.state when setting SavedAt and calling writeManagerStateFile, preserving all changes even if Certificates or DomainMap are reassigned.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cmd/import_test.go`:
- Line 20: Update the test helper containing runImportCerts to save the existing
globalConfig value before assigning server.Config{} and register t.Cleanup to
restore it afterward, ensuring package-level configuration does not leak into
later tests.
In `@internal/server/traefik_import.go`:
- Around line 158-183: Update importEntry in internal/server/traefik_import.go
(lines 158-183) to compute certID and ManagedCert.Domains from winners rather
than the full domains slice, keeping the stored certificate consistent with
DomainMap ownership. Add a test case in internal/server/traefik_import_test.go
(lines 170-244) where a two-domain entry overlaps an existing longer-lived
certificate on one domain, asserting the resulting Domains and DomainMap contain
only the domains actually won.
- Around line 250-261: Update writeCertificateFiles to write cert.pem and
key.pem to temporary files in the certificate directory, then rename them into
place using the established writeManagerStateFile pattern. Ensure both files are
fully written before replacing the existing pair, and clean up temporary files
on failure so an existing certificate/key pair is not left mismatched.
---
Nitpick comments:
In `@internal/server/traefik_import.go`:
- Around line 101-124: Update the traefikImport initialization and final
persistence flow to retain and write the mutated import state directly rather
than the original state copy. Ensure imp.state is a pointer or otherwise use
imp.state when setting SavedAt and calling writeManagerStateFile, preserving all
changes even if Certificates or DomainMap are reassigned.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6008f067-ec35-4e40-94bd-52e9fd1e0592
📒 Files selected for processing (6)
internal/cmd/import.gointernal/cmd/import_test.gointernal/cmd/root.gointernal/server/san_cert_manager.gointernal/server/traefik_import.gointernal/server/traefik_import_test.go
![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 26m 33s
All reported issues were addressed across 6 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Review fixes for #91: - Create a missing --data-dir before writing state, as `run` does — an import with zero qualifying certificates still writes acme.state, and previously failed on a fresh directory - Write cert.pem/key.pem via tmp+rename so replacing a shorter-lived certificate can never leave a torn file behind - Refuse a state file that parses as JSON but is not manager state (null / missing maps), extending the no-clobber safeguard - Restore globalConfig after the import command test helper runs - Extract adoptExistingCert test helper; add coverage for the partial-overlap case (two-domain entry vs one-domain incumbent) and for schema-invalid state - Document why nearly-expired (<24h) certificates still import
There was a problem hiding this comment.
4 issues found across 4 files (changes from recent commits).
Confidence score: 2/5
- In
internal/server/traefik_import.go, import validation still accepts null certificate entries and dangling domain mappings, which can be persisted and later panicSANCertManager.loadState; this is the highest regression risk because it can take certificate loading down at runtime — reject structurally invalid state before writing and validate all referenced certificate/version records. - In
internal/server/traefik_import.go, certificate replacement is non-atomic (cert.pemcan be renamed beforekey.pem), so an interrupted run can leave a mapped cert without a key and cause TLS handshake failures — switch to an atomic pair update/rollback strategy so both files move together or not at all. - In
internal/server/traefik_import.go(writeCertificateFiles), the comment saysloadStatefails on mismatched pairs, butSANCertManager.loadState()behavior does not match that claim; this can hide real failure modes during future fixes — align the comment with actual loader behavior (or adjust loader behavior to match). - In
internal/cmd/import.go(importCertsCommand.run), data-directory bootstrap logic is duplicated frominternal/cmd/run.go, increasing drift risk between import and serve paths — extract shared initialization into one helper to keep behavior consistent.
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/cmd/import.go">
<violation number="1" location="internal/cmd/import.go:62">
P2: Custom agent: **Enforce Strict Maintainability Standards**
The `importCertsCommand.run` function duplicates the data-directory bootstrap logic already present in the serve `run` command (`internal/cmd/run.go:110-112`). The comment explicitly acknowledges this as a mirror of `run`. Instead of repeating `os.MkdirAll` with identical error wrapping in both commands, extract a single shared helper in `internal/cmd` (or add a method to `globalConfig`) so the bootstrap layer owns the concept in one place.</violation>
</file>
<file name="internal/server/traefik_import.go">
<violation number="1" location="internal/server/traefik_import.go:254">
P2: Custom agent: **Flag AI Slop and Fabricated Changes**
The `writeCertificateFiles` comment claims that `loadState` "simply fails to load the mismatched pair," but `SANCertManager.loadState()` does not fail on a torn cert/key pair. It silently ignores `tls.LoadX509KeyPair` errors via `if err == nil { cert.Certificate = &tlsCert }`, leaving `cert.Certificate` as `nil` while still loading the domain mappings and metadata into memory. This misrepresents a safety property and could mislead maintainers into believing torn pairs are rejected at boot when they are actually silently accepted with nil certificates.</violation>
<violation number="2" location="internal/server/traefik_import.go:278">
P2: Replacing an existing certificate can destroy the usable pair if execution stops after the `cert.pem` rename but before the `key.pem` rename. The loader then keeps a mapped certificate with no key pair, causing TLS failure and reissuance; a recoverable pair-level staging/swap or rollback would retain the incumbent certificate.</violation>
<violation number="3" location="internal/server/traefik_import.go:308">
P1: Structurally corrupt state with null certificate records or dangling domain mappings is still accepted and rewritten, and a null record can later panic `SANCertManager.loadState`. Extending validation to certificate values and every `DomainMap` reference would preserve the promised refusal behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…re rename Second review round for #91: - Refuse a state file with null certificate records or dangling DomainMap references — a healthy manager never persists either (removeCertificate unmaps in the same critical section), and a null record can panic loadState when its pair files exist - Stage both cert.pem.tmp and key.pem.tmp before renaming either, so a failed write can no longer leave a replaced pair mismatched; the remaining crash-between-renames window degrades to a re-order - Correct the writeCertificateFiles comment: loadState leaves a torn pair unloaded (nil Certificate), it does not fail - Extract ensureDataDir shared by `run` and `import certs`
mhenrixon
enabled auto-merge (squash)
August 9, 2026 17:16
mhenrixon
disabled auto-merge
August 9, 2026 17:32
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 import certs --traefik-acme /path/acme.json [--resolver name] [--data-dir dir](internal/cmd/import.go) — runs against the data dir before first boot, no RPC socket neededserver.ImportTraefikCertificates(internal/server/traefik_import.go) parses Traefik's acme.json (per-resolverCertificates[]with base64-wrapped PEM), writes certs into the SAN store layout (certs/<id>/cert.pem|key.pem) and merges domain→cert mappings + expiry intoacme.stateatomically (tmp+rename)writeManagerStateFilefromSANCertManager.writeStateso the running manager and the offline import share one atomic state writerloadState()picks them up on boot and the renewal manager rotates them under whichever challenge the proxy is configured for; SAN groupings from the source are preserved until natural renewal regroups themCloses #87
Test plan
TestImportTraefikCertificates_*(10 tests): happy path incl. SAN grouping preservation and atomicity, state loads into a freshSANCertManager, expired/not-yet-valid skips, longer-lived incumbent kept vs shorter-lived replaced (on disk and in state), resolver selection incl. unknown-resolver error, cross-resolver last-writer-wins with warning, wildcard→DNS-01 warning, per-entry parse failures counted not fatal, missing/unparseable file errors, corrupt existingacme.staterefused rather than clobberedTestImportCertsCommand_*(4 tests):--traefik-acmerequired,--data-dirwiring, summary output, error propagationmake test,go vet,gofmt -lclean,go test -race ./internal/server/clean,golangci-lint0 issuesDeviations & judgment calls
upstreamremote (onlyorigin), so the /lfg upstream sync step was skipped; branchedfeature/traefik-cert-importstraight off a clean, currentdash.san_cert_import.go) skips anything within 24h of expiry, but for a cutover even a nearly-expired cert serves handshakes while the renewer re-orders it, which is the point of this feature.[]byteauto-base64 would fail the whole file on one bad entry;certificate/keyare parsed as strings and base64-decoded per entry so a single corrupt entry counts as failed-to-parse instead of aborting the import.importLegacyHTTP01Cache), not the acme.jsondomain.main/domain.sansmetadata — the leaf is what will actually serve.acme.statethe longer-lived cert wins; within one import run the last writer wins (cross-resolver case).acme.stateaborts the import (exit non-zero) rather than being clobbered.Summary by cubic
Adds an offline CLI to import certificates from a Traefik
acme.jsoninto the SAN store so cutovers serve TLS immediately. Imported certs become managed; state and cert files are written atomically, and the data dir is created if missing.New Features
kamal-proxy import certs --traefik-acme /path/acme.json [--resolver name] [--data-dir dir](runs offline, no RPC).certs/<id>/cert.pem|key.pemand merges domain→cert + expiry intoacme.state.--resolver.--data-dir; writescert.pem/key.pemvia tmp+rename; refuses an existingacme.statethat parses as JSON but isn’t valid manager state.writeManagerStateFile.Migration
kamal-proxy import certs --traefik-acme /path/to/acme.jsonagainst the target--data-dirbefore first boot.--resolverto import a single resolver; otherwise all resolvers import (last-writer-wins per domain).Written for commit 1bec1f0. Summary will update on new commits.
Summary by CodeRabbit
import certscommand for importing certificates from Traefikacme.jsonfiles.