carlos / platform Public

certsync: backfill round-2 fixes — settled memo covers not-owned residue, wildcard parent bound, mid-pass gate loss still reports

Review round 2: the size+mtime memo only covered entries in the bucket,
so a long-lived disk's detached-route residue was re-read and re-parsed
every poll; the memo now records every settled outcome (with the bucket
side as part of the stamp, so an entry that leaves the bucket re-reads).
A wildcard PEM must now be for a parent in the wildcard manager's
current set (new currentParents seam), not merely self-consistent with
its own name. Losing the gate mid-pass breaks instead of returning, so
the summary and the throttled failure counter still fire. The no-reread
test was vacuous (chmod 0o100 blocked the listing, not the reads) — it
now makes the files unreadable and proves both the skip and the re-read
on a moved stamp.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 2 months ago b44e36f4bc5b843297398ef5e9bd18c46b1a5f64 parent 17c6621

4 files changed, +98 −32

internal/edge/certbackfill_test.go +34 −5
internal/edge/certsync.go +55 −27
internal/edge/edge.go +1 −0
internal/edge/wildcard.go +8 −0

diff --git a/internal/edge/certbackfill_test.go b/internal/edge/certbackfill_test.go

index bddfe75fd..9cb8e2f7f 100644

--- a/internal/edge/certbackfill_test.go

+++ b/internal/edge/certbackfill_test.go

@@ -6,6 +6,7 @@ import (

"io"

"log"

"os"

+ "path/filepath"

"strings"

"sync"

"testing"

@@ -26,6 +27,8 @@ func issuerMirror(t *testing.T, s release.Store, local autocert.Cache, held bool

m.allowed = func(host string) bool {

return strings.HasSuffix(host, ".example.com") && !strings.HasPrefix(host, "gone.")

}

+ // The wildcard manager's parent set: acct only.

+ m.wildcardParents = func() []string { return []string{"acct.example.com"} }

if err := m.Refresh(context.Background()); err != nil {

t.Fatal(err)

}

@@ -72,6 +75,7 @@ func TestBackfillPublishesPreExistingCertsOnce(t *testing.T) {

putLocal(t, local, "gone.example.com", hostCertPEM(t, "gone.example.com", later))

putLocal(t, local, "a.example.com1762384410", live)

putLocal(t, local, "wildcard-other.example.com.pem", hostCertPEM(t, "*.acct.example.com", later))

+ putLocal(t, local, "wildcard-left.example.com.pem", hostCertPEM(t, "*.left.example.com", later)) // parent no longer current

putLocal(t, local, "acme_account+key", []byte("LOCAL-ACCOUNT"))

putLocal(t, local, wildcardAccountKeyName, []byte("LOCAL-WILDCARD-ACCOUNT"))

@@ -87,7 +91,7 @@ func TestBackfillPublishesPreExistingCertsOnce(t *testing.T) {

t.Errorf("a.example.com body differs from the local cache")

}

for _, name := range []string{"expired.example.com", "garbage.example.com", "a.example.com+http-01", "a.example.com+token",

- "gone.example.com", "a.example.com1762384410", "wildcard-other.example.com.pem"} {

+ "gone.example.com", "a.example.com1762384410", "wildcard-other.example.com.pem", "wildcard-left.example.com.pem"} {

if _, ok := bucketHas(t, s, name); ok {

t.Errorf("%s must not be published", name)

}

@@ -164,6 +168,9 @@ func TestBackfillStopsWhenTheGateIsLostMidPass(t *testing.T) {

for _, h := range []string{"a.example.com", "b.example.com", "c.example.com"} {

putLocal(t, local, h, hostCertPEM(t, h, later))

}

+ var buf strings.Builder

+ log.SetOutput(&buf)

+ t.Cleanup(func() { log.SetOutput(os.Stderr) })

m := issuerMirror(t, s, local, true)

calls := 0

m.issuing = func() bool { calls++; return calls <= 2 } // the top check, then one entry

@@ -172,6 +179,9 @@ func TestBackfillStopsWhenTheGateIsLostMidPass(t *testing.T) {

if len(keys) != 1 {

t.Fatalf("published %v after the gate was lost; want exactly one", keys)

}

+ if !strings.Contains(buf.String(), "backfilled 1 cache entry") {

+ t.Fatalf("a pass cut short by the gate did not report what it published:\n%s", buf.String())

+ }

}

// The other cert shapes ride the same freshness path as plain hosts.

@@ -253,19 +263,38 @@ func TestBackfillSteadyStateDoesNotReread(t *testing.T) {

if err := m.Refresh(context.Background()); err != nil {

t.Fatal(err)

}

- if err := os.Chmod(string(local), 0o100); err != nil { // list only, no reads

- t.Fatal(err)

- }

- t.Cleanup(func() { _ = os.Chmod(string(local), 0o700) })

+ // Residue the pass settles as not-owned must not be re-read either.

+ putLocal(t, local, "gone.example.com", hostCertPEM(t, "gone.example.com", time.Now().Add(time.Hour)))

+ m.backfill(context.Background())

if os.Getuid() == 0 {

t.Skip("root ignores mode bits")

}

+ // Listing and stat still work; any read now fails loudly.

+ for _, name := range []string{"a.example.com", "gone.example.com"} {

+ if err := os.Chmod(filepath.Join(string(local), name), 0o000); err != nil {

+ t.Fatal(err)

+ }

+ var buf strings.Builder

+ log.SetOutput(&buf)

c := &putCountingStore{Store: s}

m.s = c

m.backfill(context.Background())

if c.puts != 0 {

t.Fatalf("steady state published %d", c.puts)

}

+ if strings.Contains(buf.String(), "not published") {

+ t.Fatalf("steady state re-read the disk:\n%s", buf.String())

+ }

+ // And a moved stamp IS re-read: touch one and the read fails visibly.

+ now := time.Now().Add(time.Second)

+ if err := os.Chtimes(filepath.Join(string(local), "a.example.com"), now, now); err != nil {

+ t.Fatal(err)

+ }

+ m.backfill(context.Background())

+ if !strings.Contains(buf.String(), "not published") {

+ t.Fatal("a changed stamp was not re-read")

+ }

}

// autocert writes the cache while the backfill walks it. Whatever the

diff --git a/internal/edge/certsync.go b/internal/edge/certsync.go

index 0d8303b5c..cf85421be 100644

--- a/internal/edge/certsync.go

+++ b/internal/edge/certsync.go

@@ -429,10 +429,14 @@ type certMirror struct {

// backfill's switch (see backfill). nil = never the issuer, which is the

// safe default for a mirror built without an Edge.

issuing func() bool

- // inSync memoes the on-disk stamp each entry last matched the bucket

- // at (backfill); backfillFailures counts failed passes for the log

+ // wildcardParents is the wildcard manager's current parent set, the

+ // bound on which wildcard PEMs backfill may publish. nil = no manager =

+ // none.

+ wildcardParents func() []string

+ // settled memoes the stamp each on-disk entry was last found settled at

+ // (backfill); backfillFailures counts failed passes for the log

// cadence; backfillOnce guards the one-time "cannot walk" line.

- inSync map[string]fileStamp

+ settled map[string]fileStamp

backfillFailures int

backfillOnce sync.Once

@@ -783,7 +787,8 @@ func (m *certMirror) run(stop <-chan struct{}) {

// one-pass copy, run after every successful poll, that converges in one

// tick. Steady state on the issuer is one directory listing per poll plus

// a stat per entry: an entry whose size and mtime have not moved since it

-// was last seen in sync with the bucket is not re-read (m.inSync).

+// was last settled — in sync with the bucket, or found not to be this

+// deployment's — is not re-read (m.settled).

// Rules, each doing work:

@@ -792,7 +797,8 @@ func (m *certMirror) run(stop <-chan struct{}) {

// with a bucket but no agent is ungated for serving and must NOT dump

// its disk into the deployment's prefix. The gate is re-read per entry:

// a pass over fifty certs can outlive a holding, and the writer map

-// gives control/certs/** to the housekeeper alone.

+// gives control/certs/** to the housekeeper alone. Losing it ends the

+// pass but still reports what the pass did.

// - Only a DirCache is walked. The mirror holds the cache as an

// autocert.Cache, and that interface cannot enumerate; the on-disk

// layout is the only backend that exists (certcache.go) and the one

@@ -803,8 +809,8 @@ func (m *certMirror) run(stop <-chan struct{}) {

// a kill mid-write — neither is the deployment's to replicate onto every

// edge. A per-host entry (<host>, <host>+rsa) must pass m.allowed, the

// same registry-backed exact check both read-throughs use; a wildcard

-// PEM must be named for the parent its leaf actually covers. Account

-// keys are their own shape.

+// PEM must be for a parent in the wildcard manager's current set and

+// its leaf must cover it. Account keys are their own shape.

// - Challenge tokens never publish: they are seconds long, and a copy of

// a spent one is litter every edge would go on serving.

// - Account keys publish only when the bucket has none. The deployment's

@@ -834,7 +840,7 @@ func (m *certMirror) backfill(ctx context.Context) {

entries, err := os.ReadDir(string(dir))

if err != nil {

if !errors.Is(err, os.ErrNotExist) {

- m.logBackfillFailure(1, err)

+ m.logBackfillFailure(0, err)

}

return

}

@@ -855,12 +861,17 @@ func (m *certMirror) backfill(ctx context.Context) {

if err != nil {

continue

}

- stamp := fileStamp{size: info.Size(), mtime: info.ModTime()}

- if inBucket && m.inSyncAt(name) == stamp {

+ stamp := fileStamp{size: info.Size(), mtime: info.ModTime().UnixNano(), inBucket: 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.

+ if m.settledAt(name) == stamp {

continue

}

if !m.issuing() {

- return

+ break

}

body, err := os.ReadFile(filepath.Join(string(dir), name))

if err != nil {

@@ -872,20 +883,28 @@ func (m *certMirror) backfill(ctx context.Context) {

// Also the backstop for names the snapshot never parses: the

// freshness compare below cannot see those, and without this

// they would republish every poll.

- m.markInSync(name, stamp)

+ m.markSettled(name, stamp)

continue

}

if accountKeyName(name) {

if inBucket {

+ m.markSettled(name, stamp)

continue

}

} else {

cert, err := parseCertEntry(body)

if err != nil || !now.Before(cert.Leaf.NotAfter) || !m.ownsCertName(name, cert) {

+ // Expiry is a function of time, but a settled entry can only

+ // move from live to expired, never back; and ownership can

+ // return (a route re-attached), which is a registry change —

+ // so an entry settled as not-owned is re-examined only when

+ // its stamp moves. A re-attached route mints afresh anyway.

+ m.markSettled(name, stamp)

continue

}

if inBucket {

if have := snap.certs[name]; have != nil && !have.Leaf.NotAfter.Before(cert.Leaf.NotAfter) {

+ m.markSettled(name, stamp)

continue

}

@@ -895,7 +914,8 @@ func (m *certMirror) backfill(ctx context.Context) {

lastErr = err

continue

}

- m.markInSync(name, stamp)

+ // Not settled: the bucket side of the stamp changes on the next

+ // poll, and the bytes.Equal path settles it then.

published++

}

if published > 0 {

@@ -909,12 +929,16 @@ func (m *certMirror) backfill(ctx context.Context) {

// ownsCertName reports whether a cert-shaped cache entry is one this

// deployment should replicate: a per-host entry for a name the registry

// still serves (m.allowed — nil admits nothing, the safe default for a

-// mirror built without an Edge), or a wildcard PEM whose leaf covers

-// exactly the parent it is named for.

+// mirror built without an Edge), or a wildcard PEM for a parent in the

+// wildcard manager's CURRENT parent set (m.wildcardParents — nil, no

+// manager, admits none) whose leaf covers that parent.

func (m *certMirror) ownsCertName(name string, cert *tls.Certificate) bool {

if parent, ok := strings.CutPrefix(name, "wildcard-"); ok && strings.HasSuffix(parent, ".pem") {

parent = strings.TrimSuffix(parent, ".pem")

- return parent != "" && slices.Contains(cert.Leaf.DNSNames, "*."+parent)

+ if parent == "" || m.wildcardParents == nil || !slices.Contains(m.wildcardParents(), parent) {

+ return false

+ }

+ return slices.Contains(cert.Leaf.DNSNames, "*."+parent)

}

host := strings.TrimSuffix(name, "+rsa")

if m.allowed == nil || !m.allowed(host) {

@@ -923,27 +947,31 @@ func (m *certMirror) ownsCertName(name string, cert *tls.Certificate) bool {

return cert.Leaf.VerifyHostname(host) == nil

}

-// fileStamp is the size+mtime a backfill pass last saw an entry at while it

-// was in sync with the bucket. Not a hash: DirCache writes are atomic

-// renames, so a changed body is a changed stamp.

+// 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).

type fileStamp struct {

- size int64

- mtime time.Time

+ size int64

+ mtime int64

+ inBucket bool

}

-func (m *certMirror) inSyncAt(name string) fileStamp {

+func (m *certMirror) settledAt(name string) fileStamp {

m.mu.RLock()

defer m.mu.RUnlock()

- return m.inSync[name]

+ return m.settled[name]

}

-func (m *certMirror) markInSync(name string, st fileStamp) {

+func (m *certMirror) markSettled(name string, st fileStamp) {

m.mu.Lock()

defer m.mu.Unlock()

- if m.inSync == nil {

- m.inSync = map[string]fileStamp{}

+ if m.settled == nil {

+ m.settled = map[string]fileStamp{}

}

- m.inSync[name] = st

+ m.settled[name] = st

}

// logBackfillFailure is logPartialPoll's cadence for the publish side.

diff --git a/internal/edge/edge.go b/internal/edge/edge.go

index 7b376d8ff..c0fa82507 100644

--- a/internal/edge/edge.go

+++ b/internal/edge/edge.go

@@ -2384,6 +2384,7 @@ func RunWithActivatorReady(args []string, act Activator, cache *Cache, ready fun

// A wildcard the ISSUER minted reaches this box's handshakes on the

// next poll rather than on this manager's 12h tick.

mirror.wake = w.refresh

+ mirror.wildcardParents = w.currentParents

}

w.refresh()

go w.run()

diff --git a/internal/edge/wildcard.go b/internal/edge/wildcard.go

index 34d21e482..22bc0f261 100644

--- a/internal/edge/wildcard.go

+++ b/internal/edge/wildcard.go

@@ -227,6 +227,14 @@ func (w *wildcardManager) refresh() {

}

+// currentParents is the last computed parent set — the mirror's bound on

+// which wildcard PEMs the issuer's backfill may publish (certsync.go).

+func (w *wildcardManager) currentParents() []string {

+ w.mu.Lock()

+ defer w.mu.Unlock()

+ return append([]string(nil), w.parents...)

// certFor returns the wildcard cert for serverName, or nil when the name

// isn't exactly one label under a current parent (a wildcard doesn't cover

// the apex or deeper levels) or no usable cert is loaded — nil sends the