Regression: AuthMiddleware can't terminate the request based on the results of AuthenticateAsync for the default scheme

[Tratcher]

1.1:

Security/src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs

Lines 71 to 74 in 9ab1898

	if (!await handler.HandleRequestAsync())
	{
	await _next(context);
	}

2.0:

Security/src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs

Lines 41 to 61 in ba1eb28

	var handlers = context.RequestServices.GetRequiredService<IAuthenticationHandlerProvider>();
	foreach (var scheme in await Schemes.GetRequestHandlerSchemesAsync())
	{
	var handler = await handlers.GetHandlerAsync(context, scheme.Name) as IAuthenticationRequestHandler;
	if (handler != null && await handler.HandleRequestAsync())
	{
	return;
	}
	}
	var defaultAuthenticate = await Schemes.GetDefaultAuthenticateSchemeAsync();
	if (defaultAuthenticate != null)
	{
	var result = await context.AuthenticateAsync(defaultAuthenticate.Name);
	if (result?.Principal != null)
	{
	context.User = result.Principal;
	}
	}
	await _next(context);

The JwtBearer handle has an OnAuthenticationFailed event which used to be able to generate a response and call context.HandleResponse() to prevent the pipeline from continuing. This doesn't work anymore in 2.0. OIDC still works because it runs under HandleRequestAsync which still has flow control. The AuthMiddleware ignores all non-success results from the default handler, even Fail. See #1338 (comment)

Part of the trouble is that a handler can't tell if it's the default and if it's being invoked from a call path that makes sense to generate error responses from. The OAuth & OIDC handlers always can because they run on dedicated endpoints. JwtBearer, Cookie, Basic, etc. don't know if they're running as the default handler, if they were invoked imperatively, or if they're being invoked by Authorization. The middleware should at least honor Fail though.

[kevinchalet]

That's the expected behavior: Fail means "authentication failed", not "stop processing the request" (which is not always possible since flow control is not always guaranteed for AuthenticateAsync).

It's definitely something you won't be able to change in 2.1 as it will be a major breaking change.

Of course, there would be no ambiguity if you had not overruled the name I had proposed (i.e RejectAuthentication())...

[HaoK]

Yup Fail is return a failed auth result, its not control flow

[HaoK]

The new behavior looks reasonable to me, the handler is given a chance to handle the request, if it doesn't.... the middleware looks to do default authenticate, at this point its too late for the handler to stop control flow.

[HaoK]

Basically what I'm saying is this regression is by design. Authenticate has no control flow, the only control flow hangs off of the HandleRequests/events/remote endpoints which are not part of the core Authenticate...

Authenticate means I am asking for a ticket... You either have one or you don't, there's no longer any kind of kill the request side effect

[Tratcher]

Our JwtBearer sample does exactly this. We should figure out the correct pattern to use and update it.

Security/samples/JwtBearerSample/Startup.cs

Lines 58 to 69 in ba1eb28

	OnAuthenticationFailed = c =>
	{
	c.NoResult();
	c.Response.StatusCode = 500;
	c.Response.ContentType = "text/plain";
	if (Environment.IsDevelopment())
	{
	// Debug only, in production do not share exceptions with the remote host.
	return c.Response.WriteAsync(c.Exception.ToString());
	}
	return c.Response.WriteAsync("An error occurred processing your authentication.");

[timdoke]

For what it's worth, I agree with @Tratcher -- at least from a documentation point of view. I had no idea that the approach for the JwtBearerSample was to only notify me that the authentication failed. I thought it affected the flow.. The documentation, https://docs.microsoft.com/en-us/aspnet/core/migration/1x-to-2x/identity-2x, considers this type of authentication the same as the others.. I take it with this approach, the double hit of the HandleAuthenticateAsync is also intended (one with c.Response.HasStarted equal to false, and the second to true)? The RemoteAuthenticationContext looked more like it should be used for a full blown identity provider, rather than from a custom authentication scheme..

If I might make a humble suggestion.. would it be possible to add an optional boolean parameter to affect the flow?
services.AddAuthentication(....

[timdoke]

In addition, the documentation says this..
"The UseAuthentication method adds a single authentication middleware component which is responsible for automatic authentication and the handling of remote authentication requests. It replaces all of the individual middleware components with a single, common middleware component."
--https://docs.microsoft.com/en-us/aspnet/core/migration/1x-to-2x/identity-2x

From my perspective, I am using the AddAuthentication with UseAuthentication and from that documentation, it should affect the control flow...

[kevinchalet]

Our JwtBearer sample does exactly this. We should figure out the correct pattern to use and update it.

Just call c.Fail() from the OnAuthenticationFailed event and add an inline middleware that detects if authentication failed? Or for MVC apps, let [Authorize] return a challenge for you.

[HaoK]

JwtBearer is not a remote authentication request, nor does it handle requests at all. Its more like cookies, in that it just produces a ticket or not.

[HaoK]

Honestly the idea behind much of 2.0 was to design away stuff like JwtBearer doing control flow, it should succeed in authenticating or not. As @PinpointTownes mentions, you can still do whatever control flow you want after UseAuthentication based on the resulting httpContext.User. But yeah we should add a new sample for JwtBearer in AuthSamples

[timdoke]

@PinpointTownes The caller will still hit the end point even with c.Fail.. The only thing I've seen that stops it, is doing Context.ForbidAsync in HandleAuthenticateAsync or throwing an exception.. None of these seem to work with swagger, unfortunately.. And... You can't seem to get the correct status out of swagger without letting the flow continue to the end point. And, without that Context.ForbidAsync, the authentication code also gets hit twice..

[HaoK]

Your endpoints aren't protected with the equivalent of some kind of [Authorize("Bearer")]?

[timdoke]

No.. I wanted to do pure authentication, not authorization.. All the end points should be should be protected with authentication... The authentication code should not depend on authorization in my opinion.....

[kevinchalet]

@PinpointTownes The caller will still hit the end point even with c.Fail..

Heh no. If your actions are correctly decorated with [Authorize], MVC will detect the request is not authenticated and will return a challenge.

And as I said, if you want to make authentication required for your entire app (not just MVC), you can write a custom authorization middleware:

E.g

app.UseAuthentication();

app.Use(next => context =>
{
    if (context.User.Identity.IsAuthenticated)
    {
        return next(context);
    }

    return context.Response.WriteAsync("Boy, you're not authenticated.");
});

app.UseMvc();

app.UseAuthentication();

app.Use(next => context =>
{
    if (context.User.Identity.IsAuthenticated)
    {
        return next(context);
    }

    return context.ChallengeAsync("Bearer");
});

app.UseMvc();

[kevinchalet]

No.. I wanted to do pure authentication, not authorization..

Requiring authentication is part of the authorization process. Rejecting requests just because authentication failed is not something the authentication handlers should ever do.

[brockallen]

No.. I wanted to do pure authentication, not authorization

Requiring an authenticated caller is authorization.

[Eilon]

Discussed in triage, we should update/create a sample in the https://github.com/aspnet/AuthSamples repo for this.

[timdoke]

For what it's worth, the way I've always thought of "authentication" versus "authorization", is authentication validates that we have a valid user. Authorization validates the authenticated user has permission to get to the end point. What I've done in the past, is verify that the authenticated user has certain claims to authorize an end point. To me, you should be able to authenticate without authorization. Conversely, you should not be able to authorize without authenticating first. Some end points, for example a status end point, I want to make sure there is a valid user hitting it; but I don't care if they haven't explicitly been granted permission to view that status end point -- from that perspective,, they should be authenticated but not authorized. However, I do sort of see what you mean @brockallen when you say requiring an authenticated caller is authorization.. But, this doesn't totally jive with this: https://en.wikipedia.org/wiki/Authentication#Authorization

From what I'm hearing, it sounds like the argument is being made that when you authenticate, you should always authorize as well. Just my opinion again -- but, to me that is not intuitive and they should be distinct functions..

Of course, this is counter intuitive because it is the Authorization header that starts all this. I just checked some old code from net46, and it looks like there we registered a global AuthorizationFilterAttribute; so, maybe I'm off.

@PinpointTownes I do appreciate the example of providing a global authentication filter via an authorization middleware -- I'll try that out. Thanks!

[Tratcher]

Conceptually we're on the same level, the gap is that when implementing this we decided that AuthN should not be responsible for rejecting requests. If AuthN could not identify the user then the request would be considered anonymous and it would be up to the endpoint to decide if anonymous access was allowed. We consider the inclusion or exclusion of anonymous users as an AuthZ decision.

[timdoke]

Thanks a lot for the great explanations. And thanks @PinpointTownes for the example on the authorization middleware.. That works with my authentication handler exactly as I wanted it to.

[timdoke]

Hey @Tratcher.. Can you tell me if this is the intended behavior?

I created a global authorization filter and it works fine. I then put an allowanonymous attribute on a method. When that api method is hit, the flow still goes through the authenticate handler first (because I have the global AddAuthentication handler set up), and it fails. Then, it goes through the authorization filter and because the api method had allowanonymous, the context succeeds (in the authorize filter I check for a IAllowAnonymousFilter and if it exists, I return).

It seems to me, that if you have allowanonymous on your api method, the flow should not go through the authentication handler. I tried removing the AddAuthentication call to see if that was the problem -- and unfortunately, I got a runtime error while calling context.HttpContext.AuthenticateAsync(scheme) from inside the authorize filter from a non anonymous call. The error was that particular scheme was not mapped. The authorization filter is based on this code: https://andrewlock.net/introduction-to-authorisation-in-asp-net-core/.

That seems like odd behavior. If I have an api method set as allowanonymous, the flow shouldn't still go through the authentication handler. If I take out the global addauthentication call however, and try to replace the authentication flow to run from the authorization filter, I shouldn't get a runtime error while calling context.HttpContext.AuthenticateAsync(scheme) when the call is not anonymous..

Just wondering if this is the expected behavior. Thanks!

[timdoke]

Ah.. About my comment above.. It appears it called the authentication handler on anonymous because I had a default token scheme defined. Getting rid of the default token scheme fixed that issue.