auth: issuer mix-up mitigation (#859)
This PR functions as a Reference implementation of [SEP-2468](https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2468) / [RFC9207](https://datatracker.ietf.org/doc/rfc9207/). This PR hardens the MCP OAuth Client functionality against Mix-Up attacks: > Mix-up attacks aim to steal an authorization code or access token by > tricking the client into sending the authorization code or access > token to the attacker instead of the honest authorization or resource > server This PR hardens the client by adding support for a new `iss` parameter in authorization responses: - Authorization Servers broadcast support for the `iss` parameter via the `authorization_response_iss_parameter_supported` metadata parameter - If the parameter is supported, clients expect to receive the `iss` parameter in the authorization response - Clients compare the `iss` parameter in the authorization response to the `Issuer` parameter in the authorization metadata. The two must match exactly for the response to be processed. Fixes #941
This commit is contained in:
parent
4cbdd6a514
commit
2d47cc9664
2 changed files with 151 additions and 9 deletions
|
|
@ -50,6 +50,12 @@ type AuthorizationResult struct {
|
|||
Code string
|
||||
// State string returned by the authorization server.
|
||||
State string
|
||||
// Iss is the issuer identifier returned by the authorization server in the
|
||||
// authorization response per [RFC 9207]. The AuthorizationCodeFetcher should
|
||||
// populate this from the "iss" query parameter in the redirect URI if present.
|
||||
//
|
||||
// [RFC 9207]: https://www.rfc-editor.org/rfc/rfc9207
|
||||
Iss string
|
||||
}
|
||||
|
||||
// AuthorizationArgs is the input to [AuthorizationCodeFetcher].
|
||||
|
|
@ -318,6 +324,9 @@ func (h *AuthorizationCodeHandler) Authorize(ctx context.Context, req *http.Requ
|
|||
// Purposefully leaving the error unwrappable so it can be handled by the caller.
|
||||
return err
|
||||
}
|
||||
if err := validateIssuerResponse(authRes.Iss, asm.Issuer, asm.AuthorizationResponseIssParameterSupported); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
err = h.exchangeAuthorizationCode(ctx, cfg, authRes, prm.Resource)
|
||||
if err != nil {
|
||||
|
|
@ -560,6 +569,27 @@ func (h *AuthorizationCodeHandler) getAuthorizationCode(ctx context.Context, cfg
|
|||
}, nil
|
||||
}
|
||||
|
||||
// validateIssuerResponse validates the "iss" parameter in an authorization response
|
||||
// per [RFC 9207].
|
||||
//
|
||||
// [RFC 9207]: https://www.rfc-editor.org/rfc/rfc9207
|
||||
func validateIssuerResponse(iss, expectedIssuer string, issParameterSupported bool) error {
|
||||
if issParameterSupported {
|
||||
if iss == "" {
|
||||
return fmt.Errorf("authorization server advertises RFC 9207 iss parameter support but none was received in the authorization response")
|
||||
}
|
||||
if iss != expectedIssuer {
|
||||
return fmt.Errorf("authorization response issuer %q does not match expected issuer %q", iss, expectedIssuer)
|
||||
}
|
||||
} else {
|
||||
if iss != "" {
|
||||
return fmt.Errorf("authorization server does not advertise RFC 9207 iss parameter support but iss was received in the authorization response")
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// exchangeAuthorizationCode exchanges the authorization code for a token
|
||||
// and stores it in a token source.
|
||||
func (h *AuthorizationCodeHandler) exchangeAuthorizationCode(ctx context.Context, cfg *oauth2.Config, authResult *authResult, resourceURL string) error {
|
||||
|
|
|
|||
|
|
@ -78,6 +78,7 @@ func TestAuthorize(t *testing.T) {
|
|||
return &AuthorizationResult{
|
||||
Code: location.Query().Get("code"),
|
||||
State: location.Query().Get("state"),
|
||||
Iss: location.Query().Get("iss"),
|
||||
}, nil
|
||||
},
|
||||
})
|
||||
|
|
@ -176,6 +177,7 @@ func TestAuthorize_ScopeAccumulation(t *testing.T) {
|
|||
return &AuthorizationResult{
|
||||
Code: loc.Query().Get("code"),
|
||||
State: loc.Query().Get("state"),
|
||||
Iss: loc.Query().Get("iss"),
|
||||
}, nil
|
||||
},
|
||||
})
|
||||
|
|
@ -736,6 +738,59 @@ func TestDynamicRegistration(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
func TestValidateIssuerResponse(t *testing.T) {
|
||||
const expectedIssuer = "https://auth.example.com"
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
iss string
|
||||
issSupported bool
|
||||
wantErr bool
|
||||
wantErrContains string
|
||||
}{
|
||||
{
|
||||
name: "ValidIss",
|
||||
iss: expectedIssuer,
|
||||
issSupported: true,
|
||||
},
|
||||
{
|
||||
name: "WrongIss",
|
||||
iss: "https://attacker.example.com",
|
||||
issSupported: true,
|
||||
wantErr: true,
|
||||
wantErrContains: "does not match expected issuer",
|
||||
},
|
||||
{
|
||||
name: "MissingIssWhenRequired",
|
||||
iss: "",
|
||||
issSupported: true,
|
||||
wantErr: true,
|
||||
wantErrContains: "RFC 9207",
|
||||
},
|
||||
{
|
||||
name: "MissingIssWhenNotRequired",
|
||||
iss: "",
|
||||
issSupported: false,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
err := validateIssuerResponse(tt.iss, expectedIssuer, tt.issSupported)
|
||||
if tt.wantErr {
|
||||
if err == nil {
|
||||
t.Fatalf("validateIssuerResponse() = nil, want error containing %q", tt.wantErrContains)
|
||||
}
|
||||
if !strings.Contains(err.Error(), tt.wantErrContains) {
|
||||
t.Errorf("validateIssuerResponse() error = %q, want it to contain %q", err.Error(), tt.wantErrContains)
|
||||
}
|
||||
} else if err != nil {
|
||||
t.Fatalf("validateIssuerResponse() unexpected error = %v", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestInferApplicationType(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
|
|
|||
Loading…
Reference in a new issue