Grant pods/attach only for stdio transport (#6841)

* Grant pods/attach only for stdio transport

The proxy-runner Role created for every MCPServer carried
pods/attach (create, get) regardless of transport. Only stdio
uses it: the runner reaches a stdio MCP server by attaching to
its pod through Client.AttachToWorkload, which is called only
from pkg/transport/stdio.go. A streamable-http or sse server
talks to its pod over the network and never attaches.

Security auditors flag pods/attach as equivalent to pod exec,
so clusters with strict policies saw a finding on every
MCPServer deployment for a permission that could not be used.

Split the rule out of defaultRBACRules into stdioRBACRules and
choose between them in rbacRulesForTransport. An absent
transport is treated as stdio, matching the CRD default, so an
object stored before that default existed keeps working.

Already-deployed servers are corrected without operator action:
EnsureRBACResources replaces Rules wholesale, so the next
reconcile rewrites the Role.

Closes #6230

Signed-off-by: hellouz818 <hellouz818@gmail.com>

* Split proxy-runner Role fixtures by transport

The chainsaw operator E2E suites asserted one shared
proxyrunner-role.yaml for the stdio, sse, streamable-http and
stdio-streamable-http scenarios. That stopped holding once
pods/attach became transport-dependent: chainsaw matches the
rules array positionally and on length, so the stdio scenarios
failed on the rule having moved to the end, and the HTTP ones
would fail on it being absent.

Replace the shared file with proxyrunner-role-stdio.yaml (six
rules, pods/attach last) and proxyrunner-role-http.yaml (the
five shared rules), and point each scenario at the one matching
the transport in its mcpserver.yaml. Both files still spell out
the complete expected Role, so an over-granted rule fails the
assertion rather than slipping through.

Only the multi-tenancy run reached CI: chainsaw runs with
FailFast and both suites share one shell step, so the
single-tenancy scenarios never executed.

Refs #6230

Signed-off-by: hellouz818 <hellouz818@gmail.com>

---------

Signed-off-by: hellouz818 <hellouz818@gmail.com>

Author: hellouz818

cmd/thv-operator/controllers/mcpserver_controller.go

@@ -12,6 +12,7 @@ import (
 	"fmt"
 	"maps"
 	"os"
+	"slices"
 	"strconv"
 	"strings"
 	"time"
@@ -62,9 +63,10 @@ type MCPServerReconciler struct {
 	ImagePullSecretsDefaults imagepullsecrets.Defaults
 }
 
-// defaultRBACRules are the default RBAC rules that the
-// ToolHive ProxyRunner and/or MCP server needs to have in order to run.
+// defaultRBACRules are the RBAC rules that the ToolHive ProxyRunner and/or MCP
+// server needs to have in order to run, whatever transport is configured.
 // These permissions are needed for MCPServer which deploys and manages MCP server containers.
+// Transport-specific rules are added on top; see rbacRulesForTransport.
 var defaultRBACRules = []rbacv1.PolicyRule{
 	{
 		APIGroups: []string{"apps"},
@@ -88,16 +90,41 @@ var defaultRBACRules = []rbacv1.PolicyRule{
 	},
 	{
 		APIGroups: []string{""},
-		Resources: []string{"pods/attach"},
-		Verbs:     []string{"create", "get"},
+		Resources: []string{"configmaps"},
+		Verbs:     []string{"get", "list", "watch"},
 	},
+}
+
+// stdioRBACRules are granted on top of defaultRBACRules for stdio transport only.
+// The proxy runner talks to a stdio MCP server by attaching to the pod's stdio
+// (Client.AttachToWorkload, reached only from the stdio transport); the HTTP
+// transports speak to the pod over the network and never attach. Security
+// auditors treat pods/attach as equivalent to pod exec, so withhold it where it
+// cannot be used.
+var stdioRBACRules = []rbacv1.PolicyRule{
 	{
 		APIGroups: []string{""},
-		Resources: []string{"configmaps"},
-		Verbs:     []string{"get", "list", "watch"},
+		Resources: []string{"pods/attach"},
+		Verbs:     []string{"create", "get"},
 	},
 }
 
+// rbacRulesForTransport returns the proxy-runner Role rules for the given
+// MCPServer transport. An empty transport is treated as stdio: the CRD defaults
+// the field to stdio, so an object can only reach here with an empty value if it
+// predates that default or was built in Go, and granting the extra permission
+// keeps such a server working rather than breaking it.
+//
+// The result is always a fresh slice — appending to defaultRBACRules directly
+// would let a future rule share its backing array.
+func rbacRulesForTransport(mcpTransport string) []rbacv1.PolicyRule {
+	rules := slices.Clone(defaultRBACRules)
+	if mcpTransport == stdioTransport || mcpTransport == "" {
+		rules = append(rules, stdioRBACRules...)
+	}
+	return rules
+}
+
 // remoteProxyRBACRules defines minimal RBAC permissions for MCPRemoteProxy.
 // Remote proxies only connect to external MCP servers and do not deploy containers,
 // so they only need read access to ConfigMaps and Secrets (for OIDC/token exchange).
@@ -1105,7 +1132,7 @@ func (r *MCPServerReconciler) ensureRBACResources(ctx context.Context, mcpServer
 	if _, err := rbacClient.EnsureRBACResources(ctx, rbac.EnsureRBACResourcesParams{
 		Name:             proxyRunnerNameForRBAC,
 		Namespace:        mcpServer.Namespace,
-		Rules:            defaultRBACRules,
+		Rules:            rbacRulesForTransport(mcpServer.Spec.Transport),
 		Owner:            mcpServer,
 		ImagePullSecrets: imagePullSecrets,
 	}); err != nil {

cmd/thv-operator/controllers/mcpserver_rbac_test.go

@@ -6,6 +6,7 @@ package controllers
 import (
 	"context"
 	"fmt"
+	"slices"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -70,7 +71,25 @@ func (tc *testContext) assertRoleExists(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, tc.proxyRunnerNameForRBAC, role.Name)
 	assert.Equal(t, tc.mcpServer.Namespace, role.Namespace)
-	assert.Equal(t, defaultRBACRules, role.Rules)
+	assert.Equal(t, expectedRoleRules(tc.mcpServer.Spec.Transport), role.Rules)
+}
+
+// podAttachRuleFixture restates the pods/attach rule rather than reusing
+// stdioRBACRules, so a change to the production rule's verbs or resources shows
+// up as a test failure instead of being mirrored silently.
+var podAttachRuleFixture = rbacv1.PolicyRule{
+	APIGroups: []string{""},
+	Resources: []string{"pods/attach"},
+	Verbs:     []string{"create", "get"},
+}
+
+// expectedRoleRules is what a proxy-runner Role should carry for a transport:
+// the shared rules, plus pods/attach only where the proxy actually attaches.
+func expectedRoleRules(mcpTransport string) []rbacv1.PolicyRule {
+	if mcpTransport == "stdio" || mcpTransport == "" {
+		return append(slices.Clone(defaultRBACRules), podAttachRuleFixture)
+	}
+	return defaultRBACRules
 }
 
 func (tc *testContext) assertRoleBindingExists(t *testing.T) {
@@ -418,6 +437,91 @@ func TestEnsureRBACResources_ImagePullSecrets(t *testing.T) {
 	assert.Equal(t, expectedSecrets, mcpServerSA.ImagePullSecrets)
 }
 
+// TestEnsureRBACResources_Role_PodsAttachScopedToStdio pins the permission to
+// the one transport that uses it. The proxy runner attaches to the MCP pod's
+// stdio only for the stdio transport; granting pods/attach for the HTTP
+// transports hands every such deployment a permission equivalent to pod exec
+// that it can never exercise.
+func TestEnsureRBACResources_Role_PodsAttachScopedToStdio(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name           string
+		transport      string
+		wantPodsAttach bool
+	}{
+		{
+			name:           "stdio attaches to the pod",
+			transport:      "stdio",
+			wantPodsAttach: true,
+		},
+		{
+			name:           "streamable-http never calls AttachToWorkload",
+			transport:      "streamable-http",
+			wantPodsAttach: false,
+		},
+		{
+			name:           "sse never calls AttachToWorkload",
+			transport:      "sse",
+			wantPodsAttach: false,
+		},
+		{
+			name:           "empty transport is treated as the stdio CRD default",
+			transport:      "",
+			wantPodsAttach: true,
+		},
+	}
+
+	for i, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			tc := setupTest(t, fmt.Sprintf("test-server-attach-%d", i), "default")
+			tc.mcpServer.Spec.Transport = tt.transport
+
+			require.NoError(t, tc.ensureRBACResources())
+
+			role := &rbacv1.Role{}
+			require.NoError(t, tc.client.Get(context.TODO(), types.NamespacedName{
+				Name:      tc.proxyRunnerNameForRBAC,
+				Namespace: tc.mcpServer.Namespace,
+			}, role))
+
+			if tt.wantPodsAttach {
+				assert.Contains(t, role.Rules, podAttachRuleFixture,
+					"transport %q attaches to the pod and needs pods/attach", tt.transport)
+			} else {
+				assert.NotContains(t, role.Rules, podAttachRuleFixture,
+					"transport %q never attaches and must not be granted pods/attach", tt.transport)
+			}
+
+			// Whatever the transport, the shared rules must survive intact.
+			for _, want := range defaultRBACRules {
+				assert.Contains(t, role.Rules, want)
+			}
+		})
+	}
+}
+
+// TestRBACRulesForTransport_ReturnsFreshSlice guards the backing array of the
+// package-level defaultRBACRules: appending the stdio rule onto it directly
+// would let one call mutate what every later call sees.
+func TestRBACRulesForTransport_ReturnsFreshSlice(t *testing.T) {
+	t.Parallel()
+
+	before := slices.Clone(defaultRBACRules)
+
+	stdio := rbacRulesForTransport("stdio")
+	require.Contains(t, stdio, podAttachRuleFixture)
+
+	assert.Equal(t, before, defaultRBACRules, "defaultRBACRules must not be mutated")
+	assert.NotContains(t, rbacRulesForTransport("streamable-http"), podAttachRuleFixture)
+
+	// Mutating a returned slice must not reach a later caller.
+	stdio[0] = rbacv1.PolicyRule{APIGroups: []string{"mutated"}}
+	assert.NotEqual(t, stdio[0], rbacRulesForTransport("stdio")[0])
+}
+
 func createTestMCPServer(name, namespace string) *mcpv1beta1.MCPServer {
 	// Builder defaults (image "test-image:latest", transport "stdio",
 	// proxyPort 8080) match this fixture exactly.

test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/common/proxyrunner-role-http.yaml

@@ -0,0 +1,57 @@
+# Proxy-runner Role for the HTTP transports (sse, streamable-http).
+# The proxy reaches such an MCP server over the network, so it is not granted
+# pods/attach. Keep in step with defaultRBACRules in
+# cmd/thv-operator/controllers/mcpserver_controller.go; chainsaw matches the
+# rules positionally, so order matters.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: yardstick-proxy-runner
+  namespace: test-namespace
+rules:
+- apiGroups:
+  - apps
+  resources:
+  - statefulsets
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete
+- apiGroups:
+  - ""
+  resources:
+  - services
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - ""
+  resources:
+  - pods/log
+  verbs:
+  - get
+- apiGroups:
+  - ""
+  resources:
+  - configmaps
+  verbs:
+  - get
+  - list
+  - watch

…t-scenarios/common/proxyrunner-role.yaml …arios/common/proxyrunner-role-stdio.yamltest/e2e/chainsaw/operator/multi-tenancy/test-scenarios/common/proxyrunner-role.yaml renamed to test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/common/proxyrunner-role-stdio.yaml test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/common/proxyrunner-role.yaml renamed to test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/common/proxyrunner-role-stdio.yaml

@@ -1,3 +1,8 @@
+# Proxy-runner Role for the stdio transport.
+# The proxy attaches to the MCP pod's stdio, so it is granted pods/attach on
+# top of the shared rules. Keep in step with defaultRBACRules and
+# stdioRBACRules in cmd/thv-operator/controllers/mcpserver_controller.go;
+# chainsaw matches the rules positionally, so order matters.
 apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
@@ -45,15 +50,15 @@ rules:
 - apiGroups:
   - ""
   resources:
-  - pods/attach
+  - configmaps
   verbs:
-  - create
   - get
+  - list
+  - watch
 - apiGroups:
   - ""
   resources:
-  - configmaps
+  - pods/attach
   verbs:
+  - create
   - get
-  - list
-  - watch

test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/sse/chainsaw-test.yaml

@@ -38,7 +38,7 @@ spec:
     - assert:
         file: assert-mcpserver-proxy-runner-svc.yaml
     - assert:
-        file: ../common/proxyrunner-role.yaml
+        file: ../common/proxyrunner-role-http.yaml
     - assert:
         file: ../common/proxyrunner-rolebinding.yaml
     - assert:

test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/stdio-streamable-http/chainsaw-test.yaml

@@ -34,7 +34,7 @@ spec:
     - assert:
         file: assert-mcpserver-proxy-runner-svc.yaml
     - assert:
-        file: ../common/proxyrunner-role.yaml
+        file: ../common/proxyrunner-role-stdio.yaml
     - assert:
         file: ../common/proxyrunner-rolebinding.yaml
     - assert:

test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/stdio/chainsaw-test.yaml

@@ -34,7 +34,7 @@ spec:
     - assert:
         file: assert-mcpserver-proxy-runner-svc.yaml
     - assert:
-        file: ../common/proxyrunner-role.yaml
+        file: ../common/proxyrunner-role-stdio.yaml
     - assert:
         file: ../common/proxyrunner-rolebinding.yaml
     - assert:

test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/streamable-http/chainsaw-test.yaml

@@ -38,7 +38,7 @@ spec:
     - assert:
         file: assert-mcpserver-proxy-runner-running.yaml
     - assert:
-        file: ../common/proxyrunner-role.yaml
+        file: ../common/proxyrunner-role-http.yaml
     - assert:
         file: ../common/proxyrunner-rolebinding.yaml
     - assert:

test/e2e/chainsaw/operator/single-tenancy/test-scenarios/common/proxyrunner-role-http.yaml

@@ -0,0 +1,57 @@
+# Proxy-runner Role for the HTTP transports (sse, streamable-http).
+# The proxy reaches such an MCP server over the network, so it is not granted
+# pods/attach. Keep in step with defaultRBACRules in
+# cmd/thv-operator/controllers/mcpserver_controller.go; chainsaw matches the
+# rules positionally, so order matters.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: (join('-', [$testPrefix, 'proxy-runner']))
+  namespace: toolhive-system
+rules:
+- apiGroups:
+  - apps
+  resources:
+  - statefulsets
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete
+- apiGroups:
+  - ""
+  resources:
+  - services
+  verbs:
+  - get
+  - list
+  - watch
+  - create
+  - update
+  - patch
+  - delete
+- apiGroups:
+  - ""
+  resources:
+  - pods
+  verbs:
+  - get
+  - list
+  - watch
+- apiGroups:
+  - ""
+  resources:
+  - pods/log
+  verbs:
+  - get
+- apiGroups:
+  - ""
+  resources:
+  - configmaps
+  verbs:
+  - get
+  - list
+  - watch