add URL elicitation design doc for per-user credentials#800
Open
maleck13 wants to merge 10 commits intoKuadrant:mainfrom
Open
add URL elicitation design doc for per-user credentials#800maleck13 wants to merge 10 commits intoKuadrant:mainfrom
maleck13 wants to merge 10 commits intoKuadrant:mainfrom
Conversation
Signed-off-by: craig <cbrookes@redhat.com>
📝 WalkthroughWalkthroughIntroduces a design specification for per-user credential collection via MCP URL mode elicitation. Describes router detection of missing credentials, emission of Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: craig <cbrookes@redhat.com>
maleck13
force-pushed
the
URL-Elicitation-User-Credentials
branch
from
ca05c78 to
61690b8
Compare
Signed-off-by: craig <cbrookes@redhat.com>
Signed-off-by: craig <cbrookes@redhat.com>
Contributor
Author
|
TODO call out how an MCP could handle both bot (SA) and User (elicitation) |
Signed-off-by: craig <cbrookes@redhat.com>
Signed-off-by: craig <cbrookes@redhat.com>
Signed-off-by: craig <cbrookes@redhat.com>
Signed-off-by: craig <cbrookes@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (5)
docs/design/gateway-initiated-url-elicitation.md (5)
8-8: Hyphenate "third-party".📝 Grammar fix
-2. **Token exchange via OAuth provider** — requires the OAuth provider to support token exchange and be configured per upstream. Requires third party identity federation. +2. **Token exchange via OAuth provider** — requires the OAuth provider to support token exchange and be configured per upstream. Requires third-party identity federation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/gateway-initiated-url-elicitation.md` at line 8, In the sentence under "Token exchange via OAuth provider" update the phrase "third party identity federation" to use the hyphenated form "third-party identity federation" so it reads "...Requires third-party identity federation."; locate the string "Token exchange via OAuth provider — requires the OAuth provider to support token exchange and be configured per upstream. Requires third party identity federation." and replace the unhyphenated instance with the hyphenated one.
112-112: Consider clarifying Authorization header precedence.The current description compresses the decision tree. For implementation clarity, consider making the order explicit:
- If
Authorizationheader exists → use as-is (don't inject)- Else if cached credential exists → inject into
Authorizationheader- Else if client supports elicitation → return
-32042- Else → standard error
Based on learnings, the router must only set the Authorization header when not already present to allow AuthPolicy to validate OAuth tokens first.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/gateway-initiated-url-elicitation.md` at line 112, Update the Router decision logic to make Authorization header precedence explicit: in the Router handler (the code that checks request headers, client elicitation capability, cache, and returns -32042) implement the exact sequence: 1) if request.headers.Authorization (or equivalent header lookup) exists, leave it untouched and proceed; 2) else if a cached credential exists for the client, inject it into Authorization; 3) else if client.capabilities.elicitation.url is true, return error -32042; 4) otherwise return the standard error path; ensure AuthPolicy validation runs against an existing Authorization header and that cached injection only occurs when no header is present.
156-156: Use American English: "In the future".📝 Grammar fix
-When no `url` is set, the router generates a URL pointing to the broker's built-in credential page. In future, if OAuth fields are added (client ID, authorize endpoint, etc.), their presence on the object will imply an OAuth flow. +When no `url` is set, the router generates a URL pointing to the broker's built-in credential page. In the future, if OAuth fields are added (client ID, authorize endpoint, etc.), their presence on the object will imply an OAuth flow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/design/gateway-initiated-url-elicitation.md` at line 156, The phrase "In future" in the sentence starting "When no `url` is set, the router generates a URL..." should be changed to American English "In the future"; update that sentence so it reads "In the future, if OAuth fields are added (client ID, authorize endpoint, etc.), their presence on the object will imply an OAuth flow." to fix the grammar in the gateway-initiated-url-elicitation document.
177-177: Add language specifier to fenced code blocks.Markdown linters require language identifiers on fenced blocks. Use
textfor flow diagrams.🔧 Markdown lint fix
Line 177:
-``` +```text Router → -32042 (broker URL) → User enters PAT → Broker stores in cache → Router reads cache → sets headerLine 187: ```diff -``` +```text Router → -32042 (external URL) → User stores PAT in Vault → AuthPolicy reads from Vault → sets header</details> Also applies to: 187-187 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@docs/design/gateway-initiated-url-elicitation.mdat line 177, The two fenced
code blocks containing the flow diagrams (the blocks starting with "Router →
-32042 (broker URL) → User enters PAT → Broker stores in cache → Router reads
cache → sets header" and "Router → -32042 (external URL) → User stores PAT in
Vault → AuthPolicy reads from Vault → sets header") need a language specifier;
update each triple-backtick fence fromtotext so Markdown linters accept
them.</details> --- `212-212`: **Simplify: "to" instead of "in order to".** <details> <summary>📝 Conciseness improvement</summary> ```diff -Encryption is only applied when using an external cache store as the storage backend — it protects credentials in an external store that may be shared or persisted to disk. For the in-memory backend, encryption adds no value since a process memory dump would reveal the encryption key alongside the ciphertext and in order to be used to call a backend the token credential has to be in plain text in memory. +Encryption is only applied when using an external cache store as the storage backend — it protects credentials in an external store that may be shared or persisted to disk. For the in-memory backend, encryption adds no value since a process memory dump would reveal the encryption key alongside the ciphertext and to be used to call a backend the token credential has to be in plain text in memory. ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@docs/design/gateway-initiated-url-elicitation.md` at line 212, Replace the phrase "in order to be used to call a backend the token credential has to be in plain text in memory" with the concise version using "to": "to be used to call a backend the token credential has to be in plain text in memory" in the sentence about in-memory backend encryption: "For the in-memory backend, encryption adds no value since a process memory dump would reveal the encryption key alongside the ciphertext and to be used to call a backend the token credential has to be in plain text in memory." Ensure the only change is removing "in order" to keep meaning and flow. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In@docs/design/gateway-initiated-url-elicitation.md:
- Line 8: In the sentence under "Token exchange via OAuth provider" update the
phrase "third party identity federation" to use the hyphenated form "third-party
identity federation" so it reads "...Requires third-party identity federation.";
locate the string "Token exchange via OAuth provider — requires the OAuth
provider to support token exchange and be configured per upstream. Requires
third party identity federation." and replace the unhyphenated instance with the
hyphenated one.- Line 112: Update the Router decision logic to make Authorization header
precedence explicit: in the Router handler (the code that checks request
headers, client elicitation capability, cache, and returns -32042) implement the
exact sequence: 1) if request.headers.Authorization (or equivalent header
lookup) exists, leave it untouched and proceed; 2) else if a cached credential
exists for the client, inject it into Authorization; 3) else if
client.capabilities.elicitation.url is true, return error -32042; 4) otherwise
return the standard error path; ensure AuthPolicy validation runs against an
existing Authorization header and that cached injection only occurs when no
header is present.- Line 156: The phrase "In future" in the sentence starting "When no
urlis
set, the router generates a URL..." should be changed to American English "In
the future"; update that sentence so it reads "In the future, if OAuth fields
are added (client ID, authorize endpoint, etc.), their presence on the object
will imply an OAuth flow." to fix the grammar in the
gateway-initiated-url-elicitation document.- Line 177: The two fenced code blocks containing the flow diagrams (the blocks
starting with "Router → -32042 (broker URL) → User enters PAT → Broker stores in
cache → Router reads cache → sets header" and "Router → -32042 (external URL) →
User stores PAT in Vault → AuthPolicy reads from Vault → sets header") need a
language specifier; update each triple-backtick fence fromtotext so
Markdown linters accept them.- Line 212: Replace the phrase "in order to be used to call a backend the token
credential has to be in plain text in memory" with the concise version using
"to": "to be used to call a backend the token credential has to be in plain text
in memory" in the sentence about in-memory backend encryption: "For the
in-memory backend, encryption adds no value since a process memory dump would
reveal the encryption key alongside the ciphertext and to be used to call a
backend the token credential has to be in plain text in memory." Ensure the only
change is removing "in order" to keep meaning and flow.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Path: .coderabbit.yaml **Review profile**: CHILL **Plan**: Pro **Run ID**: `81828a6e-e685-42cd-acff-3db3edb40f2d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between ee72d691ec82d8ee34e6afc2d7e67535fb2655ec and 0a408ac485a8ded04d11b223e9daec3f2792aad7. </details> <details> <summary>📒 Files selected for processing (1)</summary> * `docs/design/gateway-initiated-url-elicitation.md` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Signed-off-by: craig <cbrookes@redhat.com>
david-martin
approved these changes
Apr 30, 2026
Member
david-martin
left a comment
david-martin
left a comment
There was a problem hiding this comment.
+1 for this.
A few comments worth discussing, but shouldn't stop this merging or progressing at this stage.
| Router → -32042 (external URL) → User stores PAT in Vault → AuthPolicy reads from Vault → sets header | ||
| ``` | ||
|
|
||
| AuthPolicy handles credential injection. The router's role simplifies to: |
Member
There was a problem hiding this comment.
For an experimental feature, having this capability split between MCPServerRegistration and AuthPolicy is reasonable.
It does tie the gateway closer to AuthPolicy capability for pulling in an external credential and swapping a header. I think this is worth further discussion later from a couple of viewpoints:
- do we tie in closer to the rest of Kuadrant policies e.g. ship with them out of the box
- is there a resource (a policy?) that would make this particular flow easier overall to configure, given there is a pattern, or perhaps authorino is configured based just on what's in the MCPServerRegistration.credentialURLElicitation stanza?
|
|
||
| | Backend | Description | | ||
| |---------|-------------| | ||
| | **Session cache** (Redis / in-memory) | Initial implementation. Credentials are session-scoped and lost on session expiry or cache eviction. | |
Member
There was a problem hiding this comment.
In memory might be sufficient for initial implementation (easier for people to try out if they don't have a redis store configured). It would also remove the need for the encryption piece in this version?
|
|
||
| The MCP specification [warns about phishing attacks](https://modelcontextprotocol.io/specification/2025-11-25/client/elicitation#phishing) where an attacker could trick another user into completing an elicitation on their behalf. The credential page must verify that the user opening the URL is the same user who triggered the elicitation. | ||
|
|
||
| Identity verification is handled by the AuthPolicy attached to the gateway route where the credential page is served. The AuthPolicy validates the OIDC token from the request, ensuring the user opening the URL is the same user who triggered the elicitation. |
Member
There was a problem hiding this comment.
highlighting, another dependency on AuthPolicy for the e2e to work.
Not against this, but highlighting the coupling
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
credentials server-side, out of the MCP client/LLM context
elicitationobject on MCPServerRegistration CRD, extensible to OAuth flows401
Related #775
Summary by CodeRabbit