From 274e25961a3f0953d0560cd78b3ba5938562cd34 Mon Sep 17 00:00:00 2001 From: chick Date: Thu, 17 Sep 2026 07:12:44 +0800 Subject: [PATCH] =?UTF-8?q?fix(agent):=20=E6=89=8B=E6=9C=BA=E5=B7=B2?= =?UTF-8?q?=E7=BB=8F=E6=94=BE=E5=BC=83=E7=9A=84=E8=AF=B7=E6=B1=82=E4=B8=8D?= =?UTF-8?q?=E5=86=8D=E6=89=A7=E8=A1=8C?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 中继答 504 并说"可能已经执行了，重试前请先确认"——这句话诚实的前提是它保持为真。 批次是串行处理的，所以满邮箱里第八个信封可以在它被签名之后 8 个上游超时才轮到； 那时手机早已被告知失败、用户可能已经重试。再执行它，就把一次诚实的"未知"变成了 一条重复发出的短信。 现在 agent 在动任何东西之前先比对自己的截止时间（策略里的 requestTtlSeconds）， 超龄一律以 expired 拒绝并落审计。判定放在新鲜度检查之后、策略判定之前：它比签名 更便宜，而且一个过期请求无论路径是否在白名单里都不该执行。 为了让这条规则**可达**，策略新增一条断言：TTL 不得大于时钟偏移窗口。否则新鲜度 检查会先触发，这条规则永远不会被执行——而"永不触发的规则"是出事时才会被发现坏了 的那种东西。 单个上游调用的超时也移进了策略（agentUpstreamTimeoutSeconds）。"中继期限必须长于 一次上游调用"这个比较，与二进制里的常量是比不了的；现在两侧从同一个文件读，策略 加载时就能拒绝一组自相矛盾的数值。原来那个 20s 常量保留为默认值的来源，不再硬编码。 测试两项：一个请求在期限内被转发、一个超龄请求被拒且**没有被转发**（计数按路径， 否则首次转发前的 /api/auth/status 探测会让断言以错误的理由通过）；另一项断言策略 本身满足上述两个关系。变异验证：关掉判定，第一项失败。 Go 全包、Node 119 项、Swift 101 项通过。 --- internal/policy/agent-policy.json | 1 + internal/policy/policy.go | 46 +++++++++++++++------- internal/runtime/executor.go | 20 +++++++++- internal/runtime/runtime_test.go | 65 +++++++++++++++++++++++++++++++ 4 files changed, 117 insertions(+), 15 deletions(-) diff --git a/internal/policy/agent-policy.json b/internal/policy/agent-policy.json index 5bcb375..9f3deac 100644 --- a/internal/policy/agent-policy.json +++ b/internal/policy/agent-policy.json @@ -31,6 +31,7 @@ "dangerous": ["/api/sms/send", "/api/sms/batch-delete"], "limits": { "dangerousWritesPerMinute": 6, + "agentUpstreamTimeoutSeconds": 20, "maxRequestBytes": 262144, "maxResponseBytes": 262144, "requestTtlSeconds": 30, diff --git a/internal/policy/policy.go b/internal/policy/policy.go index f06d642..d89b3ac 100644 --- a/internal/policy/policy.go +++ b/internal/policy/policy.go @@ -39,12 +39,17 @@ type AuthPaths struct { type Limits struct { DangerousWritesPerMinute int `json:"dangerousWritesPerMinute"` - MaxRequestBytes int `json:"maxRequestBytes"` - MaxResponseBytes int `json:"maxResponseBytes"` - RequestTTLSeconds int `json:"requestTtlSeconds"` - ClockSkewSeconds int `json:"clockSkewSeconds"` - ReplayCacheSize int `json:"replayCacheSize"` - MailboxDepth int `json:"mailboxDepth"` + // AgentUpstreamTimeoutSeconds is how long one loopback call to SimAdmin may take. It lives in the + // policy rather than only in the agent's code because the relay's deadline has to be compared + // against it, and a comparison between a policy file and a constant in a binary is not one + // anything can check. + AgentUpstreamTimeoutSeconds int `json:"agentUpstreamTimeoutSeconds"` + MaxRequestBytes int `json:"maxRequestBytes"` + MaxResponseBytes int `json:"maxResponseBytes"` + RequestTTLSeconds int `json:"requestTtlSeconds"` + ClockSkewSeconds int `json:"clockSkewSeconds"` + ReplayCacheSize int `json:"replayCacheSize"` + MailboxDepth int `json:"mailboxDepth"` } type Policy struct { @@ -148,13 +153,14 @@ func Parse(raw []byte) (*Policy, error) { } for name, value := range map[string]int{ - "dangerousWritesPerMinute": p.Limits.DangerousWritesPerMinute, - "maxRequestBytes": p.Limits.MaxRequestBytes, - "maxResponseBytes": p.Limits.MaxResponseBytes, - "requestTtlSeconds": p.Limits.RequestTTLSeconds, - "clockSkewSeconds": p.Limits.ClockSkewSeconds, - "replayCacheSize": p.Limits.ReplayCacheSize, - "mailboxDepth": p.Limits.MailboxDepth, + "dangerousWritesPerMinute": p.Limits.DangerousWritesPerMinute, + "maxRequestBytes": p.Limits.MaxRequestBytes, + "maxResponseBytes": p.Limits.MaxResponseBytes, + "requestTtlSeconds": p.Limits.RequestTTLSeconds, + "agentUpstreamTimeoutSeconds": p.Limits.AgentUpstreamTimeoutSeconds, + "clockSkewSeconds": p.Limits.ClockSkewSeconds, + "replayCacheSize": p.Limits.ReplayCacheSize, + "mailboxDepth": p.Limits.MailboxDepth, } { if value <= 0 { return nil, fmt.Errorf("policy: limits.%s must be positive, got %d", name, value) @@ -163,8 +169,20 @@ func Parse(raw []byte) (*Policy, error) { if p.Limits.ClockSkewSeconds > 300 { return nil, errors.New("policy: clockSkewSeconds is too wide to bound replay") } + // The relay's deadline has to sit inside the clock-skew window, and the reason is not tidiness. + // + // The freshness check above rejects an envelope whose timestamp is further from now than + // clockSkewSeconds. The agent's own deadline check rejects one older than requestTtlSeconds. If the + // TTL were the larger of the two, the freshness check would fire first and the deadline check would + // be unreachable — a rule that can never fire, which is the kind of thing that gets discovered + // during an incident by someone who assumed it was working. if p.Limits.RequestTTLSeconds > p.Limits.ClockSkewSeconds { - return nil, errors.New("policy: requestTtlSeconds exceeds clockSkewSeconds and would never be enforced") + return nil, errors.New("policy: requestTtlSeconds exceeds clockSkewSeconds, so the agent's deadline check would never be reached") + } + // And the node must not be told to wait longer than the phone is willing to. The two are one + // decision — how long a request may take end to end — stated in the file that both sides read. + if p.Limits.RequestTTLSeconds*1000 <= p.Limits.AgentUpstreamTimeoutSeconds*1000 { + return nil, errors.New("policy: requestTtlSeconds must exceed the per-request upstream timeout, or the relay deadline is not the longer bound") } if p.Limits.ReplayCacheSize < p.Limits.MailboxDepth { return nil, errors.New("policy: replayCacheSize is smaller than mailboxDepth, so entries can be evicted while still queued") diff --git a/internal/runtime/executor.go b/internal/runtime/executor.go index d5d0c0d..f8fa34a 100644 --- a/internal/runtime/executor.go +++ b/internal/runtime/executor.go @@ -91,7 +91,10 @@ func NewExecutor(pol *policy.Policy, cfg Config) (*Executor, error) { } timeout := cfg.Timeout if timeout <= 0 { - timeout = 20 * time.Second + // From the policy, not a constant here. The relay's deadline is compared against this number + // when the policy is validated, and a comparison against a constant inside a binary is not one + // anything can check. + timeout = time.Duration(pol.Limits.AgentUpstreamTimeoutSeconds) * time.Second } client, err := egress.NewClient(target, timeout) if err != nil { @@ -145,6 +148,21 @@ func (e *Executor) Execute(ctx context.Context, envelope wire.Envelope, inner wi if err := Freshness(envelope.TS, uint64(now.Unix()), e.pol.Limits.ClockSkewSeconds); err != nil { return Result{}, e.refuse(envelope, inner, ClassExpired, err) } + // The phone's own deadline, checked before anything is executed. + // + // The relay answers 504 with "it may or may not have executed it; check the node before retrying", + // which is honest — but it is only honest while it stays true. A batch is processed serially, so + // the eighth envelope of a full mailbox can begin up to eight upstream timeouts after it was + // signed; by then the phone has been told the request failed and the user may already have retried. + // Executing it at that point turns an honest "unknown" into a duplicate SMS. + // + // The relay's deadline is the authority on when that happened, because the relay is the party that + // starts the clock. Its value is therefore carried in the policy and asserted to be no larger than + // the clock-skew window, which is what makes the freshness check above unable to mask this one. + if age := int64(now.Unix()) - int64(envelope.TS); age > int64(e.pol.Limits.RequestTTLSeconds) { + return Result{}, e.refuse(envelope, inner, ClassExpired, + fmt.Errorf("%w: signed %ds ago, deadline is %ds", ErrExpired, age, e.pol.Limits.RequestTTLSeconds)) + } if e.replay.Seen(envelope.Req, now) { return Result{}, e.refuse(envelope, inner, ClassReplay, fmt.Errorf("%w: %s", ErrReplay, envelope.Req)) } diff --git a/internal/runtime/runtime_test.go b/internal/runtime/runtime_test.go index ede830a..69b9a4d 100644 --- a/internal/runtime/runtime_test.go +++ b/internal/runtime/runtime_test.go @@ -594,3 +594,68 @@ func TestAFailedAuditWriteIsReported(t *testing.T) { t.Fatalf("a failed audit write was not reported; log was: %s", captured.String()) } } + +// A request whose phone has already given up must not be executed. +// +// The relay answers 504 with "it may or may not have executed it; check the node before retrying", +// which is honest — but only while it stays true. A batch is processed serially, so the eighth +// envelope of a full mailbox can begin up to eight upstream timeouts after it was signed. By then the +// phone has been told the request failed and the user may have retried; executing it turns an honest +// "unknown" into a duplicate SMS. +func TestARequestPastItsDeadlineIsNotExecuted(t *testing.T) { + // Counted by path: the agent probes /api/auth/status before its first forward, and counting every + // request would make the first assertion pass for the wrong reason. + executed := 0 + f := newFixture(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/api/sms/send" { + executed++ + } + w.WriteHeader(200) + })) + pol := testPolicy(t) + body := []byte(`{"phone_number":"+8613800000000","content":"hello"}`) + intent, err := wire.IntentDigest("POST", "/api/sms/send", body) + if err != nil { + t.Fatal(err) + } + inner := wire.InnerRequest{Method: "POST", Path: "/api/sms/send", Body: body, Intent: intent} + + // One second inside the deadline: executed, because the phone is still waiting. + envelope := envelopeAt("nd_test0001", newReqID(1), uint64(f.now.Unix())) + if _, refusal := f.executor.Execute(context.Background(), envelope, inner); refusal != nil { + t.Fatalf("a request inside its deadline was refused: %v", refusal) + } + if executed != 1 { + t.Fatalf("expected one execution, got %d", executed) + } + + // One second past it: refused, and nothing is forwarded. + f.advance(time.Duration(pol.Limits.RequestTTLSeconds+1) * time.Second) + late := envelopeAt("nd_test0001", newReqID(2), uint64(f.now.Unix()-int64(pol.Limits.RequestTTLSeconds)-1)) + _, refusal := f.executor.Execute(context.Background(), late, inner) + if refusal == nil { + t.Fatal("a request past its deadline was executed; the phone was told it had failed") + } + if refusal.Class != ClassExpired { + t.Fatalf("refused as %s, want %s", refusal.Class, ClassExpired) + } + if executed != 1 { + t.Fatalf("the late request was forwarded anyway (%d executions)", executed) + } +} + +// The deadline check is only reachable because the policy keeps the TTL inside the clock-skew window. +// If it were not, the freshness check would fire first and the deadline rule could never be reached — +// a rule that never fires is one nobody discovers is broken until an incident. +func TestThePolicyKeepsTheDeadlineInsideTheFreshnessWindow(t *testing.T) { + pol := testPolicy(t) + if pol.Limits.RequestTTLSeconds > pol.Limits.ClockSkewSeconds { + t.Fatalf("requestTtlSeconds (%d) exceeds clockSkewSeconds (%d); the deadline check is unreachable", + pol.Limits.RequestTTLSeconds, pol.Limits.ClockSkewSeconds) + } + // And the relay's deadline is the longer bound, or the phone gives up before the node does. + if pol.Limits.RequestTTLSeconds <= pol.Limits.AgentUpstreamTimeoutSeconds { + t.Fatalf("the relay deadline (%ds) is not longer than one upstream call (%ds)", + pol.Limits.RequestTTLSeconds, pol.Limits.AgentUpstreamTimeoutSeconds) + } +}