Enterprise managed authorization#770
Enterprise managed authorization#770radar07 wants to merge 33 commits intomodelcontextprotocol:mainfrom
Conversation
|
Hi @radar07, thanks for submitting this PR. Could you link the issue that it is addressing? Also, as a heads-up: it will likely take some time to review your proposal. Both because it's quite large, but more importantly I'm also working on a proposal how to structure the client-side OAuth implementation and this change will need to be aligned with it. |
|
Thanks @maciej-kisiel. I updated the description with the SEP that this PR solves. |
|
@maciej-kisiel I'd be happy to contribute to OAuth implementation. Let me know if I can help with anything. Just want to know if I should add conformance tests to this because I can see that there are PRs related to conformance tests. |
maciej-kisiel
left a comment
There was a problem hiding this comment.
I took a brief look at your PR and I believe we could utilize the oauth2 library more aggressively.
Please also take a look at my PR/proposal for client-side OAuth at #785. I think this PR could be expressed with the proposed abstractions, but your OAuth expertise would make the feedback valuable.
|
Hi @radar07! I have recently merged #785 which I wrote about a few comments above. I think it's a good time to see if you could fit your desired auth flow into the I would also ask you to implement this handler and put all the related helper files in a new sub-package of the If there is any logic from Thanks for working on this project! |
b3cba20 to
f94e462
Compare
|
@maciej-kisiel Thanks for the feedback. I made the changes as you suggested. Could you please take a look again? |
maciej-kisiel
left a comment
There was a problem hiding this comment.
I didn't manage to look at all files, I'll continue tomorrow.
In general I still believe we can use the oauth2 package more to reduce the amount of code to maintain. This should be an implementation detail, so we can always go back to custom logic if oauth2 stops being sufficient.
I think I would also simplify the API for OIDC login, to be similar to AuthorizationCodeHandler. Consistency would be an additional plus.
maciej-kisiel
left a comment
There was a problem hiding this comment.
Few more comments. I looked at all Go files, excluding the tests. I will take a look at them when we align on the main logic.
- Adds the Token Exchange (RFC 8693) for Enterprise-Managed Authorization
# Conflicts: # oauthex/oauth2.go
24931e7 to
a4fd173
Compare
maciej-kisiel
left a comment
There was a problem hiding this comment.
Oops, I forgot to publish the comments yesterday.
oauthex/token_exchange.go
Outdated
| ClientSecret: clientSecret, | ||
| Endpoint: oauth2.Endpoint{ | ||
| TokenURL: tokenEndpoint, | ||
| AuthStyle: oauth2.AuthStyleInParams, // Use POST body auth per SEP-990 |
There was a problem hiding this comment.
Could you point me where this is defined? I failed to find it.
There was a problem hiding this comment.
I changed it to ClientCredentials struct (if clientSecret is what you are asking 🤔 )
There was a problem hiding this comment.
Sorry for not being precise. I was trying to find where in SEP-990 is it defined to use client_secret_post and failed. Your comment claims it should be there?
oauthex/token_exchange.go
Outdated
| clientID string, | ||
| clientSecret string, |
There was a problem hiding this comment.
Are there any other client authentication methods that could be used for token exchange? I think we should be more flexible here and define a struct that could be extended with JWT-based methods in the future.
There was a problem hiding this comment.
Used a struct for client credentials to support oauth2.Config.Exchange(). Should we keep this as it is or implement support for JWT methods?
There was a problem hiding this comment.
We don't need to implement the support for JWT yet, as no-one has requested it. But I would like to have a clear idea how it would be implemented if needed. For example, I had similar concerns in
go-sdk/auth/authorization_code.go
Line 48 in ea16195
And I ended up creating a struct hierarchy that is not very rich now, but can be extended. Maybe we should follow a similar example here? I assume the secret may not always be needed, e.g. in case of JWT I assume it's the key that matters instead?
|
@maciej-kisiel Thanks for being patient and taking the time to discuss and come up with the plan. I really appreciate it!
I made the changes as suggested and with the separate examples directory for this flow. Please go through and let me what you think. |
oauthex/token_exchange.go
Outdated
| clientID string, | ||
| clientSecret string, |
There was a problem hiding this comment.
We don't need to implement the support for JWT yet, as no-one has requested it. But I would like to have a clear idea how it would be implemented if needed. For example, I had similar concerns in
go-sdk/auth/authorization_code.go
Line 48 in ea16195
And I ended up creating a struct hierarchy that is not very rich now, but can be extended. Maybe we should follow a similar example here? I assume the secret may not always be needed, e.g. in case of JWT I assume it's the key that matters instead?
oauthex/token_exchange.go
Outdated
| ClientSecret: clientSecret, | ||
| Endpoint: oauth2.Endpoint{ | ||
| TokenURL: tokenEndpoint, | ||
| AuthStyle: oauth2.AuthStyleInParams, // Use POST body auth per SEP-990 |
There was a problem hiding this comment.
Sorry for not being precise. I was trying to find where in SEP-990 is it defined to use client_secret_post and failed. Your comment claims it should be there?
You're right! I couldn't find the
Yes, the key matters in JWT. I agree with your point. We could extend the |
maciej-kisiel
left a comment
There was a problem hiding this comment.
Thanks for the update!
I believe we're almost there from the architecture and API point of view. I have added my final recommendations for the last two bigger topics: token return types and the client credentials struct schema.
Given that we're close, I also reviewed the remaining files, including tests and docs. I believe we have a fair chance of merging this PR this week, so that it would make the v1.5.0 release of the SDK.
| // in a browser) and returning the authorization code and state once the Authorization | ||
| // Server redirects back to the configured RedirectURL. | ||
| // | ||
| // The implementation MUST verify that the returned state matches the state |
There was a problem hiding this comment.
The SDK is doing state verification in all current flows. Maybe requiring this from the fetcher is not needed? I would remove these two lines of the comment.
| } | ||
|
|
||
| asm, err := h.getAuthServerMetadata(ctx, prm) | ||
| asm, err := GetAuthServerMetadata(ctx, prm.AuthorizationServers[0], h.config.Client) |
There was a problem hiding this comment.
To preserve current behavior I think the helper needs to return nil, nil when none of the endpoints responded with a non-4xx error (return statement at the end of the function). In the current version, the fallback will kick in on every error from GetAuthServerMetadata and that's not something we want, e.g. if the fetched asm has insecure links we want to propagate the error.
asm, err := h.getAuthServerMetadata(ctx, prm)
if err != nil {
return err
}
if asm == nil {
// fallback...
}Note that you will need to add nil checks at other call sites as well.
| @@ -439,15 +439,11 @@ func TestGetProtectedResourceMetadata_Error(t *testing.T) { | |||
| } | |||
|
|
|||
| func TestGetAuthServerMetadata(t *testing.T) { | |||
There was a problem hiding this comment.
Can we move this test case to shared_test.go? The test should probably not reference "fallback", since it's no longer part of the logic of GetAuthServerMetadata.
| AuthorizationServers: []string{s.URL()}, | ||
| }) | ||
| asm, err := GetAuthServerMetadata(t.Context(), s.URL(), http.DefaultClient) | ||
| if err != nil { |
There was a problem hiding this comment.
Does this test fail without the fallback? If not, I would remove it from here, since it shouldn't be relevant to the logic under test.
| asm, err := GetAuthServerMetadata(t.Context(), s.URL(), http.DefaultClient) | ||
| if err != nil { | ||
| t.Fatalf("getAuthServerMetadata() error = %v, want nil", err) | ||
| // Fallback to predefined endpoints for testing |
There was a problem hiding this comment.
Does this test fail without the fallback? If not, I would remove it from here, since it shouldn't be relevant to the logic under test.
oauthex/token_exchange.go
Outdated
| ) | ||
|
|
||
| // ClientCredentials holds client authentication credentials for OAuth token requests. | ||
| type ClientCredentials struct { |
There was a problem hiding this comment.
I've done some thinking and I believe we should follow the PreregisteredClientConfig example here. Consider:
type ClientCredentials struct {
ClientID string
ClientSecretAuth *ClientSecretAuth
}
type ClientSecretAuth struct {
ClientSecret string
}In the future we can add more *Auth fields to the struct, e.g. PrivateKeyJWTAuth. Let's also move it to a non-token exchange specific file, e.g. client.go. I would also propose to introduce a Validate method for the struct to check if only one *Auth field is set. client_test.go should then contain a test that validates the Validate method was updated when new field was added.
Once we have it, I would replace PreregisteredClientConfig with oauthex.ClientCredentials in AuthorizationCodeConfig.
auth/extauth/oidc_login.go
Outdated
| } | ||
|
|
||
| // OIDCTokenResponse contains the tokens returned from a successful OIDC login. | ||
| type OIDCTokenResponse struct { |
There was a problem hiding this comment.
Regarding the return type discussion we've had: after some thinking I believe we should use oauth2.Token to represent the output from OIDC login and token exchange.
If you don't like accessing the important fields via Extra(), you can consider embedding the oauth2.Token into the existing response types instead of repeating all the fields. The extra fields could be provided with an accessor method, e.g. oidcResp.IDToken() accessing Extra underneath, but personally I don't think we need it if we document the extra fields explicitly.
| } | ||
|
|
||
| // createMockOIDCServerWithToken creates a mock OIDC server that also handles token exchange. | ||
| func createMockOIDCServerWithToken(t *testing.T) *httptest.Server { |
There was a problem hiding this comment.
Could this server be used anywhere createMockOIDCServer is used? It seems to be a strict superset, so maybe we can remove the simpler one and the more advanced one everywhere. In that case, I would propose for the advanced server to be renamed to createMockOIDCServer since it would be the only one.
| } | ||
|
|
||
| // TestPerformOIDCLogin tests the combined OIDC login flow with callback. | ||
| func TestPerformOIDCLogin(t *testing.T) { |
There was a problem hiding this comment.
Generally it's recommended to test the public API. Do you think it would be possible to move the tests from TestInitiateOIDCLogin here?
TestCompleteOIDCLogin probably is not needed, since we own the logic that should ensure these values tested there are passed correctly.
| } | ||
|
|
||
| // TestOIDCLoginE2E tests the complete OIDC login flow end-to-end. | ||
| func TestOIDCLoginE2E(t *testing.T) { |
There was a problem hiding this comment.
If this is not different from the successful flow tested in TestPerformOIDCLogin, then I would remove it.
cc3ca13 to
4f57d93
Compare
4f57d93 to
6d86344
Compare
| } | ||
|
|
||
| hasOpenID := false | ||
| for _, scope := range config.Scopes { |
There was a problem hiding this comment.
auth,oauthex: implement Enterprise Managed Authorization (SEP-990)This PR implements Enterprise Managed Authorization (SEP-990) for the Go MCP SDK, enabling MCP Clients and Servers to leverage enterprise Identity Providers for seamless authorization without requiring users to authenticate separately to each MCP Server.
Overview
Enterprise Managed Authorization follows the Identity Assertion Authorization Grant specification (draft-ietf-oauth-identity-assertion-authz-grant), implementing a three-step flow:
This enables:
Closes: #628