fix(server): harden the kubernetes builder pod - #425
Conversation
Verification
Review focus
Follow-up
|
|
All four findings below are already fixed on this branch — this report is published for the record, with the fixing commit named on each. MAJOR 2 and the remaining #422 MINORs/NITs are left to the maintainer, as stated in the PR description.
Code Review Report — #425Base: Reviewer: not the diff's author. Two independent advisors consulted (OpenAI GPT-5, Anthropic Claude Fable) on the security default and the deadline arithmetic. Verdict
DoD Criteria (inferred from the PR body)
IssuesMAJOR 1 — the token fix does not cover the configuration the docs recommend
The stated reason is factually wrong. IRSA injects a separate Setting MINOR 2 — the documented default is not the implemented default
All four say the default is "the release task's timeout ( NIT 3 — the slack comment is truncated and gives the wrong reason
"…so the deadline only ever fires when the plugin is no longer around to." — the sentence ends mid-clause. It is also the wrong rationale: the pod deadline is anchored at pod NIT 4 — the 1-minute floor is unreachable, and a test asserts the unreachable case
Verification
Not findings (checked)
ANCHORS: Verification evidenceAll checks ran through
Both behavior fixes were mutation-tested on a committed tree, each mutation reverted with a path-scoped
Three further review passes over the fix range produced no CRITICAL and no MAJOR. They did produce four documentation corrections against my own wording, all applied: the security bullet first contradicted itself (it told operators to audit RBAC bindings that are reachable only through the token the next sentence said was never mounted), then overstated the result as "not reachable from inside the build" when the very next bullet describes a privileged container that can escape to node-level credentials — it now scopes the claim to the withheld credential and names that residual — plus three RU register slips and the removal of a vendor-mechanism claim this repo's CI never exercises. Commits Recorded but deliberately NOT fixed, as it falls outside the four findings and into the base PR range ( UNVERIFIED, unchanged from the PR description: behavior on a real cluster. The kind e2e job was not run locally; CI is the gate. |
4b40e9a to
537c2ea
Compare
alexey-igrychev
changed the title
The builder pod runs project-supplied build instructions in a privileged container. With no ServiceAccount of its own it was still given the namespace's default token, so a build could reach the API server with whatever that account carries. Disable the automount in that case. A configured serviceaccount keeps the cluster default instead of an explicit false, because IRSA, EKS Pod Identity and workload identity all deliver their credentials through the mounted token. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
Nothing outside the plugin process deletes the builder pod: a crash or a kill between creating it and removing it left a privileged container running project instructions for as long as the cluster would have it, and only an operator who had set deadline= was protected. Derive the pod's activeDeadlineSeconds from the release task's own context deadline plus a five-minute margin whenever no deadline is configured, so the pod dies shortly after the build that owns it. A configured deadline still wins, and a context without one falls back to an hour. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The namespace, image and serviceaccount options trim their values, but rootless, deadline and timeout handed the raw text to ParseBool and ParseDuration, so a pair written as "deadline= 90m" was rejected with a parse error the operator cannot spot in their own configuration. Trim those three the same way the sibling options already do. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The doc comment on unusedBuilderSettings ended with the same clause as the inline comment in NewBuilder that explains the environment/configure asymmetry. Keep the explanation in one place. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The builder pod suppressed the API token only when no `serviceaccount=` was configured, so the configuration the QUICKSTART recommends handed a token with that ServiceAccount's full RBAC to a privileged container running project-supplied build instructions. Nothing in the pod calls the Kubernetes API: the readiness probe is a local `buildctl` and the build stream rides `pods/exec` opened with the plugin's own credentials. The removed rationale claimed a configured ServiceAccount must keep the cluster default or IRSA and workload identity lose their credentials. That is wrong for all four mechanisms: IRSA injects `aws-iam-token`, EKS Pod Identity injects `eks-pod-identity-token` and Azure Workload Identity injects `azure-identity-token` as separate webhook-mounted projected volumes, while GKE Workload Identity uses the node metadata server — none is the `kube-api-access-*` volume this field suppresses. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The token bullet in both QUICKSTART locales still described the old conditional behavior — a token mounted whenever `serviceaccount` was configured. It now states the unconditional `automountServiceAccountToken: false` and scopes the claim to the mount that field actually suppresses, noting that cloud workload-identity mechanisms are unaffected because they deliver credentials through their own volumes or the node metadata server. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The field description and both QUICKSTART rows promised the release task's own timeout plus a five-minute margin, but resolvePodDeadline derives the cap from the time the task context has left when the pod is created — clone, tag verification and context tar have already been spent by then. An operator sizing task_timeout for their longest release got a cap tighter than documented. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
Picks up the corrected buildkitd_driver_opts deadline wording from path_configure.go. Generated by `task docs:gen`; no other reference page moved. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The sentence ended mid-clause ("no longer around to.") and gave the wrong
reason: the five minutes are not time for the build to finish. The pod deadline
is anchored at the pod's StartTime, at or after bootstrap, while the build
context expires at bootstrap plus the same remaining time — so the context
always fires first and the slack is clock-skew and termination margin.
Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The time.Minute floor only ever applied to an already-expired context, where createPod fails on that same context before any pod exists — so the minute was arbitrary. The floor still carries a real invariant, which is why it stays: a negative remaining would make `opts.deadline > 0` false in buildkitPod and silently drop activeDeadlineSeconds. Zero holds that just as well, and the test case now names the invariant instead of promising a production guarantee. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The bullet told operators to audit RBAC bound to the builder's ServiceAccount through RoleBindings, ClusterRoleBindings and group bindings, and then said the pod never receives an API token — but Kubernetes RBAC is exercisable from a pod only through a mounted ServiceAccount credential, so that enumeration named privileges the build cannot reach. It now states the unreachability as the consequence of the missing mount. The same sentence framed surviving cloud workload identity as reassurance inside a threat-model bullet, when it is the residual leak: a privileged container running project-supplied instructions still reaches whatever cloud role is bound to the chosen ServiceAccount, so the audit advice is re-scoped to that, `imagePullSecrets` and admission policy. The RU translation also dropped a transliterated «моунт» and «не страдают», neither of which the file uses elsewhere. The constant comment now records that the user-facing description deliberately omits the unreachable 1h fallback. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The bullet said RBAC bound to the builder's ServiceAccount "is not reachable from inside the build", but the next bullet states the container is privileged by default — and a privileged container reaches the node filesystem, so other pods' projected tokens and the kubelet's own credentials are a container escape away. The accurate claim is narrower: the build is not handed a token for that ServiceAccount and cannot act as it against the API. The bullet now says so and names the residual node-level exposure instead of implying isolation. The RU twin also translated admission policy as «политики допуска», while the file keeps «PodSecurity-admission» untranslated twice, and opened the list with a phrase that did not agree with it. Separately, the `defaultBuildkitPodDeadline` comment loses the clause about what the user-facing description omits: it described another artifact and would rot the moment that description changed. Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
The bullet explained how cloud workload identity delivers credentials — through its own volumes or the node metadata server — which is a generic claim about four vendor mechanisms this repo's CI never exercises, and it made an already long security bullet longer. The operative statement is unchanged: those mechanisms are not covered by the suppressed mount, so the build reaches whatever cloud role is bound to the chosen ServiceAccount. The RU twin also carried a calque of "act as it against the API"; it now reads «обращаться к API от его имени». Signed-off-by: Aleksei Igrychev <aleksei.igrychev@palark.com>
e7ae3b6 to
ecc4f7e
Compare
1 participant
Summary
Stacked on #422 (base
d7b21fa5ce5b): the thirteen commits ind7b21fa..HEADare new. They implement four findings from the final review published in #422, plus four findings from a self-review of that work: the privileged builder pod no longer receives a ServiceAccount token it never uses, and its lifetime is always bounded, so a plugin crash mid-build cannot leave a privileged pod running indefinitely.What
Builder pod hardening
automountServiceAccountToken: false, including whenserviceaccount=is configured — the configuration the QUICKSTART recommends. Nothing in the pod calls the Kubernetes API: the readiness probe is a localbuildctl, and the build stream ridespods/execopened with the plugin's own credentials.aws-iam-token), EKS Pod Identity (eks-pod-identity-token) and Azure Workload Identity (azure-identity-token) arrive as separate webhook-injected projected volumes, and GKE Workload Identity uses the node metadata server — none is thekube-api-access-*volume this field suppresses.serviceaccount=.activeDeadlineSeconds:deadline=when configured, otherwise the release task's remaining time at pod creation plus a five-minute margin (1h only when the context carries no deadline, unreachable in production).Failedpod stays visible until removed by hand or by pod GC — documented.Option parsing
rootless= true,deadline= 90m,timeout= 10s(whitespace-padded values) now parse like the other options already did; padded-but-invalid values (deadline= 90) still error.deadline=0sis still rejected — the whole-seconds ≥1svalidation is untouched.Docs
buildkitd_driver_optsfield description, its generated reference row and both QUICKSTART locales (en/ru) state the unconditional token behavior and give the deadline default as the task's remaining time at pod creation, not its fulltask_timeout.builder.gois dropped — no behavior change.Deliberately unchanged
buildkitd_driver=kubernetesvalue naming) and the remaining MINORs/NITs are not addressed here — maintainers' calls, tracked in the feat(server): build releases in kubernetes without the docker binary #422 review comment.buildx_driver=kubernetespath orphans a self-healing Deployment on the same crash and has nodeadlinecounterpart. Out of scope for this PR.Why
buildkitPodsuppressed the token only when noserviceaccount=was configured, so the documented happy path handed a token carrying that ServiceAccount's full RBAC to a privileged container running untrusted project build instructions. The rationale in the code claimed a configured ServiceAccount must keep the cluster default or IRSA and workload identity would lose their credentials; that is wrong for all four mechanisms, and the built-in ServiceAccount admission plugin runs before mutating webhooks anyway, sofalsecannot strip what a webhook adds afterwards. Separately,activeDeadlineSecondswas set only whendeadline=was configured — with no ownerReferences and all cleanup in-process, a Vault restart or plugin crash mid-build orphaned a privileged, token-carrying pod forever.A constant default deadline was rejected: any fixed value either leaves the orphan alive for hours at the default 30m task timeout or silently kills legitimate builds the moment an operator raises
task_timeout. Deriving from the task context's remaining time tracks the operator's own knob and guarantees the build context always expires before the pod deadline fires — which is also why the five-minute slack is clock-skew and termination margin, not time for the build to finish.Fixes findings MAJOR 1, MAJOR 3, MINOR 9 and NIT 15 of the review in #422.