aspnet / AspNetKatana

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

OpenIdConnect: SecurityTokenValidated is called before validation is fully complete #405

Closed nickmqb closed 3 years ago

nickmqb commented 3 years ago

Hi, I'm using Katana's OpenIdConnect middleware in an id token + implicit flow scenario. I noticed that OpenIdConnectAuthenticationNotifications.SecurityTokenValidated is called when the token has only been partially validated. For example, the nonce has not yet been validated at this point.

Looking at the code it seems like part of the token is validated (https://github.com/aspnet/AspNetKatana/blob/dev/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L356), then SecurityTokenValidated is called (L377), then the rest of the token is validated (L390).

This seems to be unexpected behavior to me, so I was wondering if there was a particular reason for this? We're running some code with side effects (database write) in the SecurityTokenValidated callback, but we don't want to run this code unless the token has passed validation first. A cursory glance at the rest of the code did not reveal any opportunities for a notification callback after token validation but before user sign in, but I may have missed it. Any help would be appreciated. Thanks!

Tratcher commented 3 years ago

You've read through the code so you know the validation has many steps to it. In the case of SecurityTokenValidated, that event is referring to only a specific step in the validation process. The confusion makes sense though and we'll want to clarify the API docs.

As for a later notification, you can turn on SaveTokens to stash them in the AuthenticationProperties collection. Then you can use the cookie auth events like CookieAuthenticationProvider.OnResponseSignIn as an indication that everything is truly done validating and do your DB work there. In that later event you can remove the tokens from the properties collection to avoid any cookie bloat. Let me know how that works for you.

nickmqb commented 3 years ago

I'm still a bit confused regarding the term "SecurityToken". Is that the same (in our case) as the id token? And would the clarification consist of pointing out that SecurityTokenValidated does not validate the entire security token? Or is my understanding of the term incorrect?

Our situation is a bit more complicated than I initially described actually. Something I didn't mention yet is that we also want to replace the ClaimsIdentity after token validation. Basically, we offer multiple ways for users to log in, so we don't want to use the ClaimsIdentity from OIDC as-is. Instead, we'd like to replace it with our own claims identity, which will have a standardized structure across all login methods. When OnResponseSignIn happens, AuthenticationManager.SignIn has already been called, not sure if it's too late at that point to safely replace the user identity?

An alternative that we're considering is to add another cookie middleware layer with a different auth type. We let the OIDC callback complete, and let it set a different "external" cookie which contains the id token claims. Then we redirect to a custom controller action to exchange that cookie for our main auth cookie. (Somewhat similar to what's discussed here: https://stackoverflow.com/questions/26166826/usecookieauthentication-vs-useexternalsignincookie). It's a bit slower (requires an additional request) and slightly more complicated (more cookie types to deal with), but it may be a better solution in terms of "going with the grain" of the framework. Would be great to hear your thoughts.

Tratcher commented 3 years ago

The external cookie approach is exactly how multiple identities are reconciled when using the ASP.NET Identity framework with external logins. It nicely separates the specialized logins from the standard ones. Give it a try.

SecurityTokenValidated indicates that the id_token itself passed validation checks (signing, audience, etc), but there are subsequent checks that validate the implicit flow was completed correctly. That's where we check the token's nonce vs the one in the cookie for example. https://github.com/aspnet/AspNetKatana/blob/d196e785e277452f1382dded08ca12974d29170e/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L390-L396

There are similar checks for the code flow: https://github.com/aspnet/AspNetKatana/blob/d196e785e277452f1382dded08ca12974d29170e/src/Microsoft.Owin.Security.OpenIdConnect/OpenidConnectAuthenticationHandler.cs#L529-L535

nickmqb commented 3 years ago

OK, going ahead and closing this. Thanks for your help!

nickmqb commented 3 years ago

Actually, I'll re-open this issue to track the doc clarification. (Feel free to close and track elsewhere if you want, of course.)