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.44k stars 10.01k forks source link

Introduce support for defining Authorization policies via Configuration #42172

Open DamianEdwards opened 2 years ago

DamianEdwards commented 2 years ago

Spin-off of #39855 focusing on addition of top-level authorization configuration API.

Example matching Authorization changes to consider, allowing sharing of policies, etc.:

var builder = WebApplication.CreateBuilder(args);

builder.Authentication.AddJwtBearer();
builder.Authorization.AddPolicy("HasProtectedAccess", policy =>
    policy.RequireClaim("scope", "myapi:protected-access"));

var app = builder.Build();

app.MapGet("/hello", () => "Hello!");

app.MapGet("/hello-protected", () => "Hello, you are authorized to see this!")
    .RequireAuthorization("HasProtectedAccess");

app.MapGet("/hello-also-protected", () => "Hello, you authorized to see this to!")
    .RequireAuthorization("HasProtectedAccess");

app.Run();

The WebApplicationBuilder.Authorization property is typed as AuthorizationOptions allowing simple creation of policies and configuration of the default and fallback policies:

builder.Authorization.AddPolicy("HasProtectedAccess", policy => policy.RequireClaim("scope", "myapi:protected-access"));
builder.Authorization.DefaultPolicy = builder.Authorization.GetPolicy("HasProtectedAccess");

// Consider new methods to enable easily setting default/fallback policies by name
builder.Authorization.SetDefaultPolicy("HasProtectedAccess");
builder.Authorization.SetFallbackPolicy("HasProtectedAccess");

The WebApplicationBuilder would register an IConfigureOptions<AuthorizationOptions> in the services collection with a delegate that applies the settings.

Note this suggestion has a fundamental issue in that the AuthorizationOptions isn't designed to be mutated in this way, rather it should be configured via a callback registered in DI so that it runs at the appropriate time during app startup and composes with other code that wishes to configure it.

Perhaps instead the Authentication property should also read from configuration for authorization settings, and the Authorization property would be a new type that simply provides easy access to adding a configuration delegate, e.g.:

{
  "Authorization": {
    "DefaultPolicy": "HasProtectedAccess",
    "FallbackPolicy": "",
    "InvokeHandlersAfterFailure": true,
    "Policies": {
      "HasProtectedAccess": {
        "Claims": [
          { "scope" : "myapi:protected-access" }
        ]
      }
    }
  }
}
builder.Authentication.AddJwtBearer();
builder.Authorization.Configure(authz =>
{
    // Following is the code-based equivalent of config above
    authz.AddPolicy("HasProtectedAccess", policy => policy.RequireClaim("scope", "myapi:protected-access"));
    authz.DefaultPolicy = authz.GetPolicy("HasProtectedAccess");
});

Some other potential example policies as defined via configuration:

{
  "Authorization": {
    "DefaultPolicy": "HasProtectedAccess",
    "Policies": {
      "AuthenticatedUsers": {
        "AuthenticationRequired": true
      },
      "Employees": {
        "AuthenticationRequired": true,
        "Roles": [ "Employees" ]
      },
      "OnlyHomers": {
        "AuthenticationRequired": true,
        "UserName": "Homer"
      },
      "ApiClients": {
        "AuthenticationRequired": true,
        // Any unrecognized properties are auto-mapped as claims perhaps?
        "scope": [ "myapi:read", "myapi:protected-access" ]
      }
    }
  }
}
DamianEdwards commented 2 years ago

FYI @HaoK @Tratcher @davidfowl @captainsafia @jcjiang @blowdart

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. 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 issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

captainsafia commented 2 years ago

Perhaps instead the Authentication property should also read from configuration for authorization settings, and the Authorization property would be a new type that simply provides easy access to adding a configuration delegate, e.g.:

I'm a fan of this approach:

I'm not aware of any implementation complexities that make the options-from-config approach not viable for authorization but @HaoK might be able to add some color here.

HaoK commented 2 years ago

Yeah config binding seems the cleanest for the properties (everything that's not a policy). Directly configuring an instance of AuthorizationOptions off of the builder seems more trouble than what's it worth. If you really want something like that, maybe just have an alternate dictionary of named policies, and settings, which you use in a regular options configuration of AuthorizationOptions, basically an AuthorizationOptionsBuilder property would be the analogy here.

In regards to authZ policies in config. Personally I would rather define that in code rather than muck around with json but seems ok as long as we don't go too insane with what requirements we support. We can certainly start with seeing what config definition of some simple policies look like, requiring the presence of a specific claim/role seems reasonably easy.

DamianEdwards commented 2 years ago

Ok so the feature would be IServiceCollection.AddAuthorization() will now also register an IConfigureOptions<AuthorizationOptions> that enables the configuration of AuthorizationOptions including defining policies from configuration.

~A new property, WebApplicationBuilder.Authorization will be added that enables registering custom Action<AuthorizationOptions> configuration delegates that run in addition to the one added by WebApplicationBuilder.Authentication, essentially an alias for builder.Services.Configure(Action<AuthorizationOptions> configure).~

Regarding the AuthZ policy configuration binding support, here's a strawman:

davidfowl commented 2 years ago

Does this really add much over the call to AddAuthorization call?

DamianEdwards commented 2 years ago

Does this really add much over the call to AddAuthorization call?

The top-level WebApplicationBuilder.Authorization property doesn't no, we could not do that and just focus on the configuration binding aspect instead.

HaoK commented 2 years ago

+1 on just focusing on the configuration binding

DamianEdwards commented 2 years ago

Removed the proposed API addition.

captainsafia commented 2 years ago

Assuming it's just config work, the only public API that we might need is an IAuthorizationConfigProvider interface for implementing getters for the Authorization config and a GetPolicyByName extension method for resolving individual policies from the config. The options implementations can likely remain internal.

namespace Microsoft.AspNetCore.Authorization;

interface IAuthorizationConfigurationProvider
{
  IConfiguration AuthorizationConfiguration { get; }
}

public static class AuthorizationConfigurationProviderExtensions
{
  public static IConfiguration GetAuthorizationPolicyByName(
    this IAuthorizationConfigurationProvider provider,
    string policyName)
}

EDIT: actually no extension method needed. The following should suffice:

namespace Microsoft.AspNetCore.Authorization;

interface IAuthorizationConfigurationProvider
{
  IConfiguration AuthorizationConfiguration { get; }
}
DamianEdwards commented 2 years ago

@HaoK any thoughts on the strawman binding rules?

HaoK commented 2 years ago

What do we think about doing something spicier to make this extensible. I was thinking about how we'd implement this, and that lead me to something like the following schema instead, with a similar pluggable pattern we could use for authN as well for schemes.

{ "Policies": // Master list of named polices
   { "<policyName>" : // policyName : [list of requirements] all instances must be met for a success
      [  
          // General schema for a requirement, its identifier (for dispatching to the right factory, and its config data)
          "<requirementIdentifier>" : {
                // Arbitrary config data that gets passed to the requirement to initialize itself
          },
          // Examples of instances of our built in requirements
          "RequireAuthenticatedUsers" : { }, // DenyAnonymous takes no data
          "Roles" : {
              "AllowedRoles" : [ "<role1>" , "<role2>"] // Maps directly to AllowedRoles property on requirement 
           }
          "Claim" : {
               "ClaimType": "<requiredClaimType>",
               "AllowedValues": [ "<value1>", "<value2>" ] // must match one of these if present.
          }
      ]
   }
}

// We register our build in requirements and have them implement the factory to parse the config into instances
public interface IConfigurationRequirementRegistry {
     // Hopefully we can let the factory use things in DI to create the requirements?
     public void Register<IConfigurationAuthorizationRequirementFactory>(string requirementIdentifier) 
}

public interface IConfigurationAuthorizationRequirementFactory {
       public IAuthorizationRequirement Create(IConfiguration configData); // Given the config data for a requirement, knows how create an instance
}

// Analog on the authN side could be IConfigurationAuthenticationSchemeRegistry / IConfigurationAuthenticationSchemeFactory or something which also would be passed something like:

"<schemeName>" : {
    "<schemeIdentifer>" : // i.e. "Jwt | Cookie | Certificate", Extensible string -> factory registration
       {
        // <scheme configuration data
       }
}
DamianEdwards commented 2 years ago

@HaoK my first thought is: wow, that's a lot. But I certainly appreciate that this kind of configuration extensibility is not uncommon at all, although I wonder if we can reduce by a couple of levels?

HaoK commented 2 years ago

I was mostly thinking about the extensibility model, so I didn't put too much thought into the exact shape of the schema.

Why do the policies have to be an array rather than named objects (aka a dictionary)? You can't add two policies with the same name anyway so structuring it as dictionary would remove a layer of nesting.

Good point, the names are required to be unique so we can get rid of that outer array, i'll edit that above to remove

It would need to support multiple claims in the list of requirements right? Your proposal seems as though it's limited to a single claim in the requirements list. What was the reasoning to having a single nested object property called "Requirements" instead of just having the policy itself be an array? Schema extensibility?

I was just demonstrating examples of what some requirement would look like, you can have duplicate requirements of the same instance. The requirements can be dropped, with the downside of not leaving space if we ever want to add new metadata to AuthorizationPolicy instances (although we could just add logic that branched on if the value was an array or a subobject in the future, so seems fine)

     // <PolicyName> : <List of requirements>, all instances must be satisfied to succeed a policy
     { "exampleAdminPolicy" : 
          [ 
            "RequireAuthenticatedUsers" : { }, // DenyAnonymous takes no data
            "Roles" : {
                "AllowedRoles" : [ "Admin" ] // Maps directly to AllowedRoles property on requirement 
             },
            "Claim" : {
                 "ClaimType": "someOrgClaim",
                 "AllowedValues": [ "aspnet", "devdiv" ] // must match one of these if present.
            },
            "Claim" : {
                 "ClaimType": "empId" // Must be an employee
            },
            "CustomAdminRequirement": // Some custom requirement they register
            {
                  "authenticatorRequired" : "true",
                  "lastTfaVerifiedDate" : "5/14/2022"
                  "passwordComplexityCheck" : "true"
           }
        ]
     }

Basically this lets each policy have space to store all of the custom data that any requirements would ever need, as they get passed the section underneath the requirement Identifier

                    "<requirementIdentifier>" : {
                          // Arbitrary config data that gets passed to the requirement to initialize itself
                    },

We can certainly make our built in requirements have shorter/more concise config, I was just doing this mechanically to match the types at this point (basically viewing our requirements no differently than a 3rd party requirement that attempted to use the config binder even), our requirements could be implemented basically be configSection.Get<TRequirement>() with this schema since we only have very simple requirements with strings

DamianEdwards commented 2 years ago

@HaoK yep that makes sense and your updated example looks reasonable.

DamianEdwards commented 2 years ago

@HaoK actually your example is invalid JSON now I think, as you have an array that contains object notation under the "exampleAdminPolicy". I think we'd likely support defining schemes for policies too yes?

So revised to this?

{
  "Authorization": {
    "DefaultPolicy": "HasProtectedAccess",
    "Policies": {
      // Each property on the Policies object is a named policy comprised of an array of requirement objects
      "AuthenticatedUsers": [
        { "RequireAuthenticatedUsers" : {} } // DenyAnonymous takes no data, could 'null' too
      ],
      "Employees": [
        { "Schemes" : { "AllowedSchemes": [ "Bearer" ] } },
        { "RequireAuthenticatedUsers" : {} },
        { "Roles": { "AllowedRoles": [ "Employees" ] } }
      ],
      "OnlyHomers": [
        { "RequireAuthenticatedUsers" : {} },
        { "RequiresUserName" : { "UserName": "Homer" } }
      ],
      "ApiClients": [
        { "RequireAuthenticatedUsers" : {} },
        { "Claim": { "ClaimType": "scope", "AllowedValues": [ "myapi:read", "myapi:protected-access" ] } }
      ],
      "exampleAdminPolicy": [
        { "RequireAuthenticatedUsers" : {} },
        { "Roles": { "AllowedRoles": [ "Admin" ] } }, // Maps directly to AllowedRoles property on requirement 
        { "Claim": { "ClaimType": "someOrgClaim", "AllowedValues": [ "aspnet", "devdiv" ] } }, // Must match one of these if present
        { "Claim": { "ClaimType": "empId" } }, // Must be an employee
        { "CustomAdminRequirement":  // Some custom requirement they register
            {
                "authenticatorRequired" : "true",
                "lastTfaVerifiedDate" : "5/14/2022",
                "passwordComplexityCheck" : "true"
           }
        }
      ]
    }
  }
}
HaoK commented 2 years ago

Wait, you can't have an array of json objects? Really?

Well as soon as we are setting Schemes on Policies, then I'd argue we want it to be like I had before with Requirements/Schemes being top level: so for that employees policy

      "Employees": {
        "AuthenticationSchemes" : [ "Bearer" ],
        "Requirements" : [
           { "RequireAuthenticatedUsers" : {} },
           { "Roles": { "AllowedRoles": [ "Employees" ] } }
         ],
     }
HaoK commented 2 years ago

But it feels a bit icky to mix authenticationSchemes in with the policies definitions, because now if you want to have a slightly different Employee Cookie policy, you have to duplicate everything and just say "Cookies" for the scheme.

[Authorize(AuthenticationSchemes = "Bearer", Policy = "Employees")] [Authorize(AuthenticationSchemes = "Cookie", Policy = "Employees")]

vs

[Authorize("Cookie-Employees")] [Authorize("Bearer-Employees")]

But that's already an issue today

DamianEdwards commented 2 years ago

A (fundamental?) issue that's been raised about this approach is that you can't define a policy in configuration that itself relies on values from configuration, as configuration is strictly statically defined name/value pairs with no ability to statically reference values from elsewhere.

E.g. if one wanted to create a policy with a claim requirement for "iss" (Issuer) that matches the configuration of the JWT scheme's issuer, which is also declared in configuration you'd be forced to duplicate values in configuration:

{
  "Authentication": {
    "Schemes": {
      "Bearer": {
        "Issuer": "SuperDuperIdP" // <-- Specified here
      }
    }
  },
  "Authorization": {
    "DefaultPolicy": "AuthenticatedUsers",
    "Policies": {
      "AuthenticatedUsers": [
        // DenyAnonymousAuthorizationRequirement
        "RequireAuthenticatedUsers",
        // ClaimsAuthorizationRequirement
        {
          "Claims": {
            "ClaimType": "iss",
            "AllowedValues": [ "SuperDuperIdP" ] // <-- Specified *again* here
          }
        }
      ]
    }
  }
}
DamianEdwards commented 2 years ago

This proposal obviously has some issues that make it unpragmatic to include in .NET 7, so I've spun-off #42235 to revisit the idea of a more top-level API (builder.Authorization) for setting up app-wide AuthZ options without needing to resort to builder.Services.AddAuthorization() or builder.Services.Configure<AuthorizationOptions>().

HaoK commented 2 years ago

RE the duplication of values, this seems more like a general issue for configuration. We can certainly address that by introducing our own abstraction/layer to support configuration variables or something if this is really a blocker we need to solve.

Add some kind of notion of config variables in some special section:

{ "variables" : 
 {
     "$idp$" = "superDuperIdP"
     "#var#" = "whatever"
 }
}

Have our authorization config parsing system wrap IConfiguration to replace the final Get string calls to replace any variables using this section which allows folks to define this. But this seems like a feature for config to add to me

DamianEdwards commented 1 year ago

Think we should just close or move this to backlog at this point.

ghost commented 1 year ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.