Skip to content

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

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

Open
DamianEdwards opened this issue Jun 6, 2022 · 53 comments
Open
Assignees
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer

Comments

@DamianEdwards
Copy link
Member

From #39857 (comment)

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 DamianEdwards added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 6, 2022
@DamianEdwards
Copy link
Member Author

DamianEdwards commented Jun 6, 2022

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
Copy link
Member

Tratcher commented Jun 6, 2022

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
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member Author

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
Copy link
Contributor

blowdart commented Jun 6, 2022

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

@DamianEdwards
Copy link
Member Author

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
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member Author

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

@HaoK
Copy link
Member

HaoK commented Jun 6, 2022

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

@HaoK
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member Author

Right, kind of like a "global requirement".

@HaoK
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member Author

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

@HaoK
Copy link
Member

HaoK commented Jun 6, 2022

Oops AuthorizationOptions

@DamianEdwards
Copy link
Member Author

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

@HaoK
Copy link
Member

HaoK commented Jun 6, 2022

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

@HaoK
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member

HaoK commented Jun 6, 2022

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

@HaoK
Copy link
Member

HaoK commented Jun 6, 2022

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
Copy link
Member Author

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
Copy link
Member

HaoK commented Jun 6, 2022

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

@DamianEdwards
Copy link
Member Author

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
Copy link
Member

HaoK commented Jun 6, 2022

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?

  • New apps would work via the new auto add behavior.
  • So existing apps (would be unaffected since they already contain Add/UseAuthN/Z calls).
    But old apps could also remove their UseAuthN/Z calls to take advantage of the new behavior without any changes (they could delete their default/fallback policies and switch to using RequireAuthenticatedUsers())

@captainsafia
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

HaoK commented Jun 7, 2022

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?

  • After building the service provider, similar how IStartupFilter can add middleware
  • For authentication, if default scheme has been set, we need UseAuthorization()
  • For authorization, if any policies have been registered, we need UseAuthorization()
  • If either are required, inspect the app, make sure they haven't been added already, and only then add these calls after the routing middleware

@DamianEdwards
Copy link
Member Author

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.

@kevinchalet
Copy link
Contributor

kevinchalet commented Jun 7, 2022

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
Copy link
Member Author

@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
Copy link
Contributor

kevinchalet commented Jun 7, 2022

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():

  • Force their users to explicitly register the authorization middleware (or the authentication middleware, if the builder.Authentication.RequireAuthentication() logic is performed by the authentication middleware) and put it after their own middleware. It's quite horrible and a very-hard-to-discover requirement for most users.

  • Make their middleware endpoint-aware and use the IAllowAnonymous metadata interface, which is pretty much the only option given by the authorization middleware to opt out the "fallback policy".

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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 #4656 (comment) 😄

@HaoK
Copy link
Member

HaoK commented Jun 8, 2022

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
Copy link
Contributor

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
Copy link
Member

Tratcher commented Jun 8, 2022

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

kevinchalet commented Jun 8, 2022

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 #4656, I would:

  • Update AuthorizationPolicy to always add DenyAnonymousAuthorizationRequirement by default (if backcompat' is a concern, an AppContext switch can be used to revert to the old behavior).
  • Obsolete AuthorizationPolicyBuilder.RequireAuthenticatedUser() and replace it by AuthorizationPolicyBuilder.AllowAnonymousUser() (authorization policies that work on unauthenticated identities are fairly uncommon in practice but it's important to not break this scenario).

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
Copy link
Member

HaoK commented Jun 8, 2022

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
Copy link
Member Author

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
Copy link
Contributor

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 #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
Copy link
Member

@DamianEdwards can you put this in a milestone?

@DamianEdwards DamianEdwards added this to the .NET 7 Planning milestone Jun 17, 2022
@ghost
Copy link

ghost commented Jun 17, 2022

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.

@DamianEdwards
Copy link
Member Author

Moving to .NET 8.

I implemented two approaches to enabling this via path-based authorization over at https://github.com/DamianEdwards/AspNetCorePathAuthorization

@kevinchalet you might find this matches the kind of idea you proposed.

One approach uses a custom middleware which effectively has to do its own route matching via a custom trie, etc. (eww) and another approach actually integrates with routing by attaching metadata to any endpoint based on route pattern, even if there isn't an endpoint already registered there! That means you can make endpoint-aware middleware "activate" even for requests with no endpoints, e.g. authorization can run on and protect requests for static files.

@kevinchalet
Copy link
Contributor

@DamianEdwards looks nice and flexible 👍🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

No branches or pull requests

9 participants