auth: validate unadvertised RFC 9207 iss instead of rejecting it (#1187)
## Summary Fixes #1152. Sentry, Linear, and Notion omit `authorization_response_iss_parameter_supported` from their authorization-server metadata. Sentry still sends a correct `iss` in the authorization response, so sign-in fails after the browser step: the user has already consented and an authorization code has been issued, but `validateIssuerResponse` rejects the exchange solely because the server did not advertise RFC 9207 support. RFC 9207 §2.4 says clients MUST extract and compare `iss` when the parameter is present. §2.4 also notes that clients SHOULD discard an unadvertised `iss`, but that is a recommendation and local policy may accept servers that send `iss` without advertising. Validating a matching `iss` is interoperable with those hosted MCP servers and still catches mix-up attacks: a wrong `iss` is rejected whether or not support was advertised. When `iss` is present, this change always compares it to the expected issuer (simple string equality). A match is accepted even if the server did not advertise support. A mismatch still returns `authorization response issuer %q does not match expected issuer %q`. Absence of `iss` remains an error only when the server advertised support. ## Test plan - [x] `go test ./auth/ -run TestValidateIssuerResponse -count=1` - [x] `go test ./auth/...` New `TestValidateIssuerResponse` cases: - `UnadvertisedIssCorrect`: `iss == expectedIssuer`, `issSupported == false` → no error - `UnadvertisedIssWrong`: `iss == "https://attacker.example.com"`, `issSupported == false` → error containing `does not match expected issuer` Existing advertised+match / advertised+mismatch / missing-iss cases are unchanged. --------- Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
This commit is contained in:
parent
810e6cb751
commit
32404652e6
2 changed files with 173 additions and 16 deletions
|
|
@ -124,6 +124,11 @@ type AuthorizationCodeHandlerConfig struct {
|
|||
// See https://modelcontextprotocol.io/seps/2207-oidc-refresh-token-guidance.
|
||||
RequestRefreshToken bool
|
||||
|
||||
// AcceptUnadvertisedIss accepts a matching RFC 9207 iss even when the
|
||||
// authorization server metadata omits authorization_response_iss_parameter_supported.
|
||||
// The zero value (false) keeps the historical reject-unadvertised-iss behavior.
|
||||
AcceptUnadvertisedIss bool
|
||||
|
||||
// Client is an optional HTTP client to use for HTTP requests.
|
||||
// It is used for the following requests:
|
||||
// - Fetching Protected Resource Metadata
|
||||
|
|
@ -367,7 +372,7 @@ 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 {
|
||||
if err := validateIssuerResponse(authRes.Iss, asm.Issuer, asm.AuthorizationResponseIssParameterSupported, h.config.AcceptUnadvertisedIss); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
|
|
@ -616,10 +621,14 @@ func (h *AuthorizationCodeHandler) getAuthorizationCode(ctx context.Context, cfg
|
|||
}
|
||||
|
||||
// validateIssuerResponse validates the "iss" parameter in an authorization response
|
||||
// per [RFC 9207].
|
||||
// per [RFC 9207]. When the server advertises authorization_response_iss_parameter_supported,
|
||||
// iss is required and must match expectedIssuer. When it does not advertise support,
|
||||
// an empty iss is accepted; a present iss is compared to expectedIssuer (RFC 9207 §2.4)
|
||||
// and a mismatch is always rejected. A matching unadvertised iss is accepted only when
|
||||
// acceptUnadvertisedIss is true (local policy); the default is the historical reject.
|
||||
//
|
||||
// [RFC 9207]: https://www.rfc-editor.org/rfc/rfc9207
|
||||
func validateIssuerResponse(iss, expectedIssuer string, issParameterSupported bool) error {
|
||||
func validateIssuerResponse(iss, expectedIssuer string, issParameterSupported, acceptUnadvertisedIss 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")
|
||||
|
|
@ -627,12 +636,17 @@ func validateIssuerResponse(iss, expectedIssuer string, issParameterSupported bo
|
|||
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
|
||||
}
|
||||
if iss == "" {
|
||||
return nil
|
||||
}
|
||||
if iss != expectedIssuer {
|
||||
return fmt.Errorf("authorization response issuer %q does not match expected issuer %q", iss, expectedIssuer)
|
||||
}
|
||||
if !acceptUnadvertisedIss {
|
||||
return fmt.Errorf("authorization server does not advertise RFC 9207 iss parameter support but iss was received in the authorization response")
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -919,11 +919,12 @@ func TestValidateIssuerResponse(t *testing.T) {
|
|||
const expectedIssuer = "https://auth.example.com"
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
iss string
|
||||
issSupported bool
|
||||
wantErr bool
|
||||
wantErrContains string
|
||||
name string
|
||||
iss string
|
||||
issSupported bool
|
||||
acceptUnadvertisedIss bool
|
||||
wantErr bool
|
||||
wantErrContains string
|
||||
}{
|
||||
{
|
||||
name: "ValidIss",
|
||||
|
|
@ -949,11 +950,41 @@ func TestValidateIssuerResponse(t *testing.T) {
|
|||
iss: "",
|
||||
issSupported: false,
|
||||
},
|
||||
{
|
||||
// Default / zero-value policy: reject a matching unadvertised iss
|
||||
// (released v1.7.0 behavior).
|
||||
name: "UnadvertisedIssCorrectRejectedByDefault",
|
||||
iss: expectedIssuer,
|
||||
issSupported: false,
|
||||
wantErr: true,
|
||||
wantErrContains: "does not advertise",
|
||||
},
|
||||
{
|
||||
name: "UnadvertisedIssCorrectAcceptedWhenOptedIn",
|
||||
iss: expectedIssuer,
|
||||
issSupported: false,
|
||||
acceptUnadvertisedIss: true,
|
||||
},
|
||||
{
|
||||
name: "UnadvertisedIssWrong",
|
||||
iss: "https://attacker.example.com",
|
||||
issSupported: false,
|
||||
wantErr: true,
|
||||
wantErrContains: "does not match expected issuer",
|
||||
},
|
||||
{
|
||||
name: "UnadvertisedIssWrongStillRejectedWhenOptedIn",
|
||||
iss: "https://attacker.example.com",
|
||||
issSupported: false,
|
||||
acceptUnadvertisedIss: true,
|
||||
wantErr: true,
|
||||
wantErrContains: "does not match expected issuer",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
err := validateIssuerResponse(tt.iss, expectedIssuer, tt.issSupported)
|
||||
err := validateIssuerResponse(tt.iss, expectedIssuer, tt.issSupported, tt.acceptUnadvertisedIss)
|
||||
if tt.wantErr {
|
||||
if err == nil {
|
||||
t.Fatalf("validateIssuerResponse() = nil, want error containing %q", tt.wantErrContains)
|
||||
|
|
@ -968,6 +999,112 @@ func TestValidateIssuerResponse(t *testing.T) {
|
|||
}
|
||||
}
|
||||
|
||||
// TestAuthorize_AcceptUnadvertisedIssPlumbing checks that Authorize passes
|
||||
// AuthorizationCodeHandlerConfig.AcceptUnadvertisedIss through to
|
||||
// validateIssuerResponse. The fake server returns iss without advertising
|
||||
// authorization_response_iss_parameter_supported.
|
||||
func TestAuthorize_AcceptUnadvertisedIssPlumbing(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
acceptUnadvertisedIss bool
|
||||
wantErrContains string
|
||||
}{
|
||||
{
|
||||
name: "ZeroValueRejects",
|
||||
wantErrContains: "does not advertise",
|
||||
},
|
||||
{
|
||||
name: "OptInAccepts",
|
||||
acceptUnadvertisedIss: true,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
authServer := oauthtest.NewFakeAuthorizationServer(oauthtest.Config{
|
||||
UnadvertiseIssParameter: true,
|
||||
RegistrationConfig: &oauthtest.RegistrationConfig{
|
||||
PreregisteredClients: map[string]oauthtest.ClientInfo{
|
||||
"test_client_id": {
|
||||
Secret: "test_client_secret",
|
||||
RedirectURIs: []string{"http://localhost:12345/callback"},
|
||||
},
|
||||
},
|
||||
},
|
||||
})
|
||||
authServer.Start(t)
|
||||
|
||||
resourceMux := http.NewServeMux()
|
||||
resourceServer := httptest.NewServer(resourceMux)
|
||||
t.Cleanup(resourceServer.Close)
|
||||
resourceURL := resourceServer.URL + "/resource"
|
||||
resourceMux.Handle("/.well-known/oauth-protected-resource/resource", ProtectedResourceMetadataHandler(&oauthex.ProtectedResourceMetadata{
|
||||
Resource: resourceURL,
|
||||
AuthorizationServers: []string{authServer.URL()},
|
||||
}))
|
||||
|
||||
handler, err := NewAuthorizationCodeHandler(&AuthorizationCodeHandlerConfig{
|
||||
RedirectURL: "http://localhost:12345/callback",
|
||||
AcceptUnadvertisedIss: tt.acceptUnadvertisedIss,
|
||||
PreregisteredClient: &oauthex.ClientCredentials{
|
||||
ClientID: "test_client_id",
|
||||
ClientSecretAuth: &oauthex.ClientSecretAuth{
|
||||
ClientSecret: "test_client_secret",
|
||||
},
|
||||
},
|
||||
AuthorizationCodeFetcher: func(ctx context.Context, args *AuthorizationArgs) (*AuthorizationResult, error) {
|
||||
client := &http.Client{
|
||||
CheckRedirect: func(req *http.Request, via []*http.Request) error {
|
||||
return http.ErrUseLastResponse
|
||||
},
|
||||
}
|
||||
resp, err := client.Get(args.URL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to visit auth URL: %v", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
location, err := resp.Location()
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("failed to get location header: %v", err)
|
||||
}
|
||||
return &AuthorizationResult{
|
||||
Code: location.Query().Get("code"),
|
||||
State: location.Query().Get("state"),
|
||||
Iss: location.Query().Get("iss"),
|
||||
}, nil
|
||||
},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("NewAuthorizationCodeHandler failed: %v", err)
|
||||
}
|
||||
|
||||
req := httptest.NewRequest(http.MethodGet, resourceURL, nil)
|
||||
resp := &http.Response{
|
||||
StatusCode: http.StatusUnauthorized,
|
||||
Header: make(http.Header),
|
||||
Body: http.NoBody,
|
||||
Request: req,
|
||||
}
|
||||
resp.Header.Set(
|
||||
"WWW-Authenticate",
|
||||
"Bearer resource_metadata="+resourceServer.URL+"/.well-known/oauth-protected-resource/resource",
|
||||
)
|
||||
err = handler.Authorize(context.Background(), req, resp)
|
||||
if tt.wantErrContains != "" {
|
||||
if err == nil {
|
||||
t.Fatalf("Authorize() = nil, want error containing %q", tt.wantErrContains)
|
||||
}
|
||||
if !strings.Contains(err.Error(), tt.wantErrContains) {
|
||||
t.Fatalf("Authorize() error = %q, want it to contain %q", err.Error(), tt.wantErrContains)
|
||||
}
|
||||
return
|
||||
}
|
||||
if err != nil {
|
||||
t.Fatalf("Authorize() unexpected error = %v", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestInferApplicationType(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
|
|
|||
Loading…
Reference in a new issue