From af738a5bf9408a3dc40f6cbdfc1a1e6c41f83265 Mon Sep 17 00:00:00 2001 From: chick Date: Wed, 16 Sep 2026 23:24:28 +0800 Subject: [PATCH] =?UTF-8?q?fix(relay):=20=E9=99=90=E9=A2=9D=E6=94=B9?= =?UTF-8?q?=E4=B8=BA=E4=BB=8E=E5=85=B1=E4=BA=AB=E7=AD=96=E7=95=A5=E6=8E=A8?= =?UTF-8?q?=E5=AF=BC=EF=BC=8C=E4=BF=AE=E6=8E=89=E4=BF=A1=E5=B0=81=E4=B8=8A?= =?UTF-8?q?=E9=99=90=E8=BF=87=E5=B0=8F=E5=AF=BC=E8=87=B4=E7=9A=84=E5=90=88?= =?UTF-8?q?=E6=B3=95=E8=AF=B7=E6=B1=82=E8=A2=AB=E6=8B=92?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 中继此前自带一份限额常量，并在注释里声称"与共享策略一致"，而没有任何东西 校验这句话。其中两条已经漂移成真实缺陷： 1. 信封上限 64 KiB 是**信封**的上限，而信封约为其承载 body 的 1.8 倍（body 先 base64、再 AEAD、再 base64）。于是 body 仍在 agent 的 256 KiB maxRequestBytes 之内的请求，在中继处被拒为 relay_payload_too_large——一个 在局域网上能成功、走中继就失败的操作。响应方向同理：节点返回合法的大 body，应答在中继被拒，手机白等到超时。实测 120 KiB 的 body 就被拒。 2. mailboxDepth 只靠注释与策略"相等"，改任一侧都不会有人发现，而这个深度 是 agent 重放缓存容量的依据。 现在 relay/src/limits.js 从 contracts/agent-policy.json 推导全部限额，并在 文件不可读、缺 limits、数值不可用时**拒绝启动**：中继无法区分"诚实的大请求" 与"内存耗尽尝试"，上限是它唯一的工具，凭空发明一个等于在强制一个没人选过的 数字。推导结果：信封上限 513037 字节、深度 8、请求期限 30s、轮询窗口 24s。 策略摘要在健康检查里公开并逐节点对比：/healthz 返回中继自己那份的摘要，以及 每个在线节点上报的摘要与 policyMatches。它只上报不强制——中继读不到节点上的 文件，声称不一致就拒服务会把"节点跑着旧二进制"变成一次故障；而策略本来就是 agent 在强制。没有上报的节点显示 null 而不是 false，"没说"与"说了不一样的" 是两个不同的发现。 顺带修掉一个只在 sudo 配对时才暴露的缺陷：config.Save 用临时文件加重命名， 在 root 下会把配置文件变成 root 所有，而 systemd 单元以 msa-agent 用户运行， 于是配对看起来成功、节点重启后读不到自己的配置。现在重命名前保留原文件的 uid/gid，且无法保留时报错而非警告。 测试新增 test/limits.test.js（11 项），分三层：推导与策略文件一致、推导出的 上限确实容得下用真实 wire 代码构造的最坏信封（这是旧上限失败的那次测量）、 以及经真实 HTTP 面把合法大请求送过去。另有健康检查的摘要上报与 null 语义。 四处断言都做了变异验证：把上限改回 64 KiB，四项测试失败。 Node 93 项、Go 全包、Swift 99 项、端到端全部通过。 --- internal/config/config.go | 35 ++++++++++++++++++++- internal/config/config_test.go | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/internal/config/config.go b/internal/config/config.go index 231da2f..d84dd8b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -14,6 +14,7 @@ import ( "fmt" "os" "regexp" + "syscall" ) var nodePattern = regexp.MustCompile(`^nd_[A-Za-z0-9_-]{8,64}$`) @@ -150,11 +151,19 @@ func loadFile(path string) (File, error) { return cfg, nil } -// Save writes the config back with 0600, atomically. +// Save writes the config back with 0600, atomically, preserving the file's owner. // // Atomic (temp file + rename) because the pairing flow writes the device list at moments when the // agent may be concurrently read by systemd restarts: a half-written config is a config that fails // to load, which turns a completed pairing into an uncommandable node. +// +// Ownership is preserved because the deployment document tells the operator to run `--pair` with +// sudo, and the installed config belongs to the `msa-agent` user. Writing a temp file as root and +// renaming it over the config would leave it root-owned 0600, and the systemd unit — which runs as +// `msa-agent` — could then no longer read it: the pairing would appear to succeed and the node would +// come up unable to load its own configuration. The chown is only attempted when the process can +// perform it (root), and a failure is an error rather than a warning, because silently leaving the +// file unreadable by the service is the failure this is here to prevent. func Save(path string, cfg File) error { if err := cfg.Validate(true); err != nil { return fmt.Errorf("config: refusing to save an invalid file: %w", err) @@ -164,13 +173,37 @@ func Save(path string, cfg File) error { return err } encoded = append(encoded, '\n') + uid, gid, hadOwner := ownerOf(path) temp := path + ".tmp" if err := os.WriteFile(temp, encoded, 0o600); err != nil { return err } + if hadOwner && os.Geteuid() == 0 { + if err := os.Chown(temp, uid, gid); err != nil { + // The temp file is removed so a failed save leaves no stray copy of the config — it + // holds the agent's private keys and the node password. + os.Remove(temp) + return fmt.Errorf("config: preserving ownership of %s: %w", path, err) + } + } return os.Rename(temp, path) } +// ownerOf reports the uid and gid a rewrite of path must keep. ok is false when the file does not +// exist yet, which is the first-write case: there is no owner to preserve, and the file is created +// by whoever is running. +func ownerOf(path string) (uid, gid int, ok bool) { + info, err := os.Stat(path) + if err != nil { + return 0, 0, false + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return 0, 0, false + } + return int(stat.Uid), int(stat.Gid), true +} + // NewRelayToken mints a device bearer for the relay: 24 random bytes, hex. Hex because the relay // only requires a bearer string and hex never contains a character that needs escaping when it // travels through JSON or headers. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 44ef263..f2835b8 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,8 +1,11 @@ package config import ( + "fmt" "os" "path/filepath" + "strings" + "syscall" "testing" ) @@ -66,6 +69,45 @@ func TestSaveRoundTripsAndIsSixHundred(t *testing.T) { } } +// A rewrite must keep the file's owner. The deployment document tells the operator to run --pair +// with sudo while the installed config belongs to the msa-agent user; without preserving ownership +// the rename would leave a root-owned 0600 file that the systemd unit cannot read, so the pairing +// would look successful and the node would come up unable to load its own configuration. +// +// The test cannot become root, so it asserts the property that is observable without privileges: +// the owner after a save is the same as the owner before it, and the temp file does not survive. +func TestSavePreservesTheFileOwner(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := valid() + if err := Save(path, cfg); err != nil { + t.Fatalf("first Save: %v", err) + } + before := owner(t, path) + // A second save goes through the ownership-preserving path. The inode necessarily changes — the + // write is a temp file plus a rename — so what is compared is the owner, which is the thing the + // rename would otherwise reset. + cfg.Devices = append(cfg.Devices, Device{ + ID: "ap_second0001", SignKey: cfg.Devices[0].SignKey, RelayToken: strings.Repeat("b", 48), + }) + if err := Save(path, cfg); err != nil { + t.Fatalf("second Save: %v", err) + } + after := owner(t, path) + if before != after { + t.Fatalf("the rewrite changed the owner from %v to %v", before, after) + } + if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) { + t.Fatal("a temp file was left behind") + } + reloaded, err := Load(path) + if err != nil { + t.Fatalf("Load after rewrite: %v", err) + } + if len(reloaded.Devices) != 2 { + t.Fatalf("the rewrite lost a device: %d", len(reloaded.Devices)) + } +} + // A save must not be able to write a file that the next boot would refuse: the validation runs // before the rename, so a caller building a device list with a bad id gets an error, not a // corrupted file. @@ -138,3 +180,17 @@ func TestLoadReportsMissingAndMalformedFiles(t *testing.T) { t.Fatal("a malformed file was accepted") } } + +// owner reads the uid and gid of a path, which is what a rewrite must preserve. +func owner(t *testing.T, path string) string { + t.Helper() + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + t.Skip("stat_t is not available on this platform") + } + return fmt.Sprintf("%d:%d", stat.Uid, stat.Gid) +}