From 6393f6c248b9137d44db42e76a52a72aa89455f1 Mon Sep 17 00:00:00 2001 From: Travis Wu Date: Thu, 17 Sep 2026 10:44:08 +0800 Subject: [PATCH] fix(console): read the web allowlist per channel, not once at startup The allowlist changes underneath a running agent: `advisor target_set` writes the file, and config_advisor's Commit seeds one on a node that had none. Reading it once at startup meant every such change needed a restart to take effect, and the failure it produced pointed away from the cause -- a channel refused with "target is not in this node's web allowlist", naming a target the file plainly contained. Three times on the r630 this session: cube-cos-idp, then cube-cos-skyline and cube-cos-ceph. Each looked like a proxy bug until the file was read side by side with the agent's startup line. Re-reading costs one small read per channel, and a channel is opened by a person clicking a button. Startup still parses the file once, where an operator is watching a malformed one. A file that stops parsing later refuses rather than falling back to the last good copy: an allowlist nobody can read is not one to keep enforcing from memory. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PZ5umjjCedZwWtbAbiMjfj Signed-off-by: Travis Wu --- cmd/agent/run.go | 4 ++- internal/console/web.go | 32 ++++++++++++++++++++++-- internal/console/web_test.go | 47 ++++++++++++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 3 deletions(-) diff --git a/cmd/agent/run.go b/cmd/agent/run.go index 6e8dcc2..74ef5f4 100644 --- a/cmd/agent/run.go +++ b/cmd/agent/run.go @@ -53,6 +53,8 @@ func runCmd(args []string) int { fmt.Fprintf(os.Stderr, "run: no usable identity in %s (enroll first): %v\n", *dir, err) return exitFailed } + // Once here, to refuse a malformed file where an operator is watching; + // the handler reads it again per channel. webAllow, err := console.LoadWebAllowlist(*webTargets) if err != nil { fmt.Fprintf(os.Stderr, "run: %v\n", err) @@ -148,7 +150,7 @@ func runCmd(args []string) int { srv := &agent.Server{ Tools: reg, Console: &console.Handler{NodeID: id.NodeID}, - Web: &console.WebHandler{Allow: webAllow}, + Web: &console.WebHandler{Allow: console.FileAllowlist{Path: *webTargets}}, } backoff := tunnel.DefaultBackoff() guard := tunnel.DefaultFlapGuard() diff --git a/internal/console/web.go b/internal/console/web.go index 333d921..d8cd0ec 100644 --- a/internal/console/web.go +++ b/internal/console/web.go @@ -53,10 +53,35 @@ func (a WebAllowlist) Resolve(name string) (string, error) { return hostport, nil } +// WebResolver is what a handler asks for an address. A fixed WebAllowlist is +// one; so is a file read fresh on every channel. +type WebResolver interface { + Resolve(name string) (string, error) +} + +// FileAllowlist resolves against the file each time it is asked. +// +// The file changes underneath a running agent -- `advisor target_set` writes +// it, and config_advisor's Commit seeds one -- and reading it once at startup +// meant every such change needed a restart. A read per channel costs nothing: +// a channel is opened by a person clicking a button. +// +// A parse error refuses rather than falling back to the last good copy: an +// allowlist nobody can read is not one to keep enforcing from memory. +type FileAllowlist struct{ Path string } + +func (f FileAllowlist) Resolve(name string) (string, error) { + a, err := LoadWebAllowlist(f.Path) + if err != nil { + return "", err + } + return a.Resolve(name) +} + // WebHandler serves web channels for one node. type WebHandler struct { - // Allow is the only source of addresses. Empty refuses everything. - Allow WebAllowlist + // Allow is the only source of addresses. Nil refuses everything. + Allow WebResolver // Dial opens the upstream connection; nil uses a real TCP dialler. Dial Dialer @@ -67,6 +92,9 @@ type WebHandler struct { // Bytes only: the agent parses no HTTP, so there is no request it can be // tricked into rewriting. func (h *WebHandler) Serve(ctx context.Context, name string, rw io.ReadWriter) error { + if h.Allow == nil { + return fmt.Errorf("%w: %q", ErrNotAllowed, name) + } hostport, err := h.Allow.Resolve(name) if err != nil { return err diff --git a/internal/console/web_test.go b/internal/console/web_test.go index 494a5cc..7a109d5 100644 --- a/internal/console/web_test.go +++ b/internal/console/web_test.go @@ -132,3 +132,50 @@ func TestAnUnlistedWebTargetIsNeverDialled(t *testing.T) { t.Error("an unlisted target reached the dialler") } } + +// Reading the file once at startup meant a target added later was refused +// with "not in this node's web allowlist", naming one the file contained. +func TestFileAllowlistSeesATargetAddedAfterStartup(t *testing.T) { + path := filepath.Join(t.TempDir(), "web-targets.json") + write := func(body string) { + t.Helper() + if err := os.WriteFile(path, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + write(`{"cube-cos":"10.0.0.1:443"}`) + + a := console.FileAllowlist{Path: path} + if _, err := a.Resolve("cube-cos-idp"); !errors.Is(err, console.ErrNotAllowed) { + t.Fatalf("before it was added: err = %v, want ErrNotAllowed", err) + } + + write(`{"cube-cos":"10.0.0.1:443","cube-cos-idp":"10.0.0.1:10443"}`) + + got, err := a.Resolve("cube-cos-idp") + if err != nil { + t.Fatalf("after it was added: %v", err) + } + if got != "10.0.0.1:10443" { + t.Errorf("resolved to %q, want 10.0.0.1:10443", got) + } +} + +// A file that stopped parsing refuses rather than falling back to whatever +// was read last. +func TestFileAllowlistRefusesWhenTheFileStopsParsing(t *testing.T) { + path := filepath.Join(t.TempDir(), "web-targets.json") + if err := os.WriteFile(path, []byte(`{"cube-cos":"10.0.0.1:443"}`), 0o644); err != nil { + t.Fatal(err) + } + a := console.FileAllowlist{Path: path} + if _, err := a.Resolve("cube-cos"); err != nil { + t.Fatalf("while it parsed: %v", err) + } + if err := os.WriteFile(path, []byte(`{ not json`), 0o644); err != nil { + t.Fatal(err) + } + if _, err := a.Resolve("cube-cos"); err == nil { + t.Fatal("a target resolved from an unreadable allowlist") + } +}