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.02k forks source link

Make it easier to discover how to enforce/require auth in an application #42048

Open DamianEdwards opened 2 years ago

DamianEdwards commented 2 years ago

From https://github.com/dotnet/aspnetcore/issues/39857#issuecomment-1107473315

Make it easier to discover how to enforce auth: builder.Authentication.RequireAuthentication() or similar, which could just hide the call to add the AuthorizeAttribute filter.

I like this idea. I've personally struggled with finding the right settings to "just require auth for the whole app" (FallbackPolicy et al). We'll consider this scenario.

DamianEdwards commented 2 years ago

FYI @HaoK @blowdart @Tratcher @captainsafia @jcjiang @CamiloTerevint

Interested in folks' thoughts on this one. The idea is to make it much easier to require authentication for the whole app. Today this is done something like the following which isn't terribly intuitive:

builder.Services.AddAuthorization(c =>
{
    c.FallbackPolicy = c.DefaultPolicy;
});

I think the thought was that we could enable something like this that would essentially do the above in a way that can't be overridden by other IConfigureOptions<AuthorizationOptions>:

builder.Authentication.RequireAuthentication();

Alternatively perhaps it could be inverted:

builder.Authentication.AllowAnonymousUsers = false;
Tratcher commented 2 years ago

I think the thought was that we could enable something like this that would essentially do the above in a way that can't be overridden by other IConfigureOptions<AuthorizationOptions>:

Why are you concerned about it being overridden? Do you think it would be common for people to have conflicting authz setups?

builder.Authentication.RequireAuthentication();

This is fine.

HaoK commented 2 years ago

So builder.Authentication.RequireAuthentication(); =>would be the same as today's behavior of requiring the default authZ policy (any isAuthenticated = true clams identity)? Or is this going to be slightly different than just setting the fallback policy, I believe the fallback policy only kicks in if NO policy is specified, so the fallback policy doesn't technically require authentication either.

DamianEdwards commented 2 years ago

Why are you concerned about it being overridden? Do you think it would be common for people to have conflicting authz setups?

I'm concerned about scenarios where it's unclear that something else has loosened the auth requirements.

blowdart commented 2 years ago

Definitely prefer require over AllowAnonymous=false, but allowanonymous on endpoints still needs to override

DamianEdwards commented 2 years ago

So builder.Authentication.RequireAuthentication(); =>would be the same as today's behavior of requiring the default authZ policy (any isAuthenticated = true clams identity)? Or is this going to be slightly different than just setting the fallback policy, I believe the fallback policy only kicks in if NO policy is specified, so the fallback policy doesn't technically require authentication either.

Yeah that's a great point. The idea is this should "do whatever it takes" to force all users of the app to be authenticated, with exceptions for resources that are explicitly marked as anonymous allowed.

HaoK commented 2 years ago

I'm concerned about scenarios where it's unclear that something else has loosened the auth requirements.

Right this is a valid concern, as if we don't change what RequireAuthentication does, it does not really require authentication as implemented only via fallback policy. You would easily bypass this by either changing the default policy, or specifying a policy that doesn't add the DenyAnonymous requirement.

DamianEdwards commented 2 years ago

With those points clarified, what would the implementation of this look like? Does it need a new concept?

HaoK commented 2 years ago

Right so I'm fine with the name, but it needs to be implemented differently (in addition to using the fallback policy)

HaoK commented 2 years ago

It doesn't need a new concept, it just needs to fully do what we want it to do which is probably additional logic in Combine or the actual AuthZ service to be aware of this flag

HaoK commented 2 years ago

We have an Authorization requirement doing exactly what we want already, we would just need to force that requirement for all of Authorization perhaps when this turned on.

DamianEdwards commented 2 years ago

Right, kind of like a "global requirement".

HaoK commented 2 years ago

Yeah we are getting a bit fragmented as we'd have Default Policy which is subtly different from fallback policy, and now global requirements, I guess we can introduce it that way and implement it like that

HaoK commented 2 years ago

A new IEnumerable on AuthorizationProperties (GlobalRequirements), RequireAuthentication would just add DenyAnonymous to the global requirements, we always add all global requirements to all Authorization requests.

DamianEdwards commented 2 years ago

Where is AuthorizationProperties? i.e. what types would actually change here?

HaoK commented 2 years ago

Oops AuthorizationOptions

DamianEdwards commented 2 years ago

Ah got it. That seems fairly straightforward then. Something would have to explicitly remove it then to "undo" what the proposed method does.

HaoK commented 2 years ago

We could have the global requirements as a mutable List<IAuthorizationRequirement> or something so they can just Configure<AuthorizationOptions> remove DenyAuthorizationRequirement themselves

HaoK commented 2 years ago

There is a bit of a danger here though as lifetime of these requirements becomes very tricky, as options are singleton lifetime, and for some requirements you may want to pull things from DI/request, so there's some complications here which we need to be careful about.

HaoK commented 2 years ago

So we'd probably need some kind of IGlobalAuthZRequirementProvider that enables requirements are more complicated in terms of construction

HaoK commented 2 years ago

So we have a method today policyBuilder.RequireAuthenticatedUser which does the same thing for a specific policy, so maybe for naming builder.Authentication.AlwaysRequireAuthenticatedUser if we want to be clear what this would be doing

DamianEdwards commented 2 years ago

So we have a method today policyBuilder.RequireAuthenticatedUser which does the same thing for a specific policy, so maybe for naming builder.Authentication.AlwaysRequireAuthenticatedUser if we want to be clear what this would be doing

That's... verbose 😄 How about RequireAuthenticatedUsers()? Also need to decide if the method/property is on AuthenticationBuilder or just WebApplicationAuthenticationBuilder (meaning it would have to be public now) or something else.

HaoK commented 2 years ago

Is there a reason we shouldn't build this as a full fledged security feature for all to use via AuthenticationBuilder?

DamianEdwards commented 2 years ago

Is there a reason we shouldn't build this as a full fledged security feature for all to use via AuthenticationBuilder?

It requires authorization services to be present right? WebApplicationAuthenticationBuilder already adds both sets of services (AuthN and AuthZ) but to make this work via builder.Services.AddAuthentiation().RequireAuthenticatedUsers() it would need to add the authorization services too, yes?

HaoK commented 2 years ago

Sure, but is there no way for us to support the automatic adding of these services/middleware (only when needed) in a way that everyone could take advantage?

captainsafia commented 2 years ago

That's... verbose 😄 How about RequireAuthenticatedUsers()? Also need to decide if the method/property is on AuthenticationBuilder or just WebApplicationAuthenticationBuilder (meaning it would have to be public now) or something else.

I'm leaning towards AlwaysRequireAuthentication because I think it's more important to empathize the fact that the behavior is enabled globally.

but is there no way for us to support the automatic adding of these services/middleware (only when needed) in a way that everyone could take advantage?

We could integrate this logic in the base AuthenticationBuilder with some additional glue but I think we were conservative about only supporting this in WebApplication-supporting apps only.

DamianEdwards commented 2 years ago

We could integrate this logic in the base AuthenticationBuilder with some additional glue but I think we were conservative about only supporting this in WebApplication-supporting apps only.

How would we do this without WebApplication though? We don't even control where routing gets added in those scenarios.

HaoK commented 2 years ago

I haven't looked at the host builder code since probably before the 1.0 timeframe so maybe I'm super far off base here, but couldn't we do something like this?

DamianEdwards commented 2 years ago

Yeah I think we should keep this kind of behavior to WebApplicationBuilder only as it is able to reason about the application pipeline before it decides to add (or not add) automatic middleware, an IStartupFilter can't do that and I don't think we want to update the old host with these new automatic behaviors.

HaoK commented 2 years ago

So irrespective of which host does the Use part of this, I think it makes sense for the AuthenticationBuilder to have the service side for this API, in the old host, if they aren't using Authorization it would just do nothing (no different than today, where you can set fallback policies which do nothing if you don't have UseAuthorization), basically the services are setup exactly the same, just you don't get the automatic middleware added for old hosts. But as soon as they actually use Authorization, the global requirement would kick in

DamianEdwards commented 2 years ago

@HaoK Yeah that sounds okay.

@captainsafia I know naming is the hardest thing but none of these names are hitting the sweet spot for me yet. I guess it's time to start a poll 😄

  1. builder.Authentication.RequireAuthentication()
  2. builder.Authentication.RequireAuthentication = true;
  3. builder.Authentication.RequireAuthenticatedUsers()
  4. builder.Authentication.RequireAuthenticatedUsers = true;
  5. builder.Authentication.AlwaysRequireAuthentication()
  6. builder.Authentication.AlwaysRequireAuthentication = true;
  7. builder.Authentication.AuthenticationRequired()
  8. builder.Authentication.AuthenticationRequired= true;
  9. something else?
kevinchalet commented 2 years ago

The idea is to make it much easier to require authentication for the whole app.

Can you clarify what you mean by "the whole app"? Do you mean all the requests reaching MVC actions or minimal endpoints or really all requests, no matter how they are handled?

I'm asking that because IMHO, it rarely makes sense to block all unauthenticated requests (you should at least be able to serve things like favicon.ico without any restriction). It's also not clear how things like CORS preflight requests or endpoints served by built-in handlers like OAuth 2.0 or OIDC or third-party libs like OpenIddict or IdSrv will be handled with your proposal.

DamianEdwards commented 2 years ago

@kevinchalet great question and very good points. I admit it's not super clear. On one hand we could imagine something like the old "Windows Authentication" in IIS, where when enabled on a site it's literally for the whole site by default, and then allowing anonymous access to certain resources requires configuring file/dir ACLs along with the site config. On the other hand it could be based on routing, endpoints, and metadata, such that it means all endpoints require AuthN unless they explicitly opt to allow anonymous access.

I agree with you that something like the latter is what makes sense here, i.e. turning this on effectively defaults all endpoints into requiring authorization, as if they'd added the requisite AuthZ metdata to them.

kevinchalet commented 2 years ago

On one hand we could imagine something like the old "Windows Authentication" in IIS, where when enabled on a site it's literally for the whole site by default, and then allowing anonymous access to certain resources requires configuring file/dir ACLs along with the site config.

Please no, that was absolutely horrible (that and automatic 401 responses hijacking a-la-FormsAuthenticationModule!) 😭

I agree with you that something like the latter is what makes sense here, i.e. turning this on effectively defaults all endpoints into requiring authorization, as if they'd added the requisite AuthZ metdata to them.

Well, in both cases, it's going to be a massive breaking change as all the libraries that don't currently use endpoints will need to use them to opt in a "no-op authorization policy" to be compatible with this flag: not just third-party libs, but also all the middleware and authentication handlers that ship as part of ASP.NET Core itself, including all the social/OIDC providers (e.g their callback actions can't require authorization or they'll just stop working completely 😄)

DamianEdwards commented 2 years ago

@kevinchalet I'll point back to the original motivation then: make it easier to require auth for "the app". Today, that usually involves setting both the DefaultPolicy and FallbackPolicy, which then results in everything after the AuthN/AuthZ middleware be protected by default. So it's less about endpoints, and more about where the middleware is in the pipeline (of course endpoints and their metadata are still part of that operation).

kevinchalet commented 2 years ago

Today, that usually involves setting both the DefaultPolicy and FallbackPolicy, which then results in everything after the AuthN/AuthZ middleware be protected by default.

Correct me if I'm wrong, but in another issue, you mentioned that you planned to update WebApplicationBuilder to register both the authentication and authorization middleware automatically. De facto, this means that all the middleware registered by the users when configuring the pipeline will always appear after the authentication/authorization middleware and thus be affected by FallbackPolicy in .NET 7.0 (I'm ignoring DefaultPolicy because it only applies to endpoints that are explicitly decorated with [Authorize], so it's much less impacting).

As reminded in your OP, FallbackPolicy is a quite advanced option so it's not massively used yet. But if it becomes an easy-to-use thing, you can be sure we'll all get reports asking why things break when folks will start using that new innocent-looking builder.Authentication.RequireAuthentication() API. I'm convinced most people won't realize that this method will not only affect their own APIs/endpoints but also everything present in the ASP.NET Core pipeline, including tons of middleware that need to handle unauthenticated requests (e.g ACME challenges for Let's Encrypt).

So it's less about endpoints, and more about where the middleware is in the pipeline (of course endpoints and their metadata are still part of that operation).

Once its use is generalized, library writers will have basically two options to make their middleware work with FallbackPolicy/builder.Authentication.RequireAuthentication():

Given that option 1) is rather impractical - as a library author, you can't be sure the user will re-register the authorization middleware - option 2) will be the de facto choice and that's why I mentioned endpoints. Moving to a world where all middleware declare their endpoints in the routing mechanism might not be a bad thing, but it's clearly a paradigm change and potentially very impactful.

DamianEdwards commented 2 years ago

We indeed have introduced the feature that configuring AuthN via the WebApplicationBuilder.Authentication property results in the AuthN/AuthZ middleware being added by default, if the app hasn't already added it in its pipeline. For scenarios where the middleware needs to explicitly run before AuthN/AuthZ middleware, the user will be required to manually place them in the pipeline, just like today, i.e. the experience isn't different from today.

I'm not sure I share your concern regarding the interplay between this new (opt-in) behavior, FallbackPolicy, and manually ordered middleware. Folks can continue to control the order of middleware as they do today. Also we use FallbackPolicy to enable this behavior in templates already today.

If we are to limit this to endpoints only, we'd need to name it accordingly, but even then my concern is that it's very difficult to make clear exactly which resources will be protected and which won't, as whether a middleware/framework integrates via routing/endpoints is fairly opaque.

kevinchalet commented 2 years ago

For scenarios where the middleware needs to explicitly run before AuthN/AuthZ middleware, the user will be required to manually place them in the pipeline, just like today, i.e. the experience isn't different from today.

I disagree, the experience is completely different: currently, the model is explicit: app.UseAuthentication() and app.UseAuthorization() appear very clearly in the pipeline so users have a chance to at least see that things are registered in a certain order and that it potentially matters.

By hiding all these things in your WebApplicationBuilder helpers, you're just making them more obscure than they are currently: as soon as a user will hit one of the pain points we discussed, it will be an absolute nightmare to at least understand that authentication/authorization middleware are at play and that they should re-register them manually in a different order.

So sure, it makes templates super minimalist, but it doesn't help folks understand how components are linked together and I don't think it's a good thing. Sometimes, less is not better.

(IMHO, FallbackPolicy is basically like those global static switches: it shouldn't have existed in the first place)

DamianEdwards commented 2 years ago

Can you help me understand what you would like to see instead of FallbackPolicy to facilitate the "just require auth for this whole app please" scenario?

I would far prefer that configuring AuthN/AuthnZ leads to issues due to more protection than desired than less. That said, I'm keen to find ways we can make identifying and correcting AuthN configuration issues easier, e.g. via logging, analyzers, API design, etc.

kevinchalet commented 2 years ago

Can you help me understand what you would like to see instead of FallbackPolicy to facilitate the "just require auth for this whole app please" scenario?

In my experience (and you mentioned System.Web ACLs earlier), most people are actually interested in securing whole "sections" of their apps, not really the entire app without any distinction at all, so something URL-based would have - IMHO - made more sense that a unique FallbackPolicy. This would also be more flexible, as you could use Bearer for /api and Cookies for /static-assets.

I would far prefer that configuring AuthN/AuthnZ leads to issues due to more protection than desired than less. That said, I'm keen to find ways we can make identifying and correcting AuthN configuration issues easier, e.g. via logging, analyzers, API design, etc.

Oh great to hear that, then PTAL at https://github.com/dotnet/aspnetcore/issues/4656#issuecomment-446947390 😄

HaoK commented 2 years ago

Making DenyAnonymousAuthorizationRequirement always required is already part of the plan for whatever the new [Always]RequireAuthenticatedUsers method is called, we'll probably introduce the concept of global requirements which automatically get added to all policies, which this method would add DenyAnonymousAuthorizationRequirement to as part of its implementation.

kevinchalet commented 2 years ago

we'll probably introduce the concept of global requirements which automatically get added to all policies

Wasn't the original plan supposed to simplify things? :trollface:

Tratcher commented 2 years ago

For background, FallbackPolicy was added for NegotiateAuth to emulate the IIS Windows Auth behavior of requiring all requests to authenticate. The middleware ordering still let you make exceptions for things like static files, but it's intentionally not more granular than that.

kevinchalet commented 2 years ago

For background, FallbackPolicy was added for NegotiateAuth to emulate the IIS Windows Auth behavior of requiring all requests to authenticate.

Which is super bad from a security perspective, as IWA is - like cookies or basic - an automatic authentication method. By making IWA easy to enforce globally, you're just encouraging them to shoot themselves in the foot without realizing it: as soon as they'll innocently add an API endpoint, they'll end up with a CSRF vulnerability.

I know all these automatic/default features are very appealing, but security is all about making the right choice and hiding everything behind defaults that can't work for all scenarios is, IMHO, a bad move.

DamianEdwards commented 2 years ago

In my experience (and you mentioned System.Web ACLs earlier), most people are actually interested in securing whole "sections" of their apps, not really the entire app without any distinction at all, so something URL-based would have - IMHO - made more sense that a unique FallbackPolicy. This would also be more flexible, as you could use Bearer for /api and Cookies for /static-assets.

I agree that it's often logical to think about configuring AuthN/AuthZ in a URL-based manner, similar to the old <Location> element in web.config. It would be interesting to explore this idea further. But I still think that defaulting to "everything requires AuthZ" and then explicitly marking up exceptions is the safer approach, combined with the ability to control middleware ordering and policies such that middleware can be placed before the AuthN/AuthZ stage to completely opt it out (in cases where it doesn't project endpoints).

While it's likely beyond the scope of what we can achieve now in .NET 7, given your extensive experience and interest in the area, I'll ask you, if you could make any changes to AuthN/AuthZ in ASP.NET Core, factoring in our limitations RE breaking changes, legacy, etc., what would you do?

kevinchalet commented 2 years ago

While it's likely beyond the scope of what we can achieve now in .NET 7, given your extensive experience and interest in the area, I'll ask you, if you could make any changes to AuthN/AuthZ in ASP.NET Core, factoring in our limitations RE breaking changes, legacy, etc., what would you do?

Regarding https://github.com/dotnet/aspnetcore/issues/4656, I would:

The current opt-in-for-authenticated-users approach is bad and leads to subtle security issues that could be easily mitigated by inverting the logic as I mentioned in 2018. The option suggested by @HaoK - depending on the global "require authenticated user" to add DenyAnonymousAuthorizationRequirement in the individual authorization policies - seems too complex to me.

Regarding the API suggested in this thread, if it's really the approach you want to promote, I would avoid introducing a new layer if it ends up being as limited as FallbackPolicy, so probably just a set of helpers...

public static void RequireAuthentication(this AuthorizationOptions options)
    => options.FallbackPolicy = options.DefaultPolicy = new AuthorizationPolicyBuilder()
        .RequireAuthenticatedUser()
        .Build();

public static void RequireAuthentication(this AuthorizationOptions options, params string[] schemes)
    => options.FallbackPolicy = options.DefaultPolicy = new AuthorizationPolicyBuilder()
        .AddAuthenticationSchemes(schemes)
        .RequireAuthenticatedUser()
        .Build();

... with extensive XML documentation indicating that it could break existing scenarios, as explained in this thread. Since it will stay at the authorization middleware level, authentication handlers like the OAuth 2.0/OIDC/Negotiate or the aspnet-contrib/OpenIddict projects shouldn't be impacted as their endpoints are handled by AuthenticationMiddleware.

That said, as I had mentioned in the other thread, OpenIddict will be impacted by the integration of app.UseAuthentication() or app.UseAuthorization() into WebApplicationBuilder as its "API endpoints" won't be covered by CORS policies since the CORS middleware is registered by the user after the middleware registered by the host. If you wanted to avoid this, I'd recommend updating the WebApplicationBuilder to also register the CORS middleware before the authentication middleware.

HaoK commented 2 years ago

Update AuthorizationPolicy to always add DenyAnonymousAuthorizationRequirement by default depending on the global "require authenticated user" to add DenyAnonymousAuthorizationRequirement in the individual authorization policies - seems too complex to me.

Its complex because the primary concern was with layering, and we don't want authZ to require authN always, which is why there needs to be an explicit gesture (aka the new RequireAuthentication() method). That said, effectively all AuthorizationPolicies will have the behavior of always adding DenyAnonymousAuthorizationRequirement but only when RequireAuthentication is called

DamianEdwards commented 2 years ago

Thanks for your input.

To be clear, the proposal in this thread was not to make the new method enforce any AuthN requirement earlier than when AuthorizationMiddleware runs (at least as far as I understood it). It would still be that middleware that enforces the requirement. The API appearing on builder.Authentication is more about discoverability but of course is open to discussion.

If you wanted to avoid this, I'd recommend updating the WebApplicationBuilder to also register the CORS middleware before the authentication middleware.

This has been suggested internally (hi there @halter73) and definitely seems worth exploring. Do we think it's as straightforward as always adding the CORS middleware before the authentication/authorization in the case where they'll be added (i.e. schemes are added via builder.Authentication), or is something more nuanced possible/appropriate?

kevinchalet commented 2 years ago

Its complex because the primary concern was with layering

What do you mean exactly by "layering" here?

and we don't want authZ to require authN always, which is why there needs to be an explicit gesture (aka the new RequireAuthentication() method).

That's exactly why I suggested adding an AuthorizationPolicyBuilder.AllowAnonymousUser() API to cover the (very) rare cases where an authorization policy would need to work on unauthenticated identity.

I'd suggest re-reading https://github.com/dotnet/aspnetcore/issues/4656 so you can appreciate the number of people who've been trapped by this design problem. Making it more complex is likely not going to help 😄

adityamandaleeka commented 2 years ago

@DamianEdwards can you put this in a milestone?