stacklok / stacklok/toolhive

Webhook hardening follow-ups (post-#4564 review)

Open
#5,193 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement go security
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:

  1. Split InsecureSkipVerify into two fields, or remove the plaintext escape hatch entirely.
  2. Add a CRD-level CEL rule on MCPWebhookConfig.spec.*.url enforcing self.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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.