fix(podprobe): pin probes to pod IP and reject Host header to prevent SSRF#2471
fix(podprobe): pin probes to pod IP and reject Host header to prevent SSRF#2471tanaygoyal1111 wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Welcome @tanaygoyal1111! It looks like this is your first PR to openkruise/kruise 🎉 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR hardens pod probe execution against SSRF by ensuring probes can only target the Pod IP, and by rejecting the Host HTTP header override path.
Changes:
- Reject
Hostheader inHTTPGetprobe validation to prevent host override via headers. - Enforce daemon-side probe host pinning to the Pod IP for both HTTP and TCP probes.
- Add/adjust tests to cover SSRF-style host overrides and empty Pod IP handling.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/webhook/podprobemarker/validating/probe_validating_test.go | Adds a validating-webhook test ensuring Host HTTP header is rejected. |
| pkg/webhook/podprobemarker/validating/probe_create_update_handler.go | Adds validation rejecting Host header in HTTPGetAction headers. |
| pkg/daemon/podprobe/prober_test.go | Updates/extends daemon tests for Pod-IP-only host enforcement and SSRF cases. |
| pkg/daemon/podprobe/prober.go | Implements resolveProbeHost and uses it to pin HTTP/TCP probes to the Pod IP. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The daemon copies a "Host" header into req.Host (see newProbeRequest), so allowing | ||
| // it would reintroduce the host override that the Host field above forbids. | ||
| if strings.EqualFold(header.Name, "Host") { | ||
| allErrors = append(allErrors, field.Invalid(fldPath.Child("httpHeaders"), header.Name, "Host header is not allowed")) | ||
| } |
… SSRF Signed-off-by: tanaygoyal1111 <goyaltanay.1111@gmail.com>
d7e85bc to
f5f60c6
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2471 +/- ##
==========================================
- Coverage 49.22% 49.21% -0.02%
==========================================
Files 325 325
Lines 28155 28163 +8
==========================================
Hits 13860 13860
- Misses 12713 12720 +7
- Partials 1582 1583 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Describe what this PR does
PodProbeMarker probes are executed by kruise-daemon, which runs with
hostNetwork: true. The daemon used to honor theHostfield from the probe spec directly and only fell back to the Pod IP when it was empty:The only thing rejecting a non-empty
Hostwas the PodProbeMarker validating webhook. But the daemon actually readsNodePodProbe, which has no admission webhook of its own, and the controller copies the probe spec into it verbatim. So aHostset directly on aNodePodProbewas dialed as-is from the node network namespace letting a probe reach node-local services or the cloud metadata endpoint (169.254.169.254). On top of that, the webhook forbadehttpGet.Hostbut still allowed an HTTP header literally namedHost, which the daemon copies intoreq.Host.This PR closes both gaps:
resolveProbeHostchokepoint in the daemon (pkg/daemon/podprobe/prober.go) used by both the TCP and HTTP paths. A probe may only target its Pod IP a non-emptyHostthat differs from the Pod IP is rejected with a clear error instead of being silently dropped, so a directNodePodProbeinjection is both blocked and visible in the logs. An empty Pod IP is rejected too, which also closes a latentnet.JoinHostPort("", port)→":port"loopback dial.HostinvalidateHTTPGetAction(pkg/webhook/podprobemarker/validating), since the daemon would otherwise apply it asreq.Host.The protection now lives in the daemon itself, so it no longer depends solely on admission being in the path.
Does this pull request fix one issue?
NONE
Describe how to verify it
Unit tests cover both layers:
prober_test.go: a TCP/HTTP probe withHost: 169.254.169.254and a different Pod IP is now rejected; an empty Pod IP is rejected; the happy paths (emptyHost, orHostequal to the Pod IP) still pass.probe_validating_test.go: a PodProbeMarker carrying aHostHTTP header is now rejected by the webhook.To check it end-to-end on a cluster: create a PodProbeMarker whose httpGet probe sets a
Hostheader pointing at169.254.169.254it is rejected at admission; and aNodePodProbewritten directly with aHostpointing off-pod no longer causes the daemon to dial that address.Special notes for reviews
TestNewRequestForHTTPGetActionpreviously asserted that arbitrary hosts likelocalhost/secure.example.comwere honored I updated those to use the Pod IP, since that behavior was the vulnerability. The cases that test invalid port/path now leaveHostempty so they exercise the intended validation rather than short-circuiting on the host check.Hostvalues surface in daemon logs for debugging.