Uh oh!
There was an error while loading. Please reload this page.
docs(security): state 8.8 per resource, because one blanket claim was never true (backend#2418) - #807
Conversation
… never true (backend#2418) 8.8 declared resource-exhaustion DoS out of scope BECAUSE "resources.limits are always applied". client-runtime#378 makes that false for CPU: the derived envelope now sets a cpu.weight share with no ceiling, so the sentence the section rests on stops holding the day it lands. The ticket requires the rewrite to ship with it. It was ALSO already overclaiming, independently of this change: the blanket property has never been true of disk. There is no ephemeral-storage request or limit anywhere and the resource grammar cannot express one (backend#2223). So the fix is not to patch the CPU clause -- it is to stop making one sweeping claim about three resources that are bounded in three different ways: memory hard limit, requests == limits, cgroup memory.max -> OOMKill CPU proportional share, NO ceiling, >=1/N under contention with N contenders; N bounded by the L4.1 cap (backend#2419), not here disk not bounded at all -- backend#2223 Also records why dropping the CPU ceiling does not widen the threat model: a quota bounds the CEILING, a weight bounds the FLOOR, and the floor is the property a victim actually needs. Plus memory untouched, eviction changing class rather than exposure, and the compute budget metering CPU_FLOPS_BENCHMARK x cpu_usage x time -- so burning CPU to degrade a rival spends the attacker's own allocation and gets them paused. The rejected alternative (a generous limit, which would have kept the old sentence literally true) is recorded with its reason: CPU quota throttles in bursts and hurts multi-threaded processes most, and our pods run N dataloader workers plus a torch thread pool. The sentence being replaced is one I wrote three hours ago in #805 -- "whichever path applies, a limit is always set -- this section does not depend on which one". It was true for the two paths that existed then and #2418 adds a third where it is not, which is exactly the coupling the ticket predicted. No Chart.yaml bump: docs/SECURITY.md is not packaged chart content, confirmed by running chart-version-guard.sh (guard N/A). Evidence: scripts/check-style.sh style + terminology clean make drift all 17 guards green scripts/gen-manifest.sh --check up to date Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
left a comment
There was a problem hiding this comment.
The rewrite is the right call — one thing to fix before it lands
Splitting one blanket claim into three resources bounded three different ways is correct, and the disk half is the part worth noticing: resources.limits are always applied was already false for disk long before backend#2418 came along. I checked — no ephemeral-storage anywhere in the chart outside docs/. So this isn't just reacting to #2418, it's fixing an overclaim that had been sitting there.
One finding, on the thread:client-runtime#378 is still open, so the CPU row describes behaviour no deployment has yet, in the present tense. And the qualifier it needs isn't temporary — this row describes node_sizing.py, which ships in a separately versioned image, so a customer on chart 1.9.66 with an older runtime keeps the hard quota however this file reads. "Land 807 with 378" doesn't make the row true for the fleet; only the image digest does. This file already solves that three times (:130, :546, and the ordering paragraph at :153) — the CPU row wants the same treatment.
Verified the rest rather than taking the table's word for it: priority-class.yaml exists and does designate training pods as the preemption victims to protect mysql, and the ≥1/N claim is sound because #378 gives every derived job the same cpu request with no limit, so weights really are equal. Keep the "equal weights" conditioning — a bare 1/N would be an overclaim the moment a differently-weighted pod shares the node.
Also: naming that the deleted sentence is one I approved in #805 this morning, rather than quietly overwriting it, is the right instinct. It was true for the two paths that existed then and #2418 adds a third — exactly the coupling that ticket predicted.
Not approving yet:Unit tests is still IN_PROGRESS. Once that's green and the CPU row carries a version qualifier, this is a straightforward approve from me.
@LukasWodka on #807, and the finding is deeper than the one it looks like. The obvious reading is "the row is premature because client-runtime#378 has not merged", fixable by merge ordering. It is not. The row describes node_sizing.py, which ships in the client-runtime IMAGE, and the chart and the runtime are versioned separately -- so a customer on chart 1.9.66 with an older runtime image keeps the hard CPU quota however this file reads, and upgrading the chart alone will never change that. Landing 807 with 378 would not have made the row true for the fleet; only the image digest does. So the qualifier is PERMANENT, not a merge-window note to delete later. Adopted in the shape he proposed and the shape this file already uses three times (:130, :546, and the ingestor-build ordering paragraph at :153, which is the closest precedent -- the same chart/runtime skew class): From client-runtime images containing #378: proportional share, no ceiling, >=1/N under contention. Earlier images apply requests == limits -- a hard quota. The intro paragraph carried the same defect in the past tense ("since backend#2418, is no longer true of CPU either") and is now conditioned the same way. A short paragraph under the table says why the qualifier is permanent, so nobody deletes it as stale once #378 merges. His other three checks were verification, not asks, and I have left them as they stand: disk really is unbounded (no ephemeral-storage in the chart outside docs/), priority-class.yaml really does designate training pods the preemption victims, and the ">=1/N" claim keeps its "equal weights" conditioning -- which he asked to KEEP as written, since a bare 1/N would overclaim the moment a differently-weighted pod shares the node. Evidence: scripts/check-style.sh style + terminology clean make drift all 17 guards green scripts/gen-manifest.sh --check up to date Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
LukasWodka
left a comment
There was a problem hiding this comment.
The qualifier landed, and better than what I proposed
6bcf9821 does the thing I asked and then makes it durable. Verified at the head:
- The CPU row now reads both directions — "From client-runtime images containing #378: proportional share, no ceiling … Earlier images apply
requests == limits— a hard quota" — with the enforcement column split to match (cpu.weighton newer,cpu.maxon earlier). - The lead paragraph moved from "since backend#2418, is no longer true of CPU" to "on client-runtime images containing #378 (backend#2418), is not true of CPU". That's the tense fix, not just a footnote.
- And you added the part I didn't ask for and should have: a standing paragraph saying the qualifier is permanent, because the row describes
node_sizing.pyin a separately versioned image — so it isn't a note about a merge window that gets deleted once #378 lands. That's the version of the sentence that stays true on a mixed fleet, which is the only form true of every deployment at once.
Citing the §4 ingestor-build ordering paragraph as the same treatment for the same reason is the right anchor — it makes this a convention the file already has rather than a one-off hedge.
Holding the approval only on CI: three checks still IN_PROGRESS at 6bcf9821 (Unit tests, Lint, Bugbot). Nothing outstanding from me — no open threads of mine, and my one finding is closed in the text. I'll approve next pass once it's green.
shujaatTracebloc
commented
Aug 24, 2026
CI is green at One correction to something I told you earlier, since it changes the record rather than just the wording: I had written down "merge #807 before #378" as a hard ordering constraint for the merge driver, on the theory that landing them together made the CPU row true. Your review showed that reasoning was wrong — the row describes a separately versioned image, so no merge order could have made it true for the fleet. I've retired that constraint rather than leave a stale "HOLD #378" instruction sitting there for a later automated pass to obey. The two can now land independently, in either order, which is the actual consequence of the qualifier. |
LukasWodka
left a comment
There was a problem hiding this comment.
Approving — green at 6bcf9821, and the qualifier is the durable version
Gate: 13/13 checks pass, MERGEABLE, zero unresolved threads, no standing change-request from anyone.
The rewrite was the right call before my finding and still is: splitting one blanket claim into three resources bounded three different ways. The half worth noticing is disk — "resources.limits are always applied" was false for disk long before backend#2418 came along, which I checked (no ephemeral-storage anywhere in the chart outside docs/). That's an overclaim that had been sitting in a customer-facing security document, and #2418 only exposed it.
My one finding is closed, and the fix went past what I asked for. I wanted the CPU row version-qualified because client-runtime#378 is still open. What landed also explains why the qualifier is permanent — the row describes node_sizing.py in a separately versioned image, so a customer on chart 1.9.66 with an older runtime keeps the hard quota however this file reads, and upgrading the chart alone will never change that. So it isn't a merge-window note to delete once #378 lands; it's the only form of the sentence that's true of every deployment at once. Anchoring it to the §4 ingestor-build ordering paragraph makes it a convention the file already has rather than a one-off hedge.
Verified the rest against the code rather than the table: priority-class.yaml does designate training pods as the preemption victims to protect mysql, and the ≥1/N claim holds because #378 gives every derived job the same cpu request with no limit, so the weights really are equal. Keeping the "equal weights" conditioning matters — a bare 1/N would overclaim the moment a differently-weighted pod shares the node.
One last thing worth recording. The sentence this deletes — "whichever path applies, a limit is always set" — is one I approved in #805 earlier today. It was true for the two envelope paths that existed then and #2418 adds a third where it isn't. Naming that in the PR description rather than quietly overwriting it is what lets the next reader trust the section's history, and it's a concrete argument for the qualifier convention: a flat claim about another repo's artifact goes stale on someone else's merge schedule.
Uh oh!
There was an error while loading. Please reload this page.
The paired half of backend#2418. Its policy change is in client-runtime#378; this is the
SECURITY.md§8.8 rewrite the ticket requires to ship with it. They're in different repos, so "the three parts must ship together" has to mean two PRs — please land this before or with #378, since it's the sentence that change falsifies.Why a rewrite rather than a patched clause
§8.8 declared resource-exhaustion DoS out of scope because"
resources.limitsare always applied". #378 makes that false for CPU — the derived envelope becomes acpu.weightshare with no ceiling.But it was already overclaiming, independently of #2418: the blanket property has never been true of disk. There's no
ephemeral-storagerequest or limit anywhere, and the resource grammar can't express one (backend#2223). One sweeping claim about three resources bounded in three different ways is the actual defect, so the section is now a table:requests == limitsmemory.max→ OOMKill. Unchanged, load-bearingcpu.weight; N bounded by the L4.1 cap (#2419)Why dropping the CPU ceiling doesn't widen the threat model
Recorded in the section, because a reviewer will ask:
CPU_FLOPS_BENCHMARK × cpu_usage × time, so degrading a rival spends the attacker's own allocation and gets them paused.The rejected alternative is recorded too: a generous
limitsfraction would have kept the old sentence literally true, but CPU quota throttles in bursts and hurts multi-threaded processes most — our pods run N dataloader workers plus a torch thread pool.Note on authorship
The sentence being replaced — "whichever path applies, a limit is always set — this section does not depend on which one" — is one I wrote a few hours ago in #805. It was true for the two paths that existed then, and #2418 adds a third where it isn't. That coupling is exactly what #2418's ticket predicted, so this isn't a surprise, but it's worth naming rather than quietly overwriting.
Evidence
scripts/check-style.shmake driftscripts/gen-manifest.sh --checkchart-version-guard.shdocs/isn't packaged chart content, so noChart.yamlbumpDocs-only; no template, values, or schema change.
🤖 Generated with Claude Code
Note
Low Risk
Documentation-only rewrite of a residual-risk section; no chart, template, or runtime behavior changes.
Overview
Corrects
docs/SECURITY.md§8.8, which previously claimedresources.limitsare always applied. That was never true for disk, and it becomes false for CPU on client-runtime images with #378 (backend#2418).The section now tables memory (hard cgroup limit, unchanged), CPU (share with no ceiling on newer images; hard quota on older ones), and disk (unbounded). It explains why the CPU row stays version-qualified (chart vs runtime versioning) and why dropping the CPU ceiling does not widen the threat model: memory isolation, ≥1/N share under contention, billed CPU burn, and operator-pinned limits still applying a hard ceiling.
Reviewed by Cursor Bugbot for commit 6bcf982. Bugbot is set up for automated code reviews on this repo. Configure here.