fix(scheduler): deny privileged containers in webhook - #2141
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aryansri05 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe webhook now immediately denies pods containing privileged containers. A unit test covers a pod combining a privileged container with a GPU-requesting container and verifies that admission is rejected. ChangesPrivileged admission enforcement
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
0ca177f to
04e33b8
Compare
Signed-off-by: Aryan Srivastava <aryansriva05@gmail.com>
04e33b8 to
29368dc
Compare
aryansri05
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/scheduler/webhook.go`:
- Around line 74-77: The privileged-container validation in
pkg/scheduler/webhook.go around the existing SecurityContext check must run
before the preassigned SchedulerName allow path, and must inspect Containers,
InitContainers, and EphemeralContainers. Update the relevant admission logic
while preserving denial for any privileged container, then add regression
coverage in pkg/scheduler/webhook_test.go around the existing scheduler
admission tests for privileged init containers, privileged ephemeral containers,
and privileged pods using a non-default scheduler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fde7ee19-d1a6-4141-842c-570ff4278de4
📒 Files selected for processing (2)
pkg/scheduler/webhook.gopkg/scheduler/webhook_test.go
| if ctr.SecurityContext != nil { | ||
| if ctr.SecurityContext.Privileged != nil && *ctr.SecurityContext.Privileged { | ||
| klog.Warningf(template+" - Denying admission as container %s is privileged", pod.Namespace, pod.Name, pod.UID, c.Name) | ||
| continue | ||
| return admission.Denied("privileged containers are not allowed") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)pkg/scheduler/(webhook\.go|webhook_test\.go)$|hack/.import-aliases' || true
echo "== webhook outline =="
ast-grep outline pkg/scheduler/webhook.go || true
echo "== relevant webhook.go =="
sed -n '1,130p' pkg/scheduler/webhook.go
echo "== relevant webhook_test.go =="
sed -n '530,655p' pkg/scheduler/webhook_test.go
echo "== privilege references =="
rg -n -C 3 "privilege|Privileged|InitContainers|EphemeralContainers|SchedulerName|Allowed|Denied" pkg/scheduler/webhook.go pkg/scheduler/webhook_test.goRepository: Project-HAMi/HAMi
Length of output: 26556
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== inspect init-container tests around earlier scheduler-different tests =="
sed -n '160,245p' pkg/scheduler/webhook_test.go
echo "== behavioral source probe: does Handle return Allowed before privileged check under preassigned scheduler? =="
python3 - <<'PY'
from pathlib import Path
text = Path('pkg/scheduler/webhook.go').read_text()
checks = {
'scheduler_allow_before_privileged_loop': text.index('return admission.Allowed("pod already has different scheduler assigned")') < text.index('// Denying admission as container %s is privileged'),
'only_one_privileged_loop_in_containers': text.count('for idx, ctr := range pod.Spec.Containers') == 1,
'contains_initcontainers_loop': 'for idx, ctr := range pod.Spec.InitContainers' in text,
'contains_ephemeralcontainers_loop': 'for idx, ctr := range pod.Spec.EphemeralContainers' in text,
}
for k, v in checks.items():
print(f'{k}={v}')
print('privileged check appears only inside first Containers loop:', 'Privileged' in text)
print('allowed response in Handle:', 'admission.Allowed' in text)
PYRepository: Project-HAMi/HAMi
Length of output: 2573
Check privileged containers before allowing preassigned-scheduler pods and cover init/ephemeral containers.
A pod with SchedulerName set to a non-default scheduler returns admission.Allowed before reaching the privileged container deny path, and the validation only scans Containers. Move the privileged check ahead of the scheduler-name allow path and include InitContainers and EphemeralContainers; add regression tests for privileged init/ephemeral containers and a privileged pod with a different preassigned scheduler.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 75-75: A log/format call (log.Print/Printf/Println, the Fatal/Panic variants, fmt.Sprintf, or a structured logger's Info/Warn/Error/Debug method) is given a message built by concatenating a string literal with a non-literal value such as request data. Unsanitized, attacker-controlled input written to logs enables log forging / CRLF injection: an attacker can inject newlines to spoof log entries or break log parsers. Do not concatenate raw input into the log message; pass it as a separate structured field/argument (e.g. 'log.Printf("user: %s", user)' or 'logger.Info("login", "user", user)') and strip or escape newline characters first.
Context: klog.Warningf(template+" - Denying admission as container %s is privileged", pod.Namespace, pod.Name, pod.UID, c.Name)
Note: [CWE-117] Improper Output Neutralization for Logs.
(log-injection-request-data-concat-go)
📍 Affects 2 files
pkg/scheduler/webhook.go#L74-L77(this comment)pkg/scheduler/webhook_test.go#L575-L630
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/scheduler/webhook.go` around lines 74 - 77, The privileged-container
validation in pkg/scheduler/webhook.go around the existing SecurityContext check
must run before the preassigned SchedulerName allow path, and must inspect
Containers, InitContainers, and EphemeralContainers. Update the relevant
admission logic while preserving denial for any privileged container, then add
regression coverage in pkg/scheduler/webhook_test.go around the existing
scheduler admission tests for privileged init containers, privileged ephemeral
containers, and privileged pods using a non-default scheduler.
|
Fixed in #2139 |
2 participants
What changed
Why
The webhook previously logged that it was denying privileged containers but then continued processing the pod. If another container requested GPU resources, the webhook could still allow and mutate the pod.
Fixes #2138.
Validation
go test ./pkg/schedulerSummary by CodeRabbit
Bug Fixes
Tests