Webhook hardening follow-ups (post-#4564 review)
Nobody has claimed this yet.
- Dominant language
- Go
- Stars
- 2.2k
- Forks
- 300
- Avg merge
- 1d 15h
- Merged PRs (30d)
- 184
Description
Tracking issue for the webhook hardening follow-ups identified in the round-2 review of #4564 that are not addressed by #5190 / #5191 / #5192.
Shipped
- #5190 — Block private/loopback/link-local dials in webhook HTTP client (item #5)
- #5191 — Redact webhook response body from returned errors (item #8)
- #5192 — Cap webhook middleware request body at 1 MB (webhook-layer fix for item #9)
Remaining
Item #6: InsecureSkipVerify conflates cert-skip with plaintext-HTTP
pkg/webhook/types.go ValidateDefinition allows non-HTTPS URLs whenever InsecureSkipVerify=true. pkg/webhook/client.go buildTransport propagates the same flag into ValidatingTransport.InsecureAllowHTTP=true, which (per pkg/networking/http_client.go:55-73) skips ALL request validation when set: cert check, scheme check, and now also short-circuits after the dial-time SSRF guard fires. Flip one knob, lose multiple guarantees.
RFC THV-0017 says HTTPS-only. Suggested fix:
- Split
InsecureSkipVerifyinto two fields, or remove the plaintext escape hatch entirely. - Add a CRD-level CEL rule on
MCPWebhookConfig.spec.*.urlenforcingself.startsWith('https://')at admission time.
MCP parsing layer needs an inbound body cap
The webhook-layer cap shipped in #5192 is correct for the webhook package's own re-read buffer, but mcp.ParsingMiddleware (pkg/mcp/parser.go:85) reads the inbound HTTP body via unbounded io.ReadAll BEFORE the webhook middleware in the proxy chain. So an attacker can still buffer a 10 GB body into memory upstream of the webhook cap.
pkg/mcp/tool_filter.go:229 has the same exposure. Both should be wrapped with http.MaxBytesReader or an equivalent cap. The cap may need to be configurable per-server (some MCP tools/call payloads can legitimately be larger than the default).
pkg/webhook/internal/dialer subpackage refactor
The three exported test helpers in pkg/webhook/dialer_testing.go (SetDialerControlForTesting, SetDialerControlForTestMain, AllowAnyDialerControl) inflate pkg/webhook's public API surface in service of cross-package test injection. Moving them — and the dialerControl atomic.Pointer itself — into a new pkg/webhook/internal/dialer subpackage would keep them off the project's public API while still letting pkg/webhook, pkg/webhook/validating, and pkg/webhook/mutating import them via Go's internal rule.
An even more principled fix is to refactor webhook.NewClient to accept an optional *http.Client (or a WithHTTPClient(...) option), so production passes the SSRF-protected client and tests pass an unguarded one. The package-level dialerControl then disappears entirely.
Expanded SSRF integration coverage in pkg/networking
pkg/networking/utilities_test.go already covers IPv6 cases at the unit level. The webhook PR (#5190) adds IPv6 cases to TestClientSSRFGuardBlocksPrivateAddress, but it would be worth adding parallel integration coverage for any other consumer of ProtectedDialerControl (the OAuth/discovery clients, etc.).
Webhook response body in client error log fields
#5191 moves the body preview to slog.Debug with a body_preview field. If the project's log policy forbids untrusted bytes flowing into structured log fields at any level (not just info+), the preview should be redacted further or omitted. Re-evaluate when log-injection policy is formalized.
🤖 Generated with Claude Code
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by selecting one remaining follow-up, then read the referenced entry points such as pkg/webhook/types.go, pkg/webhook/client.go, pkg/mcp/parser.go, pkg/mcp/tool_filter.go, or pkg/networking/utilities_test.go. Run the relevant existing tests before making changes. Done means the selected hardening item has a clear implementation, regression coverage where applicable, and its corresponding checklist work is resolved.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- backend, security
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 25/100