Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
|
||
| - `tools/list` shows only tools the user can call (based on `call_tool` policies); a result in which a tool repeats `name`, `annotations`, or the annotation `title` or a hint such as `readOnlyHint`, or spells one with a case-insensitive alias, fails closed with a generic internal JSON-RPC error, so policies evaluate exactly the hints the client receives. | ||
| - `tools/list` shows only tools the user can read (the `tool:read` check). `tool:call` implies `tool:read`, so a user allowed to call a tool also sees it, and existing `call_tool` policies keep their `tools/list` behavior. `tool:read` never implies `tool:call`. Cedar policies cannot grant `tool:read` alone yet, so for Cedar a tool is visible only when a `call_tool` policy allows it. An HTTP PDP receives `mcp:tool:read` and can allow it alone. This changes what an HTTP PDP deployment shows, so review it before upgrading: | ||
| - **Before:** a tool appeared in `tools/list` only if the PDP allowed `mcp:tool:call` for it, with one PDP decision per tool. |
There was a problem hiding this comment.
I'm not sure it's relevant to document a before and after in this doc. Architecture docs should represent the "today" and not be a changelog. Unless an explicit migration is needed.
There was a problem hiding this comment.
Thanks, you were right. I've rewritten the section to describe current behavior only and moved the Cedar log migration into its own section under the claim-source docs. The before-and-after is in the PR description. The docs now also cover the error handling and the audit fields. Pushed as e0dacd2.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6903 +/- ##
==========================================
+ Coverage 79.71% 79.84% +0.12%
==========================================
Files 818 828 +10
Lines 83583 84585 +1002
==========================================
+ Hits 66631 67537 +906
- Misses 16947 17043 +96
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Thanks for picking this up. The shape matches what we agreed on in #4048, and I checked that nothing that leads to a tools/call now goes through a read check, so the security story holds. Most of my comments are about making it smaller and putting the decorator where it belongs.
One thing I want to call out because it isn't obvious from the code. With Cedar, the tool:read check errors with "unsupported feature/operation combination" and the decorator drops that error, so visibility is decided by call_tool. That turns out to be the right answer, but only by accident. I wrote a quick test against the real Cedar authorizer with this policy set:
permit(principal, action, resource);
forbid(principal, action == Action::"call_tool", resource == Tool::"t");
On main, t is hidden from tools/list. With this PR it's still hidden. If Cedar evaluated a real Action::"read_tool", t would show up, because the broad permit matches and the call-only forbid doesn't. That's what #6924 proposes, so I've left a comment there. It needs a design for this case before it goes ahead. What I'd ask for here instead is that the Cedar authorizer returns false, nil for tool/read before it resolves claims. Today every tool on every tools/list goes through the full claim and upstream-token processing twice, and the first pass only exists to produce an error we throw away. With a plain deny, combineToolReadDecision and the "ignore the read error" logic can go away too.
The same combination affects the HTTP PDP (see the inline comment on docs/authz.md).
| // tool:read check, so surfacing it would hide every tool for every Cedar user. | ||
| // The tool:call answer is authoritative: an allow grants visibility, and a | ||
| // callErr is returned so the caller fails closed. | ||
| func combineToolReadDecision(_ error, callAllowed bool, callErr error) (bool, error) { |
There was a problem hiding this comment.
This function ignores its first argument and returns the other two unchanged, so the comment above it describes a decision it doesn't make. Can we drop it and return callAllowed, callErr directly? If Cedar returns a plain deny for tool/read (see the review body), there's no read error to explain anymore.
There was a problem hiding this comment.
@jhrozek Thanks. Here's what I'm doing:
- Keeping the audit tool names.
tools/callaudit events already record tool names, so this matches existing practice. - Moving the Cedar plain deny for
tool:readto a separate PR, per the one-change-per-PR rule. It's tracked in Add a read_tool action so Cedar can grant tools/list visibility without invocation #6924. - Moving vMCP
FilterToolstotool:readin this PR, withAllowToolCallstaying ontool:call. I'll close Filter vMCP tools/list with tool:read, as the single-server path does #6925 when it lands. - Making the small fixes and test cleanup, and adding the PDP behavior change to the PR body and upgrade note.
There was a problem hiding this comment.
@jhrozek thanks for the review. I squashed this to a single commit on top of current main, so the earlier inline comments show as outdated. Everything is addressed except the points marked below.
- Cedar:
tool:readis now a plain deny before claims are resolved, socall_toolpolicies decide visibility, as before. Atool:readerror is returned as is, with notool:callfallback, socombineToolReadDecisionand the ignore-the-error logic are gone. Theread_toolaction design stays in Add a read_tool action so Cedar can grant tools/list visibility without invocation #6924. - Wrap once: the decorator is applied in
Middleware(whatCreateMiddlewareFromConfigbuilds, and the one place every caller goes through) and innewCedarAdmission.authorizeToolReadcalls the authorizer directly. vMCPFilterToolsnow checkstool:readandAllowToolCallstays ontool:call, so Filter vMCP tools/list with tool:read, as the single-server path does #6925 isn't needed. I put it inMiddlewareinstead ofCreateMiddlewareFromConfig. - Audit: only
discovery_fallback_countremains. The tool names and the flag are gone. This replaces my earlier reply that said the names would stay. - Deadline:
tools/listruns under one 30s deadline on both paths, and atool:readerror or timeout skips thetool:callcheck. - Tests: one table with a
wantMarkedcolumn, error-path tests, the nil and absent-marker tests dropped, and the rows that pass onmainremoved. The fakes are one per package and still hand-written, because test types can't be shared across packages. I can generate a shared mock forAuthorizeras a follow-up if you'd prefer.
Also in the PR body: a PDP must answer mcp:tool:read, because an error from it hides the tool. @JAORMX the docs/authz.md section now describes current behavior only, and the upgrade example is in the PR body.
CI hasn't started on this PR since it comes from a fork. Could one of you approve the workflow run? That also runs the new e2e spec, which I trimmed after my last local run.
| return a.AuthorizeWithJWTClaims( | ||
| // Wrap here, so every caller gets the tool:call implication, including callers | ||
| // that pass a raw authorizer. The wrap is cheap: the fallback runs only on denial. | ||
| return authorizers.WithToolCallImpliesRead(a).AuthorizeWithJWTClaims( |
There was a problem hiding this comment.
Wrapping on every call means the decorator only ever sees tool:read, so the pass-through branch, the nil guard in WithToolCallImpliesRead and the exported ToolCallImpliesRead type never run in production. The issue asked for the decorator to sit in front of the authorizer. Could we wrap once where authorizers are built, in CreateMiddlewareFromConfig (pkg/authz/config.go:42) and newAdmission (pkg/vmcp/core/admission.go:99), and have authorizeToolRead call the authorizer directly the way the prompts and resources filters do?
That would also fix vMCP in this PR, so #6925 isn't needed as a follow-up. cedarAdmission.FilterTools still asks tool:call on a raw authorizer (admission.go:160). With the decorator applied in newAdmission, FilterTools can ask tool:read. AllowToolCall (admission.go:186) has to stay on tool:call, since it's the invocation check. The comments at admission.go:40, 44 and 172 also still refer to authorizeToolCall, which this PR renames.
| // The marker is written and read by the single request goroutine, so no | ||
| // synchronization is needed. | ||
| type DiscoveryMarker struct { | ||
| ViaCallFallback bool |
There was a problem hiding this comment.
ViaCallFallback is always the same as len(FallbackTools) > 0, and the discovery_fallback audit key is the same as discovery_fallback_count > 0. I'd also reconsider the tool names. Authz runs inside the tool filter middleware (pkg/runner/middleware.go, authz is appended after the tool filter), so these names are recorded before the allowlist and overrides are applied. With an allowlist of only weather, the client gets [weather] and the audit event says ["weather", "calculator"]. Under Cedar the list is also every visible tool on every call, so it doesn't add much. A tools/list event is already a discovery event because of its method. Could we keep just a flag (or a count) and drop the names?
| - `tools/list` shows only tools the user can call (based on `call_tool` policies); a result in which a tool repeats `name`, `annotations`, or the annotation `title` or a hint such as `readOnlyHint`, or spells one with a case-insensitive alias, fails closed with a generic internal JSON-RPC error, so policies evaluate exactly the hints the client receives. | ||
| - `tools/list` shows only tools the user can read. The read check is `tool:read`, and a `tool:call` grant also satisfies it, so a user who can call a tool can also see it. `tool:read` never grants `tool:call`. | ||
| - **Cedar:** Cedar has no `tool:read` action. Its read check returns an "unsupported" error, which is ignored, so the `tool:call` check decides visibility. In policies this is `call_tool`, so a tool is listed only when a `call_tool` policy allows it. | ||
| - **HTTP PDP:** the PDP receives `mcp:tool:read` as well as `mcp:tool:call`, and a tool is listed when it allows either one. Each listed tool costs up to two PDP decisions: a read check, then a call check when read is denied. A PDP that allows `mcp:tool:read` broadly lists every tool to every caller, so review read rules before deploying. |
There was a problem hiding this comment.
Worth spelling out one more case here. A PDP that allows mcp:tool:read broadly and denies mcp:tool:call only for one tool will now list that tool, where before it was hidden. That's a behavior change for existing PDP rules, so it belongs in the upgrade notes and the PR body.
Also, the 2N PDP calls aren't covered by maxDerivedAuthzBudget, so a slow PDP makes tools/list up to twice as slow with no overall deadline. Could the authorizedToolIndexes loop run under a shared deadline, or at least skip the call fallback when the read check failed with a context or timeout error?
| - `tools/list` shows only tools the user can read. The read check is `tool:read`, and a `tool:call` grant also satisfies it, so a user who can call a tool can also see it. `tool:read` never grants `tool:call`. | ||
| - **Cedar:** Cedar has no `tool:read` action. Its read check returns an "unsupported" error, which is ignored, so the `tool:call` check decides visibility. In policies this is `call_tool`, so a tool is listed only when a `call_tool` policy allows it. | ||
| - **HTTP PDP:** the PDP receives `mcp:tool:read` as well as `mcp:tool:call`, and a tool is listed when it allows either one. Each listed tool costs up to two PDP decisions: a read check, then a call check when read is denied. A PDP that allows `mcp:tool:read` broadly lists every tool to every caller, so review read rules before deploying. | ||
| - **vMCP:** filtering uses `tool:call` only, so a `tool:read`-only grant has no effect there. |
There was a problem hiding this comment.
This is only true for the stateless vMCP core. Legacy vMCP goes through CreateMiddlewareFromConfig and the response filter, so it gets the decorator. If the decorator moves to construction (see tool_filter.go comment), both paths behave the same and this bullet can go.
| always resolved to the first upstream, so the mirror and the pinned provider's | ||
| assertion cannot diverge there. | ||
|
|
||
| #### Upgrading the Cedar DEBUG log values |
There was a problem hiding this comment.
This section and the INFO-line sentence under "Tokens with no upstream login" are about Cedar claim-source logging, not #4048. Could they go in a separate PR? Two things to fix when they do: the removed paragraph mentioned that the session-less path now logs request:no-upstream-session, which the new mapping drops, and the quoted INFO message is only the start of the real text in cedar/core.go:681, so an exact-match parser would miss it.
| want []int | ||
| }{ | ||
| { | ||
| name: "tool:call alone makes every tool visible", |
There was a problem hiding this comment.
The "tool:call alone" and "tool:read error" rows pass on main too, since main asks tool:call. Only "tool:read alone" at :72 fails if the change is reverted. Could that row go into the existing TestAuthorizedToolIndexes table and this file be dropped? Same for TestFindToolResponseFilter_FallsBackToToolCall, which goes through the same authorizedToolIndexes path.
| assert.Nil(t, WithToolCallImpliesRead(nil)) | ||
| } | ||
|
|
||
| func TestToolCallImpliesRead_DiscoveryMarker(t *testing.T) { |
There was a problem hiding this comment.
This table repeats the inputs from TestToolCallImpliesRead at :32. A wantMarked column on the first table would cover it. TestWithToolCallImpliesRead_NilPassesThrough (:112) tests a branch production never reaches, and TestDiscoveryMarkerFromContext_Absent (:184) only tests a context lookup. What's missing is the error paths: opsAuthorizer can't return errors, so "read errors, call allows" and "call errors" aren't covered at unit level. There are also now three hand-written fakes for the same interface (opsAuthorizer, opFilterAuthorizer, mockAuthorizer). One mock, generated or shared, would do.
| for _, tool := range result.Tools { | ||
| names = append(names, tool.Name) | ||
| } | ||
| Expect(names).To(ContainElement("query_vulnerability"), |
There was a problem hiding this comment.
This assertion passes on main as well. The reads > 0 check further down is the one that proves the change. The CLI It goes through the same tools/list path, so I don't think it adds coverage.
tools/list filtered tools by tool:call. It now filters by tool:read, and a tool:call grant also satisfies the read check through a decorator around the Authorizer (pkg/authz/authorizers/tool_read.go). The implication is one way: tool:read never grants tool:call, and there is no new ingress verb. The decorator is applied once, where the authorization middleware is built (Middleware) and in the vMCP admission. vMCP FilterTools now checks tool:read; AllowToolCall still checks tool:call. Cedar denies tool:read without evaluating claims, so call_tool policies decide visibility, as before. tools/list runs under one shared deadline. A tool:read error, including a timeout, is returned without a tool:call check, so a slow PDP cannot double the wait. A tools/list entry listed only through the tool:call fallback is counted in the audit event as discovery_fallback_count. HTTP PDP rules that allow mcp:tool:read now list a tool even when the matching mcp:tool:call rule denies it. This is documented in docs/authz.md. Closes stacklok#4048 Signed-off-by: rkb32 <177729309+rkb32@users.noreply.github.com>
a60a7c1 to
0e44ae5
Compare
Summary
Closes #4048.
tools/listfiltered tools bytool:call, so a caller could not be allowed to discover a tool without also being allowed to invoke it. This PR filters bytool:readinstead, as agreed in #4048. A caller can now see a tool without being allowed to call it.tool:callimpliestool:read, one way only. The implication is a decorator aroundAuthorizer(pkg/authz/authorizers/tool_read.go). It checkstool:readfirst and falls back totool:callwhen read is denied. Existing policies that allowtool:callkeep the same tool list.The decorator is applied once, where the authorization middleware is built (
Middleware) and in the vMCP admission (newCedarAdmission). vMCPFilterToolscheckstool:read. vMCPAllowToolCallstill checkstool:call.When a tool is listed only through the
tool:callfallback, thetools/listaudit event recordsdiscovery_fallback_count, the number of tools listed that way.No new ingress verb. No signed or replayable audit receipts.
Type of change
Test plan
task test)task lint)Unit tests and lint pass on Linux, with Docker available. One unrelated timing test in
pkg/transport/proxy/streamable(TestStandaloneSSE_StopWhileStreamOpenReturnsCleanly, "connection refused" on its own local port) failed in the last full run, and this PR does not touch that package.E2E: an earlier version of the
authzandauditlabelled specs passed 22 of 22 (ginkgo --label-filter="authz || audit") against a builtthv. The new specTool discovery through tool:call fallback(test/e2e/tool_read_fallback_authz_test.go) uses a mock HTTP PDP over streamable-http and checks that the PDP is asked fortool:readfirst. The spec was trimmed after that run and not re-run locally. The fulltask test-e2esuite was not run.New unit tests:
pkg/authz/authorizers/tool_read_test.go:TestToolCallImpliesRead(including the audit marker) andTestToolCallImpliesRead_ErrorPaths.pkg/authz/authorizers/cedar/core_test.go:TestAuthorizer_ToolReadIsAPlainDeny.pkg/authz/tool_filter_test.go: atool:read-only row inTestAuthorizedToolIndexes, andTestAuthorizedToolIndexes_SharedDeadlineStopsTheLoop.pkg/authz/response_filter_test.go:TestFindToolResponseFilter_ListsToolWithReadGrant.pkg/audit/discovery_marker_test.go:TestAddMetadata_DiscoveryFallbackandTestDiscoveryFallback_FlowsFromDecoratorToEvent.pkg/vmcp/core/admission_test.go:TestCedarAdmission_ReadGrantListsButDoesNotAllowCallandTestCedarAdmission_FilterToolsStopsAtSharedDeadline.Updated:
pkg/authz/tool_filter_test.gonow expectstool:read, and the response filter tests build the filter through a helper that wraps the authorizer the wayMiddlewaredoes.pkg/vmcp/core/admission_test.goexpectstool:readfromFilterTools. The vMCP factory path (newAdmission) is also covered by the Cedar subtestcedar config lists tools through the tool:read fallback.Does this introduce a user-facing change?
call_toollist the same tools. Cedar deniestool:readwithout resolving claims, so the new read check adds no extra claim or upstream-token resolution per tool.tool:readortool:call. Each tool now makes up to two PDP requests (a read check, then a call check when read is denied), where it made one before. Tools that are not listed cost two as well. A PDP that allowstool:readfor all tools makes every tool visible, including a tool itstool:callrule denies. Example: a rule allowingmcp:tool:readfor all tools, withmcp:tool:calldenied fordelete_repo, now listsdelete_repoto every caller. The call is still denied. Narrow the read rule before upgrading if that tool must stay hidden.tools/listnow uses thetool:readcheck, sotool:readgrants and HTTP PDP read allows list tools there. The same up-to-two-request cost applies.tools/callis unchanged.tools/listevents gaindiscovery_fallback_countonly when a fallback happened. The optimizer'sfind_toolresponse is filtered the same way, so itstools/callevent can carry the count too. Other fields are unchanged.Documented in
docs/authz.md.Special notes for reviewers
mcp:tool:readmakes tools visible, in both the single-server and vMCP paths. Check those rules before upgrading.tools/listruns under one shared 30s deadline on both paths (maxDerivedAuthzBudgetfor the single-server filter,toolFilterBudgetfor vMCPFilterTools). Tools checked after it expires are hidden. Atool:readerror, including a timeout, is returned without atool:callcheck, so a slow PDP cannot double the wait.mcp:tool:read. Atool:readerror is final, so a PDP that rejects or errors on that operation hides the tool for every caller, including callers who holdmcp:tool:call. This follows the review request to drop the "ignore the read error" fallback, and the failure is closed. Falling back totool:callon non-timeout read errors would avoid it if reviewers prefer.Middlewarerather thanCreateMiddlewareFromConfig.Middlewareis the one place every caller passes through, and it is called once per middleware, not per request.pkg/authz,pkg/vmcp/core,pkg/authz/authorizers,pkg/audit), following the existing pattern in those packages. A shared or generated mock forAuthorizerwould replace all four and could be a separate change.tool:readalone yet, since it has noread_toolaction (Add a read_tool action so Cedar can grant tools/list visibility without invocation #6924).tools/listis also not filtered on the SSE transport onmain(tools/list is not filtered on the SSE transport #6926), and this PR doesn't change that.🤖 Generated with Claude Code