Skip to content
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

[proposal] Add auth as a capability with support for OAuth 2.0 and secrets. #101

Closed
wants to merge 8 commits into from

Conversation

k6l3
Copy link

@k6l3 k6l3 commented Dec 6, 2024

Motivation and Context

We need functionality that allows MCP clients to connect to remote MCP servers.

This change proposes:

  • Adding an auth capability that can be extended over time
  • Initial support for credential and oauth2 options.
    • credential support allows the server to tell the client what API keys, tokens, and secrets it may need to operate
    • oauth2 support allows the server to identify the client with OAuth 2.0. The process of obtaining OAuth 2.0 tokens also allows the server to ask the user to configure more complex flows, such as authenticating to other applications or managing secrets on another platform.

How Has This Been Tested?

This proposal has not yet been implemented in a client/server.

Breaking Changes

Any servers that require auth will error with older client versions that do not provide auth capabilities.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Protocol proposal

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Additional context

@jspahrsummers jspahrsummers added this to the DRAFT: 202X-XX-XX milestone Dec 6, 2024
Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for writing this up! Directionally, I think this is excellent. I have mostly in-the-weeds comments and thoughts—happy to discuss or be pushed back on any of them.

I also think we should support server -> client notifications for things like "token expired." Notifications like these are very optional, but it's helpful to put them in the spec, so implementors know how to structure them if they want to support it. We typically:

  1. Add a capability that indicates whether such notifications will or will not be sent at all
  2. Allow both servers and clients to use such notifications entirely optionally (i.e., it is not the only means to do something)

docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
Co-authored-by: Justin Spahr-Summers <[email protected]>
@k6l3 k6l3 requested a review from jspahrsummers December 9, 2024 01:26
@k6l3 k6l3 force-pushed the main branch 2 times, most recently from 8573bb9 to 2dd5a60 Compare December 9, 2024 01:29
@k6l3
Copy link
Author

k6l3 commented Dec 9, 2024

@jspahrsummers I reorganized this, I ended up not liking how Claude formatted things. Take a look at your leisure.

I did not add notifications to the spec yet, I'd like that to be a follow-up change because I don't feel that it blocks any critical behavior. Clients should be able to tell when tokens expire from the metadata in an access token, and token revocation is infrequent.

jspahrsummers
jspahrsummers previously approved these changes Dec 9, 2024
Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coolkid

Awesome, I think this is in a great state, modulo just a couple of things to change about the wire format (most notably the use of _meta). We can always continue to iterate now that we have our disclaimer about breaking changes within auth.

Tagging in @dsp-ant for required second review too.

docs/specification/auth/credential.md Outdated Show resolved Hide resolved
docs/specification/auth/credential.md Outdated Show resolved Hide resolved
docs/specification/auth/credential.md Outdated Show resolved Hide resolved
Comment on lines 117 to 121
"error": "ASCII error code", // REQUIRED
"errors": { // RECOMMENDED
// Breakdown of error per-credential
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this example a little hard to interpret. We don't have to be overly specific for now, since we can evolve this bit, but I'm curious what you have in mind here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's flesh this out in a followup then, since I don't have much on my mind here yet. Maybe something like this?

...
      "authRequest": {
        "credentials": {
          "error": "auth_error", 
          "errors": { // RECOMMENDED
            "API-KEY": "Missing",
            "MISC-PASSWORD": "Invalid"
          }
        }
...

docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved

### Utilizing an Access Token

Once a client has obtained an access token, it **SHOULD** include it in all requests in the parameters, including the initalization request.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a technical reason why we need to include it in all requests? (Plausibly yes, just want to check.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I envisioned that the server would present different behaviors (more capabilities, different tool functionality, etc.) if the client was authenticated. For ease of implementation, I wrote that the client should (but is not required to) include it in all requests.

docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
docs/specification/auth/oauth2.md Outdated Show resolved Hide resolved
Co-authored-by: Justin Spahr-Summers <[email protected]>
jspahrsummers
jspahrsummers previously approved these changes Dec 10, 2024
"credentials": {
"API-KEY": "api_key",
"MISC-PASSWORD": "password"
"_meta": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I meant using the _meta field outside of initialization only. 😅

Rationale:

  1. It's an essential part of initialization semantics
  2. It's an out of band addition to every other request type

I think just params.auth is fine during init.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think I would prefer consistency here. Providing it as part of _meta in initialization would be the same as making a request with headers. If the server finds valid credentials, it can present a larger suite of capabilities?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is that the initialize request is basically all headers (by analogy).

@k6l3
Copy link
Author

k6l3 commented Dec 10, 2024

@dsp-ant Blocking on your review!

Copy link
Member

@dsp-ant dsp-ant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. This is quite exciting and I think the direction is great. Note that my review here will be very detail oriented and full of bikeshedding. Please don't get discouraged by this. Since we are working on a specification, being very clear, and erroring on the side of a few turnarounds to get it perfect is essential for longterm success. Once it's in, it's hard to change. I expect this to take quite a few iterations before we are all happy.

I think we need a better way of organising the spec around the new DRAFT version. Given auth is a big change, I would rather wait before merging and getting the infrastructure in place to manage the existing version and the draft.

There are fundamentally two ways to go about this (I'll start a discussion):

  • Make a copy of the spec into a draft and deploy this.
  • Use branches and find a way to correctly handle docs.

docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/_index.md Outdated Show resolved Hide resolved
docs/specification/auth/_index.md Outdated Show resolved Hide resolved
@jspahrsummers
Copy link
Member

I think we need a better way of organising the spec around the new DRAFT version. Given auth is a big change, I would rather wait before merging and getting the infrastructure in place to manage the existing version and the draft.

There are fundamentally two ways to go about this (I'll start a discussion):

  • Make a copy of the spec into a draft and deploy this.
  • Use branches and find a way to correctly handle docs.

Do the advisories on these doc pages not address that? The changes here are all backwards compatible, the only risk is that auth itself might iterate through backwards incompatible changes.

I think we should unblock this ASAP though.

@k6l3
Copy link
Author

k6l3 commented Dec 12, 2024

Note that my review here will be very detail oriented and full of bikeshedding. Please don't get discouraged by this. Since we are working on a specification, being very clear, and erroring on the side of a few turnarounds to get it perfect is essential for longterm success.

Not a problem; I appreciate it and don't think you have any reason to think it would be discouraging. It's rather encouraging.

@k6l3
Copy link
Author

k6l3 commented Dec 12, 2024

Do the advisories on these doc pages not address that? The changes here are all backwards compatible, the only risk is that auth itself might iterate through backwards incompatible changes.

I mostly agree here, since this is called out as experimental and doesn't change any other parts of the protocol, I'm not too worried.

I can see the case for a draft version for changes that would make it into a major version change, but this could be a minor version.

Copy link
Member

@jspahrsummers jspahrsummers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the "separating out a draft version" and "define OAuth" changes should be in separate PRs, but I don't want to hold this up longer unnecessarily. I'm fine with this change if @dsp-ant is.


### Utilizing an Access Token

Once a client has obtained an access token, it **SHOULD** include it in all requests in the parameters, including the initalization request.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would there be consideration for supporting standards around Making Authenticated Requests with OAuth to make this part of the transport layer?

I noticed that here in the PR and in the discussion thread there has not been mention/exploration of using authentication built into the transport layer. I can see the rationale for the current proposal given:

  • I believe jsonrpc doesn't define how auth works, and stdio doesn't have a side channel information like HTTP request headers
  • Assumption that MCP servers are an OAuth client for some third party service

As a result, the implication here is that the MCP server effectively must act like an OAuth client for some third party service, even if that third party is themselves. An existing service adding support for MCP you can't use it's existing authentication stack provided at the transport layer. Instead, all MCP requests are unauthenticated at the HTTP level, and invisible to the web app framework.

To give a concrete example: We have implemented an MCP server in Home Assistant and reusing the existing authentication stack, already built, would go a long way. I suspect this would help with MCP server adoption for well established systems. I don't have an answer for how to make this easier to support for stdio, but IMO the complexity should be pushed to stdio servers.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I tend to think we should lean into the OAuth2 HTTP-based standard (using the Authorization header for the token), and then just specify how to pass the token in the stdio transport (e.g. via the _meta.auth.oauth2 object). If we're supporting OAuth2, we might as well get all the benefits of conforming to the standard.

MCP client SDKs can help make it easier to reason about how OAuth is handled for other transports.

Copy link

@jaredhanson jaredhanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's an opportunity here to leverage existing OAuth 2.0 authorization servers, and their redirect-based authorization endpoints. This simplifies the changes needed to MCP, by advertising URL endpoints directly in capabilities, and removing the need to add OAuth 2.0 specific JSON-RPC methods to MCP.

See further comments in-line.

"state": "state", // RECOMMENDED
}
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why the authorization request is being sent "in-band" on the JSON-RPC channel, especially given that the response is an authorization_url that the client redirects the user to (presumably by opening up a browser).

I'd suggest that it would be more in-line with OAuth 2.0's model to advertise the endpoints in the capabilities:

{
   "capabilities": {
     "auth": {
       "oauth2": {
         "authorize": { "url: "https://mcp.example.com/authorize" },
         "token": { "url: "https://mcp.example.com/token" },
         "revoke": { "url: "https://mcp.example.com/revoke" }
       }
     }
   }
 }

Given these capabilities, the client would request authorization by redirecting directly to the /authorize URL in a browser with the necessary params:

HTTP /authorize?response_type=code&...
Host: mcp.example.com

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optionally, you may specify only the Authorization Server Metadata url (defined by RFC 8414):

{
   "capabilities": {
     "auth": {
       "oauth2": {
         "metadata": { "url: "https://mcp.example.com/.well-known/oauth-authorization-server" }
       }
     }
   }
 }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. This makes sense to me, as it's valuable to support Auth Servers that are not the same as the Resource Servers.

To utilize the learnings from RFC 8414 but avoid an additional roundtrip, maybe we could use the RFC 8414 shape?

{
  "capabilities": {
    "auth": {
      "oauth2": {
        "metadata": {
          "issuer": "https://server.example.com",
          "authorization_endpoint": "https://server.example.com/authorize",
          "token_endpoint": "https://server.example.com/token",
          "response_types_supported": ["code"],
          "grant_types_supported": ["authorization_code"],
          "token_endpoint_auth_methods_supported": ["client_secret_basic"]
        }
      }
    }
  }
}

- The client **MUST** redirect the user to the authorization URL.
- The authorization URL **SHOULD** prompt the user to configure any credentials as needed. Once complete, the server provides an authorization grant in the form of a code.
- The client **MUST** implement a way to receive the authorization grant. This can be through a callback such as the redirect URI, providing some interface for the user to provide it, etc.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the above suggested change to redirect directly to /authorize, it is still the client's responsibility to receive the authorization grant (ie, the authorization code). Now, however, there's no need for the intermediary JSON-RPC request.

"state": "state"
}
}
```

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd similarly suggest that the client could make a call directly to the advertised /token endpoint, rather using a JSON-RPC method.

Note that this isn't avoiding some unnecessary intermediate request (as with the suggestion for /authorize), since there's no redirect. Its a single request either way, either via HTTP or JSON-RPC. Using the HTTP request directly allows the use of existing deployed OAuth 2.0 authorization servers.

Copy link

@nick-merrill nick-merrill Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. If we support OAuth2 fully, then that would mean tying ourselves to HTTP. That's fine (and probably desirable) when using any HTTP transport (like SSE) but problematic when using a non-HTTP transport like stdio. However, we could just note that as a limitation of the stdio transport.

stdio and OAuth2 would still work pretty well as long as the Auth Server is hosted independently. For stdio, I think we would still want to use the _meta.auth.oauth2 object to provide the access token on requests to the MCP server.

Comment on lines +78 to +81
"method": "auth/oauth2/authorize",
"params": {
"response_type": "code", // REQUIRED
"client_id": "client_id", // REQUIRED

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the MCP Client obtain the client_id value? And, what type of OAuth2 client is the MCP Client?

It seems like there are two possible approaches:

  1. Treat the MCP Client as a public client and use the Authorization Code Flow with PKCE. However, you still need to clarify how the client_id is assigned or obtained in this case.
  2. Treat the MCP Client as a third-party client, where clients are created through one of the following methods:
    2.1. Using the OAuth 2.0 Dynamic Client Registration Protocol (disadvantage: not many Authorization Servers seem to support this).
    2.2. Leveraging a developer portal provided by the Authorization Server.

@jspahrsummers
Copy link
Member

Replaced by #133.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants