Skip to content

Harden Envoy backend: pin image digest, disable admin interface #5949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ChrisJBurns wants to merge 2 commits into
base: main
Choose a base branch
from
+22 −43
Open

Harden Envoy backend: pin image digest, disable admin interface #5949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce4bf10
Harden Envoy backend: pin image by digest, disable admin interface
ChrisJBurns Jul 23, 2026
af377fd
Fix incorrect StatusBadRequest assertion in tool-call filter test
ChrisJBurns Jul 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 5 additions & 17 deletions pkg/container/docker/envoy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ import (
)

const (
// defaultEnvoyImage is pinned by tag. Digest pinning is tracked in #5903.
// Override with TOOLHIVE_ENVOY_IMAGE.
defaultEnvoyImage = "envoyproxy/envoy-distroless:v1.32.3"
// defaultEnvoyImage is pinned by tag+digest for supply-chain integrity.
// Override with TOOLHIVE_ENVOY_IMAGE (accepts any docker pull reference).
defaultEnvoyImage = "envoyproxy/envoy-distroless:v1.32.3@sha256:375aab0d80b3c0e1b42a776b4cb1743ed79012032051d2da19cbc93ea884fb81"

@glageju glageju Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH, 10/10] PullImage cannot handle this tag@digest reference — breaks Envoy setup on any non-cached host

name.ParseReference resolves this string to a name.Digest, not a name.Tag. RegistryImageManager.PullImage (pkg/container/images/registry.go:100-108) then falls back to name.NewTag(ref.String()), which errors (repository can only contain the characters ...) because the @sha256:... suffix makes the "repository" portion invalid for a tag. I reproduced this directly inside pkg/container/images against the go-containerregistry v0.21.2 version pinned in go.mod — PullImage errors on every call with this exact image string, regardless of registry availability.

In SetupIngress (envoy.go ~line 919), the failure path calls ImageExists, which only checks the local Docker daemon cache. On a fresh install or CI runner where this image isn't already cached, that returns false, and SetupIngress returns an error — the Envoy proxy (and the workload depending on it) fails to start.

Recommendation: fix RegistryImageManager.PullImage's tag-conversion fallback to handle name.Digest references without requiring a name.Tag for the daemon.Write call (e.g. derive a synthetic tag, or use a daemon-write path that accepts a name.Reference directly). This should block merge as-is — shipping this would break Envoy backend startup for most users on first run.

Raised by: security specialist, independently verified.

@glageju glageju Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH, 9/10] No test exercises the real image-pull path with a digest-pinned reference

TestGetEnvoyImage and the SetupIngress orchestration tests use fakeImageManager, which always succeeds and never touches go-containerregistry's reference-parsing/pull logic. TestEnvoyBootstrap_ValidatesAgainstRealEnvoy calls docker run directly, bypassing ToolHive's own PullImage/ImageExists entirely. As a result, the PullImage regression above shipped without any failing test.

Recommendation: add a test (can be gated behind Docker availability like the existing real-Envoy test) that calls RegistryImageManager.PullImage/ImageExists with a real tag@digest reference, or an end-to-end check that starts the Envoy backend on a clean image cache.

Raised by: security specialist.


// Protobuf type URLs required by Envoy's protobuf-JSON bootstrap format.
// Every typed_config field must carry an @type URL or Envoy will reject the
Expand Down Expand Up @@ -56,16 +56,12 @@ func getEnvoyImage() string {
// ── Bootstrap ────────────────────────────────────────────────────────────────

// envoyBootstrap is the top-level Envoy bootstrap configuration.
// The admin interface is intentionally omitted: Envoy does not start an admin
// server when the field is absent, eliminating the attack surface at runtime.
type envoyBootstrap struct {
Admin *envoyAdmin `json:"admin,omitempty"`
StaticResources envoyStaticResources `json:"static_resources"`
}

// envoyAdmin configures the Envoy admin API endpoint.
type envoyAdmin struct {
Address envoyAddress `json:"address"`
}

// envoyStaticResources holds the static listeners and clusters.
type envoyStaticResources struct {
Listeners []envoyListener `json:"listeners,omitempty"`
Expand Down Expand Up @@ -882,14 +878,6 @@ func (e *envoyProxy) SetupIngress(ctx context.Context, spec proxySpec, _ egressR
egressContainerName := fmt.Sprintf("%s-egress", spec.WorkloadName)

bootstrap := envoyBootstrap{
Admin: &envoyAdmin{
Address: envoyAddress{
SocketAddress: envoySocketAddress{
Address: "127.0.0.1", // loopback only — never 0.0.0.0
PortValue: 9901,
},
},
},
StaticResources: envoyStaticResources{
Listeners: []envoyListener{buildEgressListener(spec)},
Clusters: []envoyCluster{buildEgressCluster()},
Expand Down
33 changes: 8 additions & 25 deletions pkg/container/docker/envoy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,14 +550,6 @@ func TestWriteEnvoyBootstrap_FileMode(t *testing.T) {
t.Parallel()

b := envoyBootstrap{
Admin: &envoyAdmin{
Address: envoyAddress{
SocketAddress: envoySocketAddress{
Address: "127.0.0.1",
PortValue: 9901,
},
},
},
StaticResources: envoyStaticResources{},
}

Expand Down Expand Up @@ -588,21 +580,13 @@ func TestWriteEnvoyBootstrap_FileMode(t *testing.T) {
"bootstrap file must contain valid JSON")
}

// TestEnvoyAdmin_LoopbackOnly asserts that the admin block written by
// writeEnvoyBootstrap binds only on the loopback address and never on
// 0.0.0.0 or an empty address that would expose admin to all interfaces.
func TestEnvoyAdmin_LoopbackOnly(t *testing.T) {
// TestEnvoyAdmin_Absent asserts that the admin interface is entirely absent from
// the generated bootstrap. Omitting the admin block causes Envoy to skip the
// admin server, removing the attack surface without affecting proxy behaviour.
func TestEnvoyAdmin_Absent(t *testing.T) {
t.Parallel()

b := envoyBootstrap{
Admin: &envoyAdmin{
Address: envoyAddress{
SocketAddress: envoySocketAddress{
Address: "127.0.0.1",
PortValue: 9901,
},
},
},
StaticResources: envoyStaticResources{},
}

Expand All @@ -615,10 +599,10 @@ func TestEnvoyAdmin_LoopbackOnly(t *testing.T) {
require.NoError(t, err)
s := string(data)

assert.Contains(t, s, "127.0.0.1",
"admin address must be loopback 127.0.0.1")
assert.NotContains(t, s, "0.0.0.0",
"admin address must NOT bind on 0.0.0.0")
assert.NotContains(t, s, `"admin"`,
"bootstrap must not contain an admin block")
assert.NotContains(t, s, "9901",
"bootstrap must not reference the admin port")
Comment on lines +602 to +605

@glageju glageju Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[LOW, 6/10] TestEnvoyAdmin_Absent uses substring checks instead of structural JSON assertions

Checking that the marshaled JSON doesn't contain the substrings "admin" and 9901 is adequate for the current struct shape, but weaker than a structural check — a future field value that happens to contain "admin" as a substring could produce a false negative.

Recommendation: consider unmarshaling into map[string]any and asserting the admin key is absent (_, ok := m["admin"]; assert.False(t, ok)), which is precise regardless of what else the config contains. The 9901 check is redundant once the admin key's absence is confirmed structurally.

Raised by: go-correctness specialist, codex.

}

// TestGetEnvoyImage verifies that getEnvoyImage returns the default image when
Expand Down Expand Up @@ -752,7 +736,6 @@ func TestEnvoyBootstrap_ValidatesAgainstRealEnvoy(t *testing.T) {
clusters = append(clusters, buildIngressCluster(tc.spec))
}
b := envoyBootstrap{
Admin: &envoyAdmin{Address: envoyAddress{SocketAddress: envoySocketAddress{Address: "127.0.0.1", PortValue: 9901}}},
StaticResources: envoyStaticResources{Listeners: listeners, Clusters: clusters},
}
cfg, err := json.Marshal(b)
Expand Down
10 changes: 9 additions & 1 deletion pkg/transport/proxy/streamable/dispatcher_standalone_sse_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,11 +461,19 @@ func TestStandaloneSSE_ListChangedRefiltersThroughExistingMiddleware(t *testing.
assert.NotContains(t, string(listRespBody), "filtered_tool")

// (3) tools/call POST for the filtered tool is still blocked.
// The middleware returns HTTP 200 with a JSON-RPC error body when the
// client accepts JSON (no Accept header → clientAcceptsJSON returns true).
// This is correct MCP streamable HTTP behaviour: application-level errors
// ride in 200 responses carrying a JSON-RPC error object.
callBody := `{"jsonrpc":"2.0","id":"call-1","method":"tools/call","params":{"name":"filtered_tool"}}`
callResp, err := http.Post(endpoint, "application/json", strings.NewReader(callBody)) //nolint:noctx // test-only
require.NoError(t, err)
t.Cleanup(func() { _ = callResp.Body.Close() })
assert.Equal(t, http.StatusBadRequest, callResp.StatusCode)
callRespBody, err := io.ReadAll(callResp.Body)
require.NoError(t, err)
assert.Equal(t, http.StatusOK, callResp.StatusCode)
assert.Contains(t, string(callRespBody), `"error"`, "response must contain a JSON-RPC error for the filtered tool")
assert.NotContains(t, string(callRespBody), "filtered_tool", "error must not leak the filtered tool name")
Comment on lines +464 to +476

@glageju glageju Jul 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MEDIUM, 8/10] Unrelated test fix bundled into an Envoy-hardening PR without disclosure

This PR's title, summary, and Changes table are entirely about Envoy digest pinning and admin-interface removal, but this hunk fixes an assertion in a different subsystem (streamable-HTTP transport dispatcher), addressing a bug from a different PR (#5934). The fix itself is technically correct — it matches the established pattern elsewhere in this file for how JSON-RPC application errors ride in HTTP 200 responses — but it's undisclosed here, so reviewers scanning the title/description have no reason to look at this file, and reverting the Envoy hardening would also silently revert this fix.

Recommendation: split this into its own PR with its own description, or at minimum update this PR's title/summary/Changes table and test plan to disclose and justify it.

Raised by: go-correctness specialist, general-quality specialist, codex (3/3 agents).

}

// ------------------------- Routing security regressions -------------------------
Expand Down
Loading