ASP.NET Core 7.0's default authentication handler fallback breaks OpenIddict
kevinchalet opened this issue · 5 comments
Is there an existing issue for this?
- I have searched the existing issues
Describe the bug
As described in aspnet/Announcements#490, ASP.NET Core 7.0 now has a fallback mechanism to select a default scheme when none is set by the user. Unfortunately, this logic breaks OpenIddict.
Take the following minimal API application that sets up an instance of OpenIddict server with no backing database and adds a /token endpoint to generate grant_type=password token responses with a fake identity:
<Project Sdk="Microsoft.NET.Sdk.Web">
<PropertyGroup>
<TargetFramework>net7.0</TargetFramework>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="OpenIddict.AspNetCore" Version="3.1.1" />
</ItemGroup>
</Project>using System.Security.Claims;
using Microsoft.AspNetCore;
using Microsoft.IdentityModel.Tokens;
using OpenIddict.Abstractions;
using OpenIddict.Server.AspNetCore;
using static OpenIddict.Abstractions.OpenIddictConstants;
using static OpenIddict.Server.OpenIddictServerEvents;
var builder = WebApplication.CreateBuilder(args);
builder.Services.AddOpenIddict()
.AddServer(options =>
{
options.SetTokenEndpointUris("/token");
options.AllowPasswordFlow()
.AcceptAnonymousClients()
.EnableDegradedMode();
options.AddDevelopmentEncryptionCertificate()
.AddDevelopmentSigningCertificate();
options.UseAspNetCore()
.EnableTokenEndpointPassthrough();
options.AddEventHandler<ValidateTokenRequestContext>(builder =>
builder.UseInlineHandler(context =>
{
// Ignore client authentication and consider the request valid.
return default;
}));
});
builder.Services.AddAuthorization();
var app = builder.Build();
app.UseAuthentication();
app.UseAuthorization();
app.MapGet("/", () => "Hello World");
app.MapPost("/token", (HttpContext context) =>
{
var request = context.GetOpenIddictServerRequest()!;
if (request.IsPasswordGrantType())
{
// Build a fake identity and ask OpenIddict to return an OAuth 2.0 token response.
var identity = new ClaimsIdentity(TokenValidationParameters.DefaultAuthenticationType);
identity.AddClaim(new Claim(Claims.Subject, "alice@wonderland.com"));
return Results.SignIn(new ClaimsPrincipal(identity), null,
OpenIddictServerAspNetCoreDefaults.AuthenticationScheme);
}
throw new NotImplementedException();
});
await app.RunAsync();Expected Behavior
If you send the following request when the application uses .NET 6.0...
POST /token HTTP/1.1
Host: localhost:7273
Content-Type: application/x-www-form-urlencoded
Content-Length: 83
grant_type=password&username=alice%40wonderland.com&password=alice... you'll get a proper response:
{
"access_token": "[...]",
"token_type": "Bearer",
"expires_in": 3599
}If you send the same request when targeting .NET 7.0, you'll get an exception:
InvalidOperationException: An identity cannot be extracted from this request.
This generally indicates that the OpenIddict server stack was asked to validate a token for an endpoint it doesn't manage.
To validate tokens received by custom API endpoints, the OpenIddict validation handler (e.g OpenIddictValidationAspNetCoreDefaults.AuthenticationScheme or OpenIddictValidationOwinDefaults.AuthenticationType) must be used instead.
OpenIddict.Server.OpenIddictServerHandlers+ValidateAuthenticationDemand.HandleAsync(ProcessAuthenticationContext context)
To reduce misuses and avoid subtle security issues, OpenIddict's IAuthenticationHandler safeguards the Authenticate/Challenge/SignIn/SignOut operations to ensure they are called in a correct and safe context. For the same reason, it also registers an IPostConfigureOptions<AuthenticationOptions> implementation that ensures the OpenIddict server handler is never registered as a default scheme in the options:
public void PostConfigure(string? name, AuthenticationOptions options)
{
if (options is null)
{
throw new ArgumentNullException(nameof(options));
}
if (!TryValidate(options.SchemeMap, options.DefaultAuthenticateScheme) ||
!TryValidate(options.SchemeMap, options.DefaultChallengeScheme) ||
!TryValidate(options.SchemeMap, options.DefaultForbidScheme) ||
!TryValidate(options.SchemeMap, options.DefaultScheme) ||
!TryValidate(options.SchemeMap, options.DefaultSignInScheme) ||
!TryValidate(options.SchemeMap, options.DefaultSignOutScheme))
{
throw new InvalidOperationException(SR.GetResourceString(SR.ID0109));
}
static bool TryValidate(IDictionary<string, AuthenticationSchemeBuilder> map, string? scheme)
{
// If the scheme was not set or if it cannot be found in the map, return true.
if (string.IsNullOrEmpty(scheme) || !map.TryGetValue(scheme, out var builder))
{
return true;
}
return builder.HandlerType != typeof(OpenIddictServerAspNetCoreHandler);
}
}Trying to call AuthenticateAsync(OpenIddictServerAspNetCore.AuthenticationScheme) for every request doesn't make sense: when OpenIddict detects you tried to call AuthenticateAsync(OpenIddictServerAspNetCore.AuthenticationScheme) on an unsupported endpoint, you get an InvalidOperationException that tells you you're doing something wrong.
Unfortunately, ASP.NET Core 7.0's "use whatever authentication handler is present as the default scheme when none is set" basically ends up calling AuthenticateAsync(OpenIddictServerAspNetCore.AuthenticationScheme) for every request when the OpenIddict server is the only authentication handler present in the handlers collection.
Steps To Reproduce
No response
Exceptions (if any)
No response
.NET Version
No response
Anything else?
While .NET 7.0's current previews include a Microsoft.AspNetCore.Authentication.SuppressAutoDefaultScheme switch to that logic off, it's very likely it will be removed in future versions, so a different approach should be considered to avoid such issues.
Potential approches:
-
Revert this change to use the same approach as in previous .NET versions.
-
Add new methods to
IAuthenticationHandlerto allow determining whether the instance can be selected as a default scheme handler when none is set. Something like:
public interface IAuthenticationHandler
{
// ...
Task<bool> CanBeUsedAsDefaultAuthenticateSchemeAsync(HttpContext context)
=> Task.FromResult(true);
Task<bool> CanBeUsedAsDefaultChallengeSchemeAsync(HttpContext context)
=> Task.FromResult(true);
Task<bool> CanBeUsedAsDefaultForbidSchemeAsync(HttpContext context)
=> Task.FromResult(true);
Task<bool> CanBeUsedAsDefaultSignInSchemeAsync(HttpContext context)
=> Task.FromResult(this is IAuthenticationSignInHandler);
Task<bool> CanBeUsedAsDefaultSignOutSchemeAsync(HttpContext context)
=> Task.FromResult(this is IAuthenticationSignOutHandler);
}/cc @DamianEdwards
Just to clarify, the new behavior only sets the default scheme if a single authentication scheme is registered. If multiple schemes are registered there is no change in behavior WRT the default scheme.
We don't intend on changing the new behavior and it's much too late for .NET 7 at any rate. Given that OpenIddict is already configuring authentication and validating options when registered, it could do one of the following to ensure that when it's used it's not selected as the default scheme:
- Set the
AppContextswitch to disable the new behavior - Register more than one authentication scheme, the second being a null handler that's never used just to ensure a default scheme is not selected.
We don't intend on changing the new behavior and it's much too late for .NET 7 at any rate.
Then what's the point of releasing preview/RC versions? I thought the whole point of pre-release versions was to give the community enough time to test them and share feedback that can be taken into account in the final product. At least, that's how it used to work in the initial ASP.NET Core versions. But clearly something has changed lately 😕
(it's funny to see that design decisions resulting in regressions like this one are not considered important enough while requests to accommodate non-standard HTTP stuff from Microsoft teams are eventually implemented in LTS patch versions 😄)
Set the
AppContextswitch to disable the new behavior
As I said, that switch - like any other switch used in ASP.NET Core - is unlikely to stay so it would be a temporary thing. What will happen in .NET 8.0 when the switch is removed?
Register more than one authentication scheme, the second being a null handler that's never used just to ensure a default scheme is not selected.
Well, it's a very ugly hack. Another one would be to force the users to explicitly set .Default*Scheme in the options and throw an exception if it's not populated to opt out of the new behavior
Anyway, feel free to close this thread if you don't plan on improving things on your side. To be honest, it's sad to see the ASP.NET team no longer cares about breaking third-party projects like OpenIddict (and I'm not even the only one thinking that: FluentValidation/FluentValidation#1959). For me, this apparent lack of consideration has become my main source of OSS fatigue. Just sayin' 😕
Then what's the point of releasing preview/RC versions? I thought the whole point of pre-release versions was to give the community enough time to test them and share feedback that can be taken into account in the final product. At least, that's how it used to work in the initial ASP.NET Core versions. But clearly something has changed lately 😕
The announcement for this change was made in July as part of the preview.7 release.
As I said, that switch - like any other switch used in ASP.NET Core - is unlikely to stay so it would be a temporary thing. What will happen in .NET 8.0 when the switch is removed?
We have no intention of removing this switch. Indeed, if this scenario (single authn scheme but no default) is common enough we can consider elevating the property on AuthenticationOptions that wraps the switch to be public in .NET 8.
Well, it's a very ugly hack.
There are likely other approaches. Your existing logic to validate theAuthenticationOptionsdoes not set the default to OpenIddict's scheme is slightly flawed in that the options simply feed into the defaultIAuthenticationSchemeProvider. TheIAuthenticationSchemeProvider.GetDefaultXXXSchemeAsync()methods are actually the source of truth for which scheme is the default for any given action (including the new auto-default behavior). So another approach could be to wrap/replace the configuredIAuthenticationSchemeProviderto outright disable the behavior there (the auto-default scheme behavior is evaluated every time a scheme is added or removed).
For me, this apparent lack of consideration has become my main source of OSS fatigue. Just sayin' 😕
We strive to balance these kinds of changes by considering the trade-offs between "good" defaults, impact to ecosystem (like libraries), difficultly of applying workarounds, etc. It's often very difficult to make all parties happy, and in this case, we feel the trade-off of a simplified default experience is worthwhile.
The announcement for this change was made in July as part of the preview.7 release.
What's your point? That library authors should immediately test their libraries against the latest previews as soon as they ship (in the middle of the summer)? 😄
In this case, let me ask you why you don't immediately test popular libraries to ensure they don't regress when you introduce changes that you suspect could be a potential source of pain. In both cases, this seems unrealistic to me.
We have no intention of removing this switch.
Thanks. I'll update the OpenIddict docs to mention it so that OpenIddict 3.x users will know it exists. You'll probably want to update that announcement - and the corresponding section on learn.microsoft.com - because the name of the switch is incorrect: it's Microsoft.AspNetCore.Authentication.SuppressAutoDefaultScheme, not just SuppressAutoDefaultScheme.
The
IAuthenticationSchemeProvider.GetDefaultXXXSchemeAsync()methods are actually the source of truth for which scheme is the default for any given action (including the new auto-default behavior). So another approach could be to wrap/replace the configuredIAuthenticationSchemeProviderto outright disable the behavior there (the auto-default scheme behavior is evaluated every time a scheme is added or removed).
Overriding infrastructure types in a library affects the entire application. What if another library decided to override IAuthenticationSchemeProvider to do something else? One of the two libraries wouldn't get the expected behavior as you could have only one instance. We had exactly the same issue with ICorsPolicyProvider and some libraries (e.g IdSrv) had to use decorated services (which are not supported OOTB by MSFT's DI container) to work around that. The result was at best quite bad.
We strive to balance these kinds of changes by considering the trade-offs between "good" defaults, impact to ecosystem (like libraries), difficultly of applying workarounds, etc. It's often very difficult to make all parties happy, and in this case, we feel the trade-off of a simplified default experience is worthwhile.
Correct me if I'm wrong but I haven't seen many recent cases where the maintainer(s) of a popular library was/were informed of a potential breaking change before its adoption to have a chance to discuss it. Getting early feedback is easy as a simple @ mention 😄
Announcements such as the one posted by Jemery Skinner should be seen as red flags: it's totally normal to introduce breaking changes, but breaking scenarios is another story and just saying "well', it's too late anyway" when we report issues is honestly very bad. It's extremely worrying to see someone saying decisions you take - not you specifically but your team in general, to be clear - can a source of "OSS-burnout" for maintainers of .NET libraries. As a public figure of the .NET ecosystem, it's something that should alert you.
(regarding this specific case, I'll just add a options.AddScheme<IAuthenticationHandler>(Guid.NewGuid().ToString(), displayName: null) call and consider it done... it's the least terrible option)
I've updated the referenced announcements/docs to ensure the app context switch name is correct (thanks for the pointer).