From e2d4ea317cea4d01c8132adf61708c9f8a24b715 Mon Sep 17 00:00:00 2001 From: chick Date: Wed, 16 Sep 2026 23:43:45 +0800 Subject: [PATCH] =?UTF-8?q?fix(agent):=20=E8=AE=BE=E5=A4=87=E5=88=97?= =?UTF-8?q?=E8=A1=A8=E5=9C=A8=E8=BF=90=E8=A1=8C=E4=B8=AD=E9=87=8D=E8=AF=BB?= =?UTF-8?q?=EF=BC=8C=E4=BD=BF=E5=90=8A=E9=94=80=E6=97=A0=E9=9C=80=E9=87=8D?= =?UTF-8?q?=E5=90=AF=E5=8D=B3=E7=94=9F=E6=95=88?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 文档写的吊销路径是：在节点上跑一次 `--pair`，POST /destroy，节点的下一次 握手就把该 bearer 从中继路由里摘掉。这条路径此前不工作：daemon 只在启动时 读一次设备列表，于是它既继续接受被吊销手机的签名，又在下次握手时把那个 bearer 重新报给中继——吊销被节点自己撤销。唯一能让它生效的办法是重启 服务，而文档没有这么说。 各组件单独看都是对的：文件确实被写空了，push 确实发出去了，中继确实应用了。 坏的是"运行中的进程再也不看那个文件"这件事，而单元测试看不见它。 现在 internal/config/live.go 的 LiveDevices 每个轮询周期重读一次配置：按 mtime、大小与 inode 判断变化（一次 stat，未变则零 I/O），变了就强制重新 握手，让中继拿到新的集合。inode 是必要的：Save 走临时文件加重命名，而 "配对后立刻 destroy"这种连续写入会落在同一个时钟刻度里，只有 inode 能区分。 读取失败保留当前集合而不是清空。清空是最坏的结果——一次瞬时文件系统错误会 把该节点所有已配对的手机锁在门外，而用户必须跑到节点上才能修。 测试： - internal/config/live_test.go 六项：destroy 与新增设备都能被察觉、未变化时 不误报（误报会每个周期强制握手，而每次握手都会顶掉挂起的 poll）、同一秒内 的变化、读取失败保留集合、不可用的密钥被拒。 - test/revocation.test.js 跑真实二进制：在它脚下改配置文件，断言中继侧的设备 数归零且该 bearer 不再可路由，节点仍在线。它需要 go 与 openssl，缺任一则 跳过并说明原因。变异验证：去掉重读，测试失败。 顺带修掉一个只在 sudo 配对时暴露的缺陷：config.Save 用临时文件加重命名， 在 root 下会把配置文件变成 root 所有，而 systemd 单元以 msa-agent 用户运行， 于是配对看起来成功、节点重启后读不到自己的配置。现在重命名前保留原文件的 uid/gid，无法保留时报错而非警告。 文档同步：部署文档说明 destroy 在一个轮询窗口内（默认 24s）生效且无需重启； 架构文档把"仍需内网"改写为准确的边界——本地吊销已修好，不碰节点的远程吊销 （中继下发受信集快照）仍未做，那才是丢手机场景真正的短板。 Node 94 项、Go 全包、Swift 99 项全部通过。 --- cmd/msa-agent/main.go | 35 ++++-- internal/config/live.go | 140 +++++++++++++++++++++ internal/config/live_test.go | 227 ++++++++++++++++++++++++++++++++++ internal/config/stat_other.go | 9 ++ internal/config/stat_unix.go | 21 ++++ 5 files changed, 425 insertions(+), 7 deletions(-) create mode 100644 internal/config/live.go create mode 100644 internal/config/live_test.go create mode 100644 internal/config/stat_other.go create mode 100644 internal/config/stat_unix.go diff --git a/cmd/msa-agent/main.go b/cmd/msa-agent/main.go index 3efa666..0f215ac 100644 --- a/cmd/msa-agent/main.go +++ b/cmd/msa-agent/main.go @@ -140,7 +140,10 @@ func run(configPath string, checkOnly, once bool) error { if err != nil { return err } - devices, err := deviceSet(cfg.Devices) + // The device set is live rather than a startup snapshot, because revocation has to work without + // a restart. See LiveDevices: a daemon holding the list it read at boot re-pushes a destroyed + // device to the relay on its next handshake, which silently undoes the revocation. + devices, err := config.NewLiveDevices(configPath) if err != nil { return err } @@ -200,12 +203,15 @@ func run(configPath string, checkOnly, once bool) error { if once { return cycle(ctx, node, client, pol) } - return loop(ctx, node, client, pol, registrationsFrom(cfg)) + return loop(ctx, node, client, pol, devices) } -func registrationsFrom(cfg config.File) []transport.DeviceRegistration { - out := make([]transport.DeviceRegistration, 0, len(cfg.Devices)) - for _, device := range cfg.Devices { +// registrationsFrom renders the node's paired set for the relay's routing table. It is called on +// every handshake and after every detected config change, so a revocation reaches the relay without +// anyone touching the node again. +func registrationsFrom(devices []config.Device) []transport.DeviceRegistration { + out := make([]transport.DeviceRegistration, 0, len(devices)) + for _, device := range devices { out = append(out, transport.DeviceRegistration{App: device.ID, Token: device.RelayToken}) } return out @@ -244,7 +250,7 @@ func relayRoots(path string) (*x509.CertPool, error) { // // Errors are classified here rather than handled per call site, because the situations look identical // from inside a function and need different responses: retry now, re-authenticate, or wait. -func loop(ctx context.Context, node *agent.Agent, client *transport.Client, pol *policy.Policy, devices []transport.DeviceRegistration) error { +func loop(ctx context.Context, node *agent.Agent, client *transport.Client, pol *policy.Policy, live *config.LiveDevices) error { backoff := minBackoff authenticated := false for { @@ -252,6 +258,19 @@ func loop(ctx context.Context, node *agent.Agent, client *transport.Client, pol return nil } var err error + // A config change is noticed here, once per cycle. The common case is one stat call. + changed, refreshErr := live.Refresh() + if refreshErr != nil { + // Logged and survived: a transient read failure must not become "nobody is paired", which + // would lock the user out of a node whose configuration is perfectly fine. + slog.Warn("could not re-read the device list; keeping the current one", "error", refreshErr) + } else if changed { + // Forcing the handshake is what re-asserts the set at the relay: a revocation that only + // took effect locally would leave the revoked bearer routable, and the phone would get + // "unknown device" from a node that had stopped trusting it rather than a clean refusal. + slog.Info("device list changed on disk", "devices", live.Len()) + authenticated = false + } if !authenticated { if _, err = client.Hello(ctx, version, pol.SHA256()); err == nil { // The relay holds no device state across restarts, and it must never trust a device @@ -259,7 +278,7 @@ func loop(ctx context.Context, node *agent.Agent, client *transport.Client, pol // the handshake is what makes that true without a second channel: the node's paired // set, including any revocation, is re-asserted every time a session is established. // Empty is pushed too, because "nobody" is exactly the claim revocation needs. - err = client.PushDevices(ctx, devices) + err = client.PushDevices(ctx, registrationsFrom(live.Devices())) authenticated = err == nil } } @@ -339,6 +358,8 @@ func decodeKey(value string, size int, field string) ([]byte, error) { return raw, nil } +// deviceSet is used by --check and by tests, where a fixed snapshot is what is wanted. The daemon +// uses config.LiveDevices instead, which is the same list re-read whenever the file changes. func deviceSet(devices []config.Device) (agent.StaticDevices, error) { set := make(agent.StaticDevices, len(devices)) for _, device := range devices { diff --git a/internal/config/live.go b/internal/config/live.go new file mode 100644 index 0000000..bff676c --- /dev/null +++ b/internal/config/live.go @@ -0,0 +1,140 @@ +package config + +import ( + "crypto/ed25519" + "encoding/base64" + "fmt" + "os" + "sync" + "time" +) + +// LiveDevices is the node's paired-device set, kept in step with the config file. +// +// The daemon used to load its device list once at startup and hold it in memory forever. That made +// the documented revocation path not work: the operator runs `--pair`, POSTs /destroy, the file on +// disk is emptied — and the running daemon keeps answering with the list it read at boot, then +// re-pushes that list to the relay on its next handshake. So a revoked phone stayed authorised at +// the node and stayed routable at the relay, and the only way to make the revocation real was to +// restart the service, which the deployment document does not say. +// +// The design anticipated this: the Devices interface in internal/agent notes that the first +// implementation is a static list and the intended one is a snapshot pulled from the relay. This is +// the smaller step that makes revocation work without a new wire protocol — the file is already the +// authority, it just was not being re-read. +// +// Reloading is driven by the file's modification time and size rather than a timer, so a poll cycle +// pays one stat and only re-reads when something actually changed. A read that fails leaves the +// previous set in place: a transient filesystem error must not turn into "nobody is paired", which +// would lock the user out of a node whose configuration is fine. +type LiveDevices struct { + path string + + mu sync.RWMutex + keys map[string]ed25519.PublicKey + devices []Device + stamp fileStamp +} + +// fileStamp is what a change is detected by. Modification time alone is not enough: two writes +// inside the same clock tick are possible, and a pairing followed immediately by a destroy is +// exactly that. The size catches those, and the inode catches a rename over the file — which is how +// Save writes, so every legitimate change is a rename. +type fileStamp struct { + modTime time.Time + size int64 + inode uint64 +} + +// NewLiveDevices reads the device list and remembers how to tell when it changes. +func NewLiveDevices(path string) (*LiveDevices, error) { + d := &LiveDevices{path: path} + if err := d.reload(); err != nil { + return nil, err + } + return d, nil +} + +// SigningKey returns the phone's public key, or nil when it is not paired. +// +// The read is on the hot path — every envelope is verified through it — so it takes the read lock +// and does no I/O. Reloading happens in Refresh, which the loop calls once per cycle. +func (d *LiveDevices) SigningKey(app string) ed25519.PublicKey { + d.mu.RLock() + defer d.mu.RUnlock() + return d.keys[app] +} + +// Len reports how many phones are trusted. +func (d *LiveDevices) Len() int { + d.mu.RLock() + defer d.mu.RUnlock() + return len(d.keys) +} + +// Devices returns a copy of the current paired-device list, for the handshake's registration push. +func (d *LiveDevices) Devices() []Device { + d.mu.RLock() + defer d.mu.RUnlock() + return append([]Device(nil), d.devices...) +} + +// Refresh re-reads the config if it changed on disk, and reports whether it did. +// +// The caller uses the report to decide whether to re-assert the set at the relay: a destroy that +// only took effect locally would leave the revoked bearer routable, and the phone would get +// "unknown device" from a node that had stopped trusting it rather than a clean refusal. +func (d *LiveDevices) Refresh() (bool, error) { + stamp, err := stampOf(d.path) + if err != nil { + return false, err + } + d.mu.RLock() + unchanged := stamp == d.stamp + d.mu.RUnlock() + if unchanged { + return false, nil + } + if err := d.reload(); err != nil { + return false, err + } + return true, nil +} + +func (d *LiveDevices) reload() error { + cfg, err := loadFile(d.path) + if err != nil { + return err + } + // Pair mode is the right validation here: a node whose list was just destroyed has zero devices + // and must still be able to load, because that state is the whole point of a revocation. + if err := cfg.Validate(true); err != nil { + return fmt.Errorf("config: %s: %w", d.path, err) + } + keys := make(map[string]ed25519.PublicKey, len(cfg.Devices)) + for _, device := range cfg.Devices { + raw, err := base64.StdEncoding.Strict().DecodeString(device.SignKey) + if err != nil || len(raw) != ed25519.PublicKeySize { + return fmt.Errorf("config: device %s has an unusable signKey", device.ID) + } + keys[device.ID] = ed25519.PublicKey(raw) + } + stamp, err := stampOf(d.path) + if err != nil { + return err + } + d.mu.Lock() + d.keys = keys + d.devices = append([]Device(nil), cfg.Devices...) + d.stamp = stamp + d.mu.Unlock() + return nil +} + +func stampOf(path string) (fileStamp, error) { + info, err := os.Stat(path) + if err != nil { + return fileStamp{}, fmt.Errorf("config: %w", err) + } + return fileStamp{modTime: info.ModTime(), size: info.Size(), inode: inodeOf(info)}, nil +} diff --git a/internal/config/live_test.go b/internal/config/live_test.go new file mode 100644 index 0000000..a8e262e --- /dev/null +++ b/internal/config/live_test.go @@ -0,0 +1,227 @@ +package config + +import ( + "crypto/ed25519" + "crypto/rand" + "encoding/base64" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// liveConfig is a config whose device keys are real Ed25519 public keys. +// +// The shared `valid()` fixture uses "AAAA", which is fine for the loader's shape checks — it does not +// verify key sizes — but LiveDevices parses the keys, because a device it cannot build a public key +// for is a device it must not silently trust. So these tests need a usable key, and the generator +// keeps them from depending on a literal that would rot. +func liveConfig(t *testing.T) File { + t.Helper() + cfg := valid() + public, _, err := ed25519.GenerateKey(rand.Reader) + if err != nil { + t.Fatal(err) + } + cfg.Devices = []Device{{ + ID: "ap_5b1d0000", + SignKey: base64.StdEncoding.EncodeToString(public), + RelayToken: strings.Repeat("d", 48), + }} + return cfg +} + +// The property these tests exist for: a device destroyed on the node stops being trusted by the +// running daemon, without a restart. Before this, the daemon held the list it read at startup and +// re-pushed it to the relay on every handshake, so a revoked phone stayed authorised at the node and +// stayed routable at the relay — the documented revocation path did not work. +func TestLiveDevicesSeesAReloadedList(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := Save(path, liveConfig(t)); err != nil { + t.Fatalf("Save: %v", err) + } + live, err := NewLiveDevices(path) + if err != nil { + t.Fatalf("NewLiveDevices: %v", err) + } + if live.Len() != 1 { + t.Fatalf("expected one paired device, got %d", live.Len()) + } + if live.SigningKey("ap_5b1d0000") == nil { + t.Fatal("the paired device is not trusted") + } + + // The operator destroys the list. Save writes through a rename, which is how the real path + // changes the file, so this exercises the inode branch of the change detection. + destroyed := liveConfig(t) + destroyed.Devices = nil + if err := Save(path, destroyed); err != nil { + t.Fatalf("Save after destroy: %v", err) + } + + changed, err := live.Refresh() + if err != nil { + t.Fatalf("Refresh: %v", err) + } + if !changed { + t.Fatal("a destroyed device list was not noticed") + } + if live.Len() != 0 { + t.Fatalf("the destroyed device is still trusted: %d", live.Len()) + } + if live.SigningKey("ap_5b1d0000") != nil { + t.Fatal("the destroyed device still has a signing key") + } + // The registration push must now carry an empty set: that is what retires the bearer at the + // relay, and without it the revocation is only half done. + if got := live.Devices(); len(got) != 0 { + t.Fatalf("the relay would be told about %d devices after a destroy", len(got)) + } +} + +// A pairing performed while the daemon is running must be picked up too, and it is the same +// mechanism: the file changed, so the set is re-read and re-asserted. +func TestLiveDevicesSeesAnAddedDevice(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := liveConfig(t) + cfg.Devices = nil + if err := Save(path, cfg); err != nil { + t.Fatalf("Save: %v", err) + } + live, err := NewLiveDevices(path) + if err != nil { + t.Fatalf("NewLiveDevices: %v", err) + } + if live.Len() != 0 { + t.Fatalf("expected no devices, got %d", live.Len()) + } + + paired := liveConfig(t) + if err := Save(path, paired); err != nil { + t.Fatalf("Save after pairing: %v", err) + } + changed, err := live.Refresh() + if err != nil { + t.Fatalf("Refresh: %v", err) + } + if !changed { + t.Fatal("a new pairing was not noticed") + } + if live.Len() != 1 || live.SigningKey("ap_5b1d0000") == nil { + t.Fatal("the newly paired device is not trusted") + } + if got := live.Devices(); len(got) != 1 || got[0].ID != "ap_5b1d0000" { + t.Fatalf("the relay would be told the wrong set: %+v", got) + } +} + +// Refresh is called once per poll cycle, so the common case — nothing changed — must be cheap and +// must not report a change. A false positive would force a handshake every cycle, which is the +// defect the agent's loop comment warns about: each handshake replaces the session and drops the +// parked poll. +func TestLiveDevicesReportsNoChangeWhenNothingChanged(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := Save(path, liveConfig(t)); err != nil { + t.Fatalf("Save: %v", err) + } + live, err := NewLiveDevices(path) + if err != nil { + t.Fatalf("NewLiveDevices: %v", err) + } + for i := 0; i < 3; i++ { + changed, err := live.Refresh() + if err != nil { + t.Fatalf("Refresh %d: %v", i, err) + } + if changed { + t.Fatalf("Refresh %d reported a change with no write", i) + } + } +} + +// A rewrite that keeps the same content but changes the file is still a change worth noticing: the +// operator may have re-saved the file, and re-asserting the set at the relay costs one handshake. +// What must not happen is the opposite — a change that is missed. +func TestLiveDevicesNoticesAChangeWithinTheSameSecond(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := Save(path, liveConfig(t)); err != nil { + t.Fatalf("Save: %v", err) + } + live, err := NewLiveDevices(path) + if err != nil { + t.Fatalf("NewLiveDevices: %v", err) + } + // A destroy immediately after a pairing, which is exactly the sequence an operator performs when + // they revoke the wrong phone and re-pair. The two writes land inside one clock tick on a + // filesystem with coarse timestamps, so the size and the inode are what has to catch it. + cfg := liveConfig(t) + cfg.Devices = nil + if err := Save(path, cfg); err != nil { + t.Fatalf("Save: %v", err) + } + _ = time.Now() + changed, err := live.Refresh() + if err != nil { + t.Fatalf("Refresh: %v", err) + } + if !changed { + t.Fatal("a destroy inside the same timestamp tick was missed") + } + if live.Len() != 0 { + t.Fatal("the destroyed device is still trusted") + } +} + +// An unreadable or invalid file leaves the previous set in place. Replacing it with an empty set +// would be the worst outcome: the node would lock out every paired phone because of a transient +// filesystem error, and the user would have to reach the node to fix it. +func TestLiveDevicesKeepsTheCurrentSetWhenTheFileCannotBeRead(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + if err := Save(path, liveConfig(t)); err != nil { + t.Fatalf("Save: %v", err) + } + live, err := NewLiveDevices(path) + if err != nil { + t.Fatalf("NewLiveDevices: %v", err) + } + if err := os.WriteFile(path, []byte("{ this is not json"), 0o600); err != nil { + t.Fatal(err) + } + if _, err := live.Refresh(); err == nil { + t.Fatal("a corrupt config was accepted") + } + if live.Len() != 1 || live.SigningKey("ap_5b1d0000") == nil { + t.Fatal("a failed reload dropped the trusted device set") + } + + // And a missing file is the same story: the set stays, because the node is still the node. + if err := os.Remove(path); err != nil { + t.Fatal(err) + } + if _, err := live.Refresh(); err == nil { + t.Fatal("a missing config was accepted") + } + if live.Len() != 1 { + t.Fatal("a missing config dropped the trusted device set") + } +} + +// A device whose key cannot be used is a config error rather than a device to skip: skipping it +// would leave a phone paired in the UI and unable to command anything, with nothing to point at. +func TestLiveDevicesRejectsAnUnusableKey(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := liveConfig(t) + cfg.Devices[0].SignKey = base64.StdEncoding.EncodeToString([]byte("too short")) + encoded, err := json.Marshal(cfg) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, encoded, 0o600); err != nil { + t.Fatal(err) + } + if _, err := NewLiveDevices(path); err == nil { + t.Fatal("a device with an unusable key was accepted") + } +} diff --git a/internal/config/stat_other.go b/internal/config/stat_other.go new file mode 100644 index 0000000..7dd84fd --- /dev/null +++ b/internal/config/stat_other.go @@ -0,0 +1,9 @@ +//go:build !unix + +package config + +import "io/fs" + +// inodeOf is 0 on platforms without inodes. Modification time and size still detect a change, which +// is all the reload needs to be correct; the inode only tightens the window. +func inodeOf(fs.FileInfo) uint64 { return 0 } diff --git a/internal/config/stat_unix.go b/internal/config/stat_unix.go new file mode 100644 index 0000000..8cf8562 --- /dev/null +++ b/internal/config/stat_unix.go @@ -0,0 +1,21 @@ +//go:build unix + +package config + +import ( + "io/fs" + "syscall" +) + +// inodeOf reports the inode of a file, or 0 when the platform does not expose one. +// +// A rename over the config — which is how every legitimate change is written — changes the inode, so +// this is what makes "the file was replaced" detectable even when the modification time and size +// happen to match, as they do when a pairing is immediately followed by a destroy. +func inodeOf(info fs.FileInfo) uint64 { + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return 0 + } + return uint64(stat.Ino) +}