manfredsteyer / angular-oauth2-oidc

Support for OAuth 2 and OpenId Connect (OIDC) in Angular.
MIT License
1.9k stars 688 forks source link

Implement RFC 9207 #1213

Open bellebaum opened 2 years ago

bellebaum commented 2 years ago

Recently, a new OAuth response parameter was defined in RFC 9207: iss

The basic idea is that if a server advertises authorization_response_iss_parameter_supported as true in its metadata (or we know of support via explicit configuration), the client should only accept the response if iss matches the server's issuer identifier.

Furthermore: This Client clears several OAuth/OpenID response parameters after login (e.g. code or state). The following code should clear iss as well:

https://github.com/manfredsteyer/angular-oauth2-oidc/blob/d95d7da788e2c1390346c66de62dc31f10d2b852/projects/lib/src/oauth-service.ts#L1743-L1761

At a minimum, this should free sites of having to manually clear the iss when using compliant servers. When properly implemented, some sites might even benefit from the mix-up countermeasure.

jeroenheijmans commented 2 years ago

Interesting! Looking at the RFC it's currently "Proposed Standard" so even though not fully mature still good enough to already consider implementing it. Tagged your suggestion accordingly, as a feature-request.

buchatsky commented 1 year ago

I've created the PR to fix this. The only thing that I'm not sure is reporting the error:

  1. What should be the argument of Promise.reject() - string or OAuthErrorEvent? (Usually in Angular the HttpErrorResponse type is used for remote errors and simple string - for local errors, so I return the string. It is also one if() case less in my error handler)
  2. Should the two error cases (this.authorizationResponseIssParameterSupported && iss !== this.issuer) (!this.authorizationResponseIssParameterSupported && iss) return the same or different messages? (I return the same message to simplify the code)
bellebaum commented 1 year ago

I am unsure about the first question, but for the second I think it might make sense to not throw any error at all if we do not know of support for the spec, since an Authorization Server might throw all kinds of other parameters in there, and they may not be aware that a new spec suddenly popped up defining a new parameter in the IANA Registry.

Thanks for implementing this :+1:

buchatsky commented 1 year ago

@bellebaum do you mean throwing an error in this case: (!this.authorizationResponseIssParameterSupported && iss) ? RFC says:

Clients SHOULD discard authorization responses with the iss parameter from authorization servers that do not indicate their support for the parameter. However, there might be legitimate authorization servers that provide the iss parameter without indicating their support in their metadata. Local policy or configuration can determine whether to accept such responses

So, if authorization_response_iss_parameter_supported is not supplied in AS metadata but the 'iss' param returned by AS is really an issuer id, this can be fixed with setting OAuthService.authorizationResponseIssParameterSupported = true after loading a discovery document. If 'iss' param is returned and is anything but an issuer id, issuer verification can be turned off by setting AuthConfig.skipIssuerCheck = true (but in this case verifying issuer in id_token claim will be also skipped).

bellebaum commented 1 year ago

Not sure why the RFC says that, but I can live with it :)

masum-ulu commented 1 year ago

Hi, what's the status of this issue ? Is this gonna merge ?

bjpe commented 6 months ago

Hi @manfredsteyer , are there any plans to fix this issue?

olegdunkan commented 6 months ago

Workaround for those who use Keycloak OpenId Provider Turn on Exclude Issuer From Authentication Response in clients advance settings