From 7d174a02617c70fe64fbd471d64ff0cdf13c2ffd Mon Sep 17 00:00:00 2001 From: chick Date: Thu, 17 Sep 2026 19:12:32 +0800 Subject: [PATCH] =?UTF-8?q?fix(agent):=20=E5=8D=B8=E8=BD=BD=E6=97=B6?= =?UTF-8?q?=E6=8C=87=E5=87=BA=E8=AF=A5=E5=8E=BB=E5=93=AA=E5=88=A0=E8=8A=82?= =?UTF-8?q?=E7=82=B9=EF=BC=8C=E8=80=8C=E4=B8=8D=E6=98=AF=E4=B8=80=E5=BE=8B?= =?UTF-8?q?=E8=AF=B4=20agents.json?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit agent 不持有运维令牌——节点不是运维，一个能删掉邻居的被控节点比它解决的问题更糟——所以 它不能自己从中继上摘掉自己。此前卸载的结束语对所有节点都写"the relay's agents.json still lists this node"，而自注册的节点根本不在那里：它在中继的状态文件里，要从手机上删。运维照着 那句话去翻一个没有这条记录的文件。 现在节点在登记时把来历写进配置（`selfEnrolled`，带 `--code` 为真），卸载结束语据此给出 正确的那一句： - 自注册的 → "在中继的登记状态里，去 App 的「中继节点」移除" - 手工登记的 → "在 agents.json 里，删掉那条并重启中继" - 配置读不出来 → 两句都提，因为读不到不是让卸载失败的理由 测试两项：卸载打印的是它被告知的那一句（且不再对自注册节点说 agents.json）、登记时正确 记录来历（带码为真、不带码不写这个字段）。 Go 全包、Node 141 项、Swift 121 项通过。 --- cmd/msa-agent/enroll.go | 25 +++++++++++++++++++++++++ internal/config/config.go | 8 ++++++++ internal/enroll/enroll.go | 6 ++++++ internal/enroll/enroll_test.go | 24 ++++++++++++++++++++++++ internal/install/uninstall.go | 9 ++++++++- internal/install/uninstall_test.go | 25 +++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 1 deletion(-) diff --git a/cmd/msa-agent/enroll.go b/cmd/msa-agent/enroll.go index 4cf6edd..1cf9444 100644 --- a/cmd/msa-agent/enroll.go +++ b/cmd/msa-agent/enroll.go @@ -51,6 +51,9 @@ func runEnroll(ctx context.Context, opts EnrollOptions) error { } result, err := enroll.Generate(enroll.Options{ + // The flag is what records the node's own provenance: with a code the relay holds a + // self-enrolled entry for it, without one an operator wrote it into agents.json. + SelfEnrolled: opts.Code != "", RelayURL: opts.RelayURL, LocalPort: opts.LocalPort, AuditPath: opts.AuditPath, @@ -132,6 +135,9 @@ func runUninstall(ctx context.Context, opts install.Options, configPath string) if opts.NotifyRelay == nil && !opts.DryRun { opts.NotifyRelay = func() error { return retireAtRelay(ctx, configPath) } } + if opts.RemovalHint == "" { + opts.RemovalHint = removalHint(configPath) + } return install.Uninstall(opts) } @@ -195,3 +201,22 @@ func retireAtRelay(ctx context.Context, configPath string) error { } return nil } + +// removalHint says what is left at the relay, which depends on how this node got there. +// +// The distinction matters because the two records live in different places and are removed by different +// people. A self-enrolled node is in the relay's own state file and comes off from the app; a node an +// operator wrote into agents.json is in a file the relay only reads, and has to be edited and the relay +// restarted. Telling the operator the wrong one sends them to the wrong place. +// +// An unreadable config is not a reason to fail an uninstall, so it falls back to naming both. +func removalHint(configPath string) string { + cfg, err := config.LoadForPairing(configPath) + if err != nil { + return "the relay may still list this node; check its agents.json and, if it accepts self-enrollment, the app" + } + if cfg.SelfEnrolled { + return "the relay still lists this node in its enrollment state; remove it in the app (中继节点 → 移除)" + } + return "the relay's agents.json still lists this node; remove that entry and restart the relay" +} diff --git a/internal/config/config.go b/internal/config/config.go index d84dd8b..2798d0a 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -55,6 +55,14 @@ type File struct { // certificate. It exists for a relay behind a private CA; the default is the system store, which // is what a public certificate needs. Setting it pins trust rather than disabling verification. RelayCAFile string `json:"relayCAFile,omitempty"` + // SelfEnrolled records that this node registered itself with the relay using a one-time code, + // rather than being written into the relay's agents.json by an operator. + // + // It exists so `uninstall` can tell the operator which of two removals applies, because the agent + // cannot do either: it holds no admin token, by design — a node is not an operator, and giving it + // one would let a compromised node remove its neighbours. So the node records which half of the + // relay's state mentions it, and the operator is told exactly what to delete. + SelfEnrolled bool `json:"selfEnrolled,omitempty"` // AuditPath is where the audit log is appended. AuditPath string `json:"auditPath"` // Devices may be empty only in pair mode (--pair). In normal operation an agent with no diff --git a/internal/enroll/enroll.go b/internal/enroll/enroll.go index 737774b..0c4329e 100644 --- a/internal/enroll/enroll.go +++ b/internal/enroll/enroll.go @@ -40,6 +40,9 @@ type Options struct { RelayCAFile string // NodePassword is written into the config when non-empty. Never generated. NodePassword string + // SelfEnrolled records that this node registered itself with a one-time code, so `uninstall` can + // tell the operator which of the relay's two records mentions it. + SelfEnrolled bool } // Result is everything the caller has to place or show. @@ -129,6 +132,9 @@ func Generate(opts Options) (Result, error) { if opts.RelayCAFile != "" { config["relayCAFile"] = opts.RelayCAFile } + if opts.SelfEnrolled { + config["selfEnrolled"] = true + } encoded, err := json.MarshalIndent(config, "", " ") if err != nil { diff --git a/internal/enroll/enroll_test.go b/internal/enroll/enroll_test.go index 112e133..ec16f21 100644 --- a/internal/enroll/enroll_test.go +++ b/internal/enroll/enroll_test.go @@ -142,3 +142,27 @@ func TestMissingInputIsRefused(t *testing.T) { } } } + +// The node records how it got into the relay, because `uninstall` cannot work it out any other way: the +// agent holds no admin token, so it cannot remove its own entry, and the operator has to be told which +// of the relay's two records mentions this node. +func TestEnrollmentRecordsItsOwnProvenance(t *testing.T) { + withCode, err := Generate(Options{ + RelayURL: "https://relay.example", LocalPort: 3000, AuditPath: "/a", SelfEnrolled: true, + }) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(withCode.Config), `"selfEnrolled": true`) { + t.Fatal("a self-enrolled node does not say so") + } + // And a node an operator wrote into agents.json does not claim to have enrolled itself, or the + // uninstall would send the operator to the app to remove an entry that is not there. + manual, err := Generate(Options{RelayURL: "https://relay.example", LocalPort: 3000, AuditPath: "/a"}) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(manual.Config), "selfEnrolled") { + t.Fatal("a manually registered node claims to be self-enrolled") + } +} diff --git a/internal/install/uninstall.go b/internal/install/uninstall.go index 57c446d..8b27c4e 100644 --- a/internal/install/uninstall.go +++ b/internal/install/uninstall.go @@ -81,6 +81,10 @@ type Options struct { // NotifyRelay retires this node's device bearers at the relay, and is called before anything is // removed. Nil skips it, which is right for a node that was never paired. NotifyRelay func() error + // RemovalHint is the sentence printed at the end telling the operator what is left at the relay. + // It depends on how the node got there — self-enrolled or written into agents.json — and the caller + // is what knows that, from the config the node wrote at enrollment time. + RemovalHint string Runner Runner Log func(format string, args ...any) } @@ -164,7 +168,10 @@ func Uninstall(opts Options) error { opts.Log("dry run: nothing was removed") } else { opts.Log("msa-agent removed") - opts.Log("the relay's agents.json still lists this node; remove that entry unless you are reinstalling") + // The one thing this command cannot do for the operator, and it cannot by design: the agent holds + // no admin token, because a node is not an operator, and a compromised node that could remove its + // neighbours would be a worse problem than the one this hint solves. + opts.Log("%s", opts.RemovalHint) } return nil } diff --git a/internal/install/uninstall_test.go b/internal/install/uninstall_test.go index 25935ef..a0a68a0 100644 --- a/internal/install/uninstall_test.go +++ b/internal/install/uninstall_test.go @@ -401,3 +401,28 @@ func TestInstallDryRunReportsPreconditionsInsteadOfRefusing(t *testing.T) { t.Fatalf("the dry run printed no plan:\n%s", output) } } + +// The hint at the end has to name the right place, because the two records live in different places and +// are removed by different people. Telling the operator the wrong one sends them to the wrong place. +func TestTheRemovalHintIsTheOneItWasGiven(t *testing.T) { + paths := installed(t) + var lines []string + err := Uninstall(Options{ + Paths: paths, + Runner: &recorder{}, + Log: func(format string, args ...any) { lines = append(lines, fmt.Sprintf(format, args...)) }, + RemovalHint: "the relay still lists this node in its enrollment state; remove it in the app", + }) + if err != nil { + t.Fatalf("Uninstall: %v", err) + } + output := strings.Join(lines, "\n") + if !strings.Contains(output, "remove it in the app") { + t.Fatalf("the hint was not printed:\n%s", output) + } + // And the old hard-coded sentence is gone: it named agents.json for every node, including the + // self-enrolled ones that are not there. + if strings.Contains(output, "agents.json still lists this node") { + t.Fatalf("the uninstall still claims every node is in agents.json:\n%s", output) + } +}