From 048cd5175511c7869415bac027cfe593a51bd255 Mon Sep 17 00:00:00 2001 From: Damilola Edwards Date: Wed, 22 Jul 2026 22:56:22 +0100 Subject: [PATCH] Stop treating caller attribution as a session ownership key by default Sandbox session get/destroy/list ownership was keyed off the X-Panda-On-Behalf-Of header whenever the server had no authenticated user for a request. That header is client-supplied and unauthenticated by design, meant only for audit logging, so any caller that could reach the server could set it to someone else's value and read, destroy, or enumerate their sandbox sessions. With no header at all the ownership check was skipped entirely. Gate that fallback behind an explicit opt-in, server.attribution_session_scoping, off by default. A deployment that wants session isolation on an unauthenticated server despite the header being spoofable (an automated test harness isolating its own worker sessions, for example) can turn it on deliberately. Update the CI eval workflow and the harden loop's scratch server config to opt in, since both rely on it for per-worker session cleanup between concurrent runs. --- .github/workflows/eval-smoke.yaml | 4 ++++ config.example.yaml | 4 ++++ pkg/config/config.go | 11 ++++++++++ pkg/server/api.go | 22 ++++++++++++------- pkg/server/runtime_attribution_test.go | 29 ++++++++++++++++++++------ tests/eval/scripts/_panda_env.py | 3 +++ 6 files changed, 59 insertions(+), 14 deletions(-) diff --git a/.github/workflows/eval-smoke.yaml b/.github/workflows/eval-smoke.yaml index 7f2a0922..42818808 100644 --- a/.github/workflows/eval-smoke.yaml +++ b/.github/workflows/eval-smoke.yaml @@ -203,6 +203,10 @@ jobs: port: 2480 base_url: "http://127.0.0.1:2480" sandbox_url: "http://127.0.0.1:2480" + # This server runs unauthenticated; concurrent eval workers each tear down + # only their own sandbox sessions between tests, scoped by the + # X-Panda-On-Behalf-Of header the agent sets per worker. + attribution_session_scoping: true sandbox: backend: docker diff --git a/config.example.yaml b/config.example.yaml index 25c8f9b7..e5fc7cfa 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -8,6 +8,10 @@ server: base_url: "http://localhost:2480" # ep clients should point at this URL sandbox_url: "http://ethpandaops-panda-server:2480" # URL sandbox containers use to call the local server # uploads: false # opt out of `panda upload` (upload/publish API + /u/ preview pages) + # attribution_session_scoping: true # scope unauthenticated sandbox sessions by the + # caller-supplied X-Panda-On-Behalf-Of header. That header is untrusted and + # spoofable by any caller that can reach this server, so leave this off unless + # the deployment already accepts that tradeoff (e.g. an automated test harness). # Sandbox configuration sandbox: diff --git a/pkg/config/config.go b/pkg/config/config.go index 53738af5..3f2af637 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -79,6 +79,17 @@ type ServerConfig struct { // Nothing leaves the machine without an explicit publish either way. Uploads *bool `yaml:"uploads,omitempty"` + // AttributionSessionScoping opts into using the caller-supplied + // X-Panda-On-Behalf-Of attribution value as a sandbox session ownership + // key when the server has no authenticated user for a request. That value + // is untrusted and audit-only by design (see pkg/attribution): any caller + // that can reach the server can set it to any value, including someone + // else's. Leave this off (the default) unless the deployment already + // accepts that tradeoff, such as an automated eval harness isolating its + // own worker sessions. Off, unauthenticated requests get no ownership + // scoping at all. + AttributionSessionScoping bool `yaml:"attribution_session_scoping,omitempty"` + // Deprecated: Transport is accepted for backwards compatibility but ignored. // The server always runs HTTP with both SSE and streamable-http transports. Transport string `yaml:"transport,omitempty"` diff --git a/pkg/server/api.go b/pkg/server/api.go index 7e9b138b..530d7721 100644 --- a/pkg/server/api.go +++ b/pkg/server/api.go @@ -296,7 +296,7 @@ func (s *service) handleAPIExecute(w http.ResponseWriter, r *http.Request) { return } - ownerID := authOwnerID(r) + ownerID := s.authOwnerID(r) result, err := s.execService.Execute(r.Context(), execsvc.ExecuteRequest{ Code: req.Code, Timeout: req.Timeout, @@ -337,7 +337,7 @@ func (s *service) handleAPIListSessions(w http.ResponseWriter, r *http.Request) return } - sessions, maxSessions, err := s.execService.ListSessions(r.Context(), authOwnerID(r)) + sessions, maxSessions, err := s.execService.ListSessions(r.Context(), s.authOwnerID(r)) if err != nil { writeAPIError(w, http.StatusBadRequest, err.Error()) return @@ -372,7 +372,7 @@ func (s *service) handleAPICreateSession(w http.ResponseWriter, r *http.Request) return } - ownerID := authOwnerID(r) + ownerID := s.authOwnerID(r) sessionID, err := s.execService.CreateSession(r.Context(), ownerID) if err != nil { writeAPIError(w, http.StatusBadRequest, err.Error()) @@ -404,7 +404,7 @@ func (s *service) handleAPIDestroySession(w http.ResponseWriter, r *http.Request return } - if err := s.execService.DestroySession(r.Context(), sessionID, authOwnerID(r)); err != nil { + if err := s.execService.DestroySession(r.Context(), sessionID, s.authOwnerID(r)); err != nil { writeAPIError(w, http.StatusBadRequest, err.Error()) return } @@ -940,14 +940,20 @@ func runtimeStorageScope(r *http.Request, executionID string) storage.Scope { } } -func authOwnerID(r *http.Request) string { +func (s *service) authOwnerID(r *http.Request) string { if user := auth.GetAuthUser(r.Context()); user != nil { return fmt.Sprintf("%d", user.GitHubID) } - // Fall back to caller attribution so sessions are owner-scoped even when - // the server runs unauthenticated (e.g. local mode, the eval harness). - // Empty when no attribution is present, preserving prior behavior. + // Caller attribution is untrusted and audit-only (pkg/attribution): any + // caller that can reach this server can set it to any value, including + // someone else's. Only use it as a session-ownership key when the + // operator has explicitly opted in; otherwise an unauthenticated request + // gets no ownership scoping at all. + if !s.cfg.AttributionSessionScoping { + return "" + } + return attribution.FromContext(r.Context()) } diff --git a/pkg/server/runtime_attribution_test.go b/pkg/server/runtime_attribution_test.go index 18fd3e33..f08dfe37 100644 --- a/pkg/server/runtime_attribution_test.go +++ b/pkg/server/runtime_attribution_test.go @@ -94,14 +94,31 @@ func TestRuntimeCallbacksInheritExecutionAttribution(t *testing.T) { "runtime callbacks must inherit the spawning execution's attribution") } -func TestAuthOwnerIDAttributionFallback(t *testing.T) { - // No auth user: fall back to the caller attribution so sessions are owner-scoped - // even on an unauthenticated server (local mode, the eval harness). +func TestAuthOwnerIDIgnoresAttributionByDefault(t *testing.T) { + // Attribution is untrusted and audit-only. By default, an unauthenticated + // request with no auth user gets no ownership scoping at all, even if a + // caller-supplied attribution value is present, since any other caller + // could set the same value. + svc := &service{} + + withAttr := httptest.NewRequest(http.MethodGet, "/api/v1/sessions", nil) + withAttr = withAttr.WithContext(attribution.WithValue(withAttr.Context(), "alice")) + require.Equal(t, "", svc.authOwnerID(withAttr)) + + bare := httptest.NewRequest(http.MethodGet, "/api/v1/sessions", nil) + require.Equal(t, "", svc.authOwnerID(bare)) +} + +func TestAuthOwnerIDUsesAttributionWhenExplicitlyEnabled(t *testing.T) { + // An operator that explicitly opts into attribution-based session scoping + // (e.g. an automated eval harness isolating its own worker sessions) gets + // the caller attribution value as the owner id. + svc := &service{cfg: config.ServerConfig{AttributionSessionScoping: true}} + withAttr := httptest.NewRequest(http.MethodGet, "/api/v1/sessions", nil) withAttr = withAttr.WithContext(attribution.WithValue(withAttr.Context(), "eval-worker-abc123")) - require.Equal(t, "eval-worker-abc123", authOwnerID(withAttr)) + require.Equal(t, "eval-worker-abc123", svc.authOwnerID(withAttr)) - // Neither auth user nor attribution: empty owner, preserving prior behavior. bare := httptest.NewRequest(http.MethodGet, "/api/v1/sessions", nil) - require.Equal(t, "", authOwnerID(bare)) + require.Equal(t, "", svc.authOwnerID(bare)) } diff --git a/tests/eval/scripts/_panda_env.py b/tests/eval/scripts/_panda_env.py index 5a7727e3..7daad3cd 100644 --- a/tests/eval/scripts/_panda_env.py +++ b/tests/eval/scripts/_panda_env.py @@ -50,6 +50,9 @@ def write_scratch_config(port: int, *, source: Path | None = None) -> Path: cfg["server"]["port"] = port cfg["server"]["base_url"] = f"http://localhost:{port}" cfg["server"]["sandbox_url"] = f"http://host.docker.internal:{port}" + # This scratch server runs unauthenticated; the per-test owner-scoped cleanup + # below relies on the X-Panda-On-Behalf-Of header actually scoping sessions. + cfg["server"]["attribution_session_scoping"] = True sb = cfg.setdefault("sandbox", {}) sb["image"] = "ethpandaops-panda-sandbox:latest" sb["network"] = "ethpandaops-panda-harden"