From b16b4a2f951be33fe3dd5b31f915fee53eb79f0b Mon Sep 17 00:00:00 2001 From: Sasha Mitchell Date: Sat, 8 Aug 2026 03:11:00 +0700 Subject: [PATCH] fix(authz): make Cedar URI entity IDs collision-free authorizeResourceRead sanitized resource URIs into Cedar entity IDs by rewriting reserved characters to "_". The mapping is many-to-one: "file:///etc/passwd" and "file://_etc/passwd" both became "file____etc_passwd", and "mcp://srv/config:admin" and "mcp://srv/config/admin" both became "mcp___srv_config_admin". A policy grant on one entity ID therefore authorized every URI in the collision class. Entities are built programmatically via cedar.NewEntityUID, which accepts any string, so no rewriting is needed. Use the exact URI as the entity ID and drop the sanitizer. Attribute-based policies (resource.name / resource.uri) already matched on the exact URI and are unaffected. Policies that referenced sanitized IDs must be updated to name the exact URI. Added TestAuthorizeResourceReadEntityIDsCollisionFree: a grant on an exact URI authorizes that URI and denies the URI that used to collide with it. Signed-off-by: Sasha Mitchell --- pkg/authz/authorizers/cedar/core.go | 29 +++------- pkg/authz/authorizers/cedar/core_test.go | 68 ++++++++++++++++-------- 2 files changed, 52 insertions(+), 45 deletions(-) diff --git a/pkg/authz/authorizers/cedar/core.go b/pkg/authz/authorizers/cedar/core.go index d6198e71fe..d5696a6149 100644 --- a/pkg/authz/authorizers/cedar/core.go +++ b/pkg/authz/authorizers/cedar/core.go @@ -1084,10 +1084,12 @@ func (a *Authorizer) authorizeResourceRead( // Action is to read a resource action := "Action::read_resource" - // Resource is the resource being accessed - // Use the URI as the resource ID, but sanitize it for Cedar - sanitizedURI := sanitizeURIForCedar(resourceURI) - resource := fmt.Sprintf("Resource::%s", sanitizedURI) + // Resource is the resource being accessed. Use the exact URI as the + // entity ID: entities are built programmatically via cedar.NewEntityUID, + // which accepts any string, so no character rewriting is needed. A lossy + // mapping would let distinct URIs collide onto one entity ID, and a + // policy grant on one URI would then silently cover every colliding URI. + resource := fmt.Sprintf("Resource::%s", resourceURI) // Create attributes for the entities attributes := mergeContexts(map[string]interface{}{ @@ -1161,25 +1163,6 @@ func parseCedarEntityID(entityID string) (string, string, error) { return parts[0], parts[1], nil } -// sanitizeURIForCedar sanitizes a URI for use in Cedar policies. -// Cedar entity IDs have restrictions on characters, so we need to sanitize the URI. -func sanitizeURIForCedar(uri string) string { - // Replace characters that are not allowed in Cedar entity IDs - // This is a simple implementation - you may need to enhance it based on your needs - replacer := strings.NewReplacer( - ":", "_", - "/", "_", - "\\", "_", - "?", "_", - "&", "_", - "=", "_", - "#", "_", - " ", "_", - ".", "_", - ) - return replacer.Replace(uri) -} - // AuthorizeWithJWTClaims demonstrates how to use JWT claims with the Cedar authorization middleware. // This method: // 1. Extracts JWT claims from the context diff --git a/pkg/authz/authorizers/cedar/core_test.go b/pkg/authz/authorizers/cedar/core_test.go index 936e50fc90..bca48b8788 100644 --- a/pkg/authz/authorizers/cedar/core_test.go +++ b/pkg/authz/authorizers/cedar/core_test.go @@ -7,6 +7,7 @@ import ( "bytes" "context" "encoding/json" + "fmt" "log/slog" "maps" "sync" @@ -3347,38 +3348,61 @@ func TestParseCedarEntityID(t *testing.T) { } } -// TestSanitizeURIForCedar tests the sanitizeURIForCedar helper function. -func TestSanitizeURIForCedar(t *testing.T) { +// TestAuthorizeResourceReadEntityIDsCollisionFree verifies that resource URIs +// become Cedar entity IDs verbatim. Distinct URIs that a lossy sanitization +// would map onto the same entity ID must stay distinct, so a policy grant on +// one URI never authorizes a colliding URI. +func TestAuthorizeResourceReadEntityIDsCollisionFree(t *testing.T) { t.Parallel() tests := []struct { - name string - input string - want string + name string + grantedURI string + collidingURI string }{ - {name: "empty_string", input: "", want: ""}, - {name: "already_clean", input: "simple_resource", want: "simple_resource"}, - {name: "colon", input: "a:b", want: "a_b"}, - {name: "forward_slash", input: "a/b", want: "a_b"}, - {name: "backslash", input: `a\b`, want: "a_b"}, - {name: "question_mark", input: "a?b", want: "a_b"}, - {name: "ampersand", input: "a&b", want: "a_b"}, - {name: "equals", input: "a=b", want: "a_b"}, - {name: "hash", input: "a#b", want: "a_b"}, - {name: "space", input: "a b", want: "a_b"}, - {name: "dot", input: "a.b", want: "a_b"}, - { - name: "complex_uri", - input: "https://api.example.com/v1/data?key=val&other=123#fragment", - want: "https___api_example_com_v1_data_key_val_other_123_fragment", + { + name: "file_uri_colon_and_slash_run", + grantedURI: "file:///etc/passwd", + collidingURI: "file://_etc/passwd", + }, + { + name: "mcp_uri_colon_vs_slash", + grantedURI: "mcp://srv/config:admin", + collidingURI: "mcp://srv/config/admin", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - got := sanitizeURIForCedar(tt.input) - assert.Equal(t, tt.want, got) + + authorizer, err := NewCedarAuthorizer(ConfigOptions{ + Policies: []string{ + fmt.Sprintf(`permit(principal, action == Action::"read_resource", resource == Resource::"%s");`, tt.grantedURI), + }, + EntitiesJSON: `[]`, + }, "") + require.NoError(t, err, "Failed to create Cedar authorizer") + + identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{ + Subject: "test-user", + Claims: jwt.MapClaims{"sub": "user123"}, + }} + claimsCtx := auth.WithIdentity(context.Background(), identity) + + // The exact URI named in the policy is authorized. + authorized, err := authorizer.AuthorizeWithJWTClaims( + claimsCtx, authorizers.MCPFeatureResource, authorizers.MCPOperationRead, tt.grantedURI, nil) + require.NoError(t, err) + assert.True(t, authorized, "exact URI named in the policy should be authorized") + + // The colliding URI, which mapped to the same entity ID under the + // previous lossy sanitization, must not be authorized. + authorized, err = authorizer.AuthorizeWithJWTClaims( + claimsCtx, authorizers.MCPFeatureResource, authorizers.MCPOperationRead, tt.collidingURI, nil) + require.NoError(t, err) + assert.False(t, authorized, + "colliding URI %q must not be authorized by a grant on %q", tt.collidingURI, tt.grantedURI) }) } }