Grant pods/attach only for stdio transport

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

test/e2e/chainsaw/operator/multi-tenancy/test-scenarios/common/proxyrunner-role.yaml → 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