Comments (18)
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.
Related Issues (20)
- About policy in Multiple Schemes 403 problem HOT 2
- AVRO serialization error - Could not find any matching known type for 'Newtonsoft.Json.Linq.JToken' using c# HOT 3
- How to Validate AccessToken in WebAPI core 2.0 getting from IdentityServer3 HOT 1
- Multiple refresh for authentication on server HOT 33
- Oauth MicrosoftAccount Token Request Issue HOT 6
- OAuth redirect_uri using http instead of https on Azure App Service Linux HOT 9
- Is there a way to Decrypt encrypted SAML token?? HOT 9
- Set redirect from authorization handler HOT 5
- JWT payload "unique_name" not mapped correctly HOT 2
- .
- HttpContext.SignInAsync sets null to Identity claims data HOT 3
- Handling incomplete remote signouts HOT 15
- .net core api authentication using ws-federation HOT 7
- AuthenticationProperties object is not passed to ChallengeAsync / ForbidAsync HOT 11
- [Net Core 2.0] AccessDeniedPath ignored in cookie authentication HOT 1
- Docker Swarm + nginx + WS-Federation: multiple redirection issue HOT 33
- SSO for Microsoft Outlook using auth0 HOT 6
- Redirect URI is ignored HOT 2
- Wctx parameter getting overridden, breaking functionality HOT 8
- ASP.NET Core OpenId Authentication in Container with TLS Termination in WAF HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from security.