amadan
edge: fix Task 4 review findings — HostPolicy bound, delegated-source cache
CRITICAL-1: Allowed() (autocert HostPolicy) reverts to the exact map lookup instead of lookupUpstream's wildcard fallback. Widening it let any stranger's SNI <label>.<catch-all domain> pass HostPolicy and drive a fresh per-host ACME order whenever the wildcard cert wasn't yet obtained (the normal state right after an attach) — unbounded, attacker-chosen labels against the box's one shared ACME account/quota. ServeHTTP keeps lookupUpstream; only request routing gets the wildcard fallback, never cert issuance policy. CRITICAL-2: TestHostPolicyExcludesWildcardFallback pins the split — request routing for a wildcard-alias host still resolves, but Allowed() refuses it until it has its own exact row. IMPORTANT-1: newWildcardFromEnv's delegated closure now caches the last successful ListWildcardDomains() result and returns it on a read error, instead of returning nil. refresh() only runs at startup and on OnReload (run()'s 12h ticker never re-invokes the delegated source), so returning nil on a transient error emptied the delegated set until an unrelated registry write next moved the fingerprint — unbounded on a quiet box, and it also stopped certFor from serving an already-cached, still-valid delegated wildcard. Extended TestDelegatedSourceReadsRegistry to close the registry after a successful read and assert the closure still returns the last-good list. Also: mended registry.go's Fingerprint doc comment (a stray mid-sentence line break from the wildcard_domains rewrap).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
5 files changed,
+126
−24
internal/edge/alias_test.go+60 −0internal/edge/edge.go+26 −15internal/edge/wildcard.go+20 −5internal/edge/wildcard_test.go+17 −0internal/registry/registry.go+3 −4
diff --git a/internal/edge/alias_test.go b/internal/edge/alias_test.go| index 07429a114..c872bf05e 100644 |
| --- a/internal/edge/alias_test.go |
| +++ b/internal/edge/alias_test.go |
| @@ -372,3 +372,63 @@ func TestWildcardAliasRequestRouting(t *testing.T) { |
| t.Fatalf("lookupUpstream(paul.woodstar.app) route.Host = %q, want the EXACT alias's primary other.bab.oncarlos.com", up.route.Host) |
| } |
| } |
| + |
| +// TestHostPolicyExcludesWildcardFallback is the CRITICAL-1 review fix: the |
| +// autocert HostPolicy (Allowed) must stay on the EXACT map only, never |
| +// lookupUpstream's wildcard fallback. A catch-all alias ("*.<domain>") gets |
| +// its TLS coverage from the WILDCARD cert (wrapGetCertificate checks certFor |
| +// before ever reaching autocert) — Allowed only gates a NEW per-host ACME |
| +// order, which is unbounded (attacker-chosen labels) and shared across every |
| +// tenant on the box (one ACME account). Before this fix, a stranger's SNI |
| +// random.woodstar.app would pass Allowed and drive a fresh per-host order |
| +// for as long as the wildcard cert wasn't yet obtained — the normal state |
| +// right after an attach, before the customer's CNAME exists. |
| +// |
| +// Request-time routing (lookupUpstream, unaffected by this fix) must still |
| +// resolve the same one-label wildcard host — the policy split is HostPolicy |
| +// vs. serving, not a routing regression. |
| +func TestHostPolicyExcludesWildcardFallback(t *testing.T) { |
| + backend, _ := hostEchoBackend(t, "primary") |
| + reg := openReg(t) |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "woodstar-relay.bab.oncarlos.com", Kind: "service", |
| + Addr: backend.Listener.Addr().String(), Status: "ready", |
| + }); err != nil { |
| + t.Fatalf("upsert: %v", err) |
| + } |
| + upsertAlias(t, reg, "*.woodstar.app", "woodstar-relay.bab.oncarlos.com") |
| + |
| + e, err := New(reg, nil) |
| + if err != nil { |
| + t.Fatalf("New: %v", err) |
| + } |
| + |
| + // (a) Request routing is unchanged: the wildcard-alias host still |
| + // resolves at request time. |
| + if _, ok := e.lookupUpstream("paul.woodstar.app"); !ok { |
| + t.Fatalf("lookupUpstream(paul.woodstar.app) missed — routing must still resolve via the wildcard alias") |
| + } |
| + |
| + // (b) HostPolicy refuses ANY one-label host under the catch-all that |
| + // isn't an exact route/alias row of its own — only exact hosts are |
| + // certable via the per-host path. |
| + if e.Allowed("random.woodstar.app") { |
| + t.Fatalf("Allowed(random.woodstar.app) = true, want false — HostPolicy must not follow the wildcard fallback (unbounded per-host ACME orders from attacker-chosen SNI)") |
| + } |
| + if e.Allowed("paul.woodstar.app") { |
| + t.Fatalf("Allowed(paul.woodstar.app) = true, want false — same rule, no exact row for this host either") |
| + } |
| + |
| + // An EXACT alias row is still certable, same as any other routed host. |
| + upsertAlias(t, reg, "paul.woodstar.app", "woodstar-relay.bab.oncarlos.com") |
| + if err := e.Reload(); err != nil { |
| + t.Fatalf("reload: %v", err) |
| + } |
| + if !e.Allowed("paul.woodstar.app") { |
| + t.Fatalf("Allowed(paul.woodstar.app) = false, want true once it has its OWN exact alias row") |
| + } |
| + // The still-wildcard-only host stays refused. |
| + if e.Allowed("random.woodstar.app") { |
| + t.Fatalf("Allowed(random.woodstar.app) = true, want false — still no exact row for it") |
| + } |
| +} |
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index a64db5841..f173c8eed 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -543,24 +543,35 @@ func (e *Edge) Allowed(host string) bool { |
| if e.tunnels != nil && host == e.tunnels.Host() { |
| return true |
| } |
| - _, ok := e.lookupUpstream(host) |
| + // Deliberately the EXACT map only — not lookupUpstream's wildcard |
| + // fallback. A catch-all alias ("*.<domain>") is a bounded, registry-backed |
| + // name; autocert's per-host HostPolicy gates a NEW per-host ACME order, |
| + // which is unbounded (labels are attacker-chosen) and shared across every |
| + // tenant on the box (one ACME account, one rate-limit bucket). Coverage |
| + // for a one-label host under a catch-all comes from the WILDCARD cert |
| + // (wrapGetCertificate calls certFor before ever reaching autocert); this |
| + // exact check only decides per-host HTTP-01 fallback for ROUTED hosts — |
| + // widening it to the wildcard fallback would let a stranger's SNI |
| + // <anything>.<domain> drive a fresh order and exhaust the box's shared |
| + // ACME quota for every tenant on it. |
| + _, ok := e.current()[host] |
| return ok |
| } |
| -// lookupUpstream resolves host to its upstream entry — the single lookup |
| -// the serve path (and Allowed's autocert HostPolicy) uses. An exact match |
| -// (a real route, or a plain #151 alias landed in the same map) always wins; |
| -// on a miss, a one-label wildcard key folds in: "*.<parent>" is the shape |
| -// Task 3's reconcile pass derives from a claim's catch-all row, and it lands |
| -// in the SAME map as every other alias (up.alias = true, primary's route + |
| -// proxy verbatim) so this fallback resolves to the primary's own entry — the |
| -// identical map value the #151/#152 agreed rule requires, not a value that |
| -// merely behaves the same. A wildcard matches exactly one label, mirroring |
| -// the TLS cert (wildcard.go's certFor): host[i:] keeps the LEADING dot of |
| -// the FIRST label only, so a two-label host (a.b.woodstar.app) composes |
| -// "*.b.woodstar.app" and misses, and the apex itself (woodstar.app) composes |
| -// "*.app" — never its own "*.woodstar.app" — so it can never match its own |
| -// wildcard. |
| +// lookupUpstream resolves host to its upstream entry — the request-routing |
| +// lookup ServeHTTP uses (NOT Allowed's autocert HostPolicy — see the comment |
| +// there). An exact match (a real route, or a plain #151 alias landed in the |
| +// same map) always wins; on a miss, a one-label wildcard key folds in: |
| +// "*.<parent>" is the shape Task 3's reconcile pass derives from a claim's |
| +// catch-all row, and it lands in the SAME map as every other alias |
| +// (up.alias = true, primary's route + proxy verbatim) so this fallback |
| +// resolves to the primary's own entry — the identical map value the |
| +// #151/#152 agreed rule requires, not a value that merely behaves the same. |
| +// A wildcard matches exactly one label, mirroring the TLS cert: host[i:] |
| +// keeps the LEADING dot of the FIRST label only, so a two-label host |
| +// (a.b.woodstar.app) composes "*.b.woodstar.app" and misses, and the apex |
| +// itself (woodstar.app) composes "*.app" — never its own "*.woodstar.app" — |
| +// so it can never match its own wildcard. |
| func (e *Edge) lookupUpstream(host string) (upstream, bool) { |
| m := e.current() |
| if up, ok := m[host]; ok { |
diff --git a/internal/edge/wildcard.go b/internal/edge/wildcard.go| index dcb09200f..1d013f2b6 100644 |
| --- a/internal/edge/wildcard.go |
| +++ b/internal/edge/wildcard.go |
| @@ -625,20 +625,35 @@ func newWildcardFromEnv(e *Edge, cache autocert.Cache) *wildcardManager { |
| } |
| // Delegated source: the claim-derivation pass (Task 3, internal/domains) |
| // writes wildcard_domains; refresh() reads it fresh on every call, same |
| - // as e.currentHosts above. A read error leaves the delegated set empty |
| - // for this pass only — refresh() runs again on the next reload or 12h |
| - // tick, so a transient registry hiccup never wedges delegation off for |
| - // good, it just costs one pass of visibility (logged below). |
| + // as e.currentHosts above. A read error returns the LAST successful |
| + // list, not nil/empty: refresh() only runs on startup and on OnReload |
| + // (run()'s 12h ticker never re-invokes the delegated source itself), so |
| + // on a quiet box a transient error would otherwise empty the delegated |
| + // set until some unrelated registry write next moves the fingerprint — |
| + // unbounded, and it would also stop parentsFor from including a parent |
| + // whose cert is already cached and valid (certFor iterates w.parents). |
| + // Caching here is what makes the log line's "unchanged this pass" |
| + // literally true. |
| + var ( |
| + mu sync.Mutex |
| + lastGood []string |
| + ) |
| delegated := func() []string { |
| wds, err := e.reg.ListWildcardDomains() |
| if err != nil { |
| + mu.Lock() |
| + out := lastGood |
| + mu.Unlock() |
| log.Printf("edge: wildcard domains read failed: %v — delegated set unchanged this pass", err) |
| - return nil |
| + return out |
| } |
| out := make([]string, 0, len(wds)) |
| for _, w := range wds { |
| out = append(out, w.Host) |
| } |
| + mu.Lock() |
| + lastGood = out |
| + mu.Unlock() |
| return out |
| } |
| return newWildcardManager(static, appsDomain, e.currentHosts, delegated, zone, cache, dns) |
diff --git a/internal/edge/wildcard_test.go b/internal/edge/wildcard_test.go| index 9b8baeb64..eead55f42 100644 |
| --- a/internal/edge/wildcard_test.go |
| +++ b/internal/edge/wildcard_test.go |
| @@ -585,6 +585,23 @@ func TestDelegatedSourceReadsRegistry(t *testing.T) { |
| t.Errorf("isDelegated(%s) = false, want true", host) |
| } |
| } |
| + |
| + // IMPORTANT-1 (review fix): a read error after a successful read must |
| + // return the LAST-GOOD list, not empty it out — refresh() only runs at |
| + // startup and on OnReload (run()'s 12h ticker never re-invokes the |
| + // delegated source), so on a quiet box an emptied set would stay empty |
| + // until some unrelated write next moved the registry fingerprint, and |
| + // certFor would stop serving an already-cached, still-valid wildcard for |
| + // every delegated parent in the meantime. |
| + if err := reg.Close(); err != nil { |
| + t.Fatalf("close registry: %v", err) |
| + } |
| + got := w.delegated() |
| + want := []string{"example.net", "woodstar.app"} |
| + slices.Sort(got) |
| + if !slices.Equal(got, want) { |
| + t.Fatalf("delegated() after a read error = %v, want the last-good list %v", got, want) |
| + } |
| } |
| // TestRefreshDropsOverlongChallengeNames pins sub-change 2: a delegated host |
diff --git a/internal/registry/registry.go b/internal/registry/registry.go| index ade5e24a8..250ca1721 100644 |
| --- a/internal/registry/registry.go |
| +++ b/internal/registry/registry.go |
| @@ -1323,10 +1323,9 @@ func (r *Registry) DeleteWildcardDomain(host string) error { |
| // Fingerprint summarizes the routes, fleets, fleet_boxes, route_aliases and |
| // wildcard_domains tables over a fresh connection, for the edge's cheap |
| -// change poll. It is |
| -// content-accurate: it hashes the actual VALUES of the reload-relevant |
| -// columns, not a |
| -// length-weighted aggregate, so two DIFFERENT values of the SAME length |
| +// change poll. It is content-accurate: it hashes the actual VALUES of the |
| +// reload-relevant columns, not a length-weighted aggregate, so two |
| +// DIFFERENT values of the SAME length |
| // (channel canary/1 -> canary/2, a git short-sha version bump, a rotated |
| // sha256 token hash, a failover restamp box-a -> box-b) change the result |
| // exactly like any other write. `grants` stays deliberately excluded |