diff --git a/auth/extauth/oidc_login.go b/auth/extauth/oidc_login.go index cf5d9272..5a0b75f9 100644 --- a/auth/extauth/oidc_login.go +++ b/auth/extauth/oidc_login.go @@ -88,6 +88,9 @@ func PerformOIDCLogin( if authResult.State != authReq.state { return nil, fmt.Errorf("state mismatch: expected %q, got %q", authReq.state, authResult.State) } + if err := checkAuthorizationResponseIssuer(authResult.Iss, authReq.issuer, authReq.issParameterSupported); err != nil { + return nil, err + } tokens, err := completeOIDCLogin(ctx, config, oauth2Config, authResult.Code, authReq.codeVerifier) if err != nil { @@ -102,6 +105,10 @@ type oidcAuthorizationRequest struct { authURL string state string codeVerifier string + // issuer and issParameterSupported come from the IdP's metadata, to + // validate the iss parameter of the authorization response (RFC 9207). + issuer string + issParameterSupported bool } // initiateOIDCLogin initiates an OIDC Authorization Code flow with PKCE. @@ -177,12 +184,34 @@ func initiateOIDCLogin( authURL := oauth2Config.AuthCodeURL(state, authURLOpts...) return &oidcAuthorizationRequest{ - authURL: authURL, - state: state, - codeVerifier: codeVerifier, + authURL: authURL, + state: state, + codeVerifier: codeVerifier, + issuer: meta.Issuer, + issParameterSupported: meta.AuthorizationResponseIssParameterSupported, }, oauth2Config, nil } +// checkAuthorizationResponseIssuer validates the iss parameter of an +// authorization response, the mitigation for mix-up attacks in [RFC 9207]. +// If the IdP advertises authorization_response_iss_parameter_supported, iss +// is required. A present iss must equal the issuer from the IdP's metadata, +// whether or not support is advertised (RFC 9207, section 2.4). +// +// [RFC 9207]: https://www.rfc-editor.org/rfc/rfc9207 +func checkAuthorizationResponseIssuer(iss, issuer string, issParameterSupported bool) error { + if iss == "" { + if issParameterSupported { + return fmt.Errorf("IdP advertises RFC 9207 iss parameter support but none was received in the authorization response") + } + return nil + } + if iss != issuer { + return fmt.Errorf("authorization response issuer %q does not match expected issuer %q", iss, issuer) + } + return nil +} + // completeOIDCLogin completes the OIDC Authorization Code flow by exchanging // the authorization code for tokens. func completeOIDCLogin( diff --git a/auth/extauth/oidc_login_test.go b/auth/extauth/oidc_login_test.go index 9cd9b935..7db0e0ed 100644 --- a/auth/extauth/oidc_login_test.go +++ b/auth/extauth/oidc_login_test.go @@ -261,3 +261,96 @@ func TestPerformOIDCLogin_ValidationErrors(t *testing.T) { }) } } + +// TestPerformOIDCLogin_Issuer checks the iss parameter of the authorization +// response (RFC 9207), the mitigation for mix-up attacks. +func TestPerformOIDCLogin_Issuer(t *testing.T) { + const otherIssuer = "https://attacker.example.com" + tests := []struct { + name string + advertised bool + // iss returns the iss the fetcher reports, given the one in the + // IdP's redirect and the IdP's issuer. + iss func(fromRedirect, issuer string) string + wantErr string + }{ + { + name: "advertised, iss from the IdP", + advertised: true, + iss: func(fromRedirect, _ string) string { return fromRedirect }, + }, + { + name: "advertised, iss missing", + advertised: true, + iss: func(string, string) string { return "" }, + wantErr: "none was received", + }, + { + name: "advertised, other issuer", + advertised: true, + iss: func(string, string) string { return otherIssuer }, + wantErr: "does not match expected issuer", + }, + { + name: "not advertised, iss missing", + iss: func(string, string) string { return "" }, + }, + { + name: "not advertised, matching iss", + iss: func(_, issuer string) string { return issuer }, + }, + { + name: "not advertised, other issuer", + iss: func(string, string) string { return otherIssuer }, + wantErr: "does not match expected issuer", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + idpServer := oauthtest.NewFakeIdPServer(oauthtest.IdPConfig{ + PreregisteredClients: map[string]oauthtest.ClientInfo{ + "test-client": { + Secret: "test-secret", + RedirectURIs: []string{testRedirectURL}, + }, + }, + IssParameterSupported: tt.advertised, + }) + idpServer.Start(t) + + fetch := func(ctx context.Context, args *auth.AuthorizationArgs) (*auth.AuthorizationResult, error) { + client := &http.Client{ + CheckRedirect: func(*http.Request, []*http.Request) error { + return http.ErrUseLastResponse + }, + } + resp, err := client.Get(args.URL) + if err != nil { + return nil, err + } + defer resp.Body.Close() + loc, err := resp.Location() + if err != nil { + return nil, err + } + q := loc.Query() + return &auth.AuthorizationResult{ + Code: q.Get("code"), + State: q.Get("state"), + Iss: tt.iss(q.Get("iss"), idpServer.URL()), + }, nil + } + + _, err := PerformOIDCLogin(context.Background(), validOIDCLoginConfig(idpServer.URL()), fetch) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("PerformOIDCLogin() error = %v", err) + } + return + } + if err == nil || !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("PerformOIDCLogin() error = %v, want error containing %q", err, tt.wantErr) + } + }) + } +} diff --git a/internal/oauthtest/fake_idp_server.go b/internal/oauthtest/fake_idp_server.go index e5c2c2ef..e84ef923 100644 --- a/internal/oauthtest/fake_idp_server.go +++ b/internal/oauthtest/fake_idp_server.go @@ -13,6 +13,7 @@ import ( "maps" "net/http" "net/http/httptest" + "net/url" "slices" "testing" "time" @@ -39,6 +40,10 @@ type IdPConfig struct { // TokenExchangeConfig enables RFC 8693 token exchange at the /token endpoint. // If non-nil, the server accepts grant_type=urn:ietf:params:oauth:grant-type:token-exchange. TokenExchangeConfig *TokenExchangeConfig + + // IssParameterSupported advertises authorization_response_iss_parameter_supported + // and adds the iss parameter to authorization responses (RFC 9207). + IssParameterSupported bool } // FakeIdPServer is a fake OIDC Identity Provider for testing. @@ -93,8 +98,7 @@ func (s *FakeIdPServer) handleMetadata(w http.ResponseWriter, r *http.Request) { if s.config.TokenExchangeConfig != nil { grantTypes = append(grantTypes, oauthex.GrantTypeTokenExchange) } - w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(map[string]any{ + meta := map[string]any{ "issuer": s.URL(), "authorization_endpoint": s.URL() + "/authorize", "token_endpoint": s.URL() + "/token", @@ -102,7 +106,12 @@ func (s *FakeIdPServer) handleMetadata(w http.ResponseWriter, r *http.Request) { "response_types_supported": []string{"code"}, "code_challenge_methods_supported": []string{"S256"}, "grant_types_supported": grantTypes, - }) + } + if s.config.IssParameterSupported { + meta["authorization_response_iss_parameter_supported"] = true + } + w.Header().Set("Content-Type", "application/json") + json.NewEncoder(w).Encode(meta) } func (s *FakeIdPServer) handleAuthorize(w http.ResponseWriter, r *http.Request) { @@ -134,6 +143,9 @@ func (s *FakeIdPServer) handleAuthorize(w http.ResponseWriter, r *http.Request) s.codes[code] = codeInfo{CodeChallenge: codeChallenge} state := r.URL.Query().Get("state") redirectURL := fmt.Sprintf("%s?code=%s&state=%s", redirectURI, code, state) + if s.config.IssParameterSupported { + redirectURL += "&iss=" + url.QueryEscape(s.URL()) + } http.Redirect(w, r, redirectURL, http.StatusFound) }