Enterprise managed authorization#770
Enterprise managed authorization#770radar07 wants to merge 14 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.
oauthex/jwt_bearer.go
Outdated
|
|
||
| // JWTBearerResponse represents the response from a JWT Bearer grant request | ||
| // per RFC 7523. This uses the standard OAuth 2.0 token response format. | ||
| type JWTBearerResponse struct { |
There was a problem hiding this comment.
Could https://pkg.go.dev/golang.org/x/oauth2#Token be used for this?
oauthex/jwt_bearer.go
Outdated
| } | ||
|
|
||
| // JWTBearerError represents an error response from a JWT Bearer grant request. | ||
| type JWTBearerError struct { |
There was a problem hiding this comment.
Could https://pkg.go.dev/golang.org/x/oauth2#RetrieveError be used for this?
There was a problem hiding this comment.
Yes, changed to oauth2.RetrieveError.
| // "mcp-client-secret", | ||
| // nil, | ||
| // ) | ||
| func ExchangeJWTBearer( |
There was a problem hiding this comment.
It feels like https://pkg.go.dev/golang.org/x/oauth2#Config.Exchange could be used to replicate the behavior of the function. Is there any reason not to use it?
There was a problem hiding this comment.
oauth2#Config.Exchange always expect a code which we don't have in our Token Ecxhange. IdPs can reject the request if we don't pass code or pass it as an empty string.
There was a problem hiding this comment.
IdPs can reject the request if we don't pass
codeor pass it as an empty string.
I don't think that should happen. Based on RFC 6749, Section 3.2:
Parameters sent without a value MUST be treated as if they were
omitted from the request. The authorization server MUST ignore
unrecognized request parameters.
There was a problem hiding this comment.
I did a sample test and you're right. Changed to use Config.Exchange
| // } | ||
| // | ||
| // resp, err := ExchangeToken(ctx, idpTokenEndpoint, req, clientID, clientSecret, nil) | ||
| func ExchangeToken( |
There was a problem hiding this comment.
It looks like https://pkg.go.dev/golang.org/x/oauth2#Config.Exchange can be used for Token Exchange as well? golang/oauth2#409
There was a problem hiding this comment.
You're right. But it is the same comment as above.
There was a problem hiding this comment.
Also, the Token struct don't have issued_token_type which is required to indicate the type of the token.
There was a problem hiding this comment.
Also, the
Tokenstruct don't haveissued_token_typewhich is required to indicate the type of the token.
I believe it could be accessible via Token.Extra, no?
There was a problem hiding this comment.
Yes. Extracted issued_token_type from Token.Extra here.
|
|
||
| // TokenExchangeResponse represents the response from a token exchange request | ||
| // per RFC 8693 Section 2.2. | ||
| type TokenExchangeResponse struct { |
There was a problem hiding this comment.
Same comments as in oauthex/jwt_bearer.go apply to the request and error types.
|
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.
| type IDTokenFetcher func(ctx context.Context) (string, error) | ||
|
|
||
| // EnterpriseHandlerConfig is the configuration for [EnterpriseHandler]. | ||
| type EnterpriseHandlerConfig struct { |
There was a problem hiding this comment.
Could we document which fields are required and which are optional in their doc comments?
auth/extauth/enterprise_handler.go
Outdated
| defer func() { | ||
| if resp != nil && resp.Body != nil { | ||
| io.Copy(io.Discard, resp.Body) | ||
| resp.Body.Close() | ||
| } | ||
| }() |
There was a problem hiding this comment.
| defer func() { | |
| if resp != nil && resp.Body != nil { | |
| io.Copy(io.Discard, resp.Body) | |
| resp.Body.Close() | |
| } | |
| }() | |
| defer resp.Body.Close() | |
| defer io.Copy(io.Discard, resp.Body) |
I think we can assume that the response is correctly passed. Good idea with draining Body, could you also apply it in authorization_code.go?
| // } | ||
| // | ||
| // // Use accessToken to call MCP Server APIs | ||
| func EnterpriseAuthFlow( |
There was a problem hiding this comment.
Do we still need this if we have the EnterpriseHandler?
There was a problem hiding this comment.
This uses OAuthHandler and allows users to use the Enterprise Auth Flow without the MCP imports. However, if we prefer keeping only the handler, we can remove this.
auth/enterprise_auth.go
Outdated
|
|
||
| // GetAuthServerMetadatForIssuer fetches authorization server metadata for the given issuer URL. | ||
| // It tries standard well-known endpoints (OAuth 2.0 and OIDC) and returns the first successful result. | ||
| func GetAuthServerMetadatForIssuer(ctx context.Context, IssuerURL string, httpClient *httpClient) (*oauthex.AuthServerMeta, error) { |
There was a problem hiding this comment.
It would be good to align it with authorization_code.go's getAuthServerMetadata. It can be generalized to not be a method on AuthorizationCodeHandler, but receive the URL as parameter instead of PRM and the client as a parameter instead of using the receiver. It should probably be moved to a different file, maybe something like shared.go?
There was a problem hiding this comment.
Moved the code to shared.go (which can be renamed if needed). Also, the getAuthServerMetadata method has a different fallback behavior. I want to know your thoughts.
There was a problem hiding this comment.
Please move this file inside extauth/, it is Enterprise flow specific.
auth/oidc_login.go
Outdated
|
|
||
| // generateCodeChallenge generates the PKCE code challenge from the verifier | ||
| // using SHA256 per RFC 7636 Section 4.2. | ||
| func generateCodeChallenge(verifier string) string { |
There was a problem hiding this comment.
auth/oidc_login.go
Outdated
|
|
||
| // generateState generates a cryptographically random state parameter | ||
| // for CSRF protection per RFC 6749 Section 10.12. | ||
| func generateState() (string, error) { |
There was a problem hiding this comment.
Could https://pkg.go.dev/crypto/rand#Text be used here?
| ExpiresAt int64 | ||
| } | ||
|
|
||
| // InitiateOIDCLogin initiates an OIDC Authorization Code flow with PKCE. |
There was a problem hiding this comment.
Do you see a situation occurring of InitiateOICDLogin not being used with CompleteOIDCLogin? If not, maybe we could be more opinionated and implement the whole flow as a single function that just takes the "direct user to authorization URL and return the authorization result" as a functional argument, similar to how AuthorizationCodeHandlerConfig takes AuthorizationCodeFetcher?
In general this flow looks very similar to the normal authorization code flow, so maybe we could even reuse some of the data structures? Do you see a risk of the OAuth flow and the OIDC flow diverging in the future?
auth/oidc_login.go
Outdated
| } | ||
|
|
||
| // buildAuthorizationURL constructs the OIDC authorization URL. | ||
| func buildAuthorizationURL( |
There was a problem hiding this comment.
Have you considered reusing https://godoc.corp.google.com/pkg/google3/third_party/golang/oauth2/oauth2#Config.AuthCodeURL? I believe it supports all that's needed here.
auth/oidc_login.go
Outdated
| } | ||
|
|
||
| // exchangeAuthorizationCode exchanges the authorization code for tokens. | ||
| func exchangeAuthorizationCode( |
There was a problem hiding this comment.
I think https://godoc.corp.google.com/pkg/google3/third_party/golang/oauth2/oauth2#Config.Exchange could be used here and if you would follow the suggestion to make the whole OIDC a single function, you could even reuse the oauth2.Config.
There was a problem hiding this comment.
I see what you mean. Good call.
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.
oauthex/id_jag.go
Outdated
| // } | ||
| // fmt.Printf("Subject: %s\n", claims.Subject) | ||
| // fmt.Printf("Expires: %v\n", claims.Expiry()) | ||
| func ParseIDJAG(jwt string) (*IDJAGClaims, error) { |
There was a problem hiding this comment.
Maybe we can use the golang-jwt/jwt library as an implementation detail for parsing to not replicate the logic?
There was a problem hiding this comment.
Also, is this function used anywhere?
There was a problem hiding this comment.
I added this in case if any one wants to verify the claims of the JWT. But, I believe it is not needed for this. Please let me know what you think.
| // "mcp-client-secret", | ||
| // nil, | ||
| // ) | ||
| func ExchangeJWTBearer( |
There was a problem hiding this comment.
IdPs can reject the request if we don't pass
codeor pass it as an empty string.
I don't think that should happen. Based on RFC 6749, Section 3.2:
Parameters sent without a value MUST be treated as if they were
omitted from the request. The authorization server MUST ignore
unrecognized request parameters.
| // } | ||
| // | ||
| // resp, err := ExchangeToken(ctx, idpTokenEndpoint, req, clientID, clientSecret, nil) | ||
| func ExchangeToken( |
There was a problem hiding this comment.
Also, the
Tokenstruct don't haveissued_token_typewhich is required to indicate the type of the token.
I believe it could be accessible via Token.Extra, no?
- Adds the Token Exchange (RFC 8693) for Enterprise-Managed Authorization
# Conflicts: # oauthex/oauth2.go
24931e7 to
a4fd173
Compare
a4fd173 to
d5b2a5d
Compare
|
@maciej-kisiel Thank you so much for taking the time to review this PR and your valuable feedback. I have made the changes as you suggested and need your suggestions on the changes. |
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