diff --git a/CLAUDE.md b/CLAUDE.md index d06b113..d09eb86 100644 --- a/CLAUDE.md +++ b/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. diff --git a/app/tools.go b/app/tools.go index 1553672..0d57311 100644 --- a/app/tools.go +++ b/app/tools.go @@ -2,6 +2,7 @@ package app import ( "archive/zip" + "bufio" "crypto/sha256" "encoding/json" "fmt" @@ -51,11 +52,13 @@ func testReachability(host string, port int) (string, error) { 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 @@ func writeDiagnostics(path string, cfg *config.Config, configDir, health string, } } + // 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 @@ func writeDiagnostics(path string, cfg *config.Config, configDir, health string, 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 + } } } diff --git a/app/tools_test.go b/app/tools_test.go index f15d3e2..37d0401 100644 --- a/app/tools_test.go +++ b/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) } diff --git a/internal/e2e/e2e_test.go b/internal/e2e/e2e_test.go index a1fb751..171f0b5 100644 --- a/internal/e2e/e2e_test.go +++ b/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 diff --git a/internal/gateway/agentstore.go b/internal/gateway/agentstore.go index 62c2fd4..e47b66d 100644 --- a/internal/gateway/agentstore.go +++ b/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...), diff --git a/internal/gateway/agentstore_test.go b/internal/gateway/agentstore_test.go index 6be4b53..2b75cb1 100644 --- a/internal/gateway/agentstore_test.go +++ b/internal/gateway/agentstore_test.go @@ -11,6 +11,49 @@ import ( func pub(b byte) []byte { return bytes.Repeat([]byte{b}, 32) } +// TestEnrollRejectsAgentIDCollision: two different keys may never share one agentID. +// The store is keyed by pubkey, but every management op below it — Revoke, SetScope, +// Rename, DesiredConfig, AdoptConfig — resolves an agent by scanning for the *label* +// and taking the first match, and the actor's live-session map is keyed by it too. So +// two records answering to one name make revocation and scope land on whichever +// record Go's map iteration happens to yield: an operator revokes an agent and the +// wrong one dies, nondeterministically. agentID derives from the key, so a clash is +// either an astronomical accident or someone who went looking for one; either way the +// join fails closed and the label stays a true primary key. (identity) +func TestEnrollRejectsAgentIDCollision(t *testing.T) { + s, err := LoadAgentStore(t.TempDir()) + if err != nil { + t.Fatal(err) + } + now := time.Now() + mkTicket := func() string { + tk, err := s.IssueEnrollment(false, time.Time{}, Scope{}) + if err != nil { + t.Fatal(err) + } + return tk + } + + if _, err := s.Enroll(pub(1), "agt_collide", mkTicket(), now); err != nil { + t.Fatalf("first enroll: %v", err) + } + // A different key deriving the same label must be refused. + if _, err := s.Enroll(pub(2), "agt_collide", mkTicket(), now); !errors.Is(err, ErrAgentIDCollision) { + t.Fatalf("colliding key: err = %v, want ErrAgentIDCollision", err) + } + // The incumbent is untouched: a collision attempt must not evict or rewrite it. + if rec, ok := s.Lookup(pub(1)); !ok || rec.AgentID != "agt_collide" { + t.Fatalf("incumbent record damaged by the collision attempt: %+v ok=%v", rec, ok) + } + if _, ok := s.Lookup(pub(2)); ok { + t.Fatal("the colliding key must not have been enrolled") + } + // Re-enrolling the same key under its own label is a reconnect, not a clash. + if _, err := s.Enroll(pub(1), "agt_collide", mkTicket(), now); err != nil { + t.Fatalf("re-enrolling the same key must succeed: %v", err) + } +} + // TestAgentStoreEnrollLookupRevoke: a single-use ticket enrolls one agent, is then // spent, and revoke flips the record. (identity) func TestAgentStoreEnrollLookupRevoke(t *testing.T) { diff --git a/internal/gateway/auth.go b/internal/gateway/auth.go index 77fd14f..0f61f32 100644 --- a/internal/gateway/auth.go +++ b/internal/gateway/auth.go @@ -32,11 +32,28 @@ var ( ErrBadToken = errors.New("bad token") ErrMissingAgentID = errors.New("missing agentId") ErrRevoked = errors.New("agent identity revoked") + // ErrReservedAgentID rejects a shared-token peer that names itself in the + // key-derived agt_ namespace. Only a verified key mints those, so the claim is + // always a lie — and an accepted one would hand over the named agent's session, + // ports, and scope. + ErrReservedAgentID = errors.New("agentId is reserved for enrolled identities — this agent must enroll with a pairing code") ) // sharedTokenValidator is the legacy authenticator: one token admits every agent, // told apart only by the self-asserted agentID. Retained as a migration fallback, // gated by Gateway.AcceptSharedToken. +// +// Because the agentID here is *asserted* rather than proved, it must stay out of the +// key-derived namespace: the gateway keys supersede, per-conn delivery, scope, and +// gateway-authoritative config on that label, so a token holder allowed to name +// itself agt_ would evict the real agent, inherit its ports, and — a +// shared-token identity carrying no Scope, and an empty Scope meaning unrestricted — +// escape the victim's grant entirely. Revoking the victim would not even help: the +// impersonator never presents the revoked key, so identityValidator's Revoked check +// is never reached. Rejecting the prefix is what keeps the documented promise that +// enrolled agents are protected by their key and by revocation; the residual risk of +// this path stays confined to peers that are legacy on both sides. +// Regression: TestSharedTokenCannotClaimDerivedAgentID. type sharedTokenValidator struct { token string } @@ -48,6 +65,9 @@ func (v sharedTokenValidator) Validate(hello *control.Hello, _ string) (Identity if hello.AgentID == "" { return Identity{}, ErrMissingAgentID } + if link.IsDerivedAgentID(hello.AgentID) { + return Identity{}, ErrReservedAgentID + } return Identity{AgentID: hello.AgentID}, nil } diff --git a/internal/gateway/auth_test.go b/internal/gateway/auth_test.go index 1c5b8ec..add3f3d 100644 --- a/internal/gateway/auth_test.go +++ b/internal/gateway/auth_test.go @@ -27,6 +27,8 @@ func TestSharedTokenValidator(t *testing.T) { {"wrong token", control.Hello{Token: "nope", AgentID: "agent-1"}, "", ErrBadToken}, {"empty token", control.Hello{Token: "", AgentID: "agent-1"}, "", ErrBadToken}, {"good token but empty agentID", control.Hello{Token: "s3cret", AgentID: ""}, "", ErrMissingAgentID}, + // The agt_ namespace belongs to proved keys; a token holder may not wear it. + {"good token but derived agentID", control.Hello{Token: "s3cret", AgentID: "agt_10524h9zg1n66yk1"}, "", ErrReservedAgentID}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -41,6 +43,64 @@ func TestSharedTokenValidator(t *testing.T) { } } +// TestSharedTokenCannotClaimDerivedAgentID is the regression for the shared-token +// identity takeover: the legacy path authenticates a gateway-wide bearer token and +// then believes whatever agentID the peer names. Since the gateway keys supersede, +// per-conn delivery, scope, and gateway-config on that label, a token holder allowed +// to name itself agt_ would evict the enrolled agent, inherit its ports, +// and — a shared-token identity carrying no Scope, and an empty Scope meaning +// unrestricted — escape the victim's grant. Revoking the victim would not help: the +// impersonator never presents the revoked key, so identityValidator's Revoked check +// is never reached. Reserving the agt_ namespace is what keeps the documented +// promise that enrolled agents are protected by their key and by revocation. (D7) +func TestSharedTokenCannotClaimDerivedAgentID(t *testing.T) { + const fp = "sha256:deadbeef" + vpub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + store, err := LoadAgentStore(t.TempDir()) + if err != nil { + t.Fatal(err) + } + ticket, err := store.IssueEnrollment(false, time.Time{}, Scope{}) + if err != nil { + t.Fatal(err) + } + rec, err := store.Enroll(vpub, link.AgentID(vpub), ticket, time.Now()) + if err != nil { + t.Fatal(err) + } + victimID := rec.AgentID + store.SetScope(victimID, Scope{Ports: []int{25565}, TunnelIDs: []string{"tnl_victim"}}) + + v := compositeValidator{ + identity: identityValidator{store: store, now: time.Now}, + shared: &sharedTokenValidator{token: "s3cret"}, + } + + // An attacker holding only the shared token, presenting no key at all. + if _, err := v.Validate(&control.Hello{Token: "s3cret", AgentID: victimID}, fp); !errors.Is(err, ErrReservedAgentID) { + t.Fatalf("shared-token peer claiming enrolled id %q: err = %v, want ErrReservedAgentID", victimID, err) + } + + // Same after the victim is revoked, so revocation cannot be walked around by + // dropping the key and falling back to the token. + if !store.Revoke(victimID) { + t.Fatal("revoke failed") + } + if _, err := v.Validate(&control.Hello{Token: "s3cret", AgentID: victimID}, fp); !errors.Is(err, ErrReservedAgentID) { + t.Fatalf("revoked identity %q re-admitted via shared token: err = %v", victimID, err) + } + + // A genuinely legacy agent (bare 32-hex id from config.NewID) still pairs, so + // the migration path this fallback exists for is untouched. + const legacyID = "9f86d081884c7d659a2feaa0c55ad015" + if id, err := v.Validate(&control.Hello{Token: "s3cret", AgentID: legacyID}, fp); err != nil || id.AgentID != legacyID { + t.Fatalf("legacy agent must still authenticate: id=%+v err=%v", id, err) + } +} + // enrollHello builds a per-identity hello: pubkey + proof-of-possession signature // bound to fp, optionally carrying an enrollment ticket. func enrollHello(t *testing.T, priv ed25519.PrivateKey, pub ed25519.PublicKey, fp, ticket string) control.Hello { diff --git a/internal/gateway/gateway.go b/internal/gateway/gateway.go index 40c7caf..641fcc6 100644 --- a/internal/gateway/gateway.go +++ b/internal/gateway/gateway.go @@ -142,8 +142,16 @@ func RunStarted(ctx context.Context, g *Gateway, cfg *config.Config, logger *slo Token: cfg.Gateway.Token, Fingerprint: g.Fingerprint(), } - logger.Info("gateway ready — pair agents with the code below (replace YOUR-PUBLIC-ADDRESS with this machine's public hostname or IP)") - logger.Info("pairing code", "code", code.String()) + // The pairing code embeds the shared gateway token, so it goes to the console and + // nowhere else. slog would fan it out to three places that outlive the moment: the + // rotating log file, the GUI ring, and — because app/tools.go ships both verbatim + // — every diagnostics bundle. Bundles are built to be handed to someone else, and + // redactConfig masking Gateway.Token there is worth nothing while the same token + // rides along in cleartext inside a logged pairing code. stdout is transient and + // unshipped; the GUI never needed this line (it mints codes over IPC — app.go + // PairingCode), and the CLI operator reads it off the console they're standing at. + logger.Info("gateway ready — pairing code printed on the console") + fmt.Printf("\npair agents with this code (replace YOUR-PUBLIC-ADDRESS with this machine's public hostname or IP):\n\n %s\n\n", code.String()) <-ctx.Done() g.Shutdown() return nil @@ -294,14 +302,40 @@ func (g *Gateway) RevokeAgent(agentID string) bool { return ok } -// RenameAgent sets an agent's display nickname; SetAgentScope replaces its bind -// scope. Both report whether the agent was found. +// RenameAgent sets an agent's display nickname. A nickname is display sugar with no +// bearing on what the agent may bind, so a live session needs no disturbing. Reports +// whether the agent was found. func (g *Gateway) RenameAgent(agentID, nickname string) bool { return g.agents != nil && g.agents.Rename(agentID, nickname) } +// SetAgentScope replaces an agent's bind scope and evicts any live session, so the +// new grant takes effect now rather than whenever the agent next reconnects. +// +// A session captures identity.Scope at admission (buildAndAdmit) and validateSpec +// reads that copy for the session's whole life, so writing the store alone would +// leave an agent an operator just narrowed running on its old grant indefinitely — +// keeping the ports it already holds — while the GUI showed the new scope. Narrowing +// is the urgent case precisely because it is how you contain a misbehaving agent. +// Evicting is how RevokeAgent already makes this guarantee: the agent reconnects, +// re-reads its record, and every bind is re-checked against the narrowed scope. It +// also sidesteps mutating sess.scope under readers on other goroutines. Reports +// whether the agent was found. +// Regression: e2e TestSetAgentScopeEvictsLiveSession. func (g *Gateway) SetAgentScope(agentID string, scope Scope) bool { - return g.agents != nil && g.agents.SetScope(agentID, scope) + if g.agents == nil { + return false + } + ok := g.agents.SetScope(agentID, scope) + if a := g.act(); a != nil && ok { + // One actor hop: eviction is lifecycle, and lifecycle is actor-owned. + a.do(func() { + if s := a.agents[agentID]; s != nil { + a.evict(s, "agent scope changed") + } + }) + } + return ok } // AgentView is one agent's management row for the GUI roster: its persisted diff --git a/internal/gateway/gateway_test.go b/internal/gateway/gateway_test.go new file mode 100644 index 0000000..b7a7c1d --- /dev/null +++ b/internal/gateway/gateway_test.go @@ -0,0 +1,49 @@ +package gateway + +import ( + "bytes" + "context" + "log/slog" + "strings" + "testing" + + "proxyforward/internal/config" +) + +// TestRunStartedKeepsThePairingCodeOutOfLogs pins the pairing code to the console. +// +// The code embeds the shared gateway token, and slog fans out to three places that +// outlive the moment it is useful: the rotating log file, the GUI ring, and — since +// app/tools.go ships both verbatim — every diagnostics bundle. Bundles exist to be +// handed to someone else, so a logged pairing code hands over agent-level access to +// the gateway; redactConfig masking Gateway.Token there buys nothing while the same +// token rides along in cleartext inside a logged code. (security) +func TestRunStartedKeepsThePairingCodeOutOfLogs(t *testing.T) { + var logs bytes.Buffer + logger := slog.New(slog.NewTextHandler(&logs, nil)) + + cfg := config.Default() + cfg.Gateway.BindAddr = "127.0.0.1" // loopback: no firewall prompt on a test bind + cfg.Gateway.ControlPort = 0 // ephemeral + cfg.Gateway.QUICEnabled = false + cfg.Gateway.Token = "GWTOKENSECRET" + + // An already-cancelled ctx: RunStarted still starts, mints the code and emits it, + // then returns at its own <-ctx.Done(). Deterministic, and no sleep. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + if err := RunStarted(ctx, New(cfg, t.TempDir(), logger), cfg, logger); err != nil { + t.Fatal(err) + } + + got := logs.String() + if got == "" { + t.Fatal("RunStarted logged nothing at all — this test would pass vacuously") + } + if strings.Contains(got, cfg.Gateway.Token) { + t.Errorf("the gateway token reached the logs, and therefore diagnostics bundles:\n%s", got) + } + if strings.Contains(got, "pxf://") { + t.Errorf("the pairing code reached the logs:\n%s", got) + } +} diff --git a/internal/link/cred.go b/internal/link/cred.go index 99c915d..e7d2ab2 100644 --- a/internal/link/cred.go +++ b/internal/link/cred.go @@ -54,18 +54,49 @@ const crockfordLower = "0123456789abcdefghjkmnpqrstvwxyz" var idEncoding = base32.NewEncoding(crockfordLower).WithPadding(base32.NoPadding) // fingerprint renders the first 40 bits of sha256(seed) as an 8-char base32 tag. -// 40 bits is ample to tell a home fleet's agents apart; the astronomically rare -// clash is resolved with a -2 suffix by whoever mints against a namespace. +// 40 bits is a *display* budget, sized to tell a home fleet's surfaces apart at a +// glance — it is deliberately not used where a label carries authority. GatewayID +// is its only caller: the gateway's trust rests on the full sha256 pin carried in +// the pairing code (cert.go Fingerprint), so gw_ is a name, never a credential. +// Contrast AgentID, which the gateway keys authorization on and which therefore +// takes agentFingerprintBytes. func fingerprint(seed []byte) string { sum := sha256.Sum256(seed) return idEncoding.EncodeToString(sum[:5]) } -// AgentID derives an agent's stable, unforgeable public identity from its Ed25519 -// public key: agt_. Derived rather than stored, so the same -// machine always re-derives the same ID and no two keys render alike. +// AgentIDPrefix marks an agentID as *derived from an Ed25519 public key*. The +// prefix is a reserved namespace, not decoration: a legacy shared-token agent +// self-asserts its agentID (config.NewID gives it bare 32-hex), so anything wearing +// this prefix must have been minted by AgentID from a key the gateway verified. The +// shared-token path therefore refuses to assert it (gateway/auth.go +// sharedTokenValidator) — otherwise a mere token holder could name itself an +// enrolled agent and inherit its session, ports, and scope. +const AgentIDPrefix = "agt_" + +// agentFingerprintBytes is how much of sha256(pubkey) an agentID carries. It is a +// security parameter, not a display choice: the gateway keys supersede, revocation, +// scope, and gateway-authoritative config on this label, so anyone who can find a +// second key hashing to the same one inherits the victim's authority. At the +// original 5 bytes (40 bits) that search cost ~2^40 keygens — hours on one rented +// machine, i.e. forgeable. 10 bytes puts it at 2^80, which no amount of money buys, +// and still renders as a 16-char tag. AgentStore.Enroll enforces uniqueness besides, +// so even a found collision fails closed instead of silently sharing an identity. +const agentFingerprintBytes = 10 + +// AgentID derives an agent's stable public identity from its Ed25519 public key: +// agt_. Derived rather than stored, so the same machine always +// re-derives the same ID and no two keys render alike. func AgentID(pub ed25519.PublicKey) string { - return "agt_" + fingerprint(pub) + sum := sha256.Sum256(pub) + return AgentIDPrefix + idEncoding.EncodeToString(sum[:agentFingerprintBytes]) +} + +// IsDerivedAgentID reports whether s claims the key-derived agent namespace. Only +// AgentID mints these; a self-asserted one is a peer claiming an identity it did not +// prove, which is why the shared-token validator rejects it outright. +func IsDerivedAgentID(s string) bool { + return strings.HasPrefix(s, AgentIDPrefix) } // GatewayID derives the gateway's display identity from its (pinned) certificate diff --git a/internal/link/cred_test.go b/internal/link/cred_test.go index 9c94600..2a7efc5 100644 --- a/internal/link/cred_test.go +++ b/internal/link/cred_test.go @@ -59,17 +59,14 @@ func TestAgentIDDerivation(t *testing.T) { } } -// TestAgentIDAlphabet: the fingerprint is 8 chars of lowercase Crockford base32, -// so it never contains the confusable i/l/o/u. (identity) +// TestAgentIDAlphabet: the fingerprint is lowercase Crockford base32, so it never +// contains the confusable i/l/o/u. (identity) func TestAgentIDAlphabet(t *testing.T) { pub, _, err := ed25519.GenerateKey(rand.Reader) if err != nil { t.Fatal(err) } fp := strings.TrimPrefix(AgentID(pub), "agt_") - if len(fp) != 8 { - t.Fatalf("fingerprint should be 8 chars, got %d (%q)", len(fp), fp) - } const allowed = "0123456789abcdefghjkmnpqrstvwxyz" for _, r := range fp { if !strings.ContainsRune(allowed, r) { @@ -78,6 +75,49 @@ func TestAgentIDAlphabet(t *testing.T) { } } +// TestAgentIDIsWideEnoughToBeUnforgeable pins the agentID's width as the security +// parameter it is. The gateway keys supersede, revocation, scope, and config on this +// label, so its width *is* the cost of forging a second key that answers to a +// victim's name. At the original 40 bits that search was ~2^40 keygens — hours of +// rented CPU, which is not a threat model, it's a budget line. Anything below 80 +// bits here puts that attack back on the table. Asserts the floor, not the exact +// encoding. (identity) +func TestAgentIDIsWideEnoughToBeUnforgeable(t *testing.T) { + if bits := agentFingerprintBytes * 8; bits < 80 { + t.Fatalf("agentID carries %d bits; below 80 a chosen-id collision is purchasable", bits) + } + pub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + // The rendered tag must actually carry those bits (base32 packs 5 bits/char). + fp := strings.TrimPrefix(AgentID(pub), "agt_") + if got := len(fp) * 5; got < agentFingerprintBytes*8 { + t.Fatalf("rendered agentID carries only %d bits, want >= %d", got, agentFingerprintBytes*8) + } +} + +// TestIsDerivedAgentID: the agt_ namespace is reserved for key-derived ids, which is +// what lets the gateway refuse a shared-token peer that self-asserts one +// (gateway/auth.go sharedTokenValidator). Legacy ids from config.NewID are bare hex +// and must not be mistaken for derived ones, or the migration path breaks. (identity) +func TestIsDerivedAgentID(t *testing.T) { + pub, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + if id := AgentID(pub); !IsDerivedAgentID(id) { + t.Fatalf("a minted agentID must be recognized as derived: %q", id) + } + // A legacy config.NewID() id: 32 hex chars, no prefix. + if IsDerivedAgentID("9f86d081884c7d659a2feaa0c55ad015") { + t.Fatal("a legacy 32-hex agentID must not read as key-derived") + } + if IsDerivedAgentID("") { + t.Fatal("an empty agentID must not read as key-derived") + } +} + // TestGatewayIDDerivation: the gateway's display ID derives deterministically from // its cert DER with the gw_ prefix. (identity) func TestGatewayIDDerivation(t *testing.T) {