aspnet / AspNetKatana

Microsoft's OWIN implementation, the Katana project
Apache License 2.0
960 stars 331 forks source link

OpenIdConnect middleware throws on logout messages within callback path (regression in 4.1.0) #476

Closed will-bartlett closed 8 months ago

will-bartlett commented 1 year ago

I have an OpenID Connect site https://domain.example with redirect URI https://domain.example/redir used for both login redirect_uri and logout post_logoug_redirect_uri. On 4.0.1, this worked fine. After upgrading to 4.2.2 related to CVE-2022-29117, I found a regression here that seems to have been introduced by #297 . Following RP initiated logout, the OpenId Connect middleware makes a request like https://sts.example/logout?state=OpenIdConnectProperties.Abc&redirect_uri=https%3A%2F%2Fdomain.example%2Fredir which results in a response to my site like https://domain.example/redir?state=OpenIDConnectProperties.Abc. In 4.0.1, because the response from RP initiated logout is a GET, AuthenticateCoreAsync left openIdConnectMessage null, and returned null (no authentication) on line 220. After #297 , openIdConnectMessage was renamed to authorizationResponse and set on GET requests on line 213. It then fails 70 lines later as being an invalid authorize message, as an authorize message must either include "code" or "id_token".

The prior code seems to have more of the right ideas. This message is indeed a openIdConnectMessage, and NOT an authorizationResponse yet. The middleware should first decode the state using something like Helper.LookupSignOut - and then, depending on the data in the state, either process the message as an authorizationResponse or an authorizationResponseRevoke.

Tratcher commented 1 year ago

Offline notes: post_logoug_redirect_uri and redirect_uri should use different values to avoid conflicts like this. We'll re-visit if this doesn't prove viable.

The state does not currently contain info specific to sign-in or sign-out.