AuthZ 2.0 Improve failure scenarios + [401 vs 403?]

[HaoK]

The two somewhat related scenarios we are trying to address:

1. How to determine which requirements failed and easily display them
2. Remove the automatic challenge behavior/logic and make 401/403 more explicit (somehow)

How things work today:

1. bool IAuthorizationService.AuthenticateAsync only returns sucess or failure, its impossible to deduce what caused the failure.
2. See AuthorizeFilter.OnAuthorization.

Basically the caller is responsible for building up the ClaimsPrincipal for authorization. There's a hint on the policy via the AuthenticationSchemes which the caller should use for Authentication and merge together for the final principal. The first important point here is that we no longer know what claims came from which authentication scheme (claims transformation also is a wrinkle so even if only a single scheme was used, there's no guarantee).

Next the IAuthorizationService checks the requirements from the AuthorizationPolicy against the merged principal and if it fails, we challenge all of the schemes. So today the Challenge(401) vs Forbid(403) behavior is completely left to the implementations of the authentication middleware for how they respond to the challenges. The default implementation of that behavior is to return 401 if the scheme does not have an authentication ticket, and 403 if it does (i.e. if a cookie/bearer token exists, or the remote identity already has a signed in identity)

Ideas for improvement:

1. The needed change is to enable getting back a new AuthorizationResult that contains the failed requirements, that should be sufficient to enable callers to display error information. We should also add some mechanism to describe the requirement failure to make it easy to generate this data, maybe a AuthZFailureDescriber<TRequirement>?

2. We'd like to try and eliminate the ChallengeBehavior.Automatic and change the meaning of Challenge to always result in Unauthorized/401 behavior. And Forbid should be called explicitly instead of something that happens to some challenges. Callers like AuthorizeFilter would need to have enough information to make the choice.

Scenarios to target:

1. [Authorize("Administrator")] protects an action, when a regular user hits the action, an error page stating that the user was forbidden because he was either not in the Adminstrator role, or did not have the Administrator claim (depending on how the policy was implemented)

Related issues that should also be addressed:

aspnet/Diagnostics#316
#860
#872
openiddict/openiddict-samples#33 (comment)

cc @blowdart @Tratcher @Eilon @davidfowl @PinpointTownes

[HaoK]

per #860 consider adding verbose logging for successful requirements

[HaoK]

As part of this work, we can consider revisiting the automatic challenge / 401 vs 403 logic that currently is baked into authN.

See openiddict/openiddict-samples#33 (comment)

cc @Tratcher @blowdart @PinpointTownes

[HaoK]

So lets start brainstorming how we want the new AuthorizeFilter to look with the idea being it will have logic that explicitly calls either Challenge (401), or Forbid (403) instead of relying on the current Automatic behavior that the base auth handler has today:

See https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Authorization/AuthorizeFilter.cs#L128

            // Note: Default Anonymous User is new ClaimsPrincipal(new ClaimsIdentity())
            if (!await authService.AuthorizeAsync(httpContext.User, context, effectivePolicy))
            {
                context.Result = new ChallengeResult(effectivePolicy.AuthenticationSchemes.ToArray());
            }

@PinpointTownes suggested previously that we basically introduce a new RequireAuthentication bit, which by default only the RequireAuthenticatedUser requirement would flip. So the filter could check that bit, and challenge only when RequireAuthentication is the failure, otherwise it would 403.

So we'd have something like

   // New result shape for Authorize
   public class AuthorizationResult {
     bool Success { get; }
     bool RequiresAuthentication { get; }
     bool IsHardFail { get; } // the kill bit call to context.Fail()
     IEnumerable<IAuthorizationRequirement> FailedRequirements { get; }
   }

   // filter code becomes
   var result = await services.AuthorizeAsync(...);
   if (!result.Success) {
      if (result.RequiresAuthentication) {
          context.Result = new ChallengeResult(effectivePolicy.AuthenticationSchemes.ToArray());
      }
      else {
           // Assuming this result exists, otherwise call context.Forbid on each scheme
           context.Result = new ForbidResult(effectivePolicy.AuthenticationSchemes.ToArray());
      }
   }

Given that the result will have all the failed requirements, we could eliminate the RequiresAuthentication flag and just have the Filter look for the presence of that requirement not being met instead of baking it in to the default authentication service...

Thoughts? @blowdart @Tratcher @davidfowl @PinpointTownes

[HaoK]

Regarding the 'friendly' error reporting part of this, we could make it possible to associate a specific error message per requirement via something like an IAuthorizationErrorDescriber which would map IAuthorizationRequirement to failure strings for display purposes.

[Tratcher]

The normal flow for Forbidden would be A) The user is Authenticated, B) With an auth type I accept (optional?), but C) lacks a permission/claim. If we challenged the user in this situation it may cause an infinite loop (though Windows Auth usually aborts if it keeps getting challenged). If it was just A or B then challenging would be appropriate. Are A, B, and C separate or combine requirements? If they're combine then it could set a Forbidden flag. If they're evaluated separately then how to you aggregate them?

[HaoK]

Yeah so the idea is, when to only challenge when A fails, C would result in a Forbid.

Scenario B is likely a hole/flaw with the proposed design, where you are Authenticated but not with one of the passive schemes the policy asks for. That looks the same as C in this design because the principal will just be missing the claim.

[kevinchalet]

Given that the result will have all the failed requirements, we could eliminate the RequiresAuthentication flag and just have the Filter look for the presence of that requirement not being met instead of baking it in to the default authentication service...

That was my initial approach, but it would prevent a few interesting scenarios like allowing an authorization handler to force a user to re-authenticate, which is not well supported in the current bits, due to the automatic Challenge -> Forbidden conversion.

(in sensitive apps, it's quite common to ask users to re-enter their password when accessing critical areas if the authentication cookie is considered "too old").

[HaoK]

Updated the issue summary to add background, I'll start add the explicit scenarios we want to support next, if anyone has any scenarios they want considered as we try to redesign this, please add a comment.

[HaoK]

I don't remember anymore what scenario exactly it was that required support for multiple authentication schemes, was it mixing cookies + bearer and having a single endpoint that worked against either one?

[blowdart]

That's not concurrent it's either or :) So in that scenario I'd expect [Authorize(Schemes = "Cookie, Bearer")] and it'd work with either. What I can't understand is what you want to do with OptionalSchemes = "D,E" in your example.

[kevinchalet]

That's not concurrent it's either or :)

For authentication, it's additive. All the scheme handlers listed in ActiveAuthenticationSchemes are invoked and the resulting identities are merged into a single principal, that is attached to HttpContext.User. When returning a challenge, all the specified handlers are invoked and can alter the response.

For authorization, I'd also go with the "either or" option.

[HaoK]

That's to have functional equivalence to what we have with today's Schemes = "Cookie, Bearer" where we'd return a 401 if authorization fails. If we make them schemes 'required', then you'd have to have Cookie AND bearer.

[blowdart]

This still doesn't make sense.

Right now we have Schemes = Cookie, Bearer. Which means it'll run through both, and if either produce an identity we're good, right?

But

[Authorize(RequiredSchemes = "A,B,C", OptionalSchemes = "D,E"]

Required indicates, to me, "You have to have all of these, no matter what", and optional is just weird. It doesn't say what it means. Does optional also run that middleware? Do they run if required has already failed? It's not clear what you're trying to do from either the parameter names, or the comment.

[HaoK]

Not quite, requirements can be satisfied from either scheme, and today we wouldn't challenge for either of them unless we didn't find a claim we were looking for.

The split is to control the 401 vs 403 behavior.

So you'd use Optional when you want to call authenticate on those schemes but don't want them to trigger a 403 if there's no information for that scheme.

[kevinchalet]

No need to end up with something complicated.

If at least one of the authentication handlers returns an identity, assume that a Forbidden result should be returned by default (i.e the existing behavior), but allow people to override the default "policy evaluator" to force returning a Challenge if they prefer requiring multiple schemes.

[HaoK]

Sure, I'm ok with that, we do something sane by default, and have the new interface for the logic so people can override and do their own thing.

[blowdart]

Yea, the attribute wording doesn't make your desired intent clear at all I'm afraid. The attribute isn't the right place to put this :)

[Tratcher]

I think this is getting sidetracked. Challenge is for when I don't know who you are (e.g. Authenticate("RequiredScheme") failed). Forbidden is for when I know who you are, but you don't have sufficient permissions/claims.

[Edit] nevermind, I hadn't finished reading to the end of the thread.

[kevinchalet]

Challenge is for when I don't know who you are (e.g. Authenticate("RequiredScheme") failed). Forbidden is for when I know who you are, but you don't have sufficient permissions/claims.

It's a bit more subtle, actually. There are cases where you know who the user is (because you retrieve a valid identity) but want him to re-authenticate anyway (e.g when you want to force your users to have a fresh cookie to access sensitive parts of your app): in this case, returning a Challenge is the right thing to do, even if the user is already authenticated.

[Tratcher]

Sure, but is that something you'd be implementing from an authz policy, or manually in a controller?

[kevinchalet]

Ideally, one should be able to do that in an authorization policy. Looks like Hao's API proposal already takes my scenario into account, as it allows flowing a RequiresAuthentication flag from the authorization stack to the higher layers (e.g MVC/SignalR's filters):

public class AuthorizationResult {
    bool Success { get; }
    bool RequiresAuthentication { get; }
    bool IsHardFail { get; } // the kill bit call to context.Fail()
    IEnumerable<IAuthorizationRequirement> FailedRequirements { get; }
}

[blowdart]

In the subtle case I'd expect that to be in a policy, and policies to have the ability to re-kick off auth; for example "I login and can see my bank account list, but if I need to transfer I must login again", you'd check for a claim saying "I did it a second time"

[kevinchalet]

@blowdart that's precisely what I'm asking for... for more than 2 years now

[HaoK]

as it allows flowing a RequiresAuthentication flag from the authorization stack to the higher layers (e.g MVC/SignalR's filters):

As part of this change, all the logic in the higher layers will get absorbed into the new AuthorizationPolicyEvaluator, so the idea is the filters will become very simple and just invoke the service and do something based on the result.

[HaoK]

So to summarize where I think we have landed?

• Nuke ChallengeBehavior
• If any of the authentication schemes were successfully authenticated, and authorization fails, we will return AuthorizationResult.Forbidden. Otherwise we will return AuthorizationResult.RequiresAuthentication. AuthZ handlers should also have a way to explicitly toggle requiring authentication which would override the forbidden case.

I'll update #1193 once we are agreed

[kevinchalet]

Nuke ChallengeBehavior

Oh yeah, kill it, kill it, KILL IT! 🔥

Sounds good. Any particular reason not to name RequiresAuthentication Challenge (to match the other result name: Forbidden)?

[HaoK]

We can hash out names in the PR, its Challenged right now which is kinda meh.

[blowdart]

Summarising from a meeting this afternoon.

The scenario for two auth schemes is for people who want to expose an API and use it both from 3rd party clients with bearer, and their own app with cookies, because that's easiest for them. The fun part in this is choosing which challenge to send back, because it's client dependent. Traditionally we had the IsAjax test, looking for a header which, if present, stops the cookie auth redirect and returns the status code. Our plan is to keep that, but make it configurable so you can dump your own code in there should you want to head down the same API route; then you can make your own bad decisions :)

As for auth upgrades, we'll add that as a potential new feature for 2.1, because it's going to take the ability to say "Hey reauth due to this reason", which requires a bunch of plumbing to expose.

[leastprivilege]

expose an API and use it both from 3rd party clients with bearer, and their own app with cookies, because that's easiest for them

And how do you protect against CSRF in such a scenario?

[blowdart]

And how do you protect against CSRF in such a scenario?

Well yea, $64,000 question. And the answer is "Don't do that, use separate endpoints". But people keep doing it anyway. At some point I have to just accept that some folks want to shoot themselves in the foot.

[HaoK]

PR has been updated with the new behavior: ecdcf2d

[HaoK]

e940cdb

[achidlow]

Based on the "Scenarios to target" in the description of this issue, and the related issue that was closed in preference to this one (#872), are we supposed to be able to add data to the body in case of a failed authorization now?

I can't see any way to do that, and this change doesn't seem to enable that to happen either, since the last return point from Security to Mvc returns a PolicyAuthorizationResult which discards the information about which policies failed.

[HaoK]

Its not all the way there yet, the failed requirements are in the result now, but we still need to do some work to make it easy to utilize that information to generate a reasonable description for why authorization failed.