Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow specifying exact authentication schemes in AuthenticationMiddleware. #55766

Open
ShadedBlink opened this issue May 17, 2024 · 1 comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-security enhancement This issue represents an ask for new feature or an enhancement to an existing one
Milestone

Comments

@ShadedBlink
Copy link

ShadedBlink commented May 17, 2024

Background and Motivation

I believe there are many different web applications that support more than one authentication mode as default like "if there is an API-key specified, use it, otherwise fallback to cookies". I had such services in development and had an urge to know current user for logging purposes before authorization takes places and cancells requests with 403. It instantly comes to mind that best place for such middleware is between app.UseAuthentication() and app.UseAuthorization() steps, but the main problem is that you can specify multiple default authentication schemes only for authorization scheme, i.e. authentication step can handle only single authentication scheme(cookies in our case), while API-key is considered only on authorization step.

Proposed API

The idea is to allow specifying exact list of authentication schemes in middleware so they are handled in same way as they are handled in authorization.

namespace Microsoft.AspNetCore.Builder;

public static class AuthAppBuilderExtensions
{
+    public static IApplicationBuilder UseAuthentication(this IApplicationBuilder app, params string[] schemes);
}

Usage Examples

     app.UseAuthentication("api-key", "cookies");

In given case this middleware should first challenge "api-key" scheme. If it succeeds, then step is finished, otherwise fallback to "cookies". This overload is not supposed to consider default authentication scheme since all schemes are specified explicitly, i.e. if default authentication scheme is "jwt", then only "api-key" and "cookies" are used, "jwt" is ignored since it is not specified. If default scheme is among the specified schemes, then it should be applied exactly in order as it is specified in schemes list. I.e. being a default scheme doesn't interfere in any way with this middleware.

Also this approach should work better with branched middleware chains since you can specify exact auth scheme to reach them. Like in main branch you use scheme "cookies", for health or prometheus branch you use scheme "api-key". Current approach doesn't support any configuration for middleware.

Risks

Since this overload is optional and new, it is not supposed to break anything.

@ShadedBlink ShadedBlink added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 17, 2024
@halter73
Copy link
Member

If we were to do this, I think the natural place for it would be in AuthenticationOptions alongside DefaultScheme, DefaultAuthenticateScheme, DefaultChallengeScheme, etc... In theory, we could try to make plural versions of all of those. I'm not sure it's a good idea though.

the main problem is that you can specify multiple default authentication schemes only for authorization scheme, i.e. authentication step can handle only single authentication scheme(cookies in our case), while API-key is considered only on authorization step.

Proposed API

The idea is to allow specifying exact list of authentication schemes in middleware so they are handled in same way as they are handled in authorization.

If it's handled the same way we handle multiple schemes specified as part of the authorization policy, we'd merge together ClaimsPrincipals rather than take the first successful result.

if (policy.AuthenticationSchemes != null && policy.AuthenticationSchemes.Count > 0)
{
ClaimsPrincipal? newPrincipal = null;
DateTimeOffset? minExpiresUtc = null;
foreach (var scheme in policy.AuthenticationSchemes)
{
var result = await context.AuthenticateAsync(scheme);
if (result != null && result.Succeeded)
{
newPrincipal = SecurityHelper.MergeUserPrincipal(newPrincipal, result.Principal);
if (minExpiresUtc is null || result.Properties?.ExpiresUtc < minExpiresUtc)
{
minExpiresUtc = result.Properties?.ExpiresUtc;
}
}
}

I don't think merging ClaimsPrincipals is necessarily wrong, but most of the time it probably is probably unessary. I'm not sure it's logic we want to copy to more places. At least authorization polices are usually isolated to particular endpoints where they can specify a subset of globally supported authentication schemes. And usually, it's just one.

If you set both a cookie and API key as global default authentication handlers, the former will attempt to redirect while the latter will attempt to set a 401 given a challenge. What happens would depend on the order they run in which wouldn't be very intuitive.

An application that needs to support both cookie and API keys for auth would probably be best served registering or configuring a policy scheme to indicate what scheme should be used what purpose.

This requires a little bit more code than just providing an array of scheme names, but it gives far more precise control over how to combine the authentication schemes. For example:

.AddPolicyScheme("B2C_OR_AAD", "B2C_OR_AAD", options =>
{
    options.ForwardDefaultSelector = context =>
    {
        string authorization = context.Request.Headers[HeaderNames.Authorization];
        if (!string.IsNullOrEmpty(authorization) && authorization.StartsWith("Bearer "))
        {
            var token = authorization.Substring("Bearer ".Length).Trim();
            var jwtHandler = new JwtSecurityTokenHandler();

            return (jwtHandler.CanReadToken(token) && jwtHandler.ReadJwtToken(token).Issuer.Equals("B2C-Authority"))
                ? "B2C" : "AAD";
        }
        return "AAD";
    };
});

https://learn.microsoft.com/en-us/aspnet/core/security/authorization/limitingidentitybyscheme?view=aspnetcore-8.0#use-multiple-authentication-schemes

@MackinnonBuck MackinnonBuck added this to the Backlog milestone May 22, 2024
@MackinnonBuck MackinnonBuck added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-security enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

4 participants
@halter73 @MackinnonBuck @ShadedBlink and others