Consider reverting https://github.com/aspnet/Security/pull/982 #1044
Comments
|
Please consider, as it breaks some 3rd party security libraries. |
|
/cc @Tratcher |
|
@sebastienros FYI, this "by design" issue will likely impact Orchard 2's OpenID module when moving the |
|
@PinpointTownes your mileage might be better if you call it a breaking change, I had a bad feeling about that one :( |
|
We've known for a long time now that having multiple active challenge middleware will cause conflicts, especially incompatible ones. The multiple auth types ambiguity can only be properly resolved by naming the desired type when issuing the challenge. I don't see how else the conflict can be deterministicly resolved in the short or long term. |
I've always thought that the whole point of returning a boolean from Obviously, this doesn't work properly (specially since this design change), but I'm not sure to understand what's the difficult point here. |
|
That bool prevented the downstream handlers from being called during that specific call to Challenge, not for the whole response. As your original bug reported (#930) this didn't affect automatic unwind challenges. They never observed the original Challenge so they didn't know that someone else handled the response already. We "fixed" the automatic unwind challenge issue by enabling all the middleware to observe the initial Challenge, which just highlighted more conflicts between multiple automatic auth challenges. |
But technically, what prevents us from extending that concept to automatic challenges (applied via the OSH callback)? IMHO, here's how it should ideally work:
|
|
Hi @PinpointTownes , we discussed this at length with @Tratcher @blowdart @HaoK and some others and we have plans on how to mitigate this in the future, but we're not planning on changing this behavior in 1.1. The mitigation plans are to do one of these:
|
|
Tracking the vNext proposals here: #1054 |
|
@Eilon hey. That's extremely unfortunate because it's really an annoying regression that impacts everyone, specially ASOS/OpenIddict and IdentityServer users (using a bearer middleware + Identity is extremely frequent in apps that use the code/implicit flow, which includes a lot of SPA apps). Since there's nothing planned to revert it, I guess the most sensible option we have is to keep using 1.0.0, which works fine (like Katana used to, BTW). /cc @leastprivilege |
|
I still don't understand how this specific change has been an improvement as compared to previous versions... |
|
The problem that existed both before this change as well as after it was that it wasn't very meaningful to have more than one auth provider that was "automatic." That configuration simply didn't make sense because the auth providers weren't design to coordinate with each other about any "hands-off" behavior if one provider already did a challenge. The fix should be easy: don't set more than one auth provider to be automatic. That the original behavior sometimes happened to work for some scenarios was entirely an accident. |
|
BTW I don't think there should be a reason to stick to 1.0.0 because of this (because it wasn't supposed to work on 1.0.0 either!). |
We're obviously not living in the same world. In the real world, nothing is as easy as you imagine, I'm afraid 😄 There are basically 2 options to work around this breaking change/regression in a mixed bearer middleware/cookie middleware scenario:
Now, if you're still convinced it's "easy" (hint for you: it's not
A breaking change like this is always a good reason to keep using the previous version: targeting the 1.1.0 bits in our libs would de facto force our users to migrate to 1.1.0 and to fix their apps to work around this change... so unless you volunteer to answer all the "why did my app break when migrating to the latest aspnet-contrib bits?" questions for me, I will keep targeting the 1.0.0 bits (even if I'd really like to get rid of ugly bugs workarounds) And it looks like I'm not the only one: https://twitter.com/leastprivilege/status/801723059149357056
If automatic challenge was so "dangerous", then why enabling it by default in both the cookies middleware and the bearer middleware? If it doesn't work well when having multiple middleware, then it should be an opt-in feature... you realize that people might want to use them in the same app, right? |
), you can meet real people in our Gitter rooms and tell them how trivial it is to fix their app after migrating to 1.1.0:
|
Oh - how I miss the times when I had the energy to write such long and passionate comments...now I mostly have the feeling that I am getting to old for this shit ;)) |
|
FWIW I agree that this is a breaking change and probably shouldn't have been made in a .1 release... That said, this scenario was one of the earliest ones as @blowdart ran into this almost immediately See https://docs.microsoft.com/en-us/aspnet/core/security/authorization/limitingidentitybyscheme |
|
I have a headache from walking thru the code, but I have a feeling this change is related to another breaking change I came across today in 1.1: aspnet/Templates#686 (comment) In short, can you please test account linking in 1.1 in the ASP.NET Identity templates to confirm if it's broken or not? |
|
That bug is the bug tracking the work to fix linking for templates, basically the manage controller actions need to explicitly clear the cookie, see aspnet/Templates#726 |
|
But yeah, those template breaks are related to: aspnet/Announcements#201 |
|
So then my comment at the end should be a new topic? In essence, the bug is somewhat across all 3 projects: templates, identity, and security. |
|
Identity isn't really involved directly, its a change in behavior at the auth layer (so security/templates) |
|
So... what would you like me to do? |
That sounds MUCH more reasonable, IMHO 👍 |
|
#1061 contains the discussion for the 2.0 way of doing stuff, which will be a breaking change. So start commenting :) |
|
Might want to unlist that breaking 1.1.0 in nuget.org as well. Not that I don't mind reading github issues, but took a while to understand why my app stopped working after that minor update. |
|
Hi guys, I was suggested to add this issue here, so I am doing this now. As you can see here in a code later, then Microsoft.AspNetCore.Mvc and Microsoft.AspNetCore.Mvc.Core are having versions 1.0.1. This is the version that works for me and does not give an exception. If I change it to 1.1.0 then I get an exception. I tried the same thing with empty project, but it seems to be working for that. The error appears basically any time before request even reaches controller at all. And it is on all requests, even on those that should return 404. I started to get an exception like this. My project.json is like this: {
"dependencies": {
"Microsoft.NETCore.App": {
"version": "1.1.0",
"type": "platform"
},
"Microsoft.Extensions.Logging": "1.1.0",
"OpenIddict": "1.0.0-beta1-0501",
"OpenIddict.Mvc": "1.0.0-beta1-0501",
"Custom.DataModel": "1.0.0-*",
"Custom.DAL": "1.0.0-*",
"Custom.Services": "1.0.0-*",
"Custom.ViewModel": "1.0.0-*",
"Microsoft.ApplicationInsights.AspNetCore": "1.0.2",
"Swashbuckle": "6.0.0-beta902",
"SendGrid.NetCore": "1.0.0-rtm-00002",
"AspNet.Security.OAuth.Validation": "1.0.0-beta1-0195",
"Microsoft.AspNetCore.Diagnostics": "1.1.0",
"Microsoft.AspNetCore.Hosting": "1.1.0",
"Microsoft.AspNetCore.Mvc": "1.0.1",
"Microsoft.AspNetCore.Mvc.Cors": "1.0.1",
"Microsoft.AspNetCore.Server.IISIntegration": "1.1.0",
"Microsoft.AspNetCore.Server.Kestrel": "1.1.0",
"Microsoft.EntityFrameworkCore.Design": "1.1.0",
"Microsoft.EntityFrameworkCore.SqlServer": "1.1.0",
"Microsoft.Extensions.Configuration.Abstractions": "1.1.0",
"Microsoft.Extensions.Configuration.EnvironmentVariables": "1.1.0",
"Microsoft.Extensions.Configuration.FileExtensions": "1.1.0",
"Microsoft.Extensions.Configuration.Json": "1.1.0",
"Microsoft.Extensions.Logging.Console": "1.1.0",
"Microsoft.Extensions.Logging.Debug": "1.1.0",
"Microsoft.Extensions.Options.ConfigurationExtensions": "1.1.0"
},
"tools": {
"Microsoft.EntityFrameworkCore.Tools": "1.1.0-preview4-final",
"Microsoft.AspNetCore.Server.IISIntegration.Tools": "1.1.0-preview4-final"
},
"frameworks": {
"netcoreapp1.1": {
"imports": [
"dotnet5.6",
"portable-net45+win8"
]
}
},
"buildOptions": {
"emitEntryPoint": true,
"preserveCompilationContext": true
},
"runtimeOptions": {
"configProperties": {
"System.GC.Server": true
}
},
"publishOptions": {
"include": [
"wwwroot",
"Views",
"Areas/**/Views",
"appsettings.json",
"web.config"
]
},
"scripts": {
"postpublish": [ "dotnet publish-iis --publish-folder %publish:OutputPath% --framework %publish:FullTargetFramework%" ]
}
}My Startup looks like this: public void ConfigureServices(IServiceCollection services)
{
// Add framework services.
services.AddMvc();
services.AddCors();
services.AddDbContext<ApplicationDbContext>(options => options.UseSqlServer(Configuration["Data:DefaultConnection:ConnectionString"]));
services.AddDbContext<SecondContext>(options => options.UseSqlServer(Configuration["Data:DefaultConnection:ConnectionString"]));
// Register the Identity services.
services.AddIdentity<IdentityUser, IdentityRole>(options =>
{
options.Cookies.ApplicationCookie.Events = new CookieAuthenticationEvents
{
OnRedirectToLogin = context =>
{
// This should now return 401 when it wants to redirect to login
context.Response.StatusCode = (int) HttpStatusCode.Unauthorized;
return Task.FromResult(0);
}
};
})
.AddEntityFrameworkStores<ApplicationDbContext>()
.AddDefaultTokenProviders();
// Register the OpenIddict services, including the default Entity Framework stores.
services.AddOpenIddict<ApplicationDbContext>()
.AddMvcBinders()
// Enable the token endpoint (required to use the password flow).
.EnableTokenEndpoint("/connect/token")
// Allow client applications to use the grant_type=password flow.
.AllowPasswordFlow()
.AllowRefreshTokenFlow()
// During development, you can disable the HTTPS requirement.
.DisableHttpsRequirement()
// Register a new ephemeral key, that is discarded when the application
// shuts down. Tokens signed using this key are automatically invalidated.
// This method should only be used during development.
.AddEphemeralSigningKey();
services.AddSwaggerGen();
services.AddApplicationInsightsTelemetry(Configuration);
// Followed by my code adding my services and other dotnet core infra not related code.
}
public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory)
{
app.UseApplicationInsightsRequestTelemetry();
app.UseApplicationInsightsExceptionTelemetry();
loggerFactory.AddConsole(Configuration.GetSection("Logging"));
loggerFactory.AddDebug();
app.UseCors(builder => builder.AllowAnyHeader().AllowAnyMethod().AllowAnyOrigin());
app.UseIdentity();
app.UseOAuthValidation();
app.UseOpenIddict();
app.UseMiddleware(typeof(ErrorHandlingMiddleware)); // Custom error handler
app.UseMvc();
app.UseSwagger();
app.UseSwaggerUi();
}Thats about it for the info you need to have. I hope you can find out what gives the error. |
|
I use plain Authorize attribute, no additions. |
|
@HaoK ? |
|
The exception shows that something is looking for a "Bearer" authorization policy, typically this is from |
Nah, it doesn't do that (it doesn't even reference the authorization stack). The only place where the "Bearer" scheme is used is in the JWT/validation/introspection middleware, but none of them will register a magic authorization policy for you. |
|
I dont have any custom filters from my side of code nor do I have any "Bearer" in my code. And as I have said, the exception occurs even before any request can go through anywhere. And I dont know where the "Bearer" comes in or why it occurs, but it is somewhere. It works fine when I change my Microsoft.AspNetCore.Mvc and Microsoft.AspNetCore.Mvc.Cors like this 1.1.0 -> 1.0.1. Then it just works fine. So the way I see it, is that somewhere in there is the issue or it makes it happen then. At the moment it works for me now, but I hope you can find out why and what is the issue there. |
|
This patch bug is approved. Please use the normal code review process w/ a PR and make sure the fix is in the correct branch, then close the bug and mark it as done. |
#930 (comment)
The text was updated successfully, but these errors were encountered: