Uh oh!
There was an error while loading. Please reload this page.
feat(seal-check): enumeration label contract + storage-assertions test hook (RFC-0003 §8.2–8.4) - #378
Conversation
…0003 S8.2 / backend#1184) Establish the seal-check enumeration contract and fill the storage gap in the chart conformance suite: - Label every conformance helm-test Job (and its pods) with tracebloc.io/seal-check="true" + tracebloc.io/seal-check-name=<check> -- the contract the tracebloc CLI consumes (cli#393). Applied to the existing egress-enforcement probe and backend-reachability check via a new tracebloc.sealCheckLabels helper. - New storage-assertions helm-test Job (sealCheck.storageAssertions.*, default on, fully nil-guarded for --reuse-values upgrades): release PVCs Bound on the expected StorageClass; in dynamic-PVC mode (hostPath.enabled=false) no release PVC backed by an unmanaged hostPath PV -- catches the RFC-0003 D3/D4 leftover-PV claimRef capture. k3s local-path node-local paths tolerated and reported (Option C); clusterScope=false degrades the PV scan to a name check with an explicit WARNING, never silently. - Least-privilege SA/RBAC as negative-weight test hooks (PVC read in the namespace; PV read only when clusterScope allows a ClusterRole). - docs/SEAL-CHECK.md: label contract, coverage today, UNSEALED philosophy, per-substrate coverage view, and the k3d/k3s NetworkPolicy egress verification runbook (documented honestly as not yet executed). - 20 new helm-unittest cases; 287/287 pass, helm lint --strict clean on all four platform values, check-drift clean. Part of tracebloc/backend#1184 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka
commented
Jul 22, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Jul 22, 2026
bugbot run |
Uh oh!
There was an error while loading. Please reload this page.
… WARNING after legacy FAIL Two Bugbot findings on the storage-assertions check (PR #378): - The pv-hostpath allowlist matched entries with a bare "$prefix"* glob, and the default /opt/local-path-provisioner entry carries no trailing slash, so sibling trees like /opt/local-path-provisioner-evil/... were accepted as node-local and could pass the storage seal. Entries are now normalized (trailing slash stripped) and matched on whole path segments: a prefix admits exactly itself or paths under it, never siblings. The default entry in values.yaml / template dig fallback / schema / docs is normalized to the trailing-slash style; either entry style now behaves identically. - With clusterScope=false (no PV read), a claim captured by a chart legacy hostPath PV printed FAIL and then still printed the PARTIAL verification WARNING claiming the claim was "verified not bound" to a legacy PV. The WARNING is now suppressed for a claim whose legacy-name check just FAILed - the FAIL alone is the honest verdict there; claims that pass the name check keep the WARNING unchanged. Validated: helm lint --strict on the 4 CI platform values; helm template (near-default / node-local / flag off / sealCheck: null); helm unittest 289/289 (2 new cases pin both guards); sh -n + shellcheck on the rendered script; mocked-kubectl runs covering sibling-path FAIL (both entry styles), exact-prefix-root OK, legacy FAIL without WARNING, no-legacy WARNING x3, unmanaged-tree FAIL, PV-read-error FAIL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
LukasWodka
commented
Jul 23, 2026
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8c37038. Configure here.
shujaatTracebloc
commented
Jul 24, 2026
Review — seal-check suite (RFC-0003 §8.2–8.4)Read the real 11-file diff and the embedded kubectl script line by line, and verified the referenced helpers + schema. All CI green (helm lint / unittest 287, 4-platform renders, Bugbot pass). Net: high-quality, security-sound — good to merge after a rebase on develop; the two edges below are optional, none blocking. What's strong
Minor edges (optional)
Cosmetic: when a legacy-PV FAIL fires and — @shujaatTracebloc (reviewed with Claude) |
LukasWodka
commented
Jul 24, 2026
Reviewed the seal-check suite end to end — the render contract, script logic, RBAC minimalism, and fail-loud semantics all hold up, and the two Bugbot findings are genuinely fixed with regression pins. LGTM with nits; one real (docs-only) defect: 1. [Docs, real footgun] The single-check filter formula silently no-ops for backend-reachability. 2. [Process] Rebase before merge. The branch is 13 commits behind develop, and develop's #368 (node-local storage) changes the very storage model 3. [Housekeeping] Cluster-scoped hook RBAC can orphan. After a failed 4. [Nit] 5. [Test honesty, acknowledged] The Bugbot behavior fixes are pinned via Also verified against @shujaatTracebloc's notes: the — |
The generic '--filter name=<release>-<check>-check' formula silently no-ops for backend-reachability (its Job is egress-reachability-check): helm test matches zero hooks and exits 0 -- a silent pass. List the literal Job name per check instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka
commented
Jul 24, 2026
Review follow-ups done on this branch:
— |
LukasWodka
commented
Jul 24, 2026
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 232d10e. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
LukasWodka
commented
Jul 25, 2026
Functional review \u2014 passed\n\nBasis: the behavioural suites that ran on this PR at merge against real environments, not mocks:\n\n- + + \u2014 a real chart install and auto-upgrade cycle on a live k3d cluster\n\nI could not reach the dev API (no dev credentials), so rather than rubber-stamp I used the strongest evidence available: these suites exercise the actual behaviour this change alters, on real infrastructure. Advancing \u2192 .\n\nIf the functional reviewer wants a manual pass on dev in addition, please move it back and say so. |
…#199-1) (#537) Test-only, no behavior change — the regression net for the egress-lockdown default flip (#199-2). The egress mechanism is fully shipped (client-runtime#102 / #378); #199 is just flipping allowExternalHttps to false by default, which drops the rule-2 external-HTTPS hole and shifts the remaining egress indices. - Pin allowExternalHttps: true in the three cases that assert the rule-2 hole or the MySQL rule at a rule-2-dependent index (external-443, MySQL, OpenShift), so they keep passing once the default flips. - Add two locked-down cases (allowExternalHttps: false): the 0.0.0.0/0 rule is gone (MySQL shifts to egress[1]), the UNCONDITIONAL requests-proxy egress (8888) survives so pods can still POST results/FLOPs, and the egress-proxy allowlist rule (3128) renders when egressProxy.enabled. 13 network-policy tests (was 11), 322 helm suite green. Lands first so #199-2 is a green-to-green flip. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@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>
… never true (backend#2418) (#807) * docs(security): state 8.8 per resource, because one blanket claim was 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> * docs(security): version-qualify the CPU row, permanently (backend#2418) @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> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Summary
Chart half of the RFC-0003 D12 seal check (§8.2–8.4): establishes the seal-check enumeration contract and fills the storage-assertions gap, without touching
scripts/(installer work stays with #368).1. The enumeration contract (consumed by tracebloc/cli#393)
Every runnable conformance check is a
helm.sh/hook: testJob carrying, on the Job and its pod template:tracebloc.io/seal-check"true"tracebloc.io/seal-check-nameegress-enforcement|backend-reachability|storage-assertionsEnumerate without running:
helm get hooks <release>. While running:kubectl get jobs,pods -l tracebloc.io/seal-check=true. Label keys + existing names are public API (never renamed; new checks = new names). Auxiliary hook resources (the new SA/RBAC) deliberately do not carry the labels — only runnable checks are enumerable. Applied to the existing egress-enforcement probe + backend-reachability check via a newtracebloc.sealCheckLabelshelper.2. New check:
storage-assertions(sealCheck.storageAssertions.*, default on)kubectl-image test-hook Job (same
alpine/k8s:1.30.5the image-refresh CronJob uses) asserting from in-cluster:client-pvc/client-logs-pvc/mysql-pvcexist and areBound(bounded wait, default 120s, forWaitForFirstConsumer+ first-install pulls)tracebloc.storageClassName)hostPath.enabled=false, only) — no release PVC backed by an unmanaged hostPath PV. Catches the RFC-0003 D3/D4 leftover-PVclaimRefcapture (a stale<release>-{data,logs,mysql}-pvgrabs the claim even in dynamic mode). k3s local-path node-local paths (nodeLocalPathPrefixes, default/var/lib/rancher/+/opt/local-path-provisioner) are tolerated and reported — they live inside the cluster node and die with it (Option C); host-visibility of those paths is a cluster-creation fact verified at install level. In hostPath mode the sub-check SKIPs (declared model).RBAC: dedicated least-privilege SA as negative-weight test hooks — PVC get/list in the namespace; PV get/list only when
clusterScopeallows a ClusterRole. WithclusterScope=falsethe PV scan degrades to the name/claimRef check and prints an explicit WARNING naming what was not verified.UNSEALED philosophy (documented in
docs/SEAL-CHECK.md): a guarantee that cannot be verified fails loudly — never silently claimed sealed. Disabling a check is a values-visible declaration, not a runtime fallback.3. Docs
docs/SEAL-CHECK.md— contract, coverage today, philosophy, per-substrate coverage table (chart-side input to the RFC §8.3 matrix), and the k3d/k3s NetworkPolicy egress verification runbook (§8.4) using the existing enforcement probe — documented honestly as not yet executed; the k3d cell stays "verification run pending" until a run is recorded. Cross-linked fromdocs/SECURITY.md§7.Compatibility
default dict/dig) —helm upgrade --reuse-valuesfrom releases predatingsealCheckrenders identically (unit-tested withsealCheck: null).hostPath.enabled, verifies whichever model the install declares.Test plan
helm lint --strictPASS on all 4 platform values (aks / bm / eks / oc)helm templateclean: 4 CI platforms + near-default values + node-local values (hostPath.enabled=false,local-pathclass) + flag off + nilsealCheck/ nilstorageAssertionshelm unittest: 287/287 (20 new cases: rendering gates, hook annotations, label contract on Job+pod, RBAC split underclusterScope, env baking, PSA-restricted fields, image pin, nil-block survival)sh -n+shellcheckclean; exercised with a mocked kubectl across 9 scenarios (happy dynamic / node-local PV / unmanaged hostPath / legacy-PV capture / wrong class / PV read error / namespace-scoped partial / hostPath mode / never-Bound) — verdicts as designedscripts/tests/check-drift.sh: no drift (run only, not modified)Deliberately left for follow-ups (tracked in backend#1184)
~/.traceblochost-tree check — not observable in-cluster; CLI/installer offboard-verify lineage (cli#389)helm template --set hostPath=nullcrashes on develop too (shared-images-pvc.yamlunguarded deref) — orthogonal to this PRPart of tracebloc/backend#1184
🤖 Generated with Claude Code
Note
Medium Risk
New default-on helm test hook talks to the API and may create cluster-scoped PV read RBAC when clusterScope is true; label keys/names are a stable public contract, but hooks still never run on install/upgrade.
Overview
Introduces the seal-check conformance suite for RFC-0003: runnable checks stay
helm.sh/hook: testJobs labeledtracebloc.io/seal-check=trueandtracebloc.io/seal-check-name=<check>on the Job and pod (public API for CLI enumeration viatracebloc.sealCheckLabels). egress-enforcement and backend-reachability adopt those labels; auxiliary hook RBAC is excluded.Adds default-on
storage-assertions: a kubectl-based test Job (plus least-privilege SA/RBAC test hooks) that waits for release PVCs to bind, asserts the expected StorageClass, and in dynamic-PVC mode rejects unmanaged hostPath backing (legacy chart PV names, optional full PV scan whenclusterScopeallows). Node-local k3s paths use segment-safe prefix allowlisting; unverifiable guarantees fail the Job (UNSEALED).Documents the contract and runbooks in
docs/SEAL-CHECK.md, wiressealCheck.storageAssertionsin values/schema, extends helm unittests, and links fromdocs/SECURITY.md§7.Reviewed by Cursor Bugbot for commit 232d10e. Bugbot is set up for automated code reviews on this repo. Configure here.