amadan
certsync: backfill only for election contenders; bucket-identity settle stamps (review round)
Two review findings, both fixed rather than argued: - mayBackfill required only an agent-handed gate func, which a grandfathered fleet box (bucket env still present, housekeeper mode "never") also has — its disk holds a customer's certs, and one pass would have replicated them into the platform's control/certs/ and onto every edge. Eligibility is now "contends in the election": a new WithIssuerContender option wired from housekeeperContends (mode sole or elect). Hand-run edges have no predicate at all and stay excluded. - fileStamp recorded bucket PRESENCE, so a box that settled against the newer leaf went blind when a stale-view writer clobbered it with an older one — durably, since nobody's stamp moved. The stamp now carries the bucket copy's sha256; the clobber changes the sum, un-settles exactly the box holding the newer leaf, and its next pass republishes. Last-writer-wins stays the write discipline (a freshness check inside putCert would still race); the repair loop is the guarantee, and it converges because the NotAfter compare is monotone. Also: the three single-writer-era comments the review flagged now describe the multi-writer rules; tests cover the fleet-box exclusion end-to-end, the older-leaf repair, the success log line, and a non-issuer publishing a wildcard PEM inside the parent set.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 files changed,
+186
−38
internal/edge/certbackfill_test.go+83 −3internal/edge/certsync.go+76 −35internal/edge/edge.go+14 −0internal/host/agent.go+5 −0internal/host/housekeeper.go+8 −0
diff --git a/internal/edge/certbackfill_test.go b/internal/edge/certbackfill_test.go| index 5fbc36e9c..b86725c4c 100644 |
| --- a/internal/edge/certbackfill_test.go |
| +++ b/internal/edge/certbackfill_test.go |
| @@ -1,6 +1,7 @@ |
| package edge |
| import ( |
| + "bytes" |
| "context" |
| "errors" |
| "io" |
| @@ -153,8 +154,10 @@ func TestBackfillRunsOnANonIssuer(t *testing.T) { |
| t.Fatal("an agent-run non-issuer did not publish its own cert") |
| } |
| - // The Edge wiring: agent-run is e.issuer != nil, independent of held; |
| - // a store-carrying edge with no agent (hand-run) may never backfill. |
| + // The Edge wiring: eligibility is issuer-gate present AND contending |
| + // in the election; a store-carrying edge with no agent (hand-run) may |
| + // never backfill, and neither may an agent-run box whose housekeeper |
| + // mode is "never" (the grandfathered fleet box). |
| e := &Edge{staticStore: s} |
| m3 := e.newCertMirror(local) |
| if m3.mayBackfill() { |
| @@ -164,8 +167,16 @@ func TestBackfillRunsOnANonIssuer(t *testing.T) { |
| t.Fatal("an ungated edge counts as the issuer") |
| } |
| e.issuer = func() (context.Context, bool) { return context.Background(), false } |
| + if m3.mayBackfill() { |
| + t.Fatal("an agent-run box that does not contend may backfill (fleet box)") |
| + } |
| + e.contends = func() bool { return false } |
| + if m3.mayBackfill() { |
| + t.Fatal("a contends()==false box may backfill (fleet box)") |
| + } |
| + e.contends = func() bool { return true } |
| if !m3.mayBackfill() { |
| - t.Fatal("an agent-run non-holder may not backfill") |
| + t.Fatal("an agent-run contender that does not hold the lease may not backfill") |
| } |
| if m3.issuing() { |
| t.Fatal("a non-holding edge counts as the issuer") |
| @@ -174,6 +185,17 @@ func TestBackfillRunsOnANonIssuer(t *testing.T) { |
| if !m3.issuing() { |
| t.Fatal("a holding edge is not the issuer") |
| } |
| + // And end-to-end through backfill itself: the hand-run shape (gate |
| + // present through serving-side wiring is irrelevant — no contender |
| + // predicate) walks nothing and publishes nothing. |
| + e2 := &Edge{staticStore: s} |
| + putLocal(t, local, "handrun.example.com", hostCertPEM(t, "handrun.example.com", time.Now().Add(time.Hour))) |
| + m4 := e2.newCertMirror(local) |
| + m4.allowed = func(string) bool { return true } |
| + m4.backfill(context.Background()) |
| + if _, ok := bucketHas(t, s, "handrun.example.com"); ok { |
| + t.Fatal("a hand-run edge published through backfill") |
| + } |
| } |
| // Seeding the deployment's ACME identity stays single-writer: a non-issuer |
| @@ -200,6 +222,64 @@ func TestBackfillAccountKeySeedsOnlyFromTheIssuer(t *testing.T) { |
| } |
| } |
| +// #318's repair loop: another box clobbers the bucket with an OLDER leaf |
| +// after this box settled against the newer one. The stamp carries the |
| +// bucket copy's identity, so the settled box un-settles on its next poll |
| +// and republishes the newer cert — the clobber is transient, not durable. |
| +func TestBackfillRepairsAnOlderLeafClobber(t *testing.T) { |
| + var buf strings.Builder |
| + log.SetOutput(&buf) |
| + t.Cleanup(func() { log.SetOutput(os.Stderr) }) |
| + s := release.NewDirStore(t.TempDir()) |
| + local := autocert.DirCache(t.TempDir()) |
| + newer := hostCertPEM(t, "a.example.com", time.Now().Add(48*time.Hour)) |
| + older := hostCertPEM(t, "a.example.com", time.Now().Add(1*time.Hour)) |
| + putLocal(t, local, "a.example.com", newer) |
| + |
| + m := issuerMirror(t, s, local, false) |
| + m.backfill(context.Background()) |
| + if got, _ := bucketHas(t, s, "a.example.com"); !bytes.Equal(got, newer) { |
| + t.Fatal("setup: newer cert not published") |
| + } |
| + if err := m.Refresh(context.Background()); err != nil { |
| + t.Fatal(err) |
| + } |
| + m.backfill(context.Background()) // settles: disk == bucket == newer |
| + if !strings.Contains(buf.String(), "backfilled 1 cache entry") { |
| + t.Fatalf("the publishing pass did not report itself:\n%s", buf.String()) |
| + } |
| + |
| + // Another box lands the older leaf (its view was stale). |
| + putBucket(t, s, "a.example.com", older) |
| + if err := m.Refresh(context.Background()); err != nil { |
| + t.Fatal(err) |
| + } |
| + m.backfill(context.Background()) |
| + if got, _ := bucketHas(t, s, "a.example.com"); !bytes.Equal(got, newer) { |
| + t.Fatal("the settled box did not repair an older-leaf clobber") |
| + } |
| +} |
| + |
| +// A wildcard PEM on a non-issuer's disk publishes like any owned cert — |
| +// bounded by the manager's parent set exactly as on the issuer. |
| +func TestBackfillWildcardFromANonIssuer(t *testing.T) { |
| + log.SetOutput(io.Discard) |
| + t.Cleanup(func() { log.SetOutput(os.Stderr) }) |
| + s := release.NewDirStore(t.TempDir()) |
| + local := autocert.DirCache(t.TempDir()) |
| + putLocal(t, local, "wildcard-acct.example.com.pem", hostCertPEM(t, "*.acct.example.com", time.Now().Add(time.Hour))) |
| + putLocal(t, local, "wildcard-other.example.com.pem", hostCertPEM(t, "*.other.example.com", time.Now().Add(time.Hour))) |
| + |
| + m := issuerMirror(t, s, local, false) |
| + m.backfill(context.Background()) |
| + if _, ok := bucketHas(t, s, "wildcard-acct.example.com.pem"); !ok { |
| + t.Fatal("a non-issuer did not publish the wildcard PEM for a current parent") |
| + } |
| + if _, ok := bucketHas(t, s, "wildcard-other.example.com.pem"); ok { |
| + t.Fatal("a wildcard PEM outside the parent set published") |
| + } |
| +} |
| + |
| // The other cert shapes ride the same freshness path as plain hosts. |
| func TestBackfillWildcardAndRSAThroughTheFreshnessPath(t *testing.T) { |
| log.SetOutput(io.Discard) |
diff --git a/internal/edge/certsync.go b/internal/edge/certsync.go| index 0e24c8297..8dbca3f72 100644 |
| --- a/internal/edge/certsync.go |
| +++ b/internal/edge/certsync.go |
| @@ -30,6 +30,7 @@ package edge |
| import ( |
| "bytes" |
| "context" |
| + "crypto/sha256" |
| "crypto/tls" |
| "crypto/x509" |
| "errors" |
| @@ -124,7 +125,10 @@ var errNotIssuer = errors.New("acme: not available here") |
| // directly, and the door is the agent's client. A grandfathered fleet box |
| // that still carries the bucket env therefore reads control/certs/ over |
| // its own IAM, and is gated AND mirrored like any other non-issuer — a |
| -// transition state, resolved by the rollout dropping that env; |
| +// transition state, resolved by the rollout dropping that env. It also |
| +// never BACKFILLS (#318): mayBackfill requires contending in the |
| +// election, and a fleet box's housekeeper mode is "never" — its disk |
| +// holds a customer's certs, not this deployment's; |
| // - boxes with neither CARLOS_DEPLOYMENT_BUCKET nor CARLOS_DEPLOYMENT_DIR; |
| // - boxes with a bucket but no CARLOS_RELEASE_PUBKEY, where |
| // enableStaticFromEnv leaves staticStore nil. |
| @@ -269,11 +273,12 @@ func (p *publishingCache) Put(ctx context.Context, key string, data []byte) erro |
| // serving. |
| // |
| // It is gated like Put, and that is a deliberate trade: an issuer that |
| -// stands down mid-order leaves its token behind in the bucket, because the |
| -// writer map gives control/certs/** to the housekeeper alone and a box that |
| -// is no longer the housekeeper does not get to delete there. The spec's |
| -// answer to that litter is a TTL stamp swept from the mirror side, not a |
| -// second writer. |
| +// stands down mid-order leaves its token behind in the bucket, because |
| +// ISSUANCE artifacts stay the housekeeper's to write and un-write (#318 |
| +// widened only the backfill of settled cert entries, never tokens), and a |
| +// box that is no longer the housekeeper does not get to delete there. The |
| +// spec's answer to that litter is a TTL stamp swept from the mirror side, |
| +// not a second writer. |
| func (p *publishingCache) Delete(ctx context.Context, key string) error { |
| err := p.inner.Delete(ctx, key) |
| if !p.publishes(key) { |
| @@ -334,11 +339,15 @@ func publishableCertName(name string) bool { |
| // same reason (release.Store.Put with an empty ifMatch is create-only, so a |
| // renewal would otherwise lose to its own previous cert). |
| // |
| -// Last-writer-wins is the DESIGN here, not a compromise: one gated issuer |
| -// means there is normally no second writer at all, and the transient |
| -// double-holder the housekeeper's poll window allows writes an equally valid |
| -// cert for the same name. Whichever lands last is servable, and the next |
| -// renewal converges the pair. |
| +// Last-writer-wins is the DESIGN here, not a compromise. On the issuance |
| +// path, the transient double-holder the housekeeper's poll window allows |
| +// writes an equally valid cert for the same name, and whichever lands last |
| +// is servable. On the backfill path (#318: any contending box), a writer |
| +// racing a stale view can land an OLDER leaf — repaired, not prevented: |
| +// fileStamp's bucket-identity half un-settles the box holding the newer |
| +// one, whose next pass republishes it. A freshness check inside this |
| +// function would still race (check-then-put), so the repair loop is the |
| +// guarantee, and it converges because the NotAfter compare is monotone. |
| func putCert(ctx context.Context, s release.Store, name string, body []byte) error { |
| key := certKey(name) |
| for attempt := 0; attempt < 3; attempt++ { |
| @@ -361,9 +370,10 @@ func putCert(ctx context.Context, s release.Store, name string, body []byte) err |
| // ---- the mirror: every edge serves every cert -------------------------------- |
| // |
| // certMirror is the serving half of the pair at the top of this file, and the |
| -// reason the issuance gate above is safe to shut. It POLLS control/certs/ (it |
| -// never writes there — the writer map gives that prefix to the housekeeper |
| -// alone) and keeps the whole prefix in memory, so any edge can answer a |
| +// reason the issuance gate above is safe to shut. It POLLS control/certs/ |
| +// (its serving side never writes there; the one writer it carries is the |
| +// backfill below, bounded to cert entries this box owns — #318) and keeps |
| +// the whole prefix in memory, so any edge can answer a |
| // handshake for a name some OTHER edge issued, and can answer an HTTP-01 |
| // challenge for an order some OTHER edge is running. |
| // |
| @@ -529,13 +539,19 @@ func (e *Edge) newCertMirror(local autocert.Cache) *certMirror { |
| // exists for a name that passed it — now that the policy admits the |
| // deployment's remote-homed routes too (#277). |
| m.allowed = e.deploymentHost |
| - // Backfill runs on any AGENT-run box (#318): the election in the process |
| - // is what makes the box an accountable member of the deployment, whether |
| - // or not it currently holds the lease. A hand-run `carlos edge` with a |
| - // bucket but no agent is "held" for serving and must still NOT dump its |
| - // disk into the deployment's prefix — that is this predicate, not the |
| - // lease. |
| - m.mayBackfill = func() bool { return e.issuer != nil } |
| + // Backfill runs on any box that CONTENDS in the deployment's issuer |
| + // election (#318): contending is what makes the box an accountable |
| + // member of the deployment, whether or not it currently holds the |
| + // lease. Two box classes are excluded, each for its own reason: a |
| + // hand-run `carlos edge` with a bucket but no agent is "held" for |
| + // serving and must NOT dump its disk into the deployment's prefix |
| + // (e.issuer nil, and no contender predicate was ever handed down); and |
| + // a grandfathered FLEET box still carrying the bucket env is agent-run |
| + // with a gate func, but its housekeeper mode is "never" — its disk |
| + // holds a customer's certs, which are not this deployment's to |
| + // replicate onto every platform edge (WithIssuerContender, and the |
| + // armCertIssuance doc's grandfathered-fleet-box paragraph). |
| + m.mayBackfill = func() bool { return e.issuer != nil && e.contends != nil && e.contends() } |
| // Strict: only the box holding the lease RIGHT NOW may seed the |
| // deployment's ACME account identity (see backfill's account-key rule). |
| m.issuing = func() bool { |
| @@ -819,7 +835,8 @@ func (m *certMirror) run(stop <-chan struct{}) { |
| // |
| // Rules, each doing work: |
| // |
| -// - Only on an agent-run box (m.mayBackfill), NOT only on the issuer. |
| +// - Only on a box that contends in the election, NOT only on the issuer |
| +// (m.mayBackfill: agent-run AND contending in the issuer election). |
| // The 2026-08-27 rung-1 rehearsal (#318) found the issuer-only rule |
| // left every cert minted on a box that never held the lease since |
| // #291 sitting on that box's disk alone — exactly the bytes a |
| @@ -827,7 +844,11 @@ func (m *certMirror) run(stop <-chan struct{}) { |
| // therefore multi-writer now, kept convergent by the freshness rules |
| // below (bytes-equal settles; an older leaf never displaces a newer |
| // one), and the writer map's housekeeper-only line for this prefix is |
| -// amended to "any agent-run box, certs only". The serving-side "held |
| +// amended to "any contending box, certs only". A concurrent writer |
| +// racing a stale view CAN transiently land an older leaf over a newer |
| +// one; the identity half of fileStamp is what makes that transient — |
| +// the box holding the newer leaf un-settles on the changed bucket sum |
| +// and republishes next pass. The serving-side "held |
| // or ungated" read is still the wrong predicate: a hand-run `carlos |
| // edge` with a bucket but no agent is ungated for serving and must |
| // NOT dump its disk into the deployment's prefix — m.mayBackfill is |
| @@ -905,12 +926,14 @@ func (m *certMirror) backfill(ctx context.Context) { |
| if err != nil { |
| continue |
| } |
| - stamp := fileStamp{size: info.Size(), mtime: info.ModTime().UnixNano(), inBucket: inBucket} |
| + stamp := fileStamp{size: info.Size(), mtime: info.ModTime().UnixNano(), bucketSum: bucketSum(remote, inBucket)} |
| // Settled at this stamp on an earlier pass — in sync with the |
| // bucket, or not this deployment's to publish — and nothing has |
| - // moved since (the bucket's side is part of the stamp: an entry |
| - // that was in sync but has since left the bucket re-reads). This |
| - // is what keeps steady state at a listing plus a stat per entry. |
| + // moved since. The bucket copy's IDENTITY is part of the stamp: an |
| + // entry that was in sync but has since left the bucket, or been |
| + // overwritten by another box, re-reads — that re-read is the #318 |
| + // repair path (see fileStamp). This is what keeps steady state at |
| + // a listing plus a stat per entry. |
| if m.settledAt(name) == stamp { |
| continue |
| } |
| @@ -995,15 +1018,33 @@ func (m *certMirror) ownsCertName(name string, cert *tls.Certificate) bool { |
| } |
| // fileStamp is the state a backfill pass last settled an entry at: the |
| -// file's size+mtime and whether the bucket held the name. Not a hash: |
| -// DirCache writes are atomic renames, so a changed body is a changed |
| -// stamp (nanosecond mtime, so a same-size rewrite within one second on a |
| -// coarse filesystem is the only alias, and autocert never rewrites a name |
| -// that fast). |
| +// file's size+mtime, and the IDENTITY of the bucket's copy (a sha256 of |
| +// its bytes; zero = absent), not merely its presence. The local half is |
| +// not a hash: DirCache writes are atomic renames, so a changed body is a |
| +// changed stamp (nanosecond mtime, so a same-size rewrite within one |
| +// second on a coarse filesystem is the only alias, and autocert never |
| +// rewrites a name that fast). The bucket half MUST be identity: with |
| +// cert entries multi-writer (#318), another box racing a stale view can |
| +// land an older leaf over a newer one this box already settled against — |
| +// a presence bool would leave the settled box blind to the clobber |
| +// forever (its file unmoved, the name still "in bucket"), while the sum |
| +// changes with the clobber, un-settles exactly the box holding the newer |
| +// leaf, and its next pass republishes it. Convergence within a poll or |
| +// two, newest leaf wins. |
| type fileStamp struct { |
| - size int64 |
| - mtime int64 |
| - inBucket bool |
| + size int64 |
| + mtime int64 |
| + bucketSum [sha256.Size]byte |
| +} |
| + |
| +// bucketSum is fileStamp's bucket-identity half. The zero value means |
| +// "absent", which cannot collide with a real entry: nothing under |
| +// control/certs/ is ever zero bytes, and sha256 of any body is nonzero. |
| +func bucketSum(body []byte, inBucket bool) [sha256.Size]byte { |
| + if !inBucket { |
| + return [sha256.Size]byte{} |
| + } |
| + return sha256.Sum256(body) |
| } |
| func (m *certMirror) settledAt(name string) fileStamp { |
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index d50ce448c..d126eeb48 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -176,6 +176,10 @@ type Edge struct { |
| // every single-box deployment's behaviour to date: mint as always. |
| // Serving is never gated by it. |
| issuer IssuerGate |
| + // contends reports whether this box takes part in the issuer election |
| + // at all (WithIssuerContender) — the cert backfill's eligibility, a |
| + // coarser question than the gate above answers. nil = does not contend. |
| + contends func() bool |
| // box is this box's own label — the SAME derivation its edges file is |
| // published under (internal/host's metricsBoxLabel), handed down through |
| // WithBoxLabel for the reason the issuer gate is: that package imports |
| @@ -214,6 +218,16 @@ type RunOption func(*Edge) |
| // as it always has. See IssuerGate (certsync.go). |
| func WithIssuerGate(g IssuerGate) RunOption { return func(e *Edge) { e.issuer = g } } |
| +// WithIssuerContender tells the edge whether this box CONTENDS in the |
| +// deployment's issuer election at all — a different question from holding |
| +// it right now (the gate above answers that, moment to moment). The cert |
| +// backfill (certsync.go, #318) is allowed exactly on contenders: an |
| +// agent-run box whose housekeeper mode is "never" (a fleet box grandfathered |
| +// with the bucket env) carries a gate func that can never be held, and its |
| +// disk holds a customer's certs — not this deployment's to replicate. nil — |
| +// no agent handed one down — reads as "does not contend". |
| +func WithIssuerContender(c func() bool) RunOption { return func(e *Edge) { e.contends = c } } |
| + |
| // WithBoxLabel hands the edge this box's own label, the one its edges file is |
| // published under (internal/host's metricsBoxLabel — the same call the edges |
| // publisher makes, so the two can never disagree about which file is ours). |
diff --git a/internal/host/agent.go b/internal/host/agent.go| index 298afdb68..7729df454 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -405,6 +405,11 @@ func RunAgent(args []string) { |
| // stops being the housekeeper. Armed above, so this closure is live |
| // and correct by the time the edge reads it. |
| edge.WithIssuerGate(housekeeperGate), |
| + // Whether this box contends in that election AT ALL — the cert |
| + // backfill's eligibility (#318). Mode is decided once from the |
| + // environment, so the closure is stable; see housekeeperContends |
| + // for the fleet-box exclusion it exists to state. |
| + edge.WithIssuerContender(housekeeperContends), |
| // This box's own label, for the home map's own-box exclusion |
| // (internal/edge/homemap.go). metricsBoxLabel is the SAME call |
| // edgesFileLoop publishes under, deliberately: the map excludes our |
diff --git a/internal/host/housekeeper.go b/internal/host/housekeeper.go| index baf4442c4..e43461f5e 100644 |
| --- a/internal/host/housekeeper.go |
| +++ b/internal/host/housekeeper.go |
| @@ -378,6 +378,14 @@ func housekeeperHeld() bool { return theHousekeeper.Load().held() } |
| // under — the gate every deployment-global duty consults. |
| func housekeeperGate() (context.Context, bool) { return theHousekeeper.Load().gate() } |
| +// housekeeperContends reports whether this box takes part in the election at |
| +// all — mode sole or elect, decided once from the environment. The edge's |
| +// cert-backfill eligibility (edge.WithIssuerContender, #318): a fleet box |
| +// grandfathered with the bucket env is agent-run and gated, but mode |
| +// "never" — its disk holds a customer's certs, never this deployment's to |
| +// publish. |
| +func housekeeperContends() bool { return theHousekeeper.Load().mode != housekeeperNever } |
| + |
| // startHousekeeper arms the election for this process and returns its |
| // stand-down. Called once from RunAgent, before the loops whose duties it |
| // gates; the returned func MUST be called on the way out (RunAgent's |