From b0ad3bfd83a0a846a7ce71fe3023caf28dd13db3 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 15 Apr 2026 22:25:20 +0100 Subject: [PATCH] Add name attribute alias on Resource entities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The enterprise Cedar schema (platform-authz branch, 02-cedar-compilation.md § entity types) declares "name" as a required String attribute on all three entity kinds — Tool, Prompt, and Resource. Tool and Prompt entities already had "name" populated by CreateResourceEntity, but authorizeResourceRead built its own attributes map with only "uri", so Resource entities lacked the attribute and would fail schema validation when the enterprise controller compiles policies. Add "name": resourceURI alongside the existing "uri": resourceURI in authorizeResourceRead so Resource entities satisfy the schema. In CreateResourceEntity, default "name" to resourceID only when the caller has not already provided one — authorizeResourceRead sets name to the original unsanitized URI before passing the sanitized form as resourceID, and the caller's value must survive. Preserving the caller-provided name also exposes the original URI to policies, so authors can match on the form they actually know (e.g. resource.name == "file:///...") rather than the Cedar-sanitized entity ID. E2E tested in a Kind cluster with real Entra ID tokens: permit(principal in THVGroup::"mcp-admin", action == Action::"call_tool", resource in MCP::"entra-role-test") when { resource.name == "echo" }; Entra JWT: { "roles": ["mcp-admin", "developer"] } call_tool "echo" -> 200 (resource.name matches) call_tool "nonexistent_tool" -> 403 (name mismatch) Fixes #4766 Co-Authored-By: Claude Opus 4.6 --- pkg/authz/authorizers/cedar/core.go | 1 + pkg/authz/authorizers/cedar/core_test.go | 63 ++++++++++++++++++++++ pkg/authz/authorizers/cedar/entity.go | 8 ++- pkg/authz/authorizers/cedar/entity_test.go | 63 ++++++++++++++++++++++ 4 files changed, 133 insertions(+), 2 deletions(-) diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index a42cdd6ed3..7c3504d73b 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -637,6 +637,7 @@ func (a *Authorizer) authorizeResourceRead( // Create attributes for the entities attributes := mergeContexts(map[string]interface{}{ + "name": resourceURI, "uri": resourceURI, "operation": "read", "feature": "resource", diff --git a/pkg/authz/authorizers/cedar/core_test.go b/pkg/authz/authorizers/cedar/core_test.go index e39e16cd1a..a0c7aa0a5c 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -269,6 +269,69 @@ func TestAuthorizeWithJWTClaims(t *testing.T) { arguments: nil, expectAuthorized: true, }, + { + name: "Resource entity exposes name attribute for Cedar schema", + policy: ` + permit( + principal, + action == Action::"read_resource", + resource + ) + when { + resource.name == "sensitive_data" + }; + `, + claims: jwt.MapClaims{ + "sub": "user123", + }, + feature: authorizers.MCPFeatureResource, + operation: authorizers.MCPOperationRead, + resourceID: "sensitive_data", + arguments: nil, + expectAuthorized: true, + }, + { + name: "Resource entity retains uri attribute for backward compat", + policy: ` + permit( + principal, + action == Action::"read_resource", + resource + ) + when { + resource.uri == "sensitive_data" + }; + `, + claims: jwt.MapClaims{ + "sub": "user123", + }, + feature: authorizers.MCPFeatureResource, + operation: authorizers.MCPOperationRead, + resourceID: "sensitive_data", + arguments: nil, + expectAuthorized: true, + }, + { + name: "Resource name and uri attributes carry the same value", + policy: ` + permit( + principal, + action == Action::"read_resource", + resource + ) + when { + resource.name == resource.uri + }; + `, + claims: jwt.MapClaims{ + "sub": "user123", + }, + feature: authorizers.MCPFeatureResource, + operation: authorizers.MCPOperationRead, + resourceID: "sensitive_data", + arguments: nil, + expectAuthorized: true, + }, { name: "User can get prompt", policy: ` diff --git a/pkg/authz/authorizers/cedar/entity.go b/pkg/authz/authorizers/cedar/entity.go index a495f154f3..ee7307f1ee 100644 --- a/pkg/authz/authorizers/cedar/entity.go +++ b/pkg/authz/authorizers/cedar/entity.go @@ -79,11 +79,15 @@ func (*EntityFactory) CreateResourceEntity( ) (cedar.EntityUID, cedar.Entity) { uid := cedar.NewEntityUID(cedar.EntityType(resourceType), cedar.String(resourceID)) - // Ensure name attribute is set + // Ensure name attribute is set — but don't overwrite if the caller + // already provided one (e.g. authorizeResourceRead sets name to the + // original URI before sanitization). if attributes == nil { attributes = make(map[string]interface{}) } - attributes["name"] = resourceID + if _, exists := attributes["name"]; !exists { + attributes["name"] = resourceID + } attrs := convertMapToCedarRecord(attributes) diff --git a/pkg/authz/authorizers/cedar/entity_test.go b/pkg/authz/authorizers/cedar/entity_test.go index 8569c3d125..c971627aa6 100644 --- a/pkg/authz/authorizers/cedar/entity_test.go +++ b/pkg/authz/authorizers/cedar/entity_test.go @@ -161,6 +161,69 @@ func TestCreateResourceEntity_Parents(t *testing.T) { } } +// TestCreateResourceEntity_NamePreservation is a regression test for the bug +// where CreateResourceEntity unconditionally overwrote attributes["name"] with +// resourceID (the sanitized entity UID). authorizeResourceRead sets name to the +// original, unsanitized URI before calling CreateResourceEntity — the caller's +// value must survive. When no name is provided, resourceID is used as fallback. +func TestCreateResourceEntity_NamePreservation(t *testing.T) { + t.Parallel() + + factory := NewEntityFactory() + + tests := []struct { + name string + resourceID string + attributes map[string]interface{} + wantName string + }{ + { + name: "caller_name_preserved", + resourceID: "sanitized-resource-id", + attributes: map[string]interface{}{ + "name": "original/unsanitized:uri", + }, + wantName: "original/unsanitized:uri", + }, + { + name: "uri_with_special_characters_preserved", + resourceID: "https___example_com_api_v1_resource_id=42", + attributes: map[string]interface{}{ + "name": "https://example.com/api/v1/resource?id=42", + }, + wantName: "https://example.com/api/v1/resource?id=42", + }, + { + name: "fallback_to_resourceID_when_name_absent", + resourceID: "weather", + attributes: map[string]interface{}{ + "description": "Weather tool", + }, + wantName: "weather", + }, + { + name: "fallback_to_resourceID_when_attributes_nil", + resourceID: "weather", + attributes: nil, + wantName: "weather", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + _, entity := factory.CreateResourceEntity( + "Resource", tt.resourceID, tt.attributes, + ) + + nameVal, found := entity.Attributes.Get(cedar.String("name")) + require.True(t, found, "name attribute must always be set") + assert.Equal(t, tt.wantName, string(nameVal.(cedar.String))) + }) + } +} + // TestCreateEntitiesForRequest_GroupsAsParents verifies that // CreateEntitiesForRequest sets THVGroup parent UIDs on the principal but // does NOT insert separate THVGroup entities into the entity map (fixing