Harden Envoy backend: pin image digest, disable admin interface - #5949
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5949 +/- ##
==========================================
- Coverage 71.87% 71.85% -0.02%
==========================================
Files 715 715
Lines 73501 73502 +1
==========================================
- Hits 52830 52818 -12
- Misses 16887 16899 +12
- Partials 3784 3785 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
glageju
left a comment
There was a problem hiding this comment.
Multi-Agent Consensus Review
Agents consulted: security-specialist, go-correctness-specialist, general-quality-specialist, codex (gpt-5.5)
Consensus Summary
| # | Finding | Consensus | Severity | Action |
|---|---|---|---|---|
| 1 | PullImage cannot handle tag@digest references — breaks Envoy setup on any non-cached host |
10/10 | HIGH | Fix |
| 2 | No test exercises the real image-pull path with a digest-pinned reference | 9/10 | HIGH | Fix |
| 3 | Unrelated streamable-HTTP test fix bundled without disclosure | 8/10 | MEDIUM | Discuss |
| 4 | PR body's Changes table omits the third modified file | 7/10 | MEDIUM | Fix |
| 5 | Test plan doesn't cover the streamable dispatcher change | 7/10 | MEDIUM | Discuss |
| 6 | "Type of change" checkbox ("New feature") doesn't match hardening/cleanup content | 6/10 | LOW | Discuss |
| 7 | TestEnvoyAdmin_Absent uses substring checks instead of structural JSON assertions |
6/10 | LOW | Discuss |
Overall
The intent here is sound and both tasks in #5903 are nominally addressed: defaultEnvoyImage is pinned by digest (verified against crane digest envoyproxy/envoy-distroless:v1.32.3 — exact match), and the admin interface is removed outright rather than merely bound to loopback, which is the stronger of the two options the issue proposed. The admin-removal half is clean, complete, and validated against a real Envoy binary via TestEnvoyBootstrap_ValidatesAgainstRealEnvoy.
The digest-pinning half has a functional regression, though. RegistryImageManager.PullImage (pkg/container/images/registry.go) converts the parsed reference to a name.Tag for the daemon.Write call, with a fallback to name.NewTag(ref.String()) when the reference isn't already a tag. For a repo:tag@digest string, name.ParseReference correctly resolves to a name.Digest, so the fallback fires — and name.NewTag rejects the string because the @ makes the "repository" portion invalid (repository can only contain the characters ...). I reproduced this directly inside pkg/container/images against the version of go-containerregistry pinned in go.mod (v0.21.2), so this isn't tool-specific or a local environment quirk — it's the exact code path PullImage runs. The practical effect: PullImage errors on this exact image string every time, and SetupIngress's fallback (ImageExists) only recovers if the image is already cached in the local Docker daemon. On a fresh install or CI runner, that's not the case, so the Envoy proxy — and the workload depending on it — fails to start. None of the existing tests catch this, since TestGetEnvoyImage and the orchestration tests use fakeImageManager (always succeeds), and TestEnvoyBootstrap_ValidatesAgainstRealEnvoy calls docker run directly, bypassing ToolHive's own pull path entirely.
Separately, the PR bundles a second, unrelated commit that fixes a test assertion in dispatcher_standalone_sse_integration_test.go (a different package, streamable-HTTP transport, addressing a bug introduced in #5934). The fix itself looks correct — it now matches the established pattern elsewhere in the same file for how JSON-RPC application errors ride in HTTP 200 responses — but it's undisclosed in the PR title, summary, and Changes table, which only account for the two Envoy files.
Generated with Claude Code
Pin defaultEnvoyImage to tag+digest for supply-chain integrity. Add a comment documenting the TOOLHIVE_ENVOY_IMAGE override for users with image-policy requirements. Fix PullImage in RegistryImageManager to handle tag@digest references. name.ParseReference returns name.Digest (not name.Tag) for these, and the previous name.NewTag(ref.String()) fallback errored because the @sha256:... suffix is not valid in a tag reference. Now extracts the tag portion (everything before the '@') from the digest reference. Remove the admin interface block from the bootstrap. Envoy does not start an admin server when the field is absent, eliminating the 9901 port surface area with no functional impact on the proxy. Use a structural JSON assertion (unmarshal → key check) instead of substring matching to verify the admin block is absent. Closes #5903 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
af377fd to
9ae04cd
Compare
Summary
Three hardening fixes for the Envoy network-proxy backend.
Closes #5903
Changes
Image digest pinning:
defaultEnvoyImagewas tag-pinned (v1.32.3). Tag mutation is a supply-chain attack vector — a registry push can silently swap the image behind the same tag. The constant is nowtag@digest, combining human readability with cryptographic integrity. Users with stricter image policies can still substitute viaTOOLHIVE_ENVOY_IMAGE.PullImagefix for tag@digest references:name.ParseReferencereturns aname.Digest(notname.Tag) forimage:tag@sha256:...references. The previous fallback calledname.NewTag(ref.String())on the full digest string, which errors because the@sha256:...suffix is invalid in a tag reference. On a fresh host where the image isn't cached, this causedSetupIngressto return an error and Envoy startup to fail entirely. Fixed by extracting the tag portion (everything before@) and creating thename.Tagfrom that.Admin interface disabled: The admin block bound to
127.0.0.1:9901on every Envoy container. Omitting the admin block entirely causes Envoy to skip the admin server — port 9901 never opens. The admin API is not used by ToolHive at runtime; it existed only as a debugging convenience. The test is updated to use structural JSON unmarshalling (unmarshal → key presence check) instead of substring matching.pkg/container/docker/envoy.goAdminfield fromenvoyBootstrappkg/container/docker/envoy_test.goAdminfrom test bootstraps; update assertion to structural JSON checkpkg/container/images/registry.goPullImagetag-derivation forname.DigestreferencesType of change
Test plan
task buildpassestask lint-fixpassesTestEnvoyAdmin_Absent— structural JSON check:adminkey absent from bootstrap mapTestWriteEnvoyBootstrap_FileMode— file mode and round-trip JSONTestGetEnvoyImage— default and override pathsTestEnvoyBootstrap_ValidatesAgainstRealEnvoy— validates admin-less bootstrap against real Envoy (CI, Docker required)Special notes for reviewers
The digest
sha256:375aab0d80b3c0e1b42a776b4cb1743ed79012032051d2da19cbc93ea884fb81was obtained viacrane digest envoyproxy/envoy-distroless:v1.32.3. When upgrading Envoy, update both tag and digest together.The
PullImagefix now uses aswitchon the concretename.Referencetype:name.Tagpasses through unchanged,name.Digeststrips the@sha256:...suffix to derive a tag, and any other type returns an explicit error.Generated with Claude Code