fix: decouple skills from the privileged sandbox and default to restricted PSS - #2462
fix: decouple skills from the privileged sandbox and default to restricted PSS#2462QuentinBisson wants to merge 1 commit into
Conversation
66b2c7e to
9e9d31f
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates the SandboxAgent manifest translation so that configuring spec.skills no longer implicitly requires privileged containers, and so that generated agent/init containers default to a Pod Security Standards (PSS) restricted-compliant container securityContext. It also adjusts the full Python runtime image to use a numeric user so runAsNonRoot can be enforced without requiring an explicit runAsUser.
Changes:
- Stop coupling skills setup to code-exec isolation/privilege requirements (skills init remains unprivileged).
- Default generated container
securityContextto PSSrestrictedsettings when no isolation is requested, and apply the same defaults to the skills init container. - Switch
python/Dockerfile.fulltoUSER 1001:1001for kubeletrunAsNonRootverification, and add translator unit tests covering these securityContext defaults.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/Dockerfile.full | Uses a numeric USER to support runAsNonRoot validation without setting runAsUser. |
| go/core/internal/controller/translator/agent/security_context_test.go | Adds tests asserting restricted defaults, skills non-privileged behavior, and the privileged branch behavior. |
| go/core/internal/controller/translator/agent/manifest_builder.go | Removes skills→privileged coupling and introduces restricted-PSS default container security context behavior. |
Suppressed comments (1)
go/core/internal/controller/translator/agent/manifest_builder.go:444
new(false)/new(true)are being called with boolean values, which does not compile in Go (the built-innewtakes a type). Initialize local bool vars and take their addresses for these pointer fields.
func restrictedSecurityContext() *corev1.SecurityContext {
return &corev1.SecurityContext{
AllowPrivilegeEscalation: new(false),
RunAsNonRoot: new(true),
Capabilities: &corev1.Capabilities{
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| base := &corev1.SecurityContext{ | ||
| AllowPrivilegeEscalation: new(false), | ||
| ReadOnlyRootFilesystem: new(true), | ||
| } | ||
|
|
There was a problem hiding this comment.
Not correct: new(expr) with a value argument is valid as of Go 1.26 (accepted spec change), and this module is on go 1.26.3. It is already the established idiom on main — e.g. NeedLeaderElection: new(true) in core/internal/controller/sandboxagent_controller.go and six sibling controllers. go build ./core/..., go vet and the package tests all pass.
| return restrictedSecurityContext() | ||
| } | ||
|
|
||
| return &corev1.SecurityContext{Privileged: new(true)} |
There was a problem hiding this comment.
Not correct: new(expr) with a value argument is valid as of Go 1.26 (accepted spec change), and this module is on go 1.26.3. It is already the established idiom on main — e.g. NeedLeaderElection: new(true) in core/internal/controller/sandboxagent_controller.go and six sibling controllers. go build ./core/..., go vet and the package tests all pass.
…icted PSS Skills no longer force Privileged=true on the agent container, and containers without an explicit securityContext now render the restricted Pod Security Standards defaults (allowPrivilegeEscalation=false, runAsNonRoot=true, drop ALL capabilities, seccomp RuntimeDefault). The skills-init container gets the same defaults. python/Dockerfile.full switches to a numeric USER so the kubelet can verify runAsNonRoot without an explicit runAsUser. Fixes kagent-dev#1997 Fixes kagent-dev#2244 Signed-off-by: QuentinBisson <quentin@giantswarm.io>
9e9d31f to
bbece3e
Compare
|
Rebased onto The three remaining red checks are not from this branch, they reproduce identically on
|
Supersedes #2334, which the stale bot closed and GitHub refuses to reopen. Reimplemented against
v1alpha3.SandboxAgentafter #2422 removed the deployment-backed agent API. Fixes #1997 and #2244, both re-verified against currentmainrather than taken from the issue text.What changes
buildSkillsRuntimeno longer setsneedCodeExecIsolation = truewheneverspec.skillsis configured. Loading skills needs no elevated privileges: theskills-initinit container does the git clone and image pull, and the privileged container only exists for the in-pod srt/bubblewrap sandbox.buildContainerSecurityContextreturns a restricted-PSS-compliant default (allowPrivilegeEscalation: false,runAsNonRoot: true,capabilities.drop: [ALL],seccompProfile: RuntimeDefault) instead ofnilwhen no isolation is requested. Theskills-initcontainer gets the same defaults.python/Dockerfile.fulluses a numericUSER 1001:1001so the kubelet can verifyrunAsNonRootwithout an explicitrunAsUser. The other three runtime images already use numeric users.Without this, agent pods render with no container securityContext at all and are rejected outright on clusters enforcing
restrictedPod Security admission.How it differs from #2334
The original PR seeded
needCodeExecIsolationfrom thedeclarative.executeCodeBlocksopt-in and left the workaround from #1997 (allowPrivilegeEscalation: falseon the agent CR) as the escape hatch. Neither exists anymore:executeCodeBlocksis gone from the API, andv1alpha3.SandboxAgenthas no user-suppliedsecurityContext, sobuildContainerSecurityContextis always called with anilbase. On currentmainthe only thing that can request a privileged container isspec.skills, and that is itself rejected by CEL onSandboxAgent— so restricted clusters have the bug with no way out.I kept the
needCodeExecIsolationparameter and the privileged branch ofbuildContainerSecurityContextrather than deleting them, since they are the seam an explicit sandbox opt-in would use when skills return toSandboxAgent. They are unreachable today; happy to delete them instead if you prefer.Verification
go build ./core/...,go test ./core/...(1289 passed),go vet,golangci-lintsecurity_context_test.gocovers the restricted default, the skills path, and the privileged branch. No other test in the package asserted the oldnildefault.