amadan
merge: edge cache follow-ups — the header class swept, a real Close race, and an honest deferral (#12)
edge cache-before-wake follow-ups: split Connection, honest purge-isolation comment, PurgeListener.Close hardening
4 files changed,
+176
−7
internal/edge/edge.go+5 −2internal/edge/edge_test.go+20 −0internal/edge/purge.go+75 −5internal/edge/purge_test.go+76 −0
diff --git a/internal/edge/edge.go b/internal/edge/edge.go| index ed9ec5a03..3e9f2f30e 100644 |
| --- a/internal/edge/edge.go |
| +++ b/internal/edge/edge.go |
| @@ -330,9 +330,12 @@ func (e *Edge) ServeHTTP(w http.ResponseWriter, r *http.Request) { |
| // isUpgradeRequest reports whether r is a protocol-upgrade request (e.g. a |
| // WebSocket handshake) per the Connection header's token list (RFC 7230 |
| // §6.7: comma-separated, case-insensitive — "Upgrade", "keep-alive, Upgrade", |
| -// etc. all qualify). |
| +// etc. all qualify). Connection is list-valued (RFC 9110 §5.3), so this reads |
| +// every line via joinHeader rather than r.Header.Get, which returns only the |
| +// first — the same class of bug joinHeader's doc comment describes for Vary |
| +// and Cache-Control, here on the request's Connection header instead. |
| func isUpgradeRequest(r *http.Request) bool { |
| - for _, token := range strings.Split(r.Header.Get("Connection"), ",") { |
| + for _, token := range strings.Split(joinHeader(r.Header, "Connection"), ",") { |
| if strings.EqualFold(strings.TrimSpace(token), "Upgrade") { |
| return true |
| } |
diff --git a/internal/edge/edge_test.go b/internal/edge/edge_test.go| index 492479c7e..08a1f2fd3 100644 |
| --- a/internal/edge/edge_test.go |
| +++ b/internal/edge/edge_test.go |
| @@ -1143,6 +1143,26 @@ func TestServeHTTPUpgradeRequestBypassesRecording(t *testing.T) { |
| e.ServeHTTP(rec, req) |
| } |
| +// TestIsUpgradeRequestDetectsConnectionSplitAcrossHeaderLines is the last of |
| +// the four Header.Get-on-a-list-valued-header bugs found on this branch |
| +// (see joinHeader's doc comment): Connection is legitimately list-valued |
| +// (RFC 9110 §5.3), and a client that sends "Connection: keep-alive" and |
| +// "Connection: Upgrade" as two separate header lines is just as much an |
| +// upgrade request as one that sends them comma-joined on one line — but |
| +// http.Header.Get only ever returns the first line, so isUpgradeRequest |
| +// would silently treat the split form as an ordinary request and let it |
| +// fall through to the cache/recordingResponseWriter path instead of the |
| +// untouched-passthrough branch. |
| +func TestIsUpgradeRequestDetectsConnectionSplitAcrossHeaderLines(t *testing.T) { |
| + req := httptest.NewRequest("GET", "http://ws.example.com/socket", nil) |
| + req.Header.Add("Connection", "keep-alive") |
| + req.Header.Add("Connection", "Upgrade") |
| + |
| + if !isUpgradeRequest(req) { |
| + t.Fatal("Connection split across two header lines must still be detected as an upgrade request") |
| + } |
| +} |
| + |
| // activatorFunc adapts a plain function to the Activator interface, for |
| // tests that want custom wake-counting behavior without a full fake type. |
| type activatorFunc func(w http.ResponseWriter, r *http.Request, rt registry.Route, proxy http.Handler) bool |
diff --git a/internal/edge/purge.go b/internal/edge/purge.go| index 2f931ef79..471876ac0 100644 |
| --- a/internal/edge/purge.go |
| +++ b/internal/edge/purge.go |
| @@ -35,9 +35,44 @@ type purgeRequest struct { |
| var listenMu sync.Mutex |
| // ListenPurge binds socketPath (removing any stale file left behind by a |
| -// crashed previous instance) and serves purge requests scoped to host — |
| -// host is fixed here, never taken from the request, so a compromised or |
| -// buggy instance can only ever purge its own cache entries. |
| +// crashed previous instance) and serves purge requests scoped to host. |
| +// |
| +// What that scoping actually guarantees: host is fixed here, at |
| +// construction, and never taken from the request — purgeRequest carries |
| +// only Path and Query, never a host — so no request body, however |
| +// malicious or malformed, can make this listener purge any host other than |
| +// the one it was built for. That closes a confused-deputy purge: an |
| +// attacker (or a bug) cannot use one instance's control channel to reach |
| +// into another host's cache entries THROUGH this listener. |
| +// |
| +// What it does NOT guarantee: that only the instance this listener was |
| +// created for can ever reach it. Every exec-backed instance on a box runs |
| +// as the SAME uid as the agent itself (they're its children, spawned by |
| +// plain exec.Command with no uid change — see backend_exec.go), and the |
| +// control socket path is deterministic (rt.Socket + ".control" — |
| +// backend_exec.go), so a compromised or buggy exec-backed sibling can |
| +// compute another host's control socket path and dial it directly, then |
| +// issue it a purge for ITS host (never a third host — the guarantee above |
| +// still holds inside that listener). The 0600 socket mode (see the umask |
| +// dance below) keeps other UNIX users on the box out; it does nothing |
| +// against a same-uid sibling, because same-uid processes can always open |
| +// each other's 0600 sockets. Worst case from that gap: a misbehaving |
| +// instance forces a sibling to re-fetch from ITS OWN backend on its next |
| +// request — bounded by the same TTL ceiling (DefaultCacheMaxTTL) that |
| +// already caps a lost purge, and it can never expose one host's cached |
| +// response body to another, since each listener only ever touches the one |
| +// host's cache keys it was built for. |
| +// |
| +// Unit-backed tenants (carlos-app@<host>.service, backend_unit.go) do NOT |
| +// share this gap, or any control socket at all, because they don't share a |
| +// uid with anything: DynamicUser=yes mints a fresh, unpredictable uid on |
| +// every unit start, with no SupplementaryGroups — never the agent's uid, |
| +// never another tenant's. That's the opposite problem from the one above: |
| +// a DynamicUser tenant can't reach a same-uid sibling's control socket |
| +// because it can't reach ANY control socket, including one built for |
| +// itself. Unit-backed tenants have no purge listener wired up yet as a |
| +// result (see backend_unit.go); their cached routes rely on the TTL |
| +// ceiling alone until that gets its own design. |
| func ListenPurge(socketPath, host string, cache *Cache) (*PurgeListener, error) { |
| if err := os.Remove(socketPath); err != nil && !errors.Is(err, os.ErrNotExist) { |
| return nil, err |
| @@ -76,10 +111,45 @@ func ListenPurge(socketPath, host string, cache *Cache) (*PurgeListener, error) |
| srv := &http.Server{Handler: mux} |
| p := &PurgeListener{ln: ln, srv: srv} |
| - go srv.Serve(ln) //nolint:errcheck // Close() below triggers the expected ErrServerClosed |
| + // Close (below) closes p.ln directly, not through srv — so the error |
| + // Serve returns here is whatever a closed-listener Accept produces, not |
| + // necessarily http.ErrServerClosed. Either way it's expected and unused. |
| + go srv.Serve(ln) //nolint:errcheck |
| return p, nil |
| } |
| +// Close stops accepting purge requests, removes the socket file, and tears |
| +// down any connection already accepted before this call — in that order, |
| +// and both steps matter independently: |
| +// |
| +// 1. p.ln.Close() first, deterministically unlinking the socket file and |
| +// ending Accept regardless of whether the Serve goroutine ListenPurge |
| +// started has ever been scheduled. p.srv.Close() alone can't be trusted |
| +// for this: net/http's Server only closes listeners its own Serve call |
| +// has already registered (in an internal map populated by Serve itself, |
| +// on entry — see trackListener in net/http/server.go), and a Close |
| +// arriving before that goroutine runs loses the race and finds nothing |
| +// registered — Serve then proceeds to Accept on a listener nobody asked |
| +// it to stop, and the socket file is never unlinked. Closing the raw |
| +// net.Listener sidesteps the tracking map entirely. |
| +// 2. p.srv.Close() second, to tear down connections ALREADY accepted |
| +// before step 1 — closing only the listener stops new Accepts but does |
| +// nothing to a connection already handed off to its own handler |
| +// goroutine, which keeps right on serving requests over a still-open |
| +// keep-alive connection. A client that pooled such a connection could |
| +// otherwise keep issuing purges indefinitely after Close returned. |
| +// Calling srv.Close() after p.ln is already closed is harmless: closing |
| +// an already-closed (or never-registered) listener a second time is a |
| +// no-op error internally, swallowed here, while every connection |
| +// srv independently tracks in its own connection map — unrelated to |
| +// listener registration — still gets closed as intended. |
| +// |
| +// The returned error is p.ln's — the unlink guarantee callers actually |
| +// depend on; srv.Close()'s error, if any, is dropped. Calling Close a |
| +// second time (nobody does today) returns "use of closed network |
| +// connection" from the second p.ln.Close(). |
| func (p *PurgeListener) Close() error { |
| - return p.srv.Close() |
| + err := p.ln.Close() |
| + p.srv.Close() |
| + return err |
| } |
diff --git a/internal/edge/purge_test.go b/internal/edge/purge_test.go| index 6d126fa2f..f3b39c6f3 100644 |
| --- a/internal/edge/purge_test.go |
| +++ b/internal/edge/purge_test.go |
| @@ -95,6 +95,82 @@ func TestListenPurgeSocketFileIsOwnerOnly(t *testing.T) { |
| } |
| } |
| +// Close, called immediately after ListenPurge returns (before the Serve |
| +// goroutine it started has necessarily run at all), must still unlink the |
| +// socket file. PurgeListener.Close used to delegate to http.Server.Close, |
| +// which only closes listeners its own Serve call has already registered in |
| +// its internal tracking map (net/http's trackListener, set inside Serve |
| +// itself) — a real race, since Serve runs in a goroutine ListenPurge doesn't |
| +// wait on. Losing that race silently no-ops Close: the goroutine's later |
| +// Serve(ln) call then Accepts on a listener nobody ever asked to stop, and |
| +// the socket file is never removed. A caller like UnitBackend.Start/Stop |
| +// that opens and closes a purge listener back-to-back around a single unit |
| +// start hits this every time, not just occasionally. |
| +func TestListenPurgeCloseImmediatelyAfterOpenRemovesSocketFile(t *testing.T) { |
| + dir := t.TempDir() |
| + sock := filepath.Join(dir, "instance.sock") |
| + pl, err := ListenPurge(sock, "a.example", NewCache(10, 1<<20)) |
| + if err != nil { |
| + t.Fatalf("ListenPurge: %v", err) |
| + } |
| + |
| + if err := pl.Close(); err != nil { |
| + t.Fatalf("Close: %v", err) |
| + } |
| + |
| + if _, err := os.Stat(sock); err == nil { |
| + t.Fatal("expected the socket file removed immediately after Close, even called before the Serve goroutine could have run") |
| + } |
| +} |
| + |
| +// Close must tear down connections already accepted before it runs, not |
| +// just stop new ones. Closing the raw listener alone (the fix for the |
| +// unlink race above) only stops future Accept calls — a connection the |
| +// server already handed off to its own handler goroutine keeps right on |
| +// serving requests, since nothing told that goroutine to stop. A client |
| +// whose transport pooled a keep-alive connection from a purge issued before |
| +// Close can therefore keep issuing purges after Close returns, defeating |
| +// the whole point of closing the listener. |
| +func TestListenPurgeCloseRefusesRequestsOnAlreadyEstablishedConnections(t *testing.T) { |
| + dir := t.TempDir() |
| + sock := filepath.Join(dir, "instance.sock") |
| + cache := NewCache(DefaultCacheMaxEntries, DefaultCacheMaxBodyBytes) |
| + cache.Put(CacheKey{Host: "a.example", Method: "GET", Path: "/event"}, "", &CachedResponse{ |
| + StatusCode: 200, Header: http.Header{}, Body: []byte("hi"), StoredAt: time.Now(), TTL: time.Minute, |
| + }) |
| + |
| + pl, err := ListenPurge(sock, "a.example", cache) |
| + if err != nil { |
| + t.Fatalf("ListenPurge: %v", err) |
| + } |
| + |
| + client := dialPurgeSocket(t, sock) |
| + body, _ := json.Marshal(map[string]string{"path": "/event", "query": ""}) |
| + |
| + // Establish (and, via keep-alive, pool) a connection with one successful |
| + // purge before Close. |
| + resp, err := client.Post("http://unix/purge", "application/json", bytes.NewReader(body)) |
| + if err != nil { |
| + t.Fatalf("first purge POST: %v", err) |
| + } |
| + resp.Body.Close() |
| + if resp.StatusCode != http.StatusOK { |
| + t.Fatalf("first purge status = %d, want 200", resp.StatusCode) |
| + } |
| + |
| + if err := pl.Close(); err != nil { |
| + t.Fatalf("Close: %v", err) |
| + } |
| + |
| + // The SAME client — free to reuse its pooled connection from the first |
| + // request — must not be able to complete a second purge after Close. |
| + resp2, err := client.Post("http://unix/purge", "application/json", bytes.NewReader(body)) |
| + if err == nil { |
| + resp2.Body.Close() |
| + t.Fatal("expected a purge over a connection established before Close to fail once Close has run, got success") |
| + } |
| +} |
| + |
| func TestListenPurgeRemovesStaleSocketFile(t *testing.T) { |
| dir := t.TempDir() |
| sock := filepath.Join(dir, "instance.sock") |