Fix: bound daemon HTTP connections and avoid protobuf lock copies - #658
Fix: bound daemon HTTP connections and avoid protobuf lock copies#658GOLDKUN wants to merge 3 commits into
Conversation
|
PR Title: Fix: bound daemon HTTP connections and avoid proto... Commit: 本次改动为守护进程的 HTTP 服务器添加连接级限制，并修正了 e2e 测试中的请求对象共享问题：
总体评估：防御性配置方向合理，e2e 测试改动是纯加固。主要风险点在于新增的 HTTP/2 超时可能中断守护进程核心的长连接 attach bidi 会话（客户端短暂不可达 >45s 或消费停滞 >30s 时连接会被主动关闭），需要确认其与交互式长会话的兼容性。 |
![monkeyscan[bot]](https://avatars.githubusercontent.com/u/231811742?s=60&v=4){kind=small}
|
已按最新评论修订：
本地 |
|
PR Title: Fix: bound daemon HTTP connections and avoid proto... Commit: 本次改动将 cmd/agent-compose 守护进程 HTTP 服务(daemonServers.add)中 h2c.NewHandler 的 http2.Server 从带显式超时(IdleTimeout 2m、ReadIdleTimeout 30s、PingTimeout 15s、WriteByteTimeout 30s)改为零值 &http2.Server{},并在测试中新增断言 http.Server.WriteTimeout 保持 0。 设计意图:撤销此前为连接"加固"而添加的 HTTP/2 层超时,避免其主动中断长时间运行的 Connect attach bidi 会话——这与本会话捆绑的历史 finding(66a7dab3,confirmed)所指问题一致,方向正确。清零后,静默/空闲的长连接不再被 HTTP/2 层读空闲探测与 idle GOAWAY 打断;http.Server 层仍保留 ReadHeaderTimeout 5s、IdleTimeout 2m、MaxHeaderBytes 64KB,无活动流的空闲连接仍会被回收,http.Server.WriteTimeout 为 0 也允许流长期存活。 总体评估:改动小且聚焦,基本修复了已确认的回归。两点需留意:(1) x/net 中 WriteByteTimeout 取零值时回退为默认 15s,写阻塞断连阈值由 30s 收紧到 15s(仅影响完全停读的慢消费者);(2) 移除 ReadIdleTimeout 后,有活动流但对端已静默死亡(如非本机网络断连)的连接失去 HTTP/2 层快速健康检查,只能依赖 OS TCP keepalive。主要不足是新增测试并未真正覆盖被清零的 http2.Server 配置(详见提交的 finding),无法防止已确认的长连接回归再次出现。 |
| } | ||
| if server.WriteTimeout != 0 { | ||
| t.Fatalf("server WriteTimeout = %s, want zero for long-running streams", server.WriteTimeout) | ||
| } |
There was a problem hiding this comment.
新增测试未覆盖实际改动(http2.Server 超时清零),已确认的长连接回归仍无测试保护
本次改动的实际行为变化是把 h2c.NewHandler 的 http2.Server 配置由带显式超时(IdleTimeout 2m、ReadIdleTimeout 30s、PingTimeout 15s、WriteByteTimeout 30s)改为零值 &http2.Server{},以避免 HTTP/2 层超时主动中断长时间运行的 Connect attach bidi 会话——这正是历史已确认问题(66a7dab3)所指向的回归,本次为修复。但新增测试只断言 http.Server.WriteTimeout == 0,该字段本就不曾设置、保持 0,并不属于本次改动;真正被清零的 http2.Server 字段被封装在 h2c.NewHandler 内部,现有测试(包括 TestDaemonTCPServerAttachAgentRunBidiUsesH2C)都无法感知。若后续有人再次以"加固"名义给 http2.Server 加上 ReadIdleTimeout/WriteByteTimeout/IdleTimeout,测试仍会全部通过,该文件历史上已经发生过一次反复(先加超时、本次移除),因此回归风险是具体且已被证实的。另外需注意:x/net 对零值 http2.Server 会把 WriteByteTimeout 回退为默认 15s,即写阻塞断连阈值从 30s 收紧到 15s,与该改动"保护长流"的目标存在细微张力,值得显式决策并在测试/注释中固化。
Problem code:
Changed code at cmd/agent-compose/cli_daemon_server_test.go:36-38
Recommendation:
为本次行为补充能真正兜底的回归保护:(1) 把传入 h2c.NewHandler 的 *http2.Server 保存在 daemonServer 结构上,并在 TestDaemonServerConfiguresConnectionLimits 中断言其 ReadIdleTimeout/IdleTimeout/WriteByteTimeout 为零(或与显式意图一致);或(2) 增加集成测试——经 h2c 打开一个 Connect bidi 流并保持静默超过原 ReadIdleTimeout+PingTimeout 窗口(约 45s),断言流仍存活且连接未被服务端关闭。同时建议显式设置或注释说明 WriteByteTimeout 零值回退 15s 的语义,避免后续误以为"未设置即无写超时"。
|
已按最新评论再次修订：
本地 |
|
PR Title: Fix: bound daemon HTTP connections and avoid proto... Commit: 本次变更涉及 cmd/agent-compose/cli_daemon.go 与对应测试文件，共 2 个文件。 变更内容：在 daemonServer 结构体中新增 h2c *http2.Server 字段；将 add() 中 h2c.NewHandler 原本内联使用的匿名 &http2.Server{} 提取为命名变量 h2cConfig，并同时传给 h2c.NewHandler 和存入结构体；新增注释说明必须保持 HTTP/2 连接超时禁用，因为该服务承载 Connect 长连接双向流，空闲属于协议一部分。测试文件新增断言 servers.items[0].h2c 的 IdleTimeout/ReadIdleTimeout/PingTimeout/WriteByteTimeout 全为零。 评估：这是一次行为中性的重构 + 回归防护测试。生产运行路径上 http2.Server 仍为零值配置（各超时禁用），与改动前完全一致，无行为回归；新增结构体字段仅用于让测试观测实际注入 handler 的配置实例，避免后续维护者为 h2c 配置加入超时而破坏长连接流。http.Server 上的 IdleTimeout=2 分钟仍保留，但 h2c 连接在首个请求即被 hijack 后由 http2.Server 接管，不受该字段影响，两者并不矛盾。测试断言与注释意图一致，未发现可操作的正确性、安全或可靠性问题。 |
1 participant
问题
Daemon HTTP servers had no read-header, idle-connection, or header-size limits, leaving the control plane exposed to slowloris and idle HTTP/2 resource exhaustion. Separately, an e2e test copied a Protobuf request value containing the generated
MessageStatelock, whichgo vetcorrectly rejected.影响
攻击者可以保持慢速或空闲连接，消耗 daemon 的连接和 goroutine 资源，导致控制面拒绝服务。按值复制 Protobuf messages 还可能复制内部同步状态并产生竞态风险。涉及 CWE-400；静态检查问题属于工程健壮性缺陷。
修复内容
ReadHeaderTimeout、IdleTimeout和MaxHeaderBytes。WriteTimeout。proto.Clone替代按值复制StopSandboxRequest。验证
git diff --check说明：测试包的完整编译当前仍被 main 基线中未同步的 Protobuf 生成文件阻塞，表现为缺少
K8SDriverSpec、RunTimeout等生成符号；本 PR 仅修复 vet 报告的按值复制问题，未改动该独立生成基线。