-
Notifications
You must be signed in to change notification settings - Fork 177
feat(auth): introduce scoped based authorization #224
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
Conversation
Signed-off-by: Arda Güçlü <[email protected]>
| klog.Warningf("authorization-url is using http://, this is not recommended production use") | ||
| } | ||
| } | ||
| if m.StaticConfig.ServerURL != "" { |
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.
Currently, server url field is used for arbitrary audience rather than a structured URL format. I'm removing this validation (and its test). But if we decide to force URL format in the future, we'll need to revert this change (and its test).
|
|
||
| func toolScopedAuthorizationMiddleware(next server.ToolHandlerFunc) server.ToolHandlerFunc { | ||
| return func(ctx context.Context, ctr mcp.CallToolRequest) (*mcp.CallToolResult, error) { | ||
| scopes, ok := ctx.Value(TokenScopesContextKey).([]string) |
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.
@manusa although we agreed that we'll add this scoped based validation in authorization.go rather than here, mcp clients all failed to handle the flow. I think, apart from here all the rest is a clear indication of model context protocol violation.
After adding the scope based check in tool call middleware, it simply works.
|
I have no idea what are these verification errors in here https://github.com/containers/kubernetes-mcp-server/pull/224/checks?check_run_id=47091334553. So I'll have to ignore them. @manusa would you please have a look at this PR, when you have a moment?. Thanks. |
No idea either. We need to migrate to our own pipelines.
👍 |
manusa
left a comment
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.
LGTM, thx!
This PR is derivation of this #217. However, #217 contains some hacky code to make agentic flow working such as intentional fall back to in-cluster config, etc.
This PR introduces scoped based authorization for tool calls. If it detects the tool name is not included in the token scopes, mcp server fails with 403 forbidden error.
Currently this PR uses tool names as scoped names. But in the future, we may want to create a mapping to customize scope names.