search

aspnet / AspNetKatana

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

Overridden RedirectUri in RedirectToIdentityProvider notification is not passed to AuthorizationCodeReceived #414

Closed lekrus closed 3 years ago

lekrus commented 3 years ago

Hello,

When we override RedirectUri in OpenIdConnectAuthenticationNotifications like:

Notifications = new OpenIdConnectAuthenticationNotifications {
  RedirectToIdentityProvider = n => {
    n.ProtocolMessage.RedirectUri = updatedRedirectUri;
    return Task.CompletedTask;
  }
}

it's not available in 

AuthorizationCodeReceived = n => {
   // n.RedirectUri is NOT equal to updatedRedirectUri
   return Task.CompletedTask;
}

Seems like there is a bug in the code: https://github.com/aspnet/AspNetKatana/blob/d00d910b2f804ae19f28ee8839aa641cd8ff8214/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L143

Similar code in NET Core adds already overridden redirect URL into authentication parameters, so it's available later: https://github.com/dotnet/aspnetcore/blob/ee11b07ec2da1e4a2b2e9720a83272bef3608adf/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs#L448

Could you check the behaviour?

Thanks!

Tratcher commented 3 years ago

In katana the value is taken from Options.RedirectUri, where in core it's a derived value: https://github.com/dotnet/aspnetcore/blob/ee11b07ec2da1e4a2b2e9720a83272bef3608adf/src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs#L378

The re-ordering that allows the RedirectUri to be replaced in the event before captured in the properties seems incidental.

Can you briefly explain why you need to replace the redirect uri?

Tratcher commented 3 years ago

Actually, there is a problem with moving it. In Katana the auth properties collection is serialized into the state field before the event is called. That can't be changed without compat risk. https://github.com/aspnet/AspNetKatana/blob/d00d910b2f804ae19f28ee8839aa641cd8ff8214/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L182

lekrus commented 3 years ago

Thank you for the prompt reply.

The original need was to support multiple domains. E.g. if we have apple.example.org and orange.example.org then users who open apple.example.org are properly authenticated, because we dynamically change RedirectUrl and ensure that token is sent back to the same redirect url.

Now, when we do

   n.ProtocolMessage.RedirectUri = updatedRedirectUri;

The request is properly authenticated, we get "id_token", but when there is a request for "code" it always fails at authentication server (IdentityServer), because that request uses wrong RedirectUri. And it's very hard to debug the issue, as all documentation and samples say you just need to override ProtocolMessage.RedirectUri, but that will not work.

There is a workaround to store that redirect url in cookie (like you do with "nonce") or some storage and then:

AuthorizationCodeReceived = n => {
   n.TokenEndpointRequest.RedirectUri = storedRedirectUrl;
   return Task.CompletedTask;
}

but I guess that is what that code was intended for, but historically happened not to work as needed I see the issue was actual in 2014 as well: https://github.com/IdentityServer/IdentityServer3/issues/509

And I agree with you that might have been incidental or a way to make the state cookie smaller, but it still looks like a bug, because storing predefined value in Properties with a special key OpenIdConnectAuthenticationDefaults.RedirectUriUsedForCodeKey to pass via state looks like useless with current implementation to me, because its value (Options.RedirectUri) is available without taking from state and it only increases size of that cookie.

However I do understand that some implementations might have used to that behaviour. What do you think about the least intrusive fix, like:

Thank you!

Tratcher commented 3 years ago

all documentation and samples say you just need to override ProtocolMessage.RedirectUri, but that will not work.

What docs? Muti-tenancy like this was never a supported feature.

You're right though, I don't remember why that value was stored in the properties collection to begin with since it can't realistically be different from Options.RedirectUri.

then move State serialization to after

This is the kind of change I don't think we can make at this point. Apps interacting with the state in the notification would be broken.

You can try this for a mitigation: In the notification you can pull out the openIdConnectMessage.State, deserialize it, update the RedirectUri, and reserialize it. It's not pretty, but it should work with your scenario.

lekrus commented 3 years ago

Thanks for the feedback.

By documentation I meant various samples found while googling of how to change the RedirectUri dynamically. The problem is that if you just change RedirectUri and add no workaround in either AuthorizationCodeReceived or by manipulating State like you suggested, the Hybrid flow, for example, will stop working, and it's impossible to tell why without checking the source code.

Thank you for suggesting another workaround. Do you think that can be included in the lib itself? E.g. if we know that RedirectUri changed after https://github.com/aspnet/AspNetKatana/blob/d00d910b2f804ae19f28ee8839aa641cd8ff8214/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L203 then it's possible to manipulate State like you suggested? That should not break other applications, and will resolve the issue?

Tratcher commented 3 years ago

We don't plan to make a change like this at this point.

lekrus commented 3 years ago

ok, understood