From c1bdae4ef28af2ef568bc8cf9c959ad38015648a Mon Sep 17 00:00:00 2001 From: Greg Haskins Date: Mon, 9 Mar 2026 08:00:23 -0400 Subject: [PATCH] Introduce tool:list verb for ingress and tool:read for egress filtering Previously, listing tools used the tool:call verb for both ingress authorization and egress response filtering, making it impossible to distinguish between a user discovering available tools versus invoking them with potential side effects. This change introduces: - A new ingress tool:list verb for tool listing requests, allowing policy authors to independently control access to tool discovery - Egress filtering switches from tool:call to tool:read for filtering listed tools in responses, reflecting that listing is a read-only discovery operation The distinction between read-only discovery (tool:list/tool:read) and potentially mutable actions (tool:call) is now clear in both policies and audit logs. NOTE: This is a breaking change for users with existing Cedar policies that relied on tool:call to gate tool listing. Signed-off-by: Greg Haskins --- pkg/authz/authorizers/cedar/core.go | 40 +++++++++++++++++++ pkg/authz/integration_test.go | 11 +++-- pkg/authz/middleware.go | 16 +++++++- pkg/authz/middleware_test.go | 21 +++++++--- pkg/authz/response_filter.go | 4 +- pkg/authz/response_filter_test.go | 4 +- pkg/vmcp/auth/factory/authz_not_wired_test.go | 1 + 7 files changed, 82 insertions(+), 15 deletions(-) diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index bf415830ac..5b85278bde 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -431,6 +431,42 @@ func (a *Authorizer) authorizeToolCall( return a.IsAuthorized(principal, action, resource, contextMap, entities) } +// authorizeToolRead authorizes a tool read operation. +// This method is used when filtering a tools list response to determine +// which tools a client is permitted to see. +func (a *Authorizer) authorizeToolRead( + clientID, toolName string, + claimsMap map[string]interface{}, + attrsMap map[string]interface{}, +) (bool, error) { + // Extract principal from client ID + principal := fmt.Sprintf("Client::%s", clientID) + + // Action is to read a tool + action := "Action::read_tool" + + // Resource is the tool being read + resource := fmt.Sprintf("Tool::%s", toolName) + + // Create attributes for the entities + attributes := mergeContexts(map[string]interface{}{ + "name": toolName, + "operation": "read", + "feature": "tool", + }, attrsMap) + + // Create Cedar entities + entities, err := a.entityFactory.CreateEntitiesForRequest(principal, action, resource, claimsMap, attributes) + if err != nil { + return false, fmt.Errorf("failed to create Cedar entities: %w", err) + } + + contextMap := mergeContexts(claimsMap, attrsMap) + + // Check authorization with entities + return a.IsAuthorized(principal, action, resource, contextMap, entities) +} + // authorizePromptGet authorizes a prompt get operation. // This method is used when a client tries to get a specific prompt. // It checks if the client is authorized to access the prompt with the given context. @@ -608,6 +644,10 @@ func (a *Authorizer) AuthorizeWithJWTClaims( // Use the authorizeToolCall function for tool call operations return a.authorizeToolCall(ctx, clientID, resourceID, processedClaims, processedArgs) + case feature == authorizers.MCPFeatureTool && operation == authorizers.MCPOperationRead: + // Use the authorizeToolRead function for tool read (list filtering) operations + return a.authorizeToolRead(clientID, resourceID, processedClaims, processedArgs) + case feature == authorizers.MCPFeaturePrompt && operation == authorizers.MCPOperationGet: // Use the authorizePromptGet function for prompt get operations return a.authorizePromptGet(clientID, resourceID, processedClaims, processedArgs) diff --git a/pkg/authz/integration_test.go b/pkg/authz/integration_test.go index 787c3719a8..17b5499326 100644 --- a/pkg/authz/integration_test.go +++ b/pkg/authz/integration_test.go @@ -29,12 +29,17 @@ func TestIntegrationListFiltering(t *testing.T) { // Create a realistic Cedar authorizer with role-based policies authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ Policies: []string{ + // All principals can perform list operations (results are filtered per role) + `permit(principal, action == Action::"list_tools", resource);`, + `permit(principal, action == Action::"list_prompts", resource);`, + `permit(principal, action == Action::"list_resources", resource);`, + // Basic users can only access weather and news tools - `permit(principal, action == Action::"call_tool", resource == Tool::"weather") when { principal.claim_role == "user" };`, - `permit(principal, action == Action::"call_tool", resource == Tool::"news") when { principal.claim_role == "user" };`, + `permit(principal, action == Action::"read_tool", resource == Tool::"weather") when { principal.claim_role == "user" };`, + `permit(principal, action == Action::"read_tool", resource == Tool::"news") when { principal.claim_role == "user" };`, // Admin users can access all tools - `permit(principal, action == Action::"call_tool", resource) when { principal.claim_role == "admin" };`, + `permit(principal, action == Action::"read_tool", resource) when { principal.claim_role == "admin" };`, // Basic users can only access public prompts `permit(principal, action == Action::"get_prompt", resource == Prompt::"greeting") when { principal.claim_role == "user" };`, diff --git a/pkg/authz/middleware.go b/pkg/authz/middleware.go index e1b11267e6..9b8535f5b7 100644 --- a/pkg/authz/middleware.go +++ b/pkg/authz/middleware.go @@ -49,8 +49,8 @@ var MCPMethodToFeatureOperation = map[string]struct { "resources/unsubscribe": {Feature: authorizers.MCPFeatureResource, Operation: authorizers.MCPOperationRead}, // Discovery and capability methods - always allowed - "features/list": {Feature: "", Operation: authorizers.MCPOperationList}, // Capability discovery - "roots/list": {Feature: "", Operation: ""}, // Root directory discovery + "features/list": {Feature: "", Operation: ""}, // Capability discovery - always allowed + "roots/list": {Feature: "", Operation: ""}, // Root directory discovery // Logging and client preferences - always allowed "logging/setLevel": {Feature: "", Operation: ""}, // Client preference for server logging @@ -209,6 +209,18 @@ func Middleware(a authorizers.Authorizer, next http.Handler) http.Handler { // Handle list operations differently - allow them through but filter the response if featureOp.Operation == authorizers.MCPOperationList { + authorized, err := a.AuthorizeWithJWTClaims( + r.Context(), + featureOp.Feature, + authorizers.MCPOperationList, + "metadata", + nil, // No arguments for the authorization check + ) + // Handle unauthorized requests + if err != nil || !authorized { + handleUnauthorized(w, parsedRequest.ID, err) + return + } // Create a response filtering writer to intercept and filter the response filteringWriter := NewResponseFilteringWriter(w, a, r, parsedRequest.Method) diff --git a/pkg/authz/middleware_test.go b/pkg/authz/middleware_test.go index d46dab4224..48a45c2396 100644 --- a/pkg/authz/middleware_test.go +++ b/pkg/authz/middleware_test.go @@ -35,6 +35,9 @@ func TestMiddleware(t *testing.T) { `permit(principal, action == Action::"call_tool", resource == Tool::"weather");`, `permit(principal, action == Action::"get_prompt", resource == Prompt::"greeting");`, `permit(principal, action == Action::"read_resource", resource == Resource::"data");`, + `permit(principal, action == Action::"list_tools", resource);`, + `permit(principal, action == Action::"list_prompts", resource);`, + `permit(principal, action == Action::"list_resources", resource);`, }, EntitiesJSON: `[]`, }) @@ -695,7 +698,8 @@ func TestMiddlewareToolsListTestkit(t *testing.T) { testkit.WithJSONClientType(), }, policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"list_tools", resource);`, }, expected: []any{ map[string]any{"name": "foo", "description": "A test tool"}, @@ -711,7 +715,8 @@ func TestMiddlewareToolsListTestkit(t *testing.T) { testkit.WithJSONClientType(), }, policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"list_tools", resource);`, }, expected: []any{ map[string]any{"name": "foo", "description": "A test tool"}, @@ -725,7 +730,8 @@ func TestMiddlewareToolsListTestkit(t *testing.T) { testkit.WithJSONClientType(), }, policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"list_tools", resource);`, }, expected: []any{}, }, @@ -739,7 +745,8 @@ func TestMiddlewareToolsListTestkit(t *testing.T) { testkit.WithSSEClientType(), }, policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"list_tools", resource);`, }, expected: []any{ map[string]any{"name": "foo", "description": "A test tool"}, @@ -755,7 +762,8 @@ func TestMiddlewareToolsListTestkit(t *testing.T) { testkit.WithSSEClientType(), }, policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"list_tools", resource);`, }, expected: []any{ map[string]any{"name": "foo", "description": "A test tool"}, @@ -769,7 +777,8 @@ func TestMiddlewareToolsListTestkit(t *testing.T) { testkit.WithSSEClientType(), }, policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"foo");`, + `permit(principal, action == Action::"list_tools", resource);`, }, expected: []any{}, }, diff --git a/pkg/authz/response_filter.go b/pkg/authz/response_filter.go index 14a0d69c82..07c54e6f57 100644 --- a/pkg/authz/response_filter.go +++ b/pkg/authz/response_filter.go @@ -279,11 +279,11 @@ func (rfw *ResponseFilteringWriter) filterToolsResponse(response *jsonrpc2.Respo // This is basically defensive programming, but for clients. filteredTools := []mcp.Tool{} for _, tool := range listResult.Tools { - // Check if the user is authorized to call this tool + // Check if the user is authorized to read this tool authorized, err := rfw.authorizer.AuthorizeWithJWTClaims( rfw.request.Context(), authorizers.MCPFeatureTool, - authorizers.MCPOperationCall, + authorizers.MCPOperationRead, tool.Name, nil, // No arguments for the authorization check ) diff --git a/pkg/authz/response_filter_test.go b/pkg/authz/response_filter_test.go index 7a258239ce..108b48c705 100644 --- a/pkg/authz/response_filter_test.go +++ b/pkg/authz/response_filter_test.go @@ -32,7 +32,7 @@ func TestResponseFilteringWriter(t *testing.T) { // Create a Cedar authorizer with specific tool permissions authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ Policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"weather");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"weather");`, `permit(principal, action == Action::"get_prompt", resource == Prompt::"greeting");`, `permit(principal, action == Action::"read_resource", resource == Resource::"data");`, }, @@ -329,7 +329,7 @@ func TestResponseFilteringWriter_ContentLengthMismatch(t *testing.T) { // The backend will return 3 tools, so filtering will shrink the response. authorizer, err := cedar.NewCedarAuthorizer(cedar.ConfigOptions{ Policies: []string{ - `permit(principal, action == Action::"call_tool", resource == Tool::"weather");`, + `permit(principal, action == Action::"read_tool", resource == Tool::"weather");`, }, EntitiesJSON: `[]`, }) diff --git a/pkg/vmcp/auth/factory/authz_not_wired_test.go b/pkg/vmcp/auth/factory/authz_not_wired_test.go index 32bd7e6c78..d703d8c3db 100644 --- a/pkg/vmcp/auth/factory/authz_not_wired_test.go +++ b/pkg/vmcp/auth/factory/authz_not_wired_test.go @@ -152,6 +152,7 @@ func TestNewIncomingAuthMiddleware_AuthzApproveAndBlock(t *testing.T) { Type: "cedar", Policies: []string{ `permit(principal, action == Action::"list_tools", resource);`, + `permit(principal, action == Action::"list_prompts", resource);`, `forbid(principal, action == Action::"call_tool", resource);`, }, },