carlos / platform Public

provision,edge: final review — Addrs elements are IP literals (both-family link-local refused), 16-upstream cap, alias-inherits-failover pin

Three findings from the whole-branch review, all door- or test-side; no edge
or prober logic moves.

1. The 169.254/16 rail was bypassable by name. validAddr only ran its
   link-local check on a parsed IP, so an element like
   "md.attacker.example:80" resolving to the metadata service skipped it
   entirely — the exact SSRF the rail exists to stop — and the To4() gate let
   IPv6 link-local ([fe80::1]:80) through besides. Elements of the NEW Addrs
   field must now be IP LITERALS, with link-local refused in both families
   (unicast and multicast). The platform dials these addresses itself, so a
   name is not validatable at the door at all: whatever it resolved to when
   the record was written is free to move before the edge connects.

   Scoped to Addrs by the wire-compat rule: no record in any bucket carries
   Addrs yet, while legacy Addr records naming a hostname do exist, so Addr
   keeps exactly the checks it shipped with. The asymmetry is pinned.

2. The upstream set had no upper bound. provision.MaxUpstreams (16) is
   refused at the same door: the prober walks a host's upstreams sequentially
   per tick, and every scale figure in the design is per-handful.

3. The #152 alias-inheritance pin ran on a legacy single-addr route, which
   cannot show the claim it is named for. Added the multiProxy variant: the
   alias's entry is pointer-identical to the primary's probed handler (a
   rebuilt one would read an unprobed live set and 503 a healthy route), the
   probe table holds only the primary, and a request over the attached
   hostname reaches a live upstream.

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

Paul Campbell 2 months ago a0fc0cde773fd08ce07442af0f61705471a4f419 parent bcde245

3 files changed, +199 −4

internal/edge/alias_test.go +72 −0
internal/provision/provision.go +58 −4
internal/provision/provision_test.go +69 −0

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

index 0951bc54f..c74d31ac5 100644

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

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

@@ -1,6 +1,7 @@

package edge

import (

+ "maps"

"net/http"

"net/http/httptest"

"slices"

@@ -102,6 +103,77 @@ func TestAliasSharesUpstreamEntry(t *testing.T) {

}

+// TestAliasSharesMultiUpstreamEntry is TestAliasSharesUpstreamEntry against

+// the shape the #152 claim is actually about: a primary carrying an Addrs set

+// and a probe config, i.e. one served by multiProxy rather than the legacy

+// single-addr tcpProxy. Two things must hold, and only this shape can show

+// either.

+//

+// The alias must ride the primary's OWN multiProxy value. That handler is

+// closed over the PRIMARY's host and reads its live set from proberSet under

+// that key, so an alias handed a rebuilt proxy would look up a host nothing

+// probes, find an empty live set, and answer 503 + Retry-After for a route

+// whose upstreams are all healthy.

+//

+// And the alias must add NO prober of its own: probeSpecs is built from the

+// route pass only, before aliases are folded in, so an alias appearing there

+// would double every upstream's probe traffic and put a duplicate host on the

+// dashboard's per-upstream table.

+func TestAliasSharesMultiUpstreamEntry(t *testing.T) {

+ addrA, _ := countingBackend(t, "A")

+ addrB, _ := countingBackend(t, "B")

+ reg := openReg(t)

+ if err := reg.Upsert(registry.Route{

+ Host: "ha.acct.oncarlos.com", Kind: "service", Status: "ready",

+ Addrs: addrA + "," + addrB,

+ Health: healthJSON(t, frozenHealth()),

+ }); err != nil {

+ t.Fatalf("upsert: %v", err)

+ }

+ upsertAlias(t, reg, "www.customer.example", "ha.acct.oncarlos.com")

+ e, err := New(reg, nil)

+ if err != nil {

+ t.Fatalf("New: %v", err)

+ }

+ defer e.probers.Stop()

+ m := e.current()

+ primary, ok := m["ha.acct.oncarlos.com"]

+ if !ok {

+ t.Fatalf("primary route missing from the upstream table")

+ }

+ alias, ok := m["www.customer.example"]

+ if !ok {

+ t.Fatalf("alias missing from the upstream table")

+ }

+ // Identity, as in the single-addr pin above: no Version on the route, so

+ // the handler is the *httputil.ReverseProxy multiProxy returned and ==

+ // compares that pointer.

+ if alias.proxy != primary.proxy {

+ t.Fatalf("alias proxy %p is not the primary's multiProxy %p — a rebuilt handler would read an unprobed live set and 503 a healthy route", alias.proxy, primary.proxy)

+ }

+ if alias.route.Host != "ha.acct.oncarlos.com" {

+ t.Fatalf("alias route.Host = %q, want the PRIMARY's host", alias.route.Host)

+ }

+ // Exactly one probed host, and it is the primary.

+ st := e.probers.State()

+ if len(st) != 1 {

+ t.Fatalf("probed hosts = %v, want exactly one (the primary) — an alias must never get a prober of its own", slices.Sorted(maps.Keys(st)))

+ }

+ if _, ok := st["ha.acct.oncarlos.com"]; !ok {

+ t.Fatalf("probed hosts = %v, want the primary probed", slices.Sorted(maps.Keys(st)))

+ }

+ // And the whole point of the identity: a request over the attached

+ // hostname reaches a live upstream instead of multiProxy's dark-set 503.

+ srv := httptest.NewServer(e)

+ defer srv.Close()

+ code, body := getHost(t, srv.URL, "www.customer.example")

+ if code != http.StatusOK || (body != "A" && body != "B") {

+ t.Fatalf("alias request = %d %q, want 200 from one of the primary's upstreams", code, body)

+ }

// TestAliasAllowed covers the autocert HostPolicy: an attached hostname is a

// host this box serves, so it may be certed; a stranger's SNI may not.

func TestAliasAllowed(t *testing.T) {

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

index a3cf664ae..0081145c1 100644

--- a/internal/provision/provision.go

+++ b/internal/provision/provision.go

@@ -323,6 +323,15 @@ func ValidKind(kind string) bool {

return false

}

+// MaxUpstreams caps the Addrs set one record may name. The prober probes a

+// host's upstreams SEQUENTIALLY once per interval (internal/edge/prober.go),

+// and every scale figure the multi-upstream design rests on — probe traffic,

+// per-tick wall time, the dashboard's per-upstream table — was drawn for a

+// handful of backends behind one route, not hundreds. The cap is stated at

+// the door so an oversized set is refused where it is typed, rather than

+// discovered as a prober tick that no longer fits inside its own interval.

+const MaxUpstreams = 16

// ValidateShape checks the §3 fields at the door, before a record is written

// or materialized. The rails live at the door, exactly as they do for

// registry.SetCaps and SetChannel — Upsert validates no column at all, so a

@@ -340,10 +349,13 @@ func ValidateShape(rec Record) error {

if rec.Addr != "" && len(rec.Addrs) > 0 {

return errors.New("addr and addrs both set: a record picks one upstream shape — the single legacy addr or the addrs set — drop one of the two")

}

+ if len(rec.Addrs) > MaxUpstreams {

+ return fmt.Errorf("addrs: %d upstreams, at most %d: the prober walks a host's upstreams one after another every tick, and the whole design is sized for a handful of backends per route, not hundreds — split the fleet across routes instead", len(rec.Addrs), MaxUpstreams)

+ }

if len(rec.Addrs) > 0 {

seen := make(map[string]bool, len(rec.Addrs))

for _, a := range rec.Addrs {

- if err := validAddr(a); err != nil {

+ if err := validAddrsElement(a); err != nil {

return err

}

if seen[a] {

@@ -386,9 +398,10 @@ func ValidateShape(rec Record) error {

}

// validAddr checks one upstream's shape: host:port, with the port an

-// integer in 1..65535. Factored out of the single-Addr check so Addrs runs

-// the identical rail per element — one bad element among many upstreams is

-// refused just as loudly as a solitary bad Addr always was.

+// integer in 1..65535. This is the LEGACY single Addr's rail, unchanged:

+// records carrying it exist in production buckets today, some of them naming

+// their backend by hostname, so nothing here may grow stricter. The new Addrs

+// set gets the stricter rail instead — see validAddrsElement.

func validAddr(a string) error {

h, p, err := net.SplitHostPort(a)

if err != nil || h == "" || p == "" {

@@ -422,6 +435,47 @@ func validAddr(a string) error {

return nil

}

+// validAddrsElement is validAddr plus the two rails the NEW Addrs set carries

+// and the legacy Addr cannot (wire-compat: no record in any bucket carries

+// Addrs yet, so this field's rules can be written the way they should have

+// been all along, while Addr keeps exactly the checks it shipped with).

+//

+// 1. The host must be an IP LITERAL. validAddr's link-local refusal only

+// looks at a parsed IP, so a NAME skips it entirely and

+// "md.attacker.example:80" resolving to 169.254.169.254 walks straight

+// through the very rail that exists to stop it. A name cannot be

+// validated at the door at all, in fact: the platform dials these

+// upstreams itself, and whatever a name resolved to when the record was

+// written is not what it must resolve to when the edge connects — the

+// target is free to move after validation. An address that is checked

+// here and dialled there has to be the same address.

+// 2. Link-local is refused in BOTH families, unicast and multicast:

+// 169.254.0.0/16 AND fe80::/10 (plus ff02::/16), because a box reaches

+// its metadata service over IPv6 as readily as IPv4 and "[fe80::1]:80"

+// is the same SSRF wearing the other family's clothes.

+//

+// Everything else validAddr allows stays allowed here — loopback and RFC1918

+// are where every legitimate addr-backed route on the fleet points today

+// (127.0.0.1:8090 consoles, private-subnet tenants). This is the metadata

+// rail closing its own holes, not a widening of it.

+func validAddrsElement(a string) error {

+ if err := validAddr(a); err != nil {

+ return err

+ }

+ h, _, err := net.SplitHostPort(a)

+ if err != nil {

+ return fmt.Errorf("addr %q: want a TCP upstream as host:port (e.g. 127.0.0.1:8090)", a)

+ }

+ ip := net.ParseIP(h)

+ if ip == nil {

+ return fmt.Errorf("addrs: upstream %q names a host, not an IP: the platform dials these addresses itself, and a name can resolve somewhere else after this check passes — state the backend's IP address as ip:port (e.g. 10.0.0.1:3000)", a)

+ }

+ if ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() {

+ return fmt.Errorf("addrs: upstream %q is link-local (169.254.0.0/16 and fe80::/10, where the cloud metadata service lives) — an upstream must be a real backend address", a)

+ }

+ return nil

// ValidRecordHost applies the host rail this record actually needs (design

// §3.2). ValidHost is path-traversal defence: it is what keeps a hostile

// host from reaching the <host>.sock and <host>.db path segments as anything

diff --git a/internal/provision/provision_test.go b/internal/provision/provision_test.go

index a8213852c..8a3c6942d 100644

--- a/internal/provision/provision_test.go

+++ b/internal/provision/provision_test.go

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

"context"

"encoding/json"

"errors"

+ "fmt"

"os"

"strings"

"testing"

@@ -728,6 +729,74 @@ func TestValidateShapeRefusesLinkLocalUpstream(t *testing.T) {

}

+// TestAddrsElementsMustBeIPLiterals closes the hole the link-local rail had

+// while it only ran on a parsed IP: a NAME skipped the check entirely, so

+// "md.attacker.example:80" resolving to the metadata service walked through

+// the door the rail exists to guard. The new Addrs set therefore takes IP

+// literals only — the platform dials these addresses itself, and a name is

+// free to resolve somewhere else after the door said yes.

+//

+// The rule is scoped to Addrs BY DESIGN (wire-compat): no record in any

+// bucket carries Addrs yet, while legacy Addr records naming a hostname do

+// exist, and those must keep validating. That asymmetry is pinned below.

+func TestAddrsElementsMustBeIPLiterals(t *testing.T) {

+ svc := func(addrs ...string) Record {

+ return Record{Host: "x", Kind: "service", Addrs: addrs}

+ }

+ for _, addr := range []string{"md.attacker.example:80", "backend.internal:8080", "localhost:8090"} {

+ if err := ValidateShape(svc("10.0.0.1:3000", addr)); err == nil {

+ t.Errorf("addrs element %s accepted, want it refused as a name", addr)

+ } else if !strings.Contains(err.Error(), "names a host") {

+ t.Errorf("refusal for %s = %v, want it to say the element names a host", addr, err)

+ }

+ // Link-local in the OTHER family: the same SSRF, IPv6 clothes. v4

+ // link-local is covered by TestValidateShapeRefusesLinkLocalUpstream.

+ for _, addr := range []string{"[fe80::1]:80", "[fe80::a00:27ff:fe4e:66a1]:8080", "[ff02::1]:80"} {

+ if err := ValidateShape(svc(addr)); err == nil {

+ t.Errorf("addrs element %s accepted, want it refused as link-local", addr)

+ } else if !strings.Contains(err.Error(), "link-local") {

+ t.Errorf("refusal for %s = %v, want it to say link-local", addr, err)

+ }

+ // Loopback and RFC1918 literals stay allowed in the Addrs spelling too —

+ // the rail closed the name hole, it did not narrow what an upstream may be.

+ for _, addr := range []string{"127.0.0.1:8090", "10.0.0.1:3000", "192.168.1.5:8080", "172.16.0.9:443", "[::1]:8090"} {

+ if err := ValidateShape(svc(addr)); err != nil {

+ t.Errorf("addrs element %s refused (%v), want it allowed", addr, err)

+ }

+ // The wire-compat half, stated explicitly: a legacy single Addr naming a

+ // hostname keeps validating. Records like this are in production buckets;

+ // tightening this line stops them materializing.

+ if err := ValidateShape(Record{Host: "x", Kind: "service", Addr: "backend.internal:8080"}); err != nil {

+ t.Errorf("legacy addr with a hostname refused (%v) — records in live buckets carry that shape", err)

+ }

+// TestValidateShapeCapsUpstreams: the prober walks a host's upstreams

+// sequentially per tick and the design is sized for a handful of backends per

+// route, so the set has an upper bound at the door.

+func TestValidateShapeCapsUpstreams(t *testing.T) {

+ addrs := func(n int) []string {

+ out := make([]string, n)

+ for i := range out {

+ out[i] = fmt.Sprintf("10.0.%d.%d:3000", i/250, i%250+1)

+ }

+ return out

+ }

+ if err := ValidateShape(Record{Host: "x", Kind: "service", Addrs: addrs(MaxUpstreams)}); err != nil {

+ t.Errorf("%d upstreams refused (%v), want the cap itself allowed", MaxUpstreams, err)

+ }

+ err := ValidateShape(Record{Host: "x", Kind: "service", Addrs: addrs(MaxUpstreams + 1)})

+ if err == nil {

+ t.Fatalf("%d upstreams accepted, want a refusal", MaxUpstreams+1)

+ }

+ if !strings.Contains(err.Error(), fmt.Sprint(MaxUpstreams)) {

+ t.Errorf("refusal = %v, want it to name the limit (%d)", err, MaxUpstreams)

+ }

func TestLegacyRecordStillParses(t *testing.T) {

// A record written before addrs/health existed — byte-for-byte old wire shape.

raw := []byte(`{"schema":1,"host":"a.example.com","slug":"a","app":"web","account":"bab","channel":"stable","created_at":"2026-08-01T00:00:00Z","kind":"service","addr":"10.0.0.1:3000"}`)