dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.38k stars 10k forks source link

[Authorize(AuthenticationSchemes = TheOnlyAuthScheme)] also runs default scheme AuthenticationHandler #47105

Open janseris opened 1 year ago

janseris commented 1 year ago

Is there an existing issue for this?

Describe the bug

The documentation https://learn.microsoft.com/en-us/aspnet/core/security/authorization/limitingidentitybyscheme?view=aspnetcore-7.0 says:

The [Authorize] attribute specifies the authentication scheme or schemes to use regardless of whether a default is configured.

[Authorize(AuthenticationSchemes=JwtBearerDefaults.AuthenticationScheme)]
public class Mixed2Controller : Controller
{
    public ContentResult Index() => Content(MyWidgets.GetMyContent());
}

In the preceding code, only the handler with the "Bearer" scheme runs. Any cookie-based identities are ignored. Note: JwtBearerDefaults.AuthenticationScheme is the "Bearer" string constant here.

This is not true for me. When a default authentication scheme is registered, its authentication handler will be executed upon success of the "only" authentication scheme which is requested by the annotation [Authorize(AuthenticationSchemes=JwtBearerDefaults.AuthenticationScheme)] on the REST API controller or REST API controller method.

Repro project: https://github.com/janseris/ASPNetCoreMultipleAuthentications

How to repro: 1) run project 2) in Swagger UI, click green Authorize button which enables you to enter HTTP Authorization header content to be passed in to the controller image

3) In the Session ID form, enter in value SessionID 123 which passes the validation in the SessionID AuthenticationHandler and click green Authorize button to apply

4) execute a controller method via Swagger UI by clicking the method, then Try it Out and then blue Execute button image

5) see debug output in Visual Studio - you will see that the Session ID authentication handler executed and finished and then HTTP Basic authentication handler executed (unexpected) which you can also observe in the browser because the a prompt for HTTP Basic authentication credentials will show as a result of failed HTTP Basic authentication (because the authentication/authorization header did not contain a valid HTTP Basic authentication value because it was SessionID 123) image

Expected Behavior

When [Authorize(AuthenticationSchemes=JwtBearerDefaults.AuthenticationScheme)] annotation is used, only that single AuthenticationHandler (registered under the authentication scheme name JwtBearerDefaults.AuthenticationScheme is executed).

That in my case would be the session ID authentication handler specified on the controller method:


public class ItemsController : ControllerBase
{
    //only "SessionID" authentication schema handler should be executed.
    [Authorize(AuthenticationSchemes = SessionIDAuthenticationHandler.AuthenticationSchemeName)] 
    [HttpGet("", Name = "GetAllItems")]
    [ProducesResponseType(typeof(IEnumerable<Item>), StatusCodes.Status200OK)]
    public Task<IEnumerable<Item>> GetAll()
    {
        var items = new List<Item>
        {
            new Item
            {
                Name = "item1"
            },
            new Item
            {
                Name = "item2"
            }
        };
        return Task.FromResult(items.AsEnumerable());
    }
}

Steps To Reproduce

Repro project: https://github.com/janseris/ASPNetCoreMultipleAuthentications

Repro steps: 1) run project 2) in Swagger UI, click green Authorize button which enables you to enter HTTP Authorization header content to be passed in to the controller image

3) In the Session ID form, enter in value SessionID 123 which passes the validation in the SessionID AuthenticationHandler and click green Authorize button to apply

4) execute a controller method via Swagger UI by clicking the method, then Try it Out and then blue Execute button image

5) see debug output in Visual Studio - you will see that the Session ID authentication handler executed and finished and the HTTP Basic authentication handler executed (unexpected) which you can also observe in the browser because the default browser dialog for HTTP Basic authentication will show image

Exceptions (if any)

No response

.NET Version

.NET SDK 7.0.200 commit 534117727b

Anything else?

No response

ghost commented 1 year ago

To learn more about what this message means, what to expect next, and how this issue will be handled you can read our Triage Process document. We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. Because it's not immediately obvious what is causing this behavior, we would like to keep this around to collect more feedback, which can later help us determine how to handle this. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact work.

halter73 commented 1 year ago

It's true that the default scheme's AuthenticateAsync/HandleAuthenticateAsync methods get called by the authentication middleware regardless of what's set by [Authorize(AuthenticationSchemes=...)]. The docs should be clearer about this.

However, this shouldn't be a problem if AuthenticateAsync does not mutate the response. You might have noticed than in your sample, the Controller action still gets run and writes the response body despite the basic auth handler setting the 401 response and WWW-Authenticate response header. That's because the failed default AuthenticateResult from HttpBasicAuthenticationHandler is being ignored by the authorization middleware since a specific scheme was specified.

If you delete all the calls to AddAuthenticationFailedInfoToResponse from HttpBasicAuthenticationHandler.HandleAuthenticateAsync and instead move it to HandleChallengeAsync everything should work because HandleChallengeAsync will only be called if the scheme is not overridden despite HandleAuthenticateAsync returning a failed AuthenticateResult.

protected override Task HandleChallengeAsync(AuthenticationProperties properties)
{
    Response.Headers.Add(HeaderNames.WWWAuthenticate, $"{AuthenticationSchemeName} realm=\"{Realm}\"" /* realm is special info added in HTTP Basic auth fail */);
    return base.HandleChallengeAsync(properties);
}

Also, UseAuthentication should always be called before UseAuthorization. It might not matter if you specify the scheme using [Authorize] since the authorization middleware runs the authentication handler inline, but it matters in other scenarios.

I think in .NET 7 we warn about this, but in .NET 7 you can also just omit those calls. WebApplicationBuilder will add both auth middlewares by default if you've configured authentication. @captainsafia

app.UseAuthentication();
app.UseAuthorization();
janseris commented 1 year ago

@halter73 thank you I will investigate that in the following days

janseris commented 1 year ago

It's true that the default scheme's AuthenticateAsync/HandleAuthenticateAsync methods get called by the authentication middleware regardless of what's set by [Authorize(AuthenticationSchemes=...)]. The docs should be clearer about this.

The docs say the exact contrary, so I think the framework is not working as intended because the behavior documented in docs makes perfect sense but is not followed by the framework. I don't want the default authentication handler executed when it doesn't need to. Why should the default handler be executed when it's explicitly not supposed to be executed?

However, this shouldn't be a problem if AuthenticateAsync does not mutate the response

I have an issue that I check in database in authentication handler. This slows down the application when unwanted authentication handlers are executed and breaks the intended authentication flow.

The intended authentication and authorization mechanism is: 1) Login endpoint - should run APIKey authentication handler. This assures only allowed application clients (e.g. only the latest mobile app version) can call the REST API. 2) Public endpoint - can be called by anyone. Does not require APIKey authentication. 3) Any other endpoint - requires authentication of the caller by APIKey and SessionID. SessionID identifies the user and is later used to load user rights and authorizes the request after authentication passes. APIKey makes sure only selected application clients can call the endpoint (e.g. only latest mobile app version or only all mobile app versions etc.) as in case 1.

janseris commented 1 year ago

Also, UseAuthentication should always be called before UseAuthorization. It might not matter if you specify the scheme using [Authorize] since the authorization middleware runs the authentication handler inline, but it matters in other scenarios. I think in .NET 7 we warn about this

I have changed target framework to .NET 7 and reopened VS and rebuilt the solution. I cannot see any warning in the IDE code editor or any compile-time warning/error.

janseris commented 1 year ago

@halter73 When HTTP Basic authentication handler (my custom) and Session ID Authentication handler (my custom) are regsitered and HTTP Basic Authentication handler's scheme name is set as default in AddScheme() and both are registered using AddScheme(), then if [Authorize(AuthenticationSchemes = "SessionID")] is written on method, the HTTP Basic authentication handler still gets executed (and succeds or fails in the background and that's ignored but it's executed while is not supposed to be executed).


This has for example an unwanted side-effect that WWW-Authenticate HTTP header is unexpectedly added to the response (as it would have been set if HTTP Basic or any other authentication failed).

Example: Endpoint is supposed to only support and check for SessionID authentication. The user passes in SessionID in HTTP Authorize header and the authentication succeeds. Then the backend finds out that the user is unauthorized. In this case, 403 is returned (OK) with WWW-Authenticate header set from failed HTTP Basic authentication handler (unexpected).

image

Btw. this is even more wrong because the purpose/semantics of sending WWW-Authenticate header in a HTTP response is to inform user about available actions to authenticate themselves. That makes no sense here because:

captainsafia commented 1 year ago

I have changed target framework to .NET 7 and reopened VS and rebuilt the solution. I cannot see any warning in the IDE code editor or any compile-time warning/error.

Interesting note. We added support for minimal APIs in the startup analyzers (like the auth-middleware ordering one here) in .NET 6 but I'm able to repro what you're seeing so there might be something going on behind the scenes.

Do you mind filing a separate issue for the problem of not seeing the analyzer warnings?

halter73 commented 1 year ago

then if [Authorize(AuthenticationSchemes = "SessionID")] is written on method, the HTTP Basic authentication handler still gets executed (and succeds or fails in the background and that's ignored but it's executed while is not supposed to be executed).

I agree that it's surprising that the HttpBasicAuthenticationHandler.AuthenticateAsync gets called in this case, but it's existing behavior that's not likely to change in a patch, because apps may rely on it. I'm even hesitant to change the behavior in .NET 8 for fear of breaking apps, but I'm sympathetic to not wanting to waste resources, so it's worth considering as long as we don't think it will be too breaking.

This has for example an unwanted side-effect that WWW-Authenticate HTTP header is unexpectedly added to the response (as it would have been set if HTTP Basic or any other authentication failed).

The good news is that if you add the header in HttpBasicAuthenticationHandler.HandleChallengeAsync instead of AuthenticateAsync as I suggested above, the header will only be sent for authentication failures when the scheme is selected. Setting the 401 manually is also unnessary if you call base.

protected override Task HandleChallengeAsync(AuthenticationProperties properties)
{
    Response.Headers.Add(HeaderNames.WWWAuthenticate, $"{AuthenticationSchemeName} realm=\"{Realm}\"" /* realm is special info added in HTTP Basic auth fail */);
    return base.HandleChallengeAsync(properties);
}

You should never modify the response in AuthenticateAsync. Even the CookieAuthenticationHandler does not do this, and it needs to modify non-challenge responses. It hooks OnStarting inside of InitializeHandlerAsync.

https://github.com/dotnet/aspnetcore/blob/10a343bbcb7a6d5013a6057620f74aef039964c0/src/Security/Authentication/Cookies/src/CookieAuthenticationHandler.cs#L58-L61

floge07 commented 1 year ago

I also have problems with this behavior. In my case, I have two different JwtBearer handlers. One of them is the default/fallback. They both use different signing credentials, so because the default one is always executed, it's throwing an exception that it can't read the token. This doesn't fail the authentication because it continues with the second handler, which is successful. It just causes unnecessary error logs.

Sonic198 commented 8 months ago

I also have problem with this. In my case I have two bearer authentication schemes which are also configured as default policy

services.AddAuthorization(options =>
{
    var defaultAuthorizationPolicyBuilder = new AuthorizationPolicyBuilder(
        AuthenticationSchemes.Azure,
        AuthenticationSchemes.Cognito);
    defaultAuthorizationPolicyBuilder = defaultAuthorizationPolicyBuilder.RequireAuthenticatedUser();
    options.DefaultPolicy = defaultAuthorizationPolicyBuilder.Build();
});

But I also have a third auth scheme for API Key authentication and I would like to secure some of my endpoints to be accessible only using API key. So I did the following:

[Authorize(AuthenticationSchemes = "ApiKey")]
[HttpGet("byApiKey")]
public Task<IActionResult> ProtectedByApiKey()
{
    return Task.FromResult<IActionResult>(Ok("Authorized by ApiKey"));
}

But unfortunately I can access this endpoint either with API key or with Azure and Cognito bearer tokens. If that is expected behavior I think the docks are not clear about that.

peter-bertok commented 5 months ago

I'm getting an issue similar to @Sonic198, I have an API service that also has a Razor page that uses Easy Auth and Entra ID, but the authentication providers can't be easily split. The documentation is also very unclear and there are no working examples on the web.

jurajpaska8 commented 4 months ago

same problem, when using default authentication, this handler is executed no matter which handler i spiecify in authorize attribute

services
            .AddAuthentication(o =>
            {
                // Any default scheme will be triggered everytime, no matter if we use different authn scheme in authorize attribute. Therefore is better to use wanted Auth handler explicitly on controller / endpoint
                // This is known microsoft issue
                // https://github.com/dotnet/aspnetcore/issues/47105
                // o.DefaultScheme = nameof(EmptyAuthenticationHandler);
                // o.DefaultScheme = nameof(AnotherAuthenticationHandler);
                // o.DefaultScheme = JwtBearerDefaults.AuthenticationScheme;
            })