Repository navigation
fix(auth): enforce scoped authorization for Kubernetes-backed resources - #3281
supreme-gg-gg wants to merge 3 commits into
Conversation
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
EItanya
left a comment
EItanya
left a comment
There was a problem hiding this comment.
🤖 AI-generated review. I looked through the findings and they all seem worth looking at.
| if err != nil { | ||
| return false, err | ||
| } | ||
| scope, err := s.authorizer.Scope(ctx, principal, auth.VerbGet, substrateResource) |
There was a problem hiding this comment.
I don't think Scope(VerbGet, "Substrate") gives the summary a stable authorization boundary. The dedicated actor and worker endpoints still use Check(VerbGet, Resource{Type: "Substrate"}), so an external authorizer can allow one path and deny the other; Scope is otherwise documented for VerbList collection filtering.
Can we give the summary an explicit capability, for example Check(VerbGet, Resource{Type: "SystemSummary"}), which policy can grant to admins or operators? If we want section-level access instead, the Substrate section should at least use the same decision as its sibling endpoints, and a denied section should be distinguishable from an empty cluster.
| - `ModelConfig` and `ModelProviderConfig` use separate typed CRUD services and resource types. Configured-provider reads and refresh are authorized independently from ModelConfig operations. | ||
| - Prompt libraries, tool discovery, and WorkerPool summary rows use `kubeauth.CollectionMatcher` for custom collection projection. | ||
| - Flat tool discovery must filter through the same exact owning-server identities as server discovery. | ||
| - WorkerPool scope is independent from the mixed summary's whole-inventory Substrate permission. |
There was a problem hiding this comment.
Since this guide treats EP-1270 as the source contract, can we update that design as part of this change? It still says ListConfiguredProviders is unchanged and lists ModelProviderConfig, tool servers, and prompt templates as non-goals. It should also document whichever summary/Substrate authorization contract we choose, otherwise external authorizer implementations don't have a consistent contract to follow.
| require.Equal(t, []string{"cached"}, cached.GetModels()) | ||
| _, err = models.ListProviderModels(ctx, &apiv1.ListProviderModelsRequest{ProviderName: "hidden"}) | ||
| assertGRPCCode(t, err, codes.PermissionDenied) | ||
| _, err = models.ListProviderModels(ctx, &apiv1.ListProviderModelsRequest{ProviderName: "allowed", Refresh: true}) |
There was a problem hiding this comment.
Can we also exercise this through a real share-bearing RPC? The service test constructs ShareContext directly, while this generated-client case only covers ordinary authorizer denial. A valid x-share-token with an otherwise permissive authorizer would verify the interceptor-to-service path and that refresh is denied before calling the refresher.
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
github-actions
Bot
added
the
enhancement-proposal
|
Addressed in 3748a85 |
2 participants
Several resource lists bypass collection scopes or authorize only a resource type. Restrictive authorizers therefore cannot consistently hide denied objects or authorize mutations against a specific resource.
This change:
Adds service, gRPC-client, and MCP regression coverage for scoped results and denial before side effects, including existing SandboxTemplate authorization paths. No protobuf changes.