-
Notifications
You must be signed in to change notification settings - Fork 136
mcp: JWT scope based authorization #1482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is ❌ Your patch status has failed because the patch coverage (62.28%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1482 +/- ##
==========================================
- Coverage 83.24% 82.92% -0.32%
==========================================
Files 137 138 +1
Lines 12042 12213 +171
==========================================
+ Hits 10024 10128 +104
- Misses 1411 1470 +59
- Partials 607 615 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
d1ea251 to
877d598
Compare
4d2e8d5 to
0ccf737
Compare
29db138 to
2dd862c
Compare
2167726 to
a95087c
Compare
b77c937 to
f8a07cc
Compare
api/v1alpha1/mcp_route.go
Outdated
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:MaxLength=4096 | ||
| // +optional | ||
| Arguments *string `json:"arguments,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussing this with @nacx offline, we chose CEL for argument evaluation.
CEL is easier to write and understand than regex evaluation when the arguments are complex object types.
For example:
To check against this argument:
{
"key-level1": {
"key-level2": {
"key-level3": "value"
}
}
}The regex match:
/"key-level1"\s*:\s*\{\s*"key-level2"\s*:\s*\{\s*"key-level3"\s*:\s*"value"\s*\}\s*\}/gm
The CEL match:
args["key-level1"]["key-level2"]["key-level3"] == "value"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about renaming this to condition? If we do this, we could also easily support meta in addition to args to have policies based on the contents of the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should meta be inside the ToolCall, or MCPAuthorizationTarget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh meta is part of the CallToolParams - then condition is a good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it again, condition feels a bit vague and also used in the status field, some alternatives:
- When
- Predicate
- Matcher
When reads naturally in the context of authorization rules:
tools:
- backend: backend1
tool: listFiles
when: args.folder == "restricted"Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
0cab6d9 to
746bdab
Compare
Signed-off-by: Huabing Zhao <[email protected]>
746bdab to
9e4df68
Compare
| // | ||
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=16 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason for this limitation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just want to set a sane limitation for the size of the tool list for one rule. We can adjust it as needed in the future, but I think 16 should be OK for now.
api/v1alpha1/mcp_route.go
Outdated
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:validation:MaxLength=4096 | ||
| // +optional | ||
| Arguments *string `json:"arguments,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT about renaming this to condition? If we do this, we could also easily support meta in addition to args to have policies based on the contents of the metadata.
| } | ||
|
|
||
| // If the rules are defined, a valid bearer token is required. | ||
| token, err := bearerToken(headers.Get("Authorization")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should decouple the "authenticated principal extraction" from the authorization code (see my other comments around interfaces, etc).
|
|
||
| for _, rule := range authorization.Rules { | ||
| if !m.toolMatches(backendName, toolName, rule.Target, arguments) { | ||
| continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, with the current logic, if the MCPRoute has, say 20 tools, and I just want to limit access so that for one particular tool I can not use a particular argument (e.g. you can't call the filesystem tool passing "/root" as an argument), then I could have to create 20 auth rules to allow all tools, and then the rule with the argument filter for that particular rule, right?
Is this the UX we want?
Should we consider bringing back the default action? Having a default allow and explicit denies can also help when MCP servers add more tools, etc (and we can consider that with the existing "tool filtering" we're only exposing the tools that we want to be called... so having to always list all allowed tools if you have any rule could not be the best UX.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps we should also support wildcards in tool names? We need to come up with a way to facilitate users doing what they need without being excessively verbose on the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I think we should bring the default action back. Supporting wildcard is not enough here - the tool names may not share a similar name pattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use case in your example should work now, with a Deny rule for restricted folders and a Allow rule with the required sopes.
authorization:
defaultAction: Deny
rules:
- action: Deny
target:
tools:
- backend: backend1
tool: listFiles
arguments: 'args.folder == "restricted"'
- action: Allow
source:
jwt:
scopes:
- read
target:
tools:
- backend: backend1
tool: listFiles**Description** Add the configured scopes to the `WWW-Authenticate` headers. At initialization time, which is when the first authentication will occur, we don't have enough information to provide a fine-grained list of scopes, so the best we can do is to default to the ones defined in the protected resource metadata. **Related Issues/PRs (if applicable)** Fixes #1578 The addition of the header on 403 requests is implemented in #1482, but this issue can be closed as soon as this PR is merged, because we'll be compatible with the latest spec. **Special notes for reviewers (if applicable)** cc @zhaohuabing can you take a look? Signed-off-by: Ignasi Barrera <[email protected]>
db4fe61 to
9f14340
Compare
Signed-off-by: Huabing Zhao <[email protected]>
9f14340 to
819b995
Compare
Signed-off-by: Huabing Zhao <[email protected]>
84e7064 to
116670d
Compare
Signed-off-by: Huabing Zhao <[email protected]>
| } else { | ||
| claims := jwt.MapClaims{} | ||
| // JWT verification is performed by Envoy before reaching here. So we only need to parse the token without verification. | ||
| if _, _, err := jwt.NewParser().ParseUnverified(token, claims); err != nil { |
Check failure
Code scanning / CodeQL
Missing JWT signature check High
this user-controlled source
Signed-off-by: Huabing Zhao <[email protected]>
https://github.com/envoyproxy/ai-gateway/pull/1482/files#r2587966887 Thinking about it again, condition feels a bit vague and also used in the status field, some alternatives:
When reads naturally in the context of authorization rules: tools:
- backend: backend1
tool: listFiles
when: args.folder == "restricted" |
Signed-off-by: Huabing Zhao <[email protected]>
b908be0 to
9271f0b
Compare
Description
This PR introduces MCP spec compatible scope based authorization for MCPRoutes.
According to the 2025-11-25 version of the MCP spec, the MCP Gateway should enforce scope-based authorization on behalf of the backend MCP server, and include the required scopes in the
WWW-Authenticateheader of the 403 response if authoriation fails.https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization#runtime-insufficient-scope-errors
Example:
Reference: https://modelcontextprotocol.io/specification/draft/basic/authorization#scope-challenge-handling
Implements #1459