Skip to content
Merged

fix bug #3120

Show file tree
Hide file tree
Changes from all commits
Commits
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
38 changes: 34 additions & 4 deletions .claude/rules/go-control-plane-xds-security.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ xDS is itself a security control plane, not just config distribution: a spoofed
1. **TLS + mTLS mandatory for every xDS gRPC server, never config-optional in production.** Every xDS/policy-xDS server (`:18000`/`:18001`-style) must default to `Enabled: true` with `tls.RequireAndVerifyClientCert`, not TLS-disabled-by-default. If TLS is disabled and the listener binds anywhere other than loopback, refuse to start (fatal error, per GO-AUTH-011's fail-closed pattern) — plaintext is acceptable only when controller and runtime are strictly co-located on loopback.
2. **Authenticate AND authorize every xDS stream — mTLS alone is not authorization.** In `OnStreamOpen`/`OnStreamRequest`, extract the verified peer identity (cert SAN or SPIFFE ID) and reject any stream not on an explicit allowlist for the expected runtime/policy-engine node. A callback that only logs the peer with no accept/reject decision is equivalent to no authorization — any client completing the handshake (including a leaked/over-broad cert) gets the full snapshot, which routinely contains API-key hashes, subscription data, and full policy chains for every tenant.
3. **Never embed private key material inline in an xDS resource.** Serve the downstream listener's cert/key via a `TlsCertificateSdsSecretConfig` reference — never `inline_bytes` in the LDS `Listener` resource. An inline key means a single successful LDS request (from any peer clearing directives 1-2) exfiltrates the private key directly.
4. **Harden the Envoy admin interface independently of the xDS channel.** Bind the admin listener (`:9901`-style) to `127.0.0.1` only, never `0.0.0.0` — it exposes TLS keys, full runtime config, and `/runtime_modify`. Point `flags_path` at a read-only mount so `/runtime_modify` is inert even if reached via another path (compromised sidecar, debug shell). Ship a default-deny NetworkPolicy for the admin port and never provision a `Service`/`Ingress`/`NodePort` exposing it.
4. **The Envoy admin interface must be disabled by default, not merely loopback-bound.** It exposes TLS keys, full runtime config, and `/runtime_modify`, so the static/bootstrap config must ship with no `admin:` block at all — the interface doesn't exist until an operator explicitly opts in (e.g. a `ROUTER_ADMIN_ENABLED`-style flag consumed by the entrypoint, never a value baked into the shipped bootstrap). `ROUTER_ADMIN_HOST` is operator-configurable and defaults to `127.0.0.1`; it may be widened (e.g. `0.0.0.0` in docker-compose/IT environments where the admin API must be reachable from another container) when the deployment's own network boundary — a docker-compose network, pod network namespace, or `NetworkPolicy` — already restricts who can reach it. That widening is not a substitute for the off-by-default posture, and it does not relax the next requirement: point `flags_path` at a read-only mount so `/runtime_modify` is inert even if reached via another path (compromised sidecar, debug shell). Never provision a `Service`/`Ingress`/`NodePort` exposing the admin port — true whether or not the interface is currently enabled, and true regardless of `ROUTER_ADMIN_HOST` — and don't treat a `NetworkPolicy` as a substitute for that; it's defense-in-depth on top of, not instead of, the Service never publishing the port.
5. **Resource-limit every xDS gRPC server.** Set `grpc.MaxRecvMsgSize`, `grpc.MaxSendMsgSize`, and `grpc.MaxConcurrentStreams` explicitly on every construction (see `go-network-service-hardening.md` directive 2) — unbounded defaults let one client exhaust memory or the stream-slot budget other clients depend on.
6. **Canonicalize request paths before any route/policy/authz match downstream.** Every generated `HttpConnectionManager` must set `NormalizePath: true`, `MergeSlashes: true`, and an explicit `PathWithEscapedSlashesAction` (`UNESCAPE_AND_REDIRECT` or `REJECT_REQUEST`). Unset, Envoy matches routes/policy/authz against an un-normalized path (`//`, `/./`, `/../`, `%2F`), desynchronizing the selected route from what the operator's policy was written against — the same bypass class as GO-AUTH-004, applied to generated Envoy config. Apply identically across the main listener, any WebSub-internal listener, and any dynamic-forward-proxy HCM; test that a new listener type can't silently omit it.

Expand Down Expand Up @@ -53,11 +53,40 @@ func (cb *serverCallbacks) OnStreamOpen(ctx context.Context, id int64, typ strin
```

```yaml
# GOOD: Envoy admin bound to loopback only; runtime_modify disabled via a
# read-only flags_path; no Service/Ingress/NodePort ever exposes this port.
# BAD: admin block present in the static bootstrap unconditionally — the
# interface exists the moment Envoy starts, with no opt-in required at all.
# Loopback-binding here is not enough; the block should not exist by default.
admin:
address:
socket_address: { address: 127.0.0.1, port_value: 9901 }
```

```bash
# GOOD: static bootstrap ships with NO admin: block. The interface is
# injected only when an operator explicitly opts in, and the host is
# validated (not just defaulted) so an env var can't widen the bind to
# a non-loopback address — e.g. an entrypoint script gating the injection:
ROUTER_ADMIN_HOST="${ROUTER_ADMIN_HOST:-127.0.0.1}"
case "${ROUTER_ADMIN_HOST}" in
127.0.0.1|::1|localhost) ;;
*) echo "FATAL: ROUTER_ADMIN_HOST must be loopback (127.0.0.1/::1), got '${ROUTER_ADMIN_HOST}'" >&2; exit 1 ;;
esac
Comment thread
coderabbitai[bot] marked this conversation as resolved.

if [ "${ROUTER_ADMIN_ENABLED}" = "true" ]; then
CONFIG_OVERRIDE="${CONFIG_OVERRIDE}
admin:
address:
socket_address:
address: ${ROUTER_ADMIN_HOST}
port_value: ${ROUTER_ADMIN_PORT}
"
fi
```

```yaml
# Also required whenever the admin interface is enabled: runtime_modify
# disabled via a read-only flags_path, so an already-authorized reach to the
# admin API still can't rewrite runtime config.
layered_runtime:
layers:
- name: static_layer
Expand All @@ -71,6 +100,7 @@ layered_runtime:
> * Does an xDS/policy-xDS gRPC server default to TLS-disabled, or allow a plaintext bind to a non-loopback address without refusing to start?
> * Does a stream callback only log the peer without an accept/reject decision against an identity allowlist?
> * Is any private key composed as `inline_bytes` inside an xDS resource, instead of an SDS secret reference?
> * Does the Envoy admin interface bind `0.0.0.0`, or is `/runtime_modify` reachable without a read-only `flags_path`/NetworkPolicy?
> * Is the Envoy admin interface present in the static/bootstrap config by default instead of injected only on an explicit opt-in, or does an enabled instance bind `0.0.0.0` instead of `127.0.0.1`, or is `/runtime_modify` reachable without a read-only `flags_path`?
> * Does a `Service`/`Ingress`/`NodePort` publish the admin port, regardless of whether the interface is currently enabled?
> * Does an xDS gRPC server construction omit `MaxRecvMsgSize`/`MaxSendMsgSize`/`MaxConcurrentStreams`?
> * Does the xDS translator generate an HCM without `NormalizePath`/`MergeSlashes`/`PathWithEscapedSlashesAction` set, on every listener type?
22 changes: 21 additions & 1 deletion gateway/configs/config-template.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,21 @@ shutdown_timeout = "15s"
gateway_id = '{{ env "APIP_GW_CONTROLLER_SERVER_GATEWAY_ID" "platform-gateway-id" }}'

[controller.admin_server]
# Dedicated admin/debug HTTP server for config dump and xDS sync endpoints
# Dedicated admin/debug HTTP server for config dump and xDS sync endpoints.
# Kept enabled by default because it also serves /health, used by Kubernetes
# liveness/readiness probes; see admin_server.config_dump below to gate the
# sensitive /config_dump route specifically.
enabled = true
port = 9092
allowed_ips = ["*"]

[controller.admin_server.config_dump]
# The /config_dump route returns a full snapshot of deployed APIs, policies,
# and resolved configuration. Off by default — enable only when needed for
# debugging, and prefer reaching it via `kubectl port-forward` over exposing
# the admin Service port.
enabled = false

[controller.admin_server.pprof]
# Go runtime profiling (net/http/pprof) served on the admin server, off by default.
# When profiling, also restrict admin_server.allowed_ips or reach it via port-forward.
Expand Down Expand Up @@ -312,10 +322,20 @@ max_decompressed_bytes = 10485760
extproc_port = 9001

[policy_engine.admin]
# Kept enabled by default because it also serves /health, used by Kubernetes
# liveness/readiness probes; see admin.config_dump below to gate the
# sensitive /config_dump route specifically.
enabled = true
port = 9002
allowed_ips = ["*", "127.0.0.1"]

[policy_engine.admin.config_dump]
# The /config_dump route returns the resolved policy chain and route
# configuration. Off by default — enable only when needed for debugging, and
# prefer reaching it via `kubectl port-forward` over exposing the admin
# Service port.
enabled = false

[policy_engine.admin.pprof]
# Go runtime profiling (net/http/pprof) served on the admin server, off by default.
# When profiling, also restrict admin.allowed_ips or reach it via port-forward.
Expand Down
3 changes: 3 additions & 0 deletions gateway/distribution/docker-compose.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ services:
format: raw
environment:
- GATEWAY_CONTROLLER_HOST=gateway-controller
# Envoy admin is disabled by default in the image; enabled here for local
# dev convenience since the port is already mapped to the host above.
- ROUTER_ADMIN_ENABLED=true
volumes:
- ./configs/config.toml:/etc/policy-engine/config.toml:ro
- ./configs/llm-pricing/model_prices.json:/etc/policy-engine/llm-pricing/model_prices.json:ro
Expand Down
3 changes: 3 additions & 0 deletions gateway/docker-compose-perf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ services:
environment:
- GATEWAY_CONTROLLER_HOST=gateway-controller
- LOG_LEVEL=info
# Envoy admin is disabled by default in the image; enabled here for local
# dev convenience since the port is already mapped to the host above.
- ROUTER_ADMIN_ENABLED=true
volumes:
- ./configs/config.toml:/etc/policy-engine/config.toml:ro
networks:
Expand Down
3 changes: 3 additions & 0 deletions gateway/docker-compose.debug.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ services:
environment:
- GATEWAY_CONTROLLER_HOST=gateway-controller
- LOG_LEVEL=info
# Envoy admin is disabled by default in the image; enabled here for local
# dev convenience since the port is already mapped to the host above.
- ROUTER_ADMIN_ENABLED=true
volumes:
- ./configs/config.toml:/etc/policy-engine/config.toml:ro
networks:
Expand Down
5 changes: 5 additions & 0 deletions gateway/gateway-controller/pkg/adminserver/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ func (s *Server) Stop(ctx context.Context) error {

// GetConfigDump implements adminapi.ServerInterface.
func (s *Server) GetConfigDump(w http.ResponseWriter, r *http.Request) {
if !s.cfg.ConfigDump.Enabled {
http.NotFound(w, r)
return
}

resp, err := s.apiServer.BuildConfigDumpResponse(s.logger)
if err != nil {
http.Error(w, "Failed to retrieve configuration dump", http.StatusInternalServerError)
Expand Down
40 changes: 36 additions & 4 deletions gateway/gateway-controller/pkg/adminserver/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,11 @@ func TestAdminServer_ConfigDumpHandler(t *testing.T) {
stub := &stubAPIServer{
configDump: adminapi.ConfigDumpResponse{Status: &status},
}
s := NewServer(&config.AdminServerConfig{Port: 9092, AllowedIPs: []string{"*"}}, stub, nil, slog.Default())
s := NewServer(&config.AdminServerConfig{
Port: 9092,
AllowedIPs: []string{"*"},
ConfigDump: config.ConfigDumpConfig{Enabled: true},
}, stub, nil, slog.Default())

req := httptest.NewRequest(http.MethodGet, AdminAPIBasePath+"/config_dump", nil)
req.RemoteAddr = "127.0.0.1:12345"
Expand All @@ -100,6 +104,22 @@ func TestAdminServer_ConfigDumpHandler(t *testing.T) {
assert.Equal(t, "ok", *body.Status)
}

func TestAdminServer_ConfigDumpHandler_DisabledByDefault(t *testing.T) {
status := "ok"
stub := &stubAPIServer{
configDump: adminapi.ConfigDumpResponse{Status: &status},
}
// ConfigDump.Enabled left at its zero value (false) — matches the production default.
s := NewServer(&config.AdminServerConfig{Port: 9092, AllowedIPs: []string{"*"}}, stub, nil, slog.Default())

req := httptest.NewRequest(http.MethodGet, AdminAPIBasePath+"/config_dump", nil)
req.RemoteAddr = "127.0.0.1:12345"
rr := httptest.NewRecorder()

s.httpSrv.Handler.ServeHTTP(rr, req)
assert.Equal(t, http.StatusNotFound, rr.Code)
}

func TestAdminServer_XDSSyncStatusHandler(t *testing.T) {
component := "gateway-controller"
version := "12"
Expand Down Expand Up @@ -221,7 +241,11 @@ func TestAdminServer_LegacyConfigDump(t *testing.T) {
stub := &stubAPIServer{
configDump: adminapi.ConfigDumpResponse{Status: &status},
}
s := NewServer(&config.AdminServerConfig{Port: 9092, AllowedIPs: []string{"*"}}, stub, nil, slog.Default())
s := NewServer(&config.AdminServerConfig{
Port: 9092,
AllowedIPs: []string{"*"},
ConfigDump: config.ConfigDumpConfig{Enabled: true},
}, stub, nil, slog.Default())

req := httptest.NewRequest(http.MethodGet, "/config_dump", nil)
req.RemoteAddr = "127.0.0.1:12345"
Expand Down Expand Up @@ -307,7 +331,11 @@ func TestAdminServer_ConfigDump_WrongCredentials(t *testing.T) {
func TestAdminServer_ConfigDump_WithValidAuth(t *testing.T) {
status := "ok"
stub := &stubAPIServer{configDump: adminapi.ConfigDumpResponse{Status: &status}}
s := NewServer(&config.AdminServerConfig{Port: 9092, AllowedIPs: []string{"*"}}, stub, newBasicAuthMiddleware(t), slog.Default())
s := NewServer(&config.AdminServerConfig{
Port: 9092,
AllowedIPs: []string{"*"},
ConfigDump: config.ConfigDumpConfig{Enabled: true},
}, stub, newBasicAuthMiddleware(t), slog.Default())

req := httptest.NewRequest(http.MethodGet, AdminAPIBasePath+"/config_dump", nil)
req.SetBasicAuth(testAdminUser, testAdminPass)
Expand Down Expand Up @@ -384,7 +412,11 @@ func TestAdminServer_LegacyConfigDump_RequiresAuth(t *testing.T) {
func TestAdminServer_ConfigDump_AdminRoleAllowed(t *testing.T) {
status := "ok"
stub := &stubAPIServer{configDump: adminapi.ConfigDumpResponse{Status: &status}}
s := NewServer(&config.AdminServerConfig{Port: 9092, AllowedIPs: []string{"*"}}, stub,
s := NewServer(&config.AdminServerConfig{
Port: 9092,
AllowedIPs: []string{"*"},
ConfigDump: config.ConfigDumpConfig{Enabled: true},
}, stub,
newAdminProtectMiddleware(t, []string{"admin"}), slog.Default())

req := httptest.NewRequest(http.MethodGet, AdminAPIBasePath+"/config_dump", nil)
Expand Down
19 changes: 15 additions & 4 deletions gateway/gateway-controller/pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -309,10 +309,18 @@ type ServerConfig struct {

// AdminServerConfig holds controller admin HTTP server configuration.
type AdminServerConfig struct {
Enabled bool `koanf:"enabled"`
Port int `koanf:"port"`
AllowedIPs []string `koanf:"allowed_ips"`
Pprof PprofConfig `koanf:"pprof"`
Enabled bool `koanf:"enabled"`
Port int `koanf:"port"`
AllowedIPs []string `koanf:"allowed_ips"`
Pprof PprofConfig `koanf:"pprof"`
ConfigDump ConfigDumpConfig `koanf:"config_dump"`
}

// ConfigDumpConfig gates the /config_dump endpoint served on the admin HTTP
// server. Disabled by default — /health and other admin routes are unaffected
// by this flag; when disabled, /config_dump returns 404 rather than a payload.
type ConfigDumpConfig struct {
Enabled bool `koanf:"enabled"`
}

// PprofConfig gates the Go runtime profiling endpoints (net/http/pprof) served on
Expand Down Expand Up @@ -834,6 +842,9 @@ func defaultConfig() *Config {
BlockProfileRate: 0,
MutexProfileFraction: 0,
},
ConfigDump: ConfigDumpConfig{
Enabled: false,
},
},
PolicyServer: PolicyServerConfig{
Port: 18001,
Expand Down
33 changes: 30 additions & 3 deletions gateway/gateway-runtime/docker-entrypoint-debug.sh
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,19 @@ export ROUTER_CONCURRENCY="${ROUTER_CONCURRENCY:-0}"
export APIP_GW_POLICY_ENGINE_METRICS_ENABLED="${APIP_GW_POLICY_ENGINE_METRICS_ENABLED:-true}"
export APIP_GW_ROUTER_RE2_MAX_PROGRAM_SIZE="${APIP_GW_ROUTER_RE2_MAX_PROGRAM_SIZE:-400}"

# Router (Envoy) admin interface configuration (see docker-entrypoint.sh for details).
# Disabled by default — not defined in the static envoy-bootstrap.yaml at all. Set
# ROUTER_ADMIN_ENABLED=true to inject it at startup, bound to ROUTER_ADMIN_HOST (loopback
# by default).
export ROUTER_ADMIN_ENABLED="${ROUTER_ADMIN_ENABLED:-false}"
export ROUTER_ADMIN_HOST="${ROUTER_ADMIN_HOST:-127.0.0.1}"
export ROUTER_ADMIN_PORT="${ROUTER_ADMIN_PORT:-9901}"

# Graceful shutdown configuration (see docker-entrypoint.sh for details).
# On SIGTERM the Router (Envoy) is drained before processes are terminated so in-flight
# requests finish and keep-alive connections close cleanly instead of being reset.
# Requires ROUTER_ADMIN_ENABLED=true — skipped otherwise.
# Keep ROUTER_DRAIN_TIME_SECONDS < the pod terminationGracePeriodSeconds; 0 disables it.
export ROUTER_ADMIN_HOST="${ROUTER_ADMIN_HOST:-127.0.0.1}"
export ROUTER_ADMIN_PORT="${ROUTER_ADMIN_PORT:-9901}"
export ROUTER_DRAIN_TIME_SECONDS="${ROUTER_DRAIN_TIME_SECONDS:-15}"

# Derive Router (Envoy) xDS config — used by envsubst on config-override.yaml
Expand All @@ -121,6 +128,11 @@ log " GOMAXPROCS: ${GOMAXPROCS}"
log " Router Concurrency: ${ROUTER_CONCURRENCY}"
log " Router RE2 Max Program Size: ${APIP_GW_ROUTER_RE2_MAX_PROGRAM_SIZE}"
log " Policy Engine Metrics: ${APIP_GW_POLICY_ENGINE_METRICS_ENABLED}"
if [ "${ROUTER_ADMIN_ENABLED}" = "true" ]; then
log " Router Admin: enabled on ${ROUTER_ADMIN_HOST}:${ROUTER_ADMIN_PORT}"
else
log " Router Admin: disabled (set ROUTER_ADMIN_ENABLED=true to enable)"
fi
[[ ${#ROUTER_ARGS[@]} -gt 0 ]] && log " Router extra args: ${ROUTER_ARGS[*]}"
[[ ${#PE_ARGS[@]} -gt 0 ]] && log " Policy Engine extra args: ${PE_ARGS[*]}"

Expand All @@ -130,6 +142,18 @@ rm -f "${POLICY_ENGINE_SOCKET}"
# Generate Envoy config override by substituting environment variables
CONFIG_OVERRIDE=$(envsubst < /etc/envoy/config-override.yaml)

# The admin interface has no entry in the static bootstrap at all, so it only exists when
# explicitly opted into here — bound to ROUTER_ADMIN_HOST (loopback by default), never 0.0.0.0.
if [ "${ROUTER_ADMIN_ENABLED}" = "true" ]; then
CONFIG_OVERRIDE="${CONFIG_OVERRIDE}
admin:
address:
socket_address:
address: ${ROUTER_ADMIN_HOST}
port_value: ${ROUTER_ADMIN_PORT}
"
fi

# Track child PIDs
PE_PID=""
ENVOY_PID=""
Expand Down Expand Up @@ -167,7 +191,10 @@ shutdown() {

# Drain the Router first so in-flight requests finish and keep-alive connections are
# closed cleanly — prevents client-visible connection resets during rolling restarts.
if [ -n "$ENVOY_PID" ] && kill -0 "$ENVOY_PID" 2>/dev/null \
# Requires ROUTER_ADMIN_ENABLED=true; skipped otherwise since draining needs an admin call.
if [ "${ROUTER_ADMIN_ENABLED}" != "true" ]; then
log "Router admin disabled (ROUTER_ADMIN_ENABLED=false); skipping graceful drain"
elif [ -n "$ENVOY_PID" ] && kill -0 "$ENVOY_PID" 2>/dev/null \
&& [ "${ROUTER_DRAIN_TIME_SECONDS}" -gt 0 ] 2>/dev/null; then
log "Draining Router (Envoy); waiting up to ${ROUTER_DRAIN_TIME_SECONDS}s for in-flight requests..."
if drain_router; then
Expand Down
Loading
Loading