Block private and loopback dials in webhook HTTP client#5190
Conversation
f1ad404 to
a48d30f
Compare
A tenant with rights to create MCPWebhookConfig could previously point url at any HTTPS endpoint, including 169.254.169.254, 127.0.0.1, RFC1918 ranges, and IPv6 loopback or link-local addresses. The webhook HTTP transport built a bare http.Transport with no DialContext; the wrapping networking.ValidatingTransport only checks the URL scheme, not the resolved peer address, so cross-tenant access to cloud metadata or in-cluster services was unblocked. Wire networking.ProtectedDialerControl into the inner transport's DialContext so private, loopback, and link-local destinations are rejected at dial time, regardless of whether ValidatingTransport's HTTPS check is bypassed via InsecureAllowHTTP. The hook is held in an atomic.Pointer so test overrides remain race-free if a future test introduces t.Parallel(). Export protectedDialerControl as ProtectedDialerControl in pkg/networking so the webhook package can install it without duplicating the body. Add cross-package test injection (SetDialerControlForTesting, SetDialerControlForTestMain, AllowAnyDialerControl) so existing httptest-based tests in pkg/webhook and its subpackages continue to work; subpackage TestMain functions install the permissive override at suite startup. The pkg/runner webhook middleware integration test installs the same override, since its production webhook clients dial httptest servers on 127.0.0.1. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3fa55f1 to
cb9b4ad
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5190 +/- ##
==========================================
+ Coverage 68.77% 68.84% +0.07%
==========================================
Files 629 630 +1
Lines 63914 63931 +17
==========================================
+ Hits 43955 44012 +57
+ Misses 16703 16664 -39
+ Partials 3256 3255 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
There was a problem hiding this comment.
One non-blocking design note from reviewing the SSRF fix. The change itself is correct and closes a real gap — this is just a thought on where the plumbing could live longer-term.
| Control: func(network, address string, c syscall.RawConn) error { | ||
| return (*dialerControl.Load())(network, address, c) | ||
| }, | ||
| }).DialContext, |
There was a problem hiding this comment.
Non-blocking / follow-up thought, not for this PR.
This hand-builds a transport with its own dialer guard, but pkg/networking.HttpClientBuilder already does almost exactly this: it installs ProtectedDialerControl and gives tests a clean opt-out via WithPrivateIPs(true) — no package-level global, no atomic.Pointer, and no exported test helpers. The bespoke dialerControl global (plus dialer_testing.go, which is the only *_testing.go file in the repo) is really what forces us to export the guard-disabling helpers.
As far as I can tell, only two gaps stop webhook from just using the builder today:
- mTLS client certs (
ClientCertPath/ClientKeyPath) - a
tls.Config.InsecureSkipVerifyknob distinct from the HTTP-allow knob
So a possible follow-up (bigger than this PR): add WithClientCert(certPath, keyPath) and WithInsecureSkipVerify(bool) to HttpClientBuilder, then have webhook call the builder and delete the global + dialer_testing.go entirely. Happy to file an issue for it rather than expand scope here.
Summary
Follow-up to the round-2 review of #4564 (item #5). A tenant with rights to create
MCPWebhookConfigcould previously pointurlat any HTTPS endpoint, including169.254.169.254(cloud metadata),127.0.0.1, RFC1918 ranges, and IPv6 loopback / link-local / ULA addresses. The webhook HTTP transport built a barehttp.Transportwith noDialContext; the wrappingnetworking.ValidatingTransportonly checks the URL scheme — it does not validate the resolved peer address — so cross-tenant access to cloud metadata or in-cluster services was unblocked.This PR wires
networking.ProtectedDialerControlinto the inner transport'sDialContextso private, loopback, and link-local destinations are rejected at dial time, regardless of whetherValidatingTransport's HTTPS check is bypassed viaInsecureAllowHTTP. The hook is held in anatomic.Pointerso test overrides remain race-free if a future test introducest.Parallel().protectedDialerControlis exported asProtectedDialerControlinpkg/networkingso the webhook package can install it without duplicating the body. Cross-package test injection (SetDialerControlForTesting,SetDialerControlForTestMain,AllowAnyDialerControl) is added so the existing httptest-based suite inpkg/webhookand its subpackages keeps working; subpackageTestMainfunctions install the permissive override at suite startup.Type of change
Test plan
task test—pkg/webhook/...andpkg/networking/...pass with-race)golangci-lintreports 0 issues)TestClientSSRFGuardBlocksPrivateAddresscovers IPv4 loopback, RFC1918, and169.254.169.254, plus IPv6 loopback (::1), link-local (fe80::1), and ULA (fc00::1).TestBuildTransportInstallsDialerGuardassertsDialContextis wired on the inner transport.Special notes for reviewers
pkg/webhook/dialer_testing.goadd public surface area in service of cross-package test injection. A reviewer raised the option of moving them to apkg/webhook/internal/dialersubpackage; that's a viable follow-up but feels like scope creep here. Each helper has aProduction code MUST NOT call this functiondeterrent in its godoc and thetesting.TBargument signals test-scoped intent.InsecureSkipVerifyconflates cert-skip with plaintext-HTTP) and item Implement secret injection #5's broader IPv6 coverage inpkg/networkingitself are tracked separately.🤖 Generated with Claude Code