security: make the agentID trust model real (identity takeover, scope escape, revocation bypass, token leak)

CLAUDE.md

@@ -77,23 +77,40 @@ Each entry: the rule, why, and the symbol that embodies it today. Numbers live i
   connGate`; defaults in `config.go`).
 - **Per-agent identity is the trust model.** Each agent proves possession of a long-term
   Ed25519 key the gateway allowlists; its derived `agentID` (`agt_…`) is unforgeable,
-  individually **scoped** (ports/tunnels) and **revocable** — revocation evicts the live
-  session and makes the next connect fatal `ErrCodeRevoked`. Agents join via a single-use
-  enrollment ticket in the pairing code (`gateway/auth.go identityValidator`, `agentstore.go`,
-  `link/cred.go`; mechanics in `docs/agent/architecture.md`). A matching agentID
-  **supersedes** (reconnect), a distinct one is admitted **alongside**; supersede is
-  anti-flap dampened so an ID collision degrades to a slow contest, not a loop
-  (`actor.go admit`, `noteSupersede`).
+  individually **scoped** (ports/tunnels) and **revocable** — revocation *and* a scope
+  change both evict the live session, and the next connect is fatal `ErrCodeRevoked`. A
+  session captures its scope at admission, so without that eviction narrowing contains
+  nothing (`gateway.go SetAgentScope`). Agents join via a single-use enrollment ticket in
+  the pairing code (`gateway/auth.go identityValidator`, `agentstore.go`, `link/cred.go`;
+  mechanics in `docs/agent/architecture.md`). A matching agentID **supersedes**
+  (reconnect), a distinct one is admitted **alongside**; supersede is anti-flap dampened
+  so an ID collision degrades to a slow contest, not a loop (`actor.go admit`,
+  `noteSupersede`).
+- "Unforgeable" above is a claim about **width**, and therefore a security parameter, not
+  a display choice: `agentID` is the label the gateway keys supersede, scope, revocation,
+  and config on, so it must stay wide enough that no second key answering to a victim's
+  name can be searched for (`cred.go agentFingerprintBytes`, floored by
+  `TestAgentIDIsWideEnoughToBeUnforgeable`); `AgentStore.Enroll` refuses a colliding join
+  besides, so a found collision fails closed.
 - The legacy **shared token** survives only as a migration fallback, accepted while
   `Gateway.AcceptSharedToken` is on (default for now). A shared-token agent self-asserts its
   `agentID`, so the old residual risk — supersede or port-squat, recoverable only by rotating
   the token — persists **for that path only**; enrolled agents are protected by their key and
-  by revocation, and disabling `AcceptSharedToken` once every agent is enrolled closes it.
+  by revocation *because* the key-derived `agt_` namespace is **reserved**, so a self-asserted
+  ID can never name an enrolled one (`link/cred.go IsDerivedAgentID`, `gateway/auth.go
+  sharedTokenValidator`). Disabling `AcceptSharedToken` once every agent is enrolled closes it.
 - The IPC pipe ACL admits Administrators, SYSTEM, and the interactive user only
   (`ipc/server_windows.go pipeSecurity`).
 - Diagnostics bundles redact every secret, host, IP, and identity; peer IPs become
   stable sha256 pseudonyms (`app/tools.go`, leak-tested in `app/tools_test.go`).
-  Anything new that exports data must pass the same no-leak test style.
+  Anything new that exports data must pass the same no-leak test style — seeding
+  *every* channel it ships, since a leak test that leaves one empty passes vacuously.
+  Bundles **ship logs**, so redacting only the config is theatre: log text is scrubbed
+  of the config's own secrets (`tools.go logScrubber`).
+- Never log a secret; the scrubber is a net, not a licence (it only knows what config
+  holds). The pairing code embeds the gateway token, so it goes to the console, never
+  through `slog` — slog fans out to the rotating file, the GUI ring, and every bundle
+  (`gateway.go RunStarted`).
 - Fatal auth errors (`bad_token`, `agent_conflict`, `version`, `revoked`) stop the agent
   instead of retry-hammering the gateway (fatal classification in `agent.go isFatal`), and
   surface in the UI via `EngineFatal` on the tick.

app/tools.go

@@ -2,6 +2,7 @@
 
 import (
 	"archive/zip"
+	"bufio"
 	"crypto/sha256"
 	"encoding/json"
 	"fmt"
@@ -51,11 +52,13 @@
 	return fmt.Sprintf("Reachable: %s answered in %s — players can connect.", addr, time.Since(start).Round(time.Millisecond)), nil
 }
 
-// writeDiagnostics builds the support bundle: version, a fully redacted config,
-// a health summary, the recent in-memory log lines, the persisted stats (peer
-// IPs pseudonymized), and every on-disk log file (rotated + crash + wails).
-// Everything that could identify a host, network, or client is masked so the
-// bundle is safe to share.
+// writeDiagnostics builds the support bundle: version, a fully redacted config, a
+// health summary, the recent in-memory log lines, the persisted stats (peer IPs
+// pseudonymized), and every on-disk log file (rotated + crash + wails). Everything
+// that could identify a host, network, or client is masked so the bundle is safe to
+// share — logs included, via logScrubber: they are shipped, so redacting only the
+// config would leave the same secrets in cleartext one file over.
+// Leak-tested by TestWriteDiagnosticsNoLeaks, which seeds every channel here.
 func writeDiagnostics(path string, cfg *config.Config, configDir, health string, ring *logging.Ring) error {
 	f, err := os.Create(path)
 	if err != nil {
@@ -93,11 +96,17 @@
 		}
 	}
 
+	// Logs ship with the config's own secrets masked. Without this the redaction
+	// above is theatre: config.redacted.toml hides Gateway.Token while a log line
+	// three files over spells it out.
+	scrub := newLogScrubber(cfg)
+
 	// logs-recent.txt — the GUI ring (what the user was just looking at).
 	if ring != nil {
 		if w, err := zw.Create("logs-recent.txt"); err == nil {
 			for _, e := range ring.EntriesSince(0) {
-				fmt.Fprintf(w, "%s %-5s %s %s\n", time.UnixMilli(e.TimeMs).Format(time.RFC3339), e.Level, e.Msg, e.Attrs)
+				line := fmt.Sprintf("%s %-5s %s %s\n", time.UnixMilli(e.TimeMs).Format(time.RFC3339), e.Level, e.Msg, e.Attrs)
+				io.WriteString(w, scrub.clean(line))
 			}
 		}
 	}
@@ -114,21 +123,86 @@
 		filepath.Join(logDir, "wails.log"),
 	}
 	for _, p := range logFiles {
-		copyIntoZip(zw, filepath.Base(p), p)
+		copyScrubbedIntoZip(zw, filepath.Base(p), p, scrub)
 	}
 	return nil
 }
 
-// copyIntoZip streams src into the archive under nameInZip; missing files are
-// skipped without error (a diagnostics bundle is best-effort).
-func copyIntoZip(zw *zip.Writer, nameInZip, src string) {
+// logScrubber replaces the exact secret values this bundle already knows — the ones
+// redactConfig masks in config.redacted.toml — wherever they appear in shipped log
+// text. It is the reason the bundle can claim to be shareable at all: redacting the
+// config is worth nothing if the same token is sitting in a log line two files over,
+// which is exactly how the pairing code used to escape (gateway.go RunStarted).
+//
+// Exact-value replacement, never pattern-guessing: every entry is a literal read out
+// of the live config, so this cannot pass off a guess as a guarantee. It is a net,
+// not a licence — secrets still must not be logged in the first place, because this
+// only knows the values config holds. Values shorter than minScrubLen are skipped:
+// they carry little entropy and would smear over unrelated log text.
+type logScrubber struct{ pairs []string } // old1, new1, old2, new2 … for strings.NewReplacer
+
+// minScrubLen is the shortest value worth exact-matching in log text. Below this a
+// "secret" is more likely to collide with ordinary words than to be the secret.
+const minScrubLen = 4
+
+func newLogScrubber(cfg *config.Config) *logScrubber {
+	const secret = "[redacted]"
+	const host = "[redacted-host]"
+	s := &logScrubber{}
+	add := func(val, with string) {
+		if len(val) >= minScrubLen {
+			s.pairs = append(s.pairs, val, with)
+		}
+	}
+	// Mirrors redactConfig field for field: whatever is a secret in the config is a
+	// secret in the logs. If a field is added there, add it here.
+	add(cfg.Gateway.Token, secret)
+	add(cfg.Agent.Token, secret)
+	add(cfg.Agent.AgentID, secret)
+	add(cfg.Agent.CertFingerprint, secret)
+	add(cfg.Gateway.PublicHost, host)
+	add(cfg.Gateway.BindAddr, host)
+	add(cfg.Agent.GatewayHost, host)
+	add(cfg.Metrics.PrometheusAddr, host)
+	for _, t := range cfg.Agent.Tunnels {
+		add(t.LocalAddr, host)
+	}
+	return s
+}
+
+// clean returns line with every known secret replaced.
+func (s *logScrubber) clean(line string) string {
+	if len(s.pairs) == 0 {
+		return line
+	}
+	return strings.NewReplacer(s.pairs...).Replace(line)
+}
+
+// copyScrubbedIntoZip streams src into the archive under nameInZip with every known
+// secret masked. Line-oriented, so memory stays bounded on a rotated log and a secret
+// (which never spans a newline) can't slip through a chunk boundary. Missing files
+// are skipped without error — a diagnostics bundle is best-effort.
+func copyScrubbedIntoZip(zw *zip.Writer, nameInZip, src string, s *logScrubber) {
 	lf, err := os.Open(src)
 	if err != nil {
 		return
 	}
 	defer lf.Close()
-	if w, err := zw.Create(nameInZip); err == nil {
-		io.Copy(w, lf)
+	w, err := zw.Create(nameInZip)
+	if err != nil {
+		return
+	}
+	br := bufio.NewReader(lf)
+	for {
+		// ReadString, not bufio.Scanner: a single pathological log line must not hit
+		// Scanner's 64 KiB token cap and silently truncate the rest of the file.
+		line, err := br.ReadString('\n')
+		if line != "" {
+			io.WriteString(w, s.clean(line))
+		}
+		if err != nil {
+			return
+		}
 	}
 }
 

app/tools_test.go

@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"proxyforward/internal/config"
+	"proxyforward/internal/logging"
 )
 
 func sampleConfig() *config.Config {
@@ -71,15 +72,51 @@ func TestRedactStatsJSONHashesPeerIPs(t *testing.T) {
 	}
 }
 
+// TestWriteDiagnosticsNoLeaks drives every channel writeDiagnostics ships, because a
+// bundle is only as shareable as its leakiest file. It previously passed ring=nil
+// into an empty dir, so the two *unredacted* channels — the GUI ring and the on-disk
+// log files — were never populated and the secret sweep below ran over an empty set.
+// That blind spot is how a logged pairing code (which embeds Gateway.Token verbatim)
+// shipped in cleartext while config.redacted.toml sat right beside it masking the
+// same token. The logger here is the real production fan-out, so the ring and the
+// rotating file are filled the way they are in a live process.
 func TestWriteDiagnosticsNoLeaks(t *testing.T) {
 	dir := t.TempDir()
 	// Seed a stats.json with a client IP that must not leak.
 	os.WriteFile(filepath.Join(dir, "stats.json"),
 		[]byte(`{"v":3,"peers":[{"ip":"203.0.113.77","totalConns":2}]}`), 0o600)
 
-	out := filepath.Join(dir, "diag.zip")
 	cfg := sampleConfig()
-	if err := writeDiagnostics(out, cfg, dir, "health: good\n", nil); err != nil {
+
+	// Fill the ring and the on-disk log through the real logger, with lines that
+	// carry the secrets the way a careless call site would.
+	ring := logging.NewRing(64)
+	logger, closeLog, err := logging.New(logging.Options{
+		FilePath: logging.DefaultFilePath(dir),
+		Ring:     ring,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	logger.Info("pairing code", "code",
+		"pxf://"+cfg.Gateway.PublicHost+":8474/v1/pair/"+cfg.Gateway.Token+"#"+cfg.Agent.CertFingerprint)
+	logger.Info("agent connected", "agentId", cfg.Agent.AgentID, "gateway", cfg.Agent.GatewayHost)
+	logger.Warn("dial failed", "local", cfg.Agent.Tunnels[0].LocalAddr, "bind", cfg.Gateway.BindAddr)
+	if err := closeLog(); err != nil {
+		t.Fatal(err)
+	}
+	// Guard against a vacuous pass: the on-disk log must really hold the secret, or
+	// this test proves nothing about the path that ships it.
+	onDisk, err := os.ReadFile(logging.DefaultFilePath(dir))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !strings.Contains(string(onDisk), cfg.Gateway.Token) {
+		t.Fatal("setup bug: the seeded log does not contain the token, so this test would pass vacuously")
+	}
+
+	out := filepath.Join(dir, "diag.zip")
+	if err := writeDiagnostics(out, cfg, dir, "health: good\n", ring); err != nil {
 		t.Fatal(err)
 	}
 
@@ -100,13 +137,27 @@ func TestWriteDiagnosticsNoLeaks(t *testing.T) {
 		names[f.Name] = string(b)
 	}
 
-	for _, want := range []string{"version.txt", "health.txt", "config.redacted.toml", "stats.redacted.json"} {
+	// The log channels must actually be in the bundle — if they silently stopped
+	// shipping, the sweep below would pass for the wrong reason.
+	for _, want := range []string{"version.txt", "health.txt", "config.redacted.toml", "stats.redacted.json", "logs-recent.txt", "proxyforward.log"} {
 		if _, ok := names[want]; !ok {
 			t.Errorf("bundle missing %s (have %v)", want, keys(names))
 		}
 	}
+	// Both log files must still be diagnostically useful after scrubbing — masking by
+	// shipping nothing would pass the sweep and defeat the point of the bundle.
+	for _, name := range []string{"logs-recent.txt", "proxyforward.log"} {
+		if !strings.Contains(names[name], "pairing code") || !strings.Contains(names[name], "dial failed") {
+			t.Errorf("%s lost its log lines to scrubbing:\n%s", name, names[name])
+		}
+	}
+
 	all := strings.Join(values(names), "\n")
-	for _, secret := range []string{"AGENTTOKENSECRET", "GWTOKENSECRET", "gw.secret.example.com", "203.0.113.77", "deadbeefsecret"} {
+	for _, secret := range []string{
+		"AGENTTOKENSECRET", "GWTOKENSECRET", "AGENTIDSECRET", "deadbeefsecret",
+		"gw.secret.example.com", "public.secret.example.com", "192.168.50.1",
+		"10.9.8.7", "127.0.0.99", "203.0.113.77",
+	} {
 		if strings.Contains(all, secret) {
 			t.Errorf("diagnostics bundle leaked %q", secret)
 		}

internal/e2e/e2e_test.go

@@ -1324,6 +1324,46 @@ func TestEnrollAndRevoke(t *testing.T) {
 	h.agentStop = nil // we already drained agentDone; keep cleanup from blocking
 }
 
+// TestSetAgentScopeEvictsLiveSession: narrowing an agent's scope must bind now, not
+// eventually. A session captures identity.Scope at admission and validateSpec reads
+// that copy for the session's whole life, so writing the store alone left an agent
+// the operator had just narrowed serving its old grant — holding the very ports the
+// operator was taking away — until it happened to reconnect, which a misbehaving
+// agent has no reason to do. Narrowing is the urgent case: it is how you contain an
+// agent. RevokeAgent already evicts to make its guarantee land; this is the same
+// guarantee for scope. (identity)
+func TestSetAgentScopeEvictsLiveSession(t *testing.T) {
+	echoAddr, closeEcho := echoServer(t)
+	defer closeEcho()
+	h := newHarnessWith(t, echoAddr, harnessOpts{enroll: true})
+	addr := h.waitPublicPort()
+
+	// The agent is enrolled and serving on its granted (unrestricted) scope.
+	agents := h.gw.ListAgents()
+	if len(agents) != 1 {
+		t.Fatalf("want 1 enrolled agent, got %d: %+v", len(agents), agents)
+	}
+	rec := agents[0]
+	roundTrip(t, addr, []byte("in scope"))
+
+	// Narrow the grant so the tunnel it is currently serving is no longer allowed.
+	if !h.gw.SetAgentScope(rec.AgentID, gateway.Scope{TunnelIDs: []string{"tnl_something-else"}}) {
+		t.Fatal("SetAgentScope reported the agent was not found")
+	}
+
+	// The listener must go away on its own. The agent stays up and reconnects, but
+	// every bind is now re-checked against the narrowed scope, so it never comes
+	// back — without the eviction the old session keeps serving here indefinitely.
+	deadline := time.Now().Add(20 * time.Second)
+	for time.Now().Before(deadline) {
+		if _, ok := h.gw.TunnelPort(h.tunnelID); !ok {
+			return
+		}
+		time.Sleep(20 * time.Millisecond)
+	}
+	t.Fatal("narrowed scope never took effect: the tunnel is still bound, so the agent kept the port the operator took away")
+}
+
 // TestBurstThroughputAndCrossStreamLatency pushes 64 MiB through one
 // connection while a second connection does small echo round-trips; the
 // burst must move fast and must not starve the small stream. This is the

internal/gateway/agentstore.go

@@ -32,6 +32,11 @@ var (
 	ErrTicketUnknown  = errors.New("pairing code not recognized (it may have been rotated)")
 	ErrTicketConsumed = errors.New("this pairing code was already used — ask for a fresh one")
 	ErrTicketExpired  = errors.New("this pairing code has expired — ask for a fresh one")
+	// ErrAgentIDCollision fires when a joining key derives an agentID another key
+	// already holds. Every management op below resolves an agent by that label, so
+	// two keys sharing one would make revocation, scope, and config land on an
+	// arbitrary record. Refusing the join keeps the label a true primary key.
+	ErrAgentIDCollision = errors.New("this agent's derived id collides with an enrolled agent — regenerate its identity key and re-pair")
 )
 
 // Scope restricts which public ports and tunnel IDs an agent may bind. An empty
@@ -181,6 +186,17 @@ func (s *AgentStore) Enroll(pubKey []byte, agentID, ticket string, now time.Time
 	}
 
 	key := hex.EncodeToString(pubKey)
+	// agentID derives from pubKey, so a clash here means a *different* key hashed to
+	// the same label — the astronomical accident, or someone who went looking for one.
+	// Fail the join rather than let two keys answer to one name: every management op
+	// below (and the actor's live-session map) resolves an agent by that label and
+	// would otherwise land on whichever record map iteration happened to yield.
+	// Re-enrolling the *same* key is a reconnect, not a clash.
+	for k, r := range s.agents {
+		if r.AgentID == agentID && k != key {
+			return AgentRecord{}, ErrAgentIDCollision
+		}
+	}
 	rec := AgentRecord{
 		AgentID:  agentID,
 		PubKey:   append([]byte(nil), pubKey...),