-
Notifications
You must be signed in to change notification settings - Fork 389
Add a blog post about secure MCP SSE server #2284
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
🙈 The PR is closed and the preview is expired. |
ade0346
to
55ec40c
Compare
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.
It's a great post. I thing we can clarify a few things:
- Explains where the authentication happens in the MCP protocol (in which message the header must be set), even diving a bit in the MCP protocol a bit
- I would actually start with curl and then having a dev ui demo (maybe even a quick video)
a3b0b6f
to
015bb69
Compare
de4f14e
to
6da0f63
Compare
@maxandersen @cescoffier Thanks very much for the thoughtful review, I know I was not quite getting along, but as always, on reflection, I know the updated PR should be better. One thing I found difficult to update was to move I honestly don't mind if it goes first, but not sure how to make it work neatly if curl goes first, Clement, if you'd like please give it a try. Please follow up tomorrow morning with suggestions, especially with respect to some further technical clarifications and we should be ready to merge soon. Cheers |
I've resolved all the comments though I may not have addressed them as you were expecting - please create new suggestions |
May be I can have a dedicated section for getting the token and then refer to it from all other sections, that might work, will try tomorrow |
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.
Way better! I made a few comments and suggestions.
The main thing I think would be to use prod mode for the inspector and curl demo. Do you think it's possible?
@cescoffier Thanks, sure, I'll have a look at having it run with jbang, I'll apply all the suggestions except those related to the versions and then let's try to do one more review round |
Hi @cescoffier, actually, unfortunately, supporting the prod mode will make it more complex. We'd still not be able to plug it in into Claude right now, until it starts supporting the MCP server user login, so that would leave the only other option, which I actually used to have but dropped:
I suggest we just keep it in Dev Mode for this one, this is 100% not the last article about MCP security, and we'll do something interesting for prod once Claude and other MCP clients actually start supporting logging in users |
cea4bba
to
e862da9
Compare
f1f4b08
to
cfd9fbd
Compare
I've another long train journey tomorrow. I will do another review tomorrow. |
cfd9fbd
to
c51c1f3
Compare
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.
a few minor nitpicks for clarity but otherwise really good stuff!
25e347b
to
e907c77
Compare
Thanks @maxandersen, I've applied your suggestions with minor tweaks, please add more suggestions if you'd like. Clement would also like to have another look tomorrow, so I guess I'll update the publish date to Monday Apr 28th |
e907c77
to
38a38ce
Compare
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.
Wow! Sergei, this is great, it's going to be awesome!
I've made a few suggestions. There is a flaw in the flow when we configure the MCP server with GitHub.
a4cff93
to
01d6030
Compare
@cescoffier Thanks Clement for a very thorough review, very appreciated. There have been quite a few comments, I hope I've addressed all of them, either applying the suggestions directly or trying to react to some suggestion with an update, where I felt the direct suggestion application may not work best. I'm going to check for various typos, the only 2 main things that I believe are open are:
Have another quick look tomorrow, I think the PR is very close to being merged now, cheers |
01d6030
to
eed0bc9
Compare
I published it locally and did a few minor follow up updates like restoring a line break between |
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.
How to start the server in prod mode needs to be reworked a bit.
The rest is just a few typos.
|
||
=== Install and run application | ||
|
||
First, add `quarkus.package.jar.type=uber-jar` to the application.properties. |
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.
Claude only supports stdio transport (for now).
So, switch to what Quarkus recommends, not planning on something we don't know.
The fact that Claude does not support SSE is not really important, it is just that this demo would be ready to be plugged in We just show how to start with jbang - we don't recommend using uber jar - I added a note to tell users that the only reason we do it is to support a jbang start up |
Anyway, I'll just update as you proposed, but add a note about jbang instead |
7b23c01
to
e69713b
Compare
the uber jar is not required for jbang. jbang supports non-uber-jar. its java that is the limit here. uber-jar is just so its easier to distributed and faster to start |
Faster to start? I really doubt. |
a29d35c
to
53dfe19
Compare
Thanks @maxandersen, @cescoffier, it should be ready to go now ? |
Thanks for the review, planning to go ahead with the merge in about an hour
No description provided.