Skip to content

fix(aiops): stop host_bash write gate from bypassing approval - #469

Merged
youzi-1122 merged 3 commits into
ongridio:mainfrom
Dragonzz27:fix/host-bash-write-gate-fail-closed
Oct 9, 2026
Merged

youzi-1122 merged 3 commits into
ongridio:mainfrom
Dragonzz27:fix/host-bash-write-gate-fail-closed

Conversation

@Dragonzz27

@Dragonzz27 Dragonzz27 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

#337 报告开启 Agent 写动作后,部分 host_bash 命令没有走"提案 → 确认 → 执行"而直接下发到 Edge。两个原因都在 manager 侧,本 PR 用两个提交分别对应,可以单独 review:

提交 1 fix(aiops): keep host_bash dispatch read-only when the write gate is on

InvokableRun 把写动作开关原样当成 BashExecRequest.Unrestricted 下发(旧代码在 bash_basetool.go 的批量分发闭包里;该处现为 348-353 行,末参已固定为 false)。Unrestricted 在 Edge 侧的语义是跳过 cmdpolicy、直接丢给 shell 跑原始字符串。于是分类器每漏判一次,开关就等于给这条命令开了免审批的写通道。现在下发路径固定 false,只有审批通过后执行的 RunApproved 才允许 Unrestricted —— 这正是 issue 第 2、3 条期望行为。

顺带改掉两处把旧行为写成文档的注释(basetool.go 的 WithHostWritePermission、host_write.go 的包注释),避免代码与文档分叉。

提交 2 fix(aiops): classify redirected and compound host_bash commands as writes

isHostBashWriteCommand 只看整条命令的第一个 token,所以 echo x > /etc/hosts、ls; rm -f y、cat f | tee copy、echo $(rm y) 全被算成读命令。改成按 shell 分隔符切段、逐段判定:

  • 切分识别引号:引号里的 >、; 仍算字面量参数,不把无辜的读命令拖进审批。
  • 输出重定向与命令替换直接判为写 —— 这类语法的影响无法从命令名读出来。
  • 跳过前导 NAME=value 与 sudo 的带值选项,否则 LC_ALL=C rm x、sudo -u nginx rm x 会把赋值或 -u 当成命令名而漏判(这两条是顺带扫出来的同类漏判,issue 没点名)。

提交 3 fix(aiops): route edge-unsupported shell operators to approval(按 review 的 [P2] 补)

提交 2 只把 ;、&&、||、&、换行当切分点,于是 df -h && free -m 被切成两个读段、整条判成读,以 Unrestricted=false 下发;而 Edge 的 cmdpolicy 解析器根本不接受这些语法(internal/edgeagent/cmdpolicy/parse.go 对 &、;、||、<、(、${ 一律直接报 forbidden),结果是写开关开启时原本能跑的命令列表现在必然失败。这是功能回归,不是新增绕过,与 review 的实测表一致。

现在把"Edge 在引号外拒绝的语法"整体判为需要审批:;、&&、||、后台 &、输入与输出重定向、$()、反引号、${}、括号,以及换行(换行不报语法错,Edge 把它当空白、会把两条命令粘成一条 argv,所以也必须进审批)。裸 | 保持只读直发,因为它是双方都支持的唯一复合语法。工具自己的 when_to_use 与 schema 描述本来就写着"读路径不支持 redirects / ; && || $() <() heredocs / backticks",这次是让实现对齐已声明的契约。

现存测试的反转与新增

这些测试把 issue 与 review 认定的问题行为写成了断言,必须一起改,否则 CI 直接红:

  • TestBashTool_AdminWriteGateBypassesReadOnlyPolicyForReadCommand → 改名 ..._KeepsReadCommandOnReadOnlyPath,断言由 !req.Unrestricted 翻成 req.Unrestricted。
  • TestBashTool_ReadCommandWithShellSyntaxDoesNotUseApproval → 原用例命令含 2>/dev/null && ...,按 issue 第 4 条现在必须进审批,因此拆成两条:TestBashTool_RedirectAndCommandListUseApproval(重定向/列表/替换/管道内写命令 → 生成提案且不提前下发)与 TestBashTool_ReadPipelineDispatchesReadOnly(纯读管道仍直发且 Unrestricted=false)。
  • 提交 3 再反转两处旧断言:df -h && free -m 从只读直发集合里移除(它根本跑不通),{"sleep 5 &", false} 改为 true。
  • 新增 TestBashTool_EdgeUnsupportedOperatorsRequireApproval 与 TestBashTool_NewlineSeparatedCommandsRequireApproval,并按 review 要求改用真实的 Edge 策略而不是写死的 Allowed=true:只读集合先过 cmdpolicy.DefaultReadOnly().Decide(不允许就判测试失败,这样"判成读"必须等于"Edge 真能跑"),审批集合则断言真实解析器确实拒绝且理由含 forbidden;换行那条断言 SplitPipes 把它解析成一条 4 token 的 argv,正是必须进审批的原因。TestIsHostBashWriteCommand 补了 &&/||/;/&/换行/</${}/括号 与对应引号内反例。

一处已定的取舍

& 的语义问题(原先"后台执行只切段、sleep 5 & 仍算读")由上面那条 [P2] 定下来了:任何引号外的 & 都进审批,因为 Edge 连单个 & 都拒绝。

分类器判成写但内容其实无害的命令(例如 echo hi > /dev/null、echo ${HOME})现在会多一次确认卡。这是 fail-closed 方向的代价,我认为符合 issue 的期望;如果你希望给 /dev/null、/dev/stdout 一类目标开白名单,我也可以补。

验证(含未验证项,如实说明)

本机能跑的:

  • go build ./internal/manager/biz/aiops/tools/... 通过;gofmt -l 对两个改动文件无输出(该包另有 3 个主干既有文件的 gofmt 问题与本改动无关,未一并处理)。
  • 分类器是纯字符串逻辑,我把这些函数程序化抽取到一个独立模块,并且把 Edge 侧 cmdpolicy/parse.go 原文也抽进同一个模块,跑 44 条用例:0 处不符。除了原有的重定向/追加/2>/>&、;/&&/||/&、$()、反引号、双引号内的 $()、引号内字面量、前导赋值、sudo -u/-n/--、空输入,这次还加了双向不变式:分类器判读 ⇔ 真实 Edge 解析器不报语法错,以及 docker system df 2>/dev/null && free -m 这类组合。
  • 变异自证:同一批断言跑在提交 3 之前的分类器上,17 条失败,且失败理由逐字复现 review 的实测表(cmdpolicy: '&' is forbidden、'||'、';'、redirect "<" is forbidden、'${' parameter expansion is forbidden);跑在改动后的分类器上是 0 条。

本机跑不了的(仓库既有限制,非本 PR 引入):

  • 该包的测试二进制在本机无法编译:bash_basetool_test.go 导入的 internal/edgeagent/cmdpolicy 是 //go:build linux,而 darwin 下"build constraints exclude all Go files";改用 GOOS=linux 交叉时又卡在依赖 onnxruntime_go 需要 cgo。go vet 因同样原因在本机失败。
  • 因此上面的 TestBashTool_* 分发级测试与提交级联只由 CI(ubuntu)实际执行,我这边是逻辑等价验证 + 编译/格式检查。上面的测试断言用的读集合我逐个用真实 DefaultReadOnly 在本机跑过(ps/df/cat/ls/grep/head/tail 全部 allow=true),刻意避开了 free、journalctl 这类"macOS 上没装、判据会变成宿主差异"的二进制。如果 CI 红了我按错误继续修。

未覆盖项:issue 验收标准里的"真实会话 E2E"和"Edge 侧 race 测试"需要现场集群,本 PR 未做;本 PR 只改 manager 侧分类与下发,未触碰 internal/edgeagent/cmdpolicy 的实现(只在测试里调用它的公开 SplitPipes / DefaultReadOnly)。

范围与回滚

无 API、.proto、数据库、配置或前端变更。三个提交相互独立:提交 3 可单独 revert(回到"复合语法只切段"),但那会重新引入上面那条功能回归;提交 2 可单独 revert(回退到只看首命令),提交 1 也可单独 revert(恢复开关转发),而只保留提交 2/3 而回退提交 1 会重新打开 fail-open 的窗口,建议一起收。

Refs #337

Author confirmation

The admin "allow Agent write actions" gate was forwarded to the edge as
BashExecRequest.Unrestricted for every command the manager classified as a
read. Unrestricted makes the edge bypass cmdpolicy and run the raw string
through a shell, so one misclassification turned the gate into an immediate
host-write path with no approval at all. That is how the session reported in
ongridio#337 reached a host without going through proposal, confirm, execute.

The gate only decides whether a mutating command may create a proposal.
Dispatch stays read-only, so a command the classifier misses is still denied by
the edge policy instead of running raw. Only RunApproved, which executes the
exact command the user approved, may send Unrestricted.

Update the two doc comments that described the old forwarding, and the test
that asserted it.
…ites

isHostBashWriteCommand only looked at the first token of the whole command, so
`echo x > /etc/hosts`, `ls; rm -f y`, `cat f | tee copy` and `echo $(rm y)` were
all classified as reads. On the dispatch path that meant they either skipped the
proposal flow entirely, or, before the previous commit, skipped edge policy too.

Split the command on shell separators and classify every segment. The split is
quote-aware: a `>` or `;` inside quotes stays a literal argument, so read
commands are not dragged into the approval inbox for nothing. Output
redirection and command substitution count as writes outright, because their
effect cannot be read off a command name. Leading NAME=value assignments and
sudo's value-taking options are skipped so that `LC_ALL=C rm x` and
`sudo -u nginx rm x` resolve to the real command instead of the token in front.

A background `&` on a read command still counts as a read; that is the reading
of the issue's "background execution" bullet that keeps `sleep 5 &` out of the
inbox. The classifier remains only a routing hint: whatever it misses now goes to
the edge in read-only mode and is denied by cmdpolicy.

Replaces the test that asserted redirect-plus-`&&` must bypass approval, and adds
a classifier table plus dispatch tests for the proposal path.
@Dragonzz27
Dragonzz27 requested a review from singchia as a code owner October 6, 2026 18:13
@youzi-1122

Copy link
Copy Markdown
Member

基于 c7e362e084942d7ce655f50d141fe23a0a24173e 的审查结论:修复方向和核心权限改动正确,建议补齐下面这一处 P2 功能问题后合并。

核心改动已确认:写开关只允许生成审批提案;未经审批的普通调用固定发送 Unrestricted=false;审批执行入口才使用 true,并执行保存的原命令和设备列表。

[P2] 复合读命令的分类结果与 Edge 支持的语法不一致

splitHostShellCommands 的分隔符分支只切分命令,没有把命令列表或后台语法标记为需要审批。如果各段都被判为读命令,整条命令会直接走 Edge 的受限执行路径。

用实际的 BashTool.InvokableRun 和 cmdpolicy.DefaultReadOnly().Decide 做了隔离验证,写开关开启时得到:

输入 实际结果
df -h && free -m 不生成审批,Unrestricted=false,Edge 返回 cmdpolicy: '&' is forbidden
df -h; free -m / `df -h
df -h 与 free -m 用换行分隔 不生成审批;Edge 将其解析为一条命令的 argv:["df", "-h", "free", "-m"]
`df -h head -5`

这会使原先在写开关开启时能执行的命令列表,在修复后无法正确执行。它是功能回归;本次没有确认新增的审批绕过问题。

建议保留普通调用固定 Unrestricted=false 的核心修复,将引号外的 ;、换行、&&、||、& 等 Edge 不支持的 shell 语法送入审批,普通只读管道继续走受限路径。

同时补一个使用真实 Edge cmdpolicy 的回归检查。当前 TestBashTool_ReadPipelineDispatchesReadOnly对 df -h && free -m 使用固定的 Allowed=true 响应,只能证明下发模式,无法证明实际可执行。

验证范围:相关 Linux go test -race 已通过(aiops/tools/...、edgeagent/cmdpolicy、edgeagent/bash、cmd/ongrid),审查时 6 项 GitHub 检查均通过。尚未进行真实 Agent 会话、审批界面和部署环境的端到端验证。

The classifier split semicolons, command lists, background operators and
newlines into simple segments and then judged only the first binary of each
segment. A read-only compound command such as `df -h && free -m` was
therefore classified as read and dispatched with Unrestricted=false, where
the edge tokenizer rejects it outright ("cmdpolicy: '&' is forbidden").
Commands that used to work while the write gate was on stopped working,
because the restricted path never supported them.

Mark every construct the edge parser refuses outside quotes as requiring
approval: ';' , '&&', '||', background '&', input and output redirects,
command substitution, '${}' parameter expansion and parentheses, plus
newline-separated commands that the edge joins into a single argv. A bare
pipe stays on the read path because it is the one compound syntax the edge
does support.

Drive both the read path and the approval path from the real cmdpolicy
parser and the DefaultReadOnly preset instead of a hard-coded Allowed
response, so the suite proves that a command classified as read is actually
executable on the edge rather than only that it was dispatched read-only.
@youzi-1122
youzi-1122 merged commit 7224375 into ongridio:main Oct 9, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants