ValidatePrincipal in CookieAuthenticationHandler not being called on some requests

[guylando]

The result of this bug is that once in a while in a multi-user scenerio, the wrong user appears in httpcontext.User. Investigating the issue, I registered a custom function for OnValidatePrincipal event and discovered that it is not being called for the problematic requests. This line not being called:
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs#L161
I registered a custom middleware after .UseAuthentication and I see those requests in my middleware even though they do not trigger the OnValidatePrincipal event.
I am now investigating further to see why OnValidatePrincipal is not called.
Anybody got any idea?

This is the code flow:

https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs#L54
https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Authentication.Abstractions/AuthenticationHttpContextExtensions.cs#L31
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs#L124
https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Cookies/CookieAuthenticationHandler.cs#L154
ValidatePrincipal: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication.Cookies/Events/CookieAuthenticationEvents.cs#L114

[guylando]

OK I did the following now which helped find the problem: turned on trace level logging + in the middleware decrypted the cookie ticket to check the identity in the cookie compared to the identity in the http context.

Conclusions:

In the problematic request which has the bug the log message:
info: Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler[8]
AuthenticationScheme: Identity.Application was successfully authenticated.
Happens immediately without ValidatePrincipal being called.

After decrypting the cookie ticket, the user in the ticket is NOT EQUAL to the user who was just authenticated by the CookieAuthenticationHandler.

Looking at AuthenticationHandler source code ( https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNetCore.Authentication/AuthenticationHandler.cs#L148 ) I found out that this happens because of the HandleAuthenticateOnceAsync method logic which in the buggy request prevents loading the correct user from the cookie and instead it uses a wrong user taken from some previous request saved in the _authenticateTask data member.

So basically the problem is that a previous task saved information in _authenticateTask which prevents some random future task from being correctly authenticated.

@HaoK @Tratcher
Can someone please help with this and explain if the _authenticateTask data member is thread safe and the logic surrounding it is correct? why would the buggy request have this data member already initialized? how is it possible? shouldnt it have not been initialized since this is a new request?

[Tratcher]

Note the validator Identity uses with ValidatePrincipal is rate limited, it only runs every 30 minutes to avoid a large performance hit accessing the database. That's likely why you don't see it execute most of the time.

[Tratcher]

Please share your Startup code. Or at least Startup.ConfigureServices. I wonder if you have registered everything correctly.

[guylando]

I know that, I registered a custom validator (because I wanted to perform validation on every request) and I call the identity validator inside. But that doesn't effect what I described in the previous comment.

Can you please comment on what I wrote regarding what I saw happening with _authenticateTask? What might make that happen?

The startup is 1000+ lines of code which mostely are not related to identity and its a private project so its problematic to share but I debugged the problem and came to the specific lines causing this behavior which I described in the previous comment. Would be glad to get a response on that.

[Tratcher]

Handlers are created per-request so thread-safety should be a nominal concern. The _authenticateTask caches the result of AuthenticateAsync because calling it again should yield exactly the same result even if not cached. E.g. it would read the same cookie and deserialize it in the same way.

[guylando]

But as I describe, if it would read in this case the cookie then it would get another result. Thats what I observed. Also in the observed case the handler holds a data member from previous request. How could that happen?

[Tratcher]

At least share the Startup code related to Identity and Auth. Descriptions often overlook usage problems.

[Tratcher]

A handler should not hold anything from a prior request because it should have been re-created for each request. We'd need a repro to debug and figure out what's going on.

[Tratcher]

You'd have to customize the DI registration of IAuthenticationHandlerProvider or CookieAuthenticationHandler to cause these kinds of problems.

[guylando]

Here it is, thanks for the help:
==UPDATE: DELETED THE CODE SINCE ITS NOT RELEVANT ANYMORE==

[guylando]

I have verified (breakpoint in the end of ConfigureServices) and all the CookieAuthenticationHandler in the service collection are Transient.

[Tratcher]

Thanks. That's a lot of configuration... the Domain settings are especially odd, but I don't see anything that would affect the behavior you report.

The next step would be to create a minimal repro app, something where you can share the whole project. We can try, but you're in the best position with an existing repro.