-
Notifications
You must be signed in to change notification settings - Fork 597
ValidatePrincipal in CookieAuthenticationHandler not being called on some requests #1602
Comments
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: 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 |
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. |
Please share your Startup code. Or at least Startup.ConfigureServices. I wonder if you have registered everything correctly. |
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. |
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. |
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? |
At least share the Startup code related to Identity and Auth. Descriptions often overlook usage problems. |
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. |
You'd have to customize the DI registration of IAuthenticationHandlerProvider or CookieAuthenticationHandler to cause these kinds of problems. |
Here it is, thanks for the help: |
I have verified (breakpoint in the end of ConfigureServices) and all the CookieAuthenticationHandler in the service collection are Transient. |
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. |
Also can you include what's in |
@Tratcher That's hard because even in our project it happens only once in a while only in some situations...if you have an idea what else to add to debug I can do that and post the debug messages. @HaoK The code of UtilsGL.CustomValidatePrincipalAsync does not get executed at all in the situation I described (i have put a debug message in the first line) so why would it be the cause of the problem? I am limited in what code I can post so I try to post only the essential. |
Here are logs when user [email protected] makes 2 POST requests to same url, one is processed correctly and the second one is buggy with the behavior I described:
@#####:LogInformation info: CustomValidatePrincipalAsync[0]
@#####:LogInformation info: CustomValidatePrincipalAsync[0]
@#####:LogInformation info: CustomValidatePrincipalAsync[0]
info: Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler[8] info: vvv.GL.Middlewares.HideExceptionsMiddleware[0]
info: Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler[8] info: vvv.GL.Middlewares.HideExceptionsMiddleware[0] info: Microsoft.AspNetCore.SignalR.Messaging.MessageBus[0] info: Microsoft.AspNetCore.Authorization.DefaultAuthorizationService[1] info: Microsoft.AspNetCore.Mvc.ViewFeatures.Internal.PartialViewResultExecutor[1]
info: Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler[8] info: Microsoft.AspNetCore.Authentication.Cookies.CookieAuthenticationHandler[8] @#####:LogInformation ticket principal name: [email protected] |
As you see the problem is that from reason the second request didn't go through all the ordinary steps and it came immidietly to: because of HandleAuthenticateOnceAsync |
And then the _authenticateTask has user [email protected] which probably was a user from some previous request \ other request, instead of the user [email protected] which appears in cookie ticket |
As @Tratcher said, we are going to need a repro or at least more code to figure out what's going on. Can you show the relevant code/middleware you are using to generate the logs messages/ticket principals/context user |
The CustomValidatePrincipalAsync log is just printing contest.Principal.Identity.Name in custom OnValidatePrincipal function. The HideExceptionsMiddleware middleware is registered after UseAuthentication and has code taken from authentication middleware for debugging purpose (code below) And other log messages are from .net framework code with debug level trace turned on.
|
This happens only on our azure server which runs kestrel with iis (it doesnt happen in the development computer) and only when multiple users have logged on\off. And it happens only sometimes when user is making 2 requests quickly (the first request is always good, the second one sometimes has the bug). |
Then I get sometimes: |
I try to assume there is a bug related to _authenticateTask so I try to understand where it could happen. It must be started from AuthenticationMiddleware.cs so So assuming there is a scope\threading bug, the AuthenticationHandler is retrieved in AuthenticationService using GetHandlerAsync method of IAuthenticationHandlerProvider. Thinking about it, it most probably is not a scope issue because if it was a scope issue then the error would happen often and not in such rare case. It smells like a threading problem because of all the requirements of number of users and requests timing to reproduce. Are different requests always on a different thread in kestrel+iis environment? because otherwise if two requests are handled by the same thread then they might both touch the same data member _authenticateTask and cause the problem (scoped http context is per thread no? so if two threads handle same request there is a mass) I will add printing thread id to the log messages. Maybe that can give a clue. |
Requests all execute on thread-pool threads but there's no shared thread context, the HttpContext is captured on the stack. The only thing we've seen cause bad cross threading issues is accidental uses of |
I have now added the following in startup.cs Configure to check on _authenticateTask:
// TODO DELETE START===========================
app.UseAuthentication();
|
YESSS! normal request:_authenticateTask before UseAuthentication: null================
|
I think this proves that this is some bug in the framework, no? |
What's before UseAuthentication in your configure. Is your print out actually at the start of the pipeline? |
|
As you see there is nothing meaningful before the first print out |
It looks like you can reproduce this pretty consistently. How are you generating the requests for your repro? |
I have an ajax post request in the site generated by a button (I add the csrf token to the post request). |
I tried the solution of adding the middleware which changes to null the data member _authenticateTask
Since it does no harm to clear this in the beginning of the authentication middleware, maybe you should add it (one line performing .Clear() on _handlerMap) to help solve such bugs, EVEN IF it was caused from some code misuse. The argument in favor of doing this is that the authentication middleware is responsible for authentication and if someone called AuthenticationHandlerProvider GetHandlerAsync previously somehow and caused a bad AuthenticationHandler to be saved in _handlerMap, it SHOULD NOT cause the authentication middleware to skip authenticating the user using the cookie (in case of cookie authentication) which is the situation now (and also possibly will solve some scoping\threading rare edge case which is hard to find). @HaoK @Tratcher |
Re-opening until we can find the root cause. It appears to be a DI scoping bug, but it's not clear how that would be caused. @pakrym can you find a reference to the bug causing cross thread contamination via loggers with corrupt async state? |
@Tratcher there was no bug, it was internal support issue which I was never able to repro, but it went away after moving to @Eilon loggers are not the cause. It's just another issue where I've seen cross thread contamination. I suspect that it was caused by the use of I don't think we can move forward on this without a repro. |
@pakrym ok thanks for the update, will take it off your plate. |
I will try to reproduce without the loggers. |
Ok now this is the first line in Startup.cs configure:
And here are the logs:
|
Steps I did to reproduce: I would still recommend to clear the handlers map in the beggining of the authentication middleware so that the authentication middleware will perform its work without possibly being tempered like it happens here. Probably its better until another solution is found. NOTE: I printed the message regarding _authenticateTask just because thats the code I used before, but after investigating it further I now know that the problem is not related to _authenticateTask and its related to the handlers map in the provider. |
@guylando thanks for the extra info. Is there anything else you can share about the configuration of your app? Any customizations to DI? Or other custom services? Any custom auth behavior? |
|
This is possibly a problem though it's hard to say without seeing the code. If any services from one container "pollute" the other container, there could be some odd behavior. Unfortunately, without seeing more about the code, we're at a loss of what the problem could be. We reviewed several code paths and were not able to find anything in ASP.NET Core that could lead to this behavior. If you can send us a small app that reproduces the problem we will investigate further. |
It will be hard to reproduce it in a small app since so much dependencies exist in our project so I don't think it will be possible in the near future especially since we have a working workaround. Maybe if the problem will return and we will have no other choice, otherwise we have a pretty tight schedule right now and reproducing this in a small app would take at-least a full day or two. |
Note that the issue in aspnet/KestrelHttpServer#1720 sounds similar if not identical and they claim having a working reproduction https://github.com/antonGritsenko/aspnetcoreLogout |
@guylando thanks again for the extra info, we'll do our best to investigate this. |
Hi @guylando , we were not able to find out anything else about what could be causing this. Unfortunately without a specific repro app we cannot investigate this any further. |
When just adding the following middleware before .UseAuthentication in startup.cs, it triggers the log a lot even though it probably shouldn't have:
In your environment it is never triggered when you put this as a kestrel app behind iis? |
https://github.com/aspnet/HttpAbstractions/blob/816516f193afcb7ce6b564facc2a36d7e86604ea/src/Microsoft.AspNetCore.Authentication.Core/AuthenticationHandlerProvider.cs#L42-L60 |
what do you mean by IIS Windows auth? because actually we are using iis, maybe related? |
The IISIntegration components call AuthenticateAsync to initialize HttpContext.User early in the pipeline. With later versions of ANCM this should not happen unless you've enabled windows auth in IIS. This would add a handler to the map, but it should not affect cookie auth in any way. |
our Main method has:
and the versions are 2.1.0-rtm-30733 |
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
The text was updated successfully, but these errors were encountered: