Re-evaluate sync vs async extension points in AuthenticationHandler about security HOT 18 CLOSED

Well, I don't have access to aspnet/WebListener or aspnet/Helios, so... 😄

The best argument I have is that currently, everybody is paying the price of a Wait() call when having an authentication middleware in an ASP.NET 5 app, even the ones who use WriteAsync. With this change, only the ones using the synchronous Write would pay for that. And ideally, everybody should be encouraged to use WriteAsync.

from security.

The first option is probably the best one! 👍

You should also make ApplyResponseChallenge(Async) and ApplyResponseGrant(Async) non-abstract, just like they were in Katana 3: http://katanaproject.codeplex.com/SourceControl/latest#src/Microsoft.Owin.Security/Infrastructure/AuthenticationHandler.cs

from security.

Here's a breakdown of the various code paths:

Request:
(Passive) App -> HttpContext.Authenticate/Async -> IAuthenticationHandler.Authenticate/Async -> AuthenticationHandler.Authenticate/Async -> AuthenticateCore/Async
(Active) Middleware.Invoke -> AuthenticationHandler.BaseInitializeAsync -> AuthenticateAsync -> AuthenticateCoreAsync
Response:
OnSendingHeaders -> ApplyResponse -> ApplyResponseCore -> ApplyResponseGrant & ApplyResponseChallenge
TeardownAsync -> ApplyResponseAsync -> ApplyResponseCoreAsync -> ApplyResponseGrantAsync & ApplyResponseChallengeAsync

All sync and async code paths are currently in use internally or exposed to application code so there are no overloads that we can just eliminate.

The problem can be broken down into two parts; the request (Authenticate) and the response (Grant/Challenge).

For the request code path the infrastructure has no dependencies on the sync APIs but they are exposed to application code. We'd have to evaluate the app and Identity usage scenarios to see if we could eliminate the sync code path.

For the response the only sync dependency is OnSendingHeaders. Making OSH async would eliminate the need for the sync overloads. OSH could be fully async except when triggered by sync response stream APIs (Write/Flush), and even then the server would have some discretion (e.g. Helios does write-behind buffering, so it could fire OSH async even on sync code paths).

from security.

For the request code path the infrastructure has no dependencies on the sync APIs but they are exposed to application code. We'd have to evaluate the app and Identity usage scenarios to see if we could eliminate the sync code path.

In Katana 3, AuthenticateAsync/AuthenticateCoreAsync had no sync equivalent. Do you remember someone complaining about this lack?

Concerning Identity, they are only using the async version:
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNet.Identity/SignInManager.cs#L218
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNet.Identity/SignInManager.cs#L320
https://github.com/aspnet/Identity/blob/dev/src/Microsoft.AspNet.Identity/SignInManager.cs#L385

from security.

Yup, identity is fully async, I'm not a huge fan of having sync and async overloads either, +1 for eliminating sync if possible

from security.

@Tratcher is this something you think we can/should do? It definitely would make things a lot cleaner

from security.

Note: We will have to convince @lodejard but I think we should try if you agree that this is the 'right' thing to do...

from security.

@HaoK I tried working through it once without much success. It's hard to get around the OnSendingHeaders code path. Take a look and let me know if you agree/disagree.

from security.

Sounds like this is DOA since we can't get any traction...closing

from security.

Any idea why this suggestion was not easy to implement?

from security.

OnSendingHeaders was the big blocker. It needs to be sync (due to Stream.Write), but we wanted everything else to be async, so we're left with dual code paths.

from security.

And calling Wait()/GetAwaiter().GetResult() in Stream.Write was not an option?
Of course, it's not perfect but it has one merit: only developers using the sync write method would pay the price of the blocking OSH callbacks.

from security.

bit of a noob, here. but, I'm messing with auth middleware. on googling i came to here so thought id post.

On my handler, it is asking for AuthenticateCore as it is abstract, I already have async method, so slightly unsure what to do with it right now. Right, now, some other things are not compiling, so i cant test but once I will let you know happens.

I expect I am doing something completely wrong, and I expect this is totally unrelated to the onheader thing, but thought just say, that asyc/sync. thing can get little confusing.
nl

from security.

AuthenticateCore is required, AuthenticateCoreAsync is an optional overload if you need to do anything async. See https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication/AuthenticationHandler.cs#L242

from security.

@Tratcher no remark concerning my last question? 😄

from security.

We don't want to have Wait's hidden in the server code, we want them to be a conscious choice at the middleware layer.

from security.

Like that? 😄

https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/KestrelEngine.cs#L102
https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/FrameResponseStream.cs#L63
https://github.com/aspnet/KestrelHttpServer/blob/dev/src/Microsoft.AspNet.Server.Kestrel/Http/Listener.cs#L112

from security.

Citing kestrel code isn't going to help your argument, we already know it's broken :-).

from security.