-
Notifications
You must be signed in to change notification settings - Fork 135
User based authentication methods #130
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
hi @bbang-dduck i understand there are a lot of big changes here, but some of them were required in order to support the features,and others are good changes that helped with the development of this feature, and also are i think good for the quality of the code base in general. I am happy to change things and structures if you dont like certain things but at the very least the refactoring and splitting up of the index.ts felt basically required given how complicated this feature was to implement. i have tested this locally using our companies gitlab instance so far. if @AndrewFarley you are interested in helping test this (both modes) that would be helpful. |
merged in the latest from master, i think the ci build should be fixed |
is there anything that you would like me to do / change that would accept this being merged? |
Hi @elee1766 thanks implementing this. Currently I can not build your branch but I would be interested in testing.
|
hmm i think this is a build error from a recent merge let me fix this |
i tested with claude code. can you show your redacted env configuration? the callback looks ok - i wonder why it says the callback failed - maybe the application is configured incorrectly? also, i have only tested with |
This is my .env:
Edit: Using the Guided OAuth Flow in the modelcontextprotocol/inspector works for getting a token. Edit 2: Both /sse and /mcp return 500 errors but I get no logs. |
first - ty for helping to test. going to bombard you with a few questions:
re: oauth flow - i am using the auth middleware from the modelcontextprovider sdk, so i would expect clients made with that sdk to be ones which works, so not surprised that the mcp inspector works for getting a token for reference, i am testing against my self hosted instance with a system level application (configured at /admin). this seems to work for me. i will push up some more detailed logging that you can enable with LOG_LEVEL=debug |
Its working now :D. I just have to get the token with the modelcontextprotocol/inspector manual mode and then i can past it in my python project. |
I agree, but i'm not super sure what needs to happen. I think maybe i need to just reply with the correct
true, i will add it
please feel free
FYI, this is the code for how client extracts the resource data from the header: https://github.com/modelcontextprotocol/typescript-sdk/blob/main/src/client/auth.ts#L444-L467 maybe you can implement this in your python project, as then it will be more compatible with things designed for the mcp typescript client. |
package.json
Outdated
@@ -32,8 +32,11 @@ | |||
"format:check": "prettier --check \"**/*.{js,ts,json,md}\"" | |||
}, | |||
"dependencies": { | |||
"@modelcontextprotocol/sdk": "^1.10.0", | |||
"@modelcontextprotocol/sdk": "1.13.3", | |||
"@node-rs/argon2": "^2.0.2", |
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.
Note that i had to run npm i @node-rs/argon2-darwin-arm64
on macos.
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.
can maybe switch to an argon2 impl with less dependencies
src/authentication.ts
Outdated
let authMiddleware: RequestHandler = (_req: Request, _res: Response, next: NextFunction) => { | ||
next(); | ||
}; |
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.
This should not be used, since all possible branches set the value of authMiddleware. If it is still the default at the end we would have a configuration problem and should throw an error.
src/config.ts
Outdated
GITLAB_PROJECT_ID: process.env.GITLAB_PROJECT_ID, | ||
|
||
|
||
ARGON2_SALT: process.env.ARGON2_SALT || "change-me-in-production", |
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.
Maybe add the "change-me-in-production" in example.env and not here.
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 dont want the server to refuse to start up if the salt is not set.
maybe a loud error message on startup if the salt is detected as the default salt
import path from "path"; | ||
|
||
// Create cookie jar with clean Netscape file parsing | ||
export const createCookieJar = (): CookieJar | undefined=> { |
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.
This really confuses me, why were cookies even used in the first place?
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 don't know, its in the original server and it said you had to read some sort of file for some cookie to access gitlab. no clue.
static async New(options: any): Promise<GitLabProxyProvider> { | ||
// we put this here so we dont initialize this unless we are using the oauth provider | ||
const Database = (await import('better-sqlite3')).default; | ||
const db = new Database(config.GITLAB_OAUTH2_DB_PATH); |
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.
Persistency should really be decoupled from the OAuth implementation. This means:
- Move all db logic to the a separate file
- Create a clean interface that hides the persistency logic
We could then even have different implementations for this interface (in-memory, sqlite, redis?). Or we could consider using a ORM.
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 persistence is specific to the oauth implementation.
different implementations for the interface is out of scope. could be done later for those who need it.
this PR really only is showing that
- token passthrough is possible
- DCR proxy over an oauth2 server that does not support DCR is possible
|
||
// Find the matching token by verifying against each hash | ||
let matchingRow = null; | ||
for (const row of rows) { |
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.
Hmm this does not seem optimal, do we not have another way to identify the correct token? Like an id?
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.
we can't store a deterministic id because that would be saving an unsalted hash in the database, which I don't want to do.
if there was a non-deterministic id, then we would need to somehow append it to the api key, and then extract it on read, but if i did that, then the token would no longer be a valid gitlab token
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.
in truth, we don't really need to do this at all. we can also
- just pass it through, because the token will fail authentication to the gitlab api anyways
- just check the api token with a request against the gitlab api every time, since it's supposed to just be a valid gitlab token anyways
import { randomBytes } from 'crypto'; | ||
|
||
// Custom provider that handles dynamic registration and maps to GitLab OAuth | ||
class GitLabProxyProvider extends ProxyOAuthServerProvider { |
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 read a little more about the spec and have one concern:
Do I understand correctly that the token that is created by the OAuth flow with GitLab is sent to the MCP client? The GitLabProxyProvider only validates that the token is stored in the db but then passes it on.
gitlabToken: token, // Return the original token since we don't store gitlab_token anymore
So there is no token that is only between the MCP server and client but instead the GitLab token is used.
Gemini sees a problem with this and I'm sure its right:
The Critical Reasons for Issuing Your Own Token
1. Security: The Principle of Least Privilege
What the GitLab token can do: The token you get from GitLab might have permissions (scopes) to read the user's private repositories, post comments, or access their profile. This is called the "scope" of the token.
What the MCP client needs: The MCP client only needs permission to perform actions on your MCP server.
The Risk: If you gave the GitLab token to the MCP client, and that client was compromised, the attacker would have access to the user's entire GitLab account. By issuing your own token, you limit the potential damage. The compromised client can only access your service, not the user's GitLab.
I would also recommend looking for a established library to do the OAuth instead having so much custom code.
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 you can find a library which supports https://datatracker.ietf.org/doc/html/rfc7591 on top of an existing oauth provider, then that would be preferred, but I could not find one
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.
So there is no token that is only between the MCP server and client but instead the GitLab token is used.
the user holds their token in the client, the MCP simply makes sure that the token is a token that it issued through its system (since it remembers the argon2 hashes of tokens it has issued)
ideally, we would not need the oauth wrapper and gitlab would support dynamic client registration, auth tokens could be passed through without any internal database validation, but gitlab doesn't
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 Risk: If you gave the GitLab token to the MCP client, and that client was compromised, the attacker would have access to the user's entire GitLab account.
correct. the token generated for use is a gitlab token, as if you were directly using the gitlab application. the mcp oauth layer exists in order to provide the facade of DCR, since it is required by the authorization spec, while still ultimately, just the gitlab application in the background.
if you compromised the server, you would receive keys. this is true.
this is an intended feature of the implementation. maintaining tokens for specifically the MCP seems like a bad idea. the MCP is not really its own oauth server, and delegates all authentication to gitlab via the configured application.
the gitlab MCP client needs a token that is scoped to YOUR user in order to send the request to gitlab as if YOU are the one doing the request. the role of the gitlab MCP is to impersonate the user - not act using a service account.
as a result - we store the salted hash of the token to validate that the token is one we issued (through the proxy), and we pass through the underlying token.
so gemini is right about what im doing, but otherwise, it is wrong. it does not understand that the mcp server must impersonate the underlying user.
there is no way to run the mcp server in a way that you either 1. do not pass through the actual gitlab keys, or 2. do not store the gitlab keys on the server side.
it would be very not good to have a database with all the plaintext gitlab api keys of the users, so it's best security wise to not store the keys, and therefore make it so that the actual key sent to the client is an api key.
the only sensible option i can think of that doesn't change this is to use a gitlab administrator api key in the mcp server, and generate a temporary impersonation token for every user request. (you can't store them or else you have a database of keys again)
but the issue is that would require that the server have its completely own oauth implementation, and not just perform DCR and delegate to gitlab, which could be an even greater attack vector. the database is really only needed for the initial DCR, and as said above, we don't really even need to validate the tokens sent to us, as the token will be passed on to the request ultimately made to gitlab.
all in all - i think it makes most sense to do what is currently done, which is to pass through a valid gitlab token, but let me know if you have other thoughts or another solution.
if someone is capable of compromising your mcp.mygitlab.com, likely they could also compromise your mygitlab.com, which could steal your tokens anyways.
you should NOT use a gitlab mcp server hosted by somebody that you do not trust. that is a given.
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.
All valid points, I think the main decision that has to be made is to choose between the two risks:
- Giving the GitLab Token to the MCP client
- Storing the Gitlab Token in the MCP server
Regarding the variant where the server stores the Gitlab Tokens:
but the issue is that would require that the server have its completely own oauth implementation, and not just perform DCR and delegate to gitlab
What about creating a second application in GitLab that has only the scope openid
and using this for the authentication between the MCP client and the MCP server and using the application with api
scope only for the authentication between the MCP server and GitLab?
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.
Another option that avoids storing the tokens in plain text is to use Secret Sharing where the token is split in two parts and one part it given to the client the other part is stored in the server. Only with both parts you can then reconstruct the secret (you could use XOR to do this securely).
But I'm not an crypto expert and this is getting quite homebrew already.
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.
yeah, its going to have to be a little home brew, my hope was to do as little as possible, and try to delegate as much as I could to gitlab
What about creating a second application in GitLab that has only the scope openid and using this for the authentication between the MCP client and the MCP server and using the application with api scope only for the authentication between the MCP server and GitLab?
yeah, so you could potentially authenticate and store this "Identity" token, and then as I said, generate an impersonation token per request based off the user.
the issue is that this introduces a vector where a bug could allow users to execute actions as other users - a path that this current implementation makes difficult to accidentally do
@elee1766 Can you fix conflict? I will test it. |
@zereight hi, i've just merged in main branch. lmk if you run into any problems. |
@iwakitakuma33 Any opinion? I think it can be merge without conflict. |
@elee1766 Hi, plz resolve conflicts, I will approve it. |
@zereight |
Fix list of tools in `README.md`
@zereight @iwakitakuma33 just merged main thank you both for the help getting this through. i hope that the PR will help future feature iteration as well. |
commit cce5d07 Author: zereight <[email protected]> Date: Fri Aug 15 18:31:25 2025 +0900 fix: update version to 2.0.0 in package.json and package-lock.json commit 841e3ab Author: a <[email protected]> Date: Fri Aug 15 04:27:49 2025 -0500 User based authentication methods (#130) * noot * noot * wip * wip * wip * noot * noot * noot * noot * noot * add some ai documentation * multiple transports * argon2 fix * npm i * change default token expiry * noot * fix build * remove unused env var * add some logging * add GITLAB_API_URL to the examples * argon2 warning * Fix list of tools * feat: Add NPM publish workflow for automated package publishing (#208) * single logger --------- Co-authored-by: Hunter Wittenborn <[email protected]> Co-authored-by: iwakitakuma <[email protected]> Co-authored-by: zereight <[email protected]> commit d5a652d Author: zereight <[email protected]> Date: Wed Aug 13 18:32:11 2025 +0900 fix: update version to 2.0.0-beta.0 and rename deploy scripts commit f25e149 Author: zereight <[email protected]> Date: Wed Aug 13 18:26:11 2025 +0900 fix: rename deploy:canary script to deploy:beta commit cf1e6d3 Author: zereight <[email protected]> Date: Wed Aug 13 18:22:15 2025 +0900 fix: update version to 2.0.0 and modify deploy:canary script commit 1751d1d Author: zereight <[email protected]> Date: Wed Aug 13 18:10:45 2025 +0900 feat: update version to 2.0.0-canary.0
@iwakitakuma33 we will rollback this at 2.0.3 About. #217 |
This PR is rollback at 2.0.3 (same as 1.0.77). I'm sorry. I will check about some issues. |
This PR completely refactors how gitlab authentication is done (along with configuration), removing a large amount of global variables.
for the sake of organization and to make it so that less things would be missed in migration, config has been moved to its own file.
all gitlab api requests are done through a gitlabsession class which is populated with authentication info for a specific request
this is required in order to support passing auth from the client to the server, instead of authorizing only at the server level.
i also moved the non-index files to src/ directory. so joyous day there isn't a horrendous 4300 line typescript file anymore. this should make it easier for llms to both work on and analyze the code :)
streaming http and sse endpoints can both be both hosted at the same time - this pr also allows this.
the PR introduces two authentication schemes:
pat-passthrough, which allows clients to send
gitlab-token
header with a PAT that will be used for all their requests to the MCP(experimental) oauth2, which uses a really hacky oauth2 proxy which fakes dynamic client registration in order to satisfy the MCP authorization standard, as gitlab themselves, or at least, my self hosted instances, does not seem to support dynamic client registration.
the oauth2 proxy uses sqlite and will only load/import the sqlite library if the oauth2 backend is used. this should make sure that users who do not need the oauth2 backend dont have issues with sqlite and the such.
by default, it uses an in memory sqlite database, but if you pass GITLAB_OAUTH2_DB_PATH then you can use a persistent sqlite db.
the advantage of this is that this allows the mcp server to be run and hosted at a single url by either the gitlab server operator or a trusted party, and developers no longer need to install the mcp server, and dont configure as many variables on their local clients.