fix: don't set Privileged when AllowPrivilegeEscalation is false for skills#1551
Open
skhedim wants to merge 2 commits intokagent-dev:mainfrom
Open
fix: don't set Privileged when AllowPrivilegeEscalation is false for skills#1551skhedim wants to merge 2 commits intokagent-dev:mainfrom
skhedim wants to merge 2 commits intokagent-dev:mainfrom
Conversation
…skills. Signed-off-by: skhedim <sebastien.khedim@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a controller bug where sandboxing logic for skills/executeCode could force SecurityContext.Privileged=true even when the user explicitly set allowPrivilegeEscalation: false, producing an invalid PodSpec rejected by Kubernetes admission.
Changes:
- Document why skills require sandboxing (BashTool → srt → bubblewrap) and set
needSandbox=truewhen skills are present. - Update container
securityContextmerging to avoid settingPrivileged=truewhenAllowPrivilegeEscalationis explicitlyfalse. - Replace the prior sandbox securityContext test with two focused tests covering default skills behavior and PSS-restricted behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| go/core/internal/controller/translator/agent/adk_api_translator.go | Adds an AllowPrivilegeEscalation=false guard to prevent generating an invalid {privileged: true, allowPrivilegeEscalation: false} security context. |
| go/core/internal/controller/translator/agent/security_context_test.go | Updates tests to validate privileged defaulting for skills and to verify privileged is not forced under PSS-restricted security contexts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When an
Agentspec includes a containersecurityContextwithallowPrivilegeEscalation: false(PSS Restricted profile) and the agenthas
spec.skills.refsconfigured, the controller generates an invalid pod spec:Kubernetes rejects this combination at admission time, leaving the agent stuck
in a reconciliation loop with all pods failing to start.
Root cause
buildManifest()setsneedSandbox = truewhen skills are present (becauseskills use
BashToolwhich callssrt→ bubblewrap for sandboxing), thenblindly sets
Privileged: trueon whateversecurityContextis provided —including one that already has
AllowPrivilegeEscalation: false.The securityContext merge did not check for this conflict before setting
Privileged: true.Fix
Add a helper
allowPrivilegeEscalationExplicitlyFalse()that detects when theuser has opted out of privilege escalation, and skip
Privileged: trueinthat case.
When
Privilegedis withheld,srtdegrades gracefully: on modern kernels(EKS, GKE, AL2023 ≥ 5.10) that have unprivileged user namespaces enabled
(
user.max_user_namespaces > 0), bubblewrap can still create sandboxes usingclone(CLONE_NEWUSER)without requiring a privileged seccomp profile.Behaviour matrix
truePrivileged: true— full bwrap sandbox (unchanged)allowPrivilegeEscalation: falsetruePrivileged— srt uses user-namespace modeallowPrivilegeEscalation: falsetruePrivileged— srt uses user-namespace modefalseChanges
go/core/internal/controller/translator/agent/adk_api_translator.goneedSandbox = truefor skills (BashTool → srt → bwrap)Privileged: truewithallowPrivilegeEscalationExplicitlyFalse()allowPrivilegeEscalationExplicitlyFalse()helpergo/core/internal/controller/translator/agent/security_context_test.goTestSecurityContext_WithSandbox(which tested an internallycontradictory state) with two focused tests:
TestSecurityContext_SkillsDefaultPrivilegedSandbox: no customsecurityContext →
Privileged: true(default sandbox path)TestSecurityContext_SkillsPSSRestricted:allowPrivilegeEscalation: false→ no
Privileged, original securityContext fields preserved intactTesting
All existing tests pass. Two new tests cover both code paths.
Related