auth: enforce per-service-account ownership on GET session endpoints

Signed-off-by: Abhinav Singh <abhinavsingh717073@gmail.com>

Author: Abhinav-kodes

pkg/common/types/sandbox.go

@@ -42,6 +42,10 @@ type SandboxInfo struct {
 	// during ListInactiveSandboxes. It is intentionally excluded from JSON serialization.
 	LastActivityAt time.Time `json:"-"`
 	Status         string    `json:"status"`
+	// CreatedBy holds the Kubernetes service account name (without the namespace prefix)
+	// that created this sandbox. It is used by the GET handlers to enforce per-service-account
+	// ownership: only the creating SA can retrieve its own session entrypoints.
+	CreatedBy string `json:"createdBy,omitempty"`
 }
 
 type SandboxEntryPoint struct {

pkg/workloadmanager/auth.go

@@ -47,18 +47,24 @@ import (
 //
 // POST /v1/sandboxes (CreateSandbox):
 //   - Any authenticated user can create sandboxes
-//   - The creator's service account name is stored in the sandbox metadata
+//   - The creator's service account name is stamped into SandboxInfo.CreatedBy
+//     and persisted in the store at creation time
 //
 // GET /v1/sandboxes (ListSandboxes):
-//   - Users can only list sandboxes they created
+//   - Users can only list sandboxes they created (CreatedBy == serviceAccountName)
+//   - For backward compatibility, sandboxes without a CreatedBy field are filtered
+//     by namespace only (entries created before this field was introduced)
 //
 // GET /v1/sandboxes/{sandboxId} (GetSandbox):
-//   - Users can only access sandboxes they created
-//   - Access is checked via checkSandboxAccess() function
+//   - Users can only access sandboxes they created (CreatedBy == serviceAccountName)
+//   - Access is first checked by namespace, then by CreatedBy
+//   - Returns 404 (not 403) for unauthorized or missing sessions to prevent
+//     session ID enumeration attacks
 //
 // DELETE /v1/sandboxes/{sandboxId} (DeleteSandbox):
 //   - Users can only delete sandboxes they created
-//   - Access is checked via checkSandboxAccess() function
+//   - Access is enforced by Kubernetes RBAC via the user's own dynamicClient;
+//     the Kubernetes API itself rejects cross-account deletes
 //
 // CONNECT /v1/sandboxes/{sandboxId} (Tunnel):
 //   - Users can only establish tunnels to sandboxes they created

pkg/workloadmanager/handlers.go

@@ -125,6 +125,7 @@ func (s *Server) handleSandboxCreate(c *gin.Context, kind string) {
 	namespace := sandbox.Namespace
 
 	dynamicClient := s.k8sClient.dynamicClient
+	var createdBy string
 	if s.config.EnableAuth {
 		userDynamicClient, errExtractClient := s.extractUserK8sClient(c)
 		if errExtractClient != nil {
@@ -133,6 +134,7 @@ func (s *Server) handleSandboxCreate(c *gin.Context, kind string) {
 			return
 		}
 		dynamicClient = userDynamicClient
+		_, _, _, createdBy = extractUserInfo(c)
 	}
 
 	// CRITICAL: Register watcher BEFORE creating sandbox
@@ -141,7 +143,7 @@ func (s *Server) handleSandboxCreate(c *gin.Context, kind string) {
 	// Ensure cleanup is called when function returns to prevent memory leak
 	defer s.sandboxController.UnWatchSandbox(namespace, sandboxName)
 
-	response, err := s.createSandbox(c.Request.Context(), dynamicClient, sandbox, sandboxClaim, sandboxEntry, resultChan)
+	response, err := s.createSandbox(c.Request.Context(), dynamicClient, sandbox, sandboxClaim, sandboxEntry, resultChan, createdBy)
 	if err != nil {
 		// Client disconnected — abort with 499 so logs/metrics reflect the cancellation.
 		if errors.Is(err, context.Canceled) {
@@ -196,8 +198,8 @@ func (s *Server) createK8sResources(ctx context.Context, dynamicClient dynamic.I
 }
 
 // createSandbox performs sandbox creation and returns the response payload or an error with an HTTP status code.
-func (s *Server) createSandbox(ctx context.Context, dynamicClient dynamic.Interface, sandbox *sandboxv1alpha1.Sandbox, sandboxClaim *extensionsv1alpha1.SandboxClaim, sandboxEntry *sandboxEntry, resultChan <-chan SandboxStatusUpdate) (*types.CreateSandboxResponse, error) {
-	placeholder := buildSandboxPlaceHolder(sandbox, sandboxEntry)
+func (s *Server) createSandbox(ctx context.Context, dynamicClient dynamic.Interface, sandbox *sandboxv1alpha1.Sandbox, sandboxClaim *extensionsv1alpha1.SandboxClaim, sandboxEntry *sandboxEntry, resultChan <-chan SandboxStatusUpdate, createdBy string) (*types.CreateSandboxResponse, error) {
+	placeholder := buildSandboxPlaceHolder(sandbox, sandboxEntry, createdBy)
 	if err := s.storeClient.StoreSandbox(ctx, placeholder); err != nil {
 		if isContextError(err) {
 			return nil, err
@@ -261,7 +263,7 @@ func (s *Server) createSandbox(ctx context.Context, dynamicClient dynamic.Interf
 		return nil, api.NewInternalError(fmt.Errorf("failed to verify sandbox %s/%s entrypoints: %w", sandbox.Namespace, sandbox.Name, err))
 	}
 
-	storeCacheInfo := buildSandboxInfo(createdSandbox, podIP, sandboxEntry)
+	storeCacheInfo := buildSandboxInfo(createdSandbox, podIP, sandboxEntry, createdBy)
 
 	response := &types.CreateSandboxResponse{
 		Kind:        storeCacheInfo.Kind,
@@ -329,12 +331,19 @@ func (s *Server) handleGetSandbox(c *gin.Context, kind string) {
 	}
 
 	if s.config.EnableAuth {
-		_, userNamespace, _, _ := extractUserInfo(c)
+		_, userNamespace, _, serviceAccountName := extractUserInfo(c)
 		if sandboxInfo.SandboxNamespace != userNamespace {
 			klog.Warningf("unauthorized GET attempt to session %s by user in namespace %s", sessionID, userNamespace)
 			respondError(c, http.StatusNotFound, fmt.Sprintf("Session ID %s not found", sessionID))
 			return
 		}
+		// Enforce per-service-account ownership: only the creating SA can retrieve its session.
+		// Returns 404 (not 403) to prevent session ID enumeration.
+		if sandboxInfo.CreatedBy != "" && sandboxInfo.CreatedBy != serviceAccountName {
+			klog.Warningf("unauthorized GET attempt to session %s by service account %s (owner: %s)", sessionID, serviceAccountName, sandboxInfo.CreatedBy)
+			respondError(c, http.StatusNotFound, fmt.Sprintf("Session ID %s not found", sessionID))
+			return
+		}
 	}
 
 	respondJSON(c, http.StatusOK, sandboxInfo)
@@ -354,13 +363,20 @@ func (s *Server) handleListSandboxes(c *gin.Context, kind string) {
 		return
 	}
 
-	_, userNamespace, _, _ := extractUserInfo(c)
+	_, userNamespace, _, serviceAccountName := extractUserInfo(c)
 	// Filter in-place to avoid a new allocation.
+	// When CreatedBy is set, enforce per-SA ownership (documented intent in auth.go).
+	// When CreatedBy is empty (legacy entries created before this field was added),
+	// fall back to namespace-scoped filtering so existing sessions remain accessible.
 	filtered := sandboxes[:0]
 	for _, sb := range sandboxes {
-		if sb.SandboxNamespace == userNamespace {
-			filtered = append(filtered, sb)
+		if sb.SandboxNamespace != userNamespace {
+			continue
+		}
+		if sb.CreatedBy != "" && sb.CreatedBy != serviceAccountName {
+			continue
 		}
+		filtered = append(filtered, sb)
 	}
 	respondJSON(c, http.StatusOK, filtered)
 }

pkg/workloadmanager/handlers_test.go

@@ -245,7 +245,7 @@ func TestServerCreateSandbox(t *testing.T) {
 				claim = &extensionsv1alpha1.SandboxClaim{ObjectMeta: metav1.ObjectMeta{Name: sb.Name, Namespace: sb.Namespace}}
 			}
 
-			resp, err := server.createSandbox(context.Background(), nil, sb, claim, makeEntry(), resultChan)
+			resp, err := server.createSandbox(context.Background(), nil, sb, claim, makeEntry(), resultChan, "")
 
 			require.Equal(t, tt.expectCreateCalls, createCalls, "createSandbox call count")
 			require.Equal(t, tt.expectClaimCalls, claimCalls, "createSandboxClaim call count")
@@ -445,7 +445,7 @@ func TestHandleSandboxCreate(t *testing.T) {
 			})
 
 			createCalls := 0
-			patches.ApplyPrivateMethod(reflect.TypeOf(fakeServer), "createSandbox", func(_ *Server, _ context.Context, _ dynamic.Interface, _ *sandboxv1alpha1.Sandbox, _ *extensionsv1alpha1.SandboxClaim, _ *sandboxEntry, _ <-chan SandboxStatusUpdate) (*types.CreateSandboxResponse, error) {
+			patches.ApplyPrivateMethod(reflect.TypeOf(fakeServer), "createSandbox", func(_ *Server, _ context.Context, _ dynamic.Interface, _ *sandboxv1alpha1.Sandbox, _ *extensionsv1alpha1.SandboxClaim, _ *sandboxEntry, _ <-chan SandboxStatusUpdate, _ string) (*types.CreateSandboxResponse, error) {
 				createCalls++
 				if tc.createErr != nil {
 					return nil, tc.createErr
@@ -542,6 +542,20 @@ func TestHandleGetSandbox(t *testing.T) {
 			expectStatus:  http.StatusNotFound,
 			expectMessage: "Session ID sess-auth-fail not found",
 		},
+		{
+			name:          "forbidden with auth matching namespace but different service account",
+			sessionID:     "sess-wrong-sa",
+			enableAuth:    true,
+			userNamespace: "user-ns",
+			storeResult: &types.SandboxInfo{
+				SessionID:        "sess-wrong-sa",
+				SandboxNamespace: "user-ns",
+				Kind:             types.AgentRuntimeKind,
+				CreatedBy:        "other-sa",
+			},
+			expectStatus:  http.StatusNotFound,
+			expectMessage: "Session ID sess-wrong-sa not found",
+		},
 		{
 			name:      "wrong kind",
 			sessionID: "sess-wrong-kind",
@@ -652,6 +666,22 @@ func TestHandleListSandboxes(t *testing.T) {
 			expectStatus: http.StatusOK,
 			expectCount:  1,
 		},
+		{
+			name:          "success with auth filters by service account",
+			kind:          types.AgentRuntimeKind,
+			enableAuth:    true,
+			userNamespace: "user-ns",
+			storeResult: []*types.SandboxInfo{
+				// Same namespace, created by the requesting SA — should be included
+				{SessionID: "1", SandboxNamespace: "user-ns", Kind: types.AgentRuntimeKind, CreatedBy: "mock-sa"},
+				// Same namespace, different SA — should be excluded
+				{SessionID: "2", SandboxNamespace: "user-ns", Kind: types.AgentRuntimeKind, CreatedBy: "other-sa"},
+				// Same namespace, no CreatedBy (legacy) — should be included (backward compat)
+				{SessionID: "3", SandboxNamespace: "user-ns", Kind: types.AgentRuntimeKind, CreatedBy: ""},
+			},
+			expectStatus: http.StatusOK,
+			expectCount:  2,
+		},
 	}
 
 	for _, tt := range tests {

pkg/workloadmanager/sandbox_helper.go

@@ -47,7 +47,7 @@ var sandboxEntrypointDial = func(ctx context.Context, endpoint string, timeout t
 	return conn.Close()
 }
 
-func buildSandboxPlaceHolder(sandboxCR *sandboxv1alpha1.Sandbox, entry *sandboxEntry) *types.SandboxInfo {
+func buildSandboxPlaceHolder(sandboxCR *sandboxv1alpha1.Sandbox, entry *sandboxEntry, createdBy string) *types.SandboxInfo {
 	var expiresAt time.Time
 	if sandboxCR.Spec.Lifecycle.ShutdownTime != nil {
 		expiresAt = sandboxCR.Spec.Lifecycle.ShutdownTime.Time
@@ -66,10 +66,11 @@ func buildSandboxPlaceHolder(sandboxCR *sandboxv1alpha1.Sandbox, entry *sandboxE
 		ExpiresAt:        expiresAt,
 		Status:           "creating",
 		IdleTimeout:      metav1.Duration{Duration: idleTimeout},
+		CreatedBy:        createdBy,
 	}
 }
 
-func buildSandboxInfo(sandbox *sandboxv1alpha1.Sandbox, podIP string, entry *sandboxEntry) *types.SandboxInfo {
+func buildSandboxInfo(sandbox *sandboxv1alpha1.Sandbox, podIP string, entry *sandboxEntry, createdBy string) *types.SandboxInfo {
 	createdAt := sandbox.GetCreationTimestamp().Time
 	expiresAt := createdAt.Add(DefaultSandboxTTL)
 	if sandbox.Spec.Lifecycle.ShutdownTime != nil {
@@ -98,6 +99,7 @@ func buildSandboxInfo(sandbox *sandboxv1alpha1.Sandbox, podIP string, entry *san
 		ExpiresAt:        expiresAt,
 		Status:           getSandboxStatus(sandbox),
 		IdleTimeout:      metav1.Duration{Duration: idleTimeout},
+		CreatedBy:        createdBy,
 	}
 }
 

pkg/workloadmanager/sandbox_helper_test.go

@@ -151,7 +151,7 @@ func TestBuildSandboxPlaceHolder_TableDriven(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			sandbox := tt.setupSandbox()
-			result := buildSandboxPlaceHolder(sandbox, tt.entry)
+			result := buildSandboxPlaceHolder(sandbox, tt.entry, "")
 			tt.validate(t, result)
 		})
 	}
@@ -322,7 +322,7 @@ func TestBuildSandboxInfo_TableDriven(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			sandbox := tt.setupSandbox()
-			result := buildSandboxInfo(sandbox, tt.podIP, tt.entry)
+			result := buildSandboxInfo(sandbox, tt.podIP, tt.entry, "")
 			tt.validateResult(t, result)
 		})
 	}