Redesign automatic authentication choice mechanism #1061

blowdart · 2016-12-07T19:12:19Z

The current mechanism for choosing which authentication middleware runs by default has been fragile and caused pain when more than one middleware has been set to automatically run, leading to what is, at best, undefined behaviour. The undefined behaviour changed in 1.1, but some folks were relying on it.

The basic idea behind automatic challenging middleware was that only one middleware should be configured to automatically challenge, however historically this was configured by a property on the individual middleware, and, as middlewares cannot inspect each other, it was too easy to set multiple middlewares to automatically challenge.

The new behaviour should take the decision of what middleware to use to issue a challenge when no middleware is specifically selected out of the hands of the middleware, and into the realm of authorization policy. It's envisaged that the default policy would contain information on the elected middleware to use to issue a challenge, and syntactic sugar added so you can set it as a property on authorization, rather than having to write a complete policy.

This leaves an open question of what happens when Challenge is called manually, without specifying which middleware to use - do we use the one from the default policy, or remove automatic challenge at that point and force a developer to specify the middleware to be used?

blowdart · 2016-12-07T19:15:59Z

Tagging interested parties: @PinpointTownes @leastprivilege @brockallen
MSFT: @davidfowl @DamianEdwards @HaoK @Eilon @Tratcher

leastprivilege · 2016-12-07T19:28:41Z

https://www.youtube.com/watch?v=GvtGVoS0ObQ

brockallen · 2016-12-07T20:00:54Z

After a bit of experience and reflection on how all of the authentication middleware works today, my sense is that the design of the authentication middleware pipeline that was done for katana is not generally appropriate here in ASP.NET Core. IOW, the whole linked list wireup per-request seems unnecessary given that we have DI and that we could inject the main logic (the handlers). There would still be the need for middleware to trigger the handler, but that's about it. API calls like Authenticate/Challenge/Signin/Signout could simply be to a service that was obtained via DI (and not via the handler chain wired up during the request).

Perhaps then more specialized interfaces (ICookeAuthentication, IExternalAuthentication, etc?) would be necessary for this. The design of a one-size-fits all authentication manager API seems like an ill suited abstraction. Of course the DI system is not a panacea and has its own issues with ambiguity when registering multiple instances of the same type.

Not sure how earth shattering this suggestion would be in practice, but again the linked list wireup seems brittle and the source of many problems.

davidfowl · 2016-12-08T05:23:53Z

I wasn't involved in the original design but would be fine re-thinking it for 2.0 now that we have a DI enabled system. The decorator pattern we use in the authentication manager wouldn't work in DI either so we'd need an alternate pattern for chaining auth handlers.

Perhaps then more specialized interfaces (ICookeAuthentication, IExternalAuthentication, etc?) would be necessary for this. The design of a one-size-fits all authentication manager API seems like an ill suited abstraction. Of course the DI system is not a panacea and has its own issues with ambiguity when registering multiple instances of the same type.

Not sure, we'd need to look at both the consumers and implementors of the interface to see who would need to implement it and how it would get triggered. I'd like to keep the Challenge/Forbid calling pattern as I think it makes sense.

Just some initial thoughts...

brockallen · 2016-12-08T13:03:03Z

The reason I suggest explicit interfaces for certain types of functionality is because sometimes the generic gestures (Challenge for example) don't really work. Should it mean show the access denied page, or should it mean go login to google. Having the framework guess hasn't worked in some cases, so an explicit API might just be easier.

davidfowl · 2016-12-08T18:23:15Z

Disclaimer: I'm a bit behind here as I wasn't involved here so bare with me as I get up to speed.

@brockallen Easier for who? What are the actors in the system that would have to know about each interface?

Should it mean show the access denied page, or should it mean go login to google. Having the framework guess hasn't worked in some cases, so an explicit API might just be easier.

I don't think the framework was guessing anything. Challenge targeted a specific middleware or thing that looked like middleware. The biggest complication besides the auth handler being a property on the http context (that's kinda weird), is the active/passive behavior. That was all to keep [Authorize] working the way it used to work in ASP.NET 4.x, like magic 😄 for cookie auth.

I might make sense to spell out all of the interfaces we have now and all the frameworks and touch points to make sure we understand where the pains are.

Tratcher · 2016-12-08T18:36:16Z

DI is a non-starter when common apps have 2+ instances of cookie auth. Many of the other auth middleware can be duplicated too. I don't see how coming up with a custom interface per scenario helps when you have to somehow associate those with specific middleware instances.

davidfowl · 2016-12-08T18:42:18Z

Don't dismiss DI it just yet. There might be more DI friendly ways of composing the system. Let's just draw out the components and interactions we currently have.

HaoK · 2016-12-08T20:44:10Z

Yeah I think we can maybe end up with something more similar to authZ. Which has a similar problem of configuring multiple named policies. The only real difference between Authentication is that the handlers aren't global and are per scheme. But I think something maybe? ConfigureAuthentication(options => { options.AddCookies("cookie1", o => o.Handler.OnRedirect = someMethod)( options.AddCookies("cookie2"); options.AddGoogle("Google", o => o.ClientId = "secret"): }; Where basically you configure the handler instance in a new single AuthenticationOptions which would contain a named instance for each scheme which would have its own configuration and handlers (similar to authZ policy configuration today) And then we could have a single UseAuthentication middleware in the app that builds the authentication infrastructure using the fully configured options instead of relying on middleware wire up. This would let us validate everything as well much easier than what we have today.

…

On Dec 8, 2016, at 10:42 AM, David Fowler ***@***.***> wrote: Don't dismiss DI it just yet. There might be more DI friendly ways of composing the system. Let's just draw out the components and interactions we currently have. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

davidfowl · 2016-12-09T07:07:57Z

@HaoK Ohhh, nice. What do you think the auth service would look like? I think we would nuke the IAuthenticationManager from HttpContext as well.? Seems like we just need a service with Challenge and Forbid (if we stick with that approach on the calling side).

HaoK · 2016-12-09T07:28:40Z

@davidfowl Yeah we can finally get rid of that weirdness with half of Auth living in HttpAbstractions, and have IAuthenticationManager just be a normal serivce. At security triage today we also discussed briefly how it might make sense to split apart the cookie/session specific SignIn/SignOut from the Challenge/Forbid as well, so maybe we need something like IAuthenticateManager and some kind of ISignInManager (which collides with Identity's SignInManager but that would need to align with these changes anyways...)

HaoK · 2016-12-09T07:36:30Z

Also solving the active problem should be fairly straightforward with a central single AuthenticationOptions:

  ConfigureAuthentication(options => {
    options.AddCookies("cookie1");
    options.AddCookies("ApplicationCookie");
    options.AddGoogle("Google");
    options.DefaultAuthenticateScheme = "ApplicationCookie";
    options.DefaultChallengeScheme = "Google";
  }

With the only magic being, if no default is specified, the default is only implied when there's a single auth scheme configured.

HaoK · 2016-12-09T07:53:23Z

Yeah we definitely should nuke the weird GetAuthenticationSchemes() method from the IAuthenticationManager, since that would be available from IOptions<AuthenticationOptions> directly:

We should also revisit the weird AuthenticationProperties/Description legacy stuff we carried over as that seems way more complicated than it should be...

    // sugar overloads omitted
   public interface IAuthenticationManager {
     // AuthenticateInfo contains Principal + metadata (i.e. cookie expiration/refresh tokens/etc)
     public AuthenticateInfo AuthenticateAsync(string scheme)
     public Task ChallengeAsync(string scheme)
     public Task ForbidAsync(string scheme)

     // maybe try move these somewhere else?
     public Task SignInAsync(string scheme, ClaimsPrincipal user);
     public Task SignOutAsync(string scheme);

     // Nuked: IEnumerable<AuthenticationDescription> GetAuthenticationSchemes()
   }

davidfowl · 2016-12-09T08:18:11Z

Who implements the interface? Today it's possible for non middleware components to implement an auth handler (IIS Integration does it as well as WebListener). How would those change as a result?

Some implementations outside of the security middleware:

Call sites that use the IAuthenticationManager:

The status code needs to be changed as a result of calling Forbid or Challenge. If we have a service we'd need to inject the IHttpContextAccessor to access the current http context since that isn't being passed in via any of the methods. Today the auth handlers either capture the http context on per request construction or you have an authentication middleware which does this

Security/src/Microsoft.AspNetCore.Authentication/AuthenticationMiddleware.cs

Lines 65 to 86 in 219617f

    
           public async Task Invoke(HttpContext context) 
        
           { 
        
               var handler = CreateHandler(); 
        
               await handler.InitializeAsync(Options, context, Logger, UrlEncoder); 
        
               try 
        
               { 
        
                   if (!await handler.HandleRequestAsync()) 
        
                   { 
        
                       await _next(context); 
        
                   } 
        
               } 
        
               finally 
        
               { 
        
                   try 
        
                   { 
        
                       await handler.TeardownAsync(); 
        
                   } 
        
                   catch (Exception) 
        
                   { 
        
                       // Don't mask the original exception, if any 
        
                   } 
        
               }

.

It's funny, after looking at this a bit, the middleware basically has no logic... I think we're on the right track.

davidfowl · 2016-12-09T08:24:44Z

/cc @PinpointTownes

HaoK · 2016-12-09T09:25:18Z

Today the manager is an abstract class, so there probably would just be our default implementation.

I'm not super familiar with the host auth handlers, @Tratcher will have to chime in with how these might work...

I'm guessing maybe these would look like this?

services.ConfigureAuthentication(options => {
    options.AddIISIntegration("Windows");
    options.DefaultChallenge = "Windows" // if there are other auth schemes configured
});

services.ConfigureAuthentication(options => {
    options.AddWebListener("WebListener"); // which also registers Kerberos/NTLM/Negotiate/Basic schemes
    options.DefaultChallenge = "WebListener" // if there are other auth schemes configured
});

Also since we want to get away from the singleton lifetime that middleware forces on us, we can
use the pattern from: #1014, for the basic building block that all of the AddXyz methods would use:

services.ConfigureAuthentication(options => options.AddScheme("Custom", 
   o => o.HandlerType = typeof(MyHandler));
services.AddScoped<MyHandler>();

public class MyHandler : IAuthenticationHandler {
   public MyHandler(IHttpContext accessor, DbContext context);

   // Implementation of IAuthenticationHandler would be similar just without any chaining to prior handlers
}

Note: for our Security based auth handlers, we'd keep a singleton handler per auth scheme type, and use an EventsType service/instance which would expose the extensibility points (something like: #1055)

brockallen · 2016-12-09T13:17:04Z

Hao, we should have a meeting where we can show you how we use this stuff in a real app today.

HaoK · 2016-12-09T15:59:08Z

That seems reasonable, @blowdart can you set something up?

blowdart · 2016-12-09T17:09:23Z

@danroth27 is already on it.

muratg · 2016-12-09T17:27:00Z

cc @Eilon

davidfowl · 2016-12-09T17:50:58Z

@HaoK forcing the use of the IHttpContextAccessor makes me sad, especially because it's not in by default. We'll need to revisit that.

kevinchalet · 2016-12-09T18:57:52Z

/cc @PinpointTownes

Sorry for the late feedback (I'm sick and stuck in my bed... 😢)

To be honest, I'm not a huge fan of the "full DI" approach suggested by @HaoK and @brockallen as it will likely introduce a whole new set of issues without really solving the initial one.

First issue: how do you control when the authentication handler is invoked if it's automatically registered for you by the security stack? In practice, it's super common to register the security middleware after things like the static files middleware, the URL rewriting module or the exception page handler to avoid wasting CPU cycles validating a cookie that won't be used at all.

Second issue: how do you support path-specific authentication scenarios with this new design?

// Add a middleware used to validate access tokens and protect the API endpoints.
app.UseWhen(context => context.Request.Path.StartsWithSegments("/api"), branch => {
    branch.UseOAuthValidation();
});
  
// Warning: starting with ASP.NET Core 1.1.0, having multiple authentication middleware
// with AutomaticChallenge = true is no longer supported. To ensure the validation and
// cookies middleware don't collide, app.UseIdentity() is only invoked for non-API calls.
app.UseWhen(context => !context.Request.Path.StartsWithSegments("/api"), branch => {
    branch.UseIdentity();
});

Personally, I'm quite happy with the existing "middleware-like"/Russian doll design, tho' there are things that can definitely be improved:

Similarly to what we do for traditional middleware when returning a response, we should update all the built-in middleware to avoid invoking the rest of the authentication handlers chain when the response is already handled by a handler.
AuthenticationHandler shouldn't be responsible of guessing whether a "forbidden" or a "challenge" response should be returned depending on whether a principal could be extracted from the request or not (this ChallengeBehavior.Automatic thingy proved to be confusing and totally inadequate). This decision should be made at a much higher level (ideally, in the authorization stack, as it's the only place where we can determine whether a request was rejected due to a lack of authentication - via policy.RequireAuthenticatedUser() or a lack of permission).

Note: for our Security based auth handlers, we'd keep a singleton handler per auth scheme type, and use an EventsType service/instance which would expose the extensibility points (something like: #1055)

Note that it will require revamping the cookies middleware, as it uses fields to store intermediate state objects (e.g to determine if a cookie should be renewed) 😄

HaoK · 2016-12-09T19:07:28Z

@PinpointTownes we still have a single instance of UseAuthentication that would be required, so you'd still be able to place that middlewhere in the proper location (after static files, etc) so I don't think this affects the ability to skip auth for some requests.

Also if there an equivalent way of splitting your configure services similar to the app.useWhen, you could just configure the services differently based on what branch of the app the request is in...

@davidfowl Actually maybe we wouldn't need the handlers to use IHttpContextAccessor, the IAuthenticationHandler methods today already take context objects, so we could start stuffing the httpContext into each context when the new single middleware invokes the handler?

    // Today's IAuthenticationHandler (notice how forbid is not there, forbid only exists as a challenge behavior)
    public interface IAuthenticationHandler
    {
        void GetDescriptions(DescribeSchemesContext context);

        Task AuthenticateAsync(AuthenticateContext context);

        Task ChallengeAsync(ChallengeContext context);

        Task SignInAsync(SignInContext context);

        Task SignOutAsync(SignOutContext context);
    }

    public enum ChallengeBehavior
    {
        Automatic,
        Unauthorized,
        Forbidden
    }

public class MyHandler : IAuthenticationHandler {
   public MyHandler(DbContext context);
 
   public Task ChallengeAsync(ChallengeContext context) {
      if (DbContextSaysDoSomething()) {
          return DoChallenge(context.HttpContext);
      }
   }

}

davidfowl · 2016-12-09T19:08:15Z

To be honest, I'm not a huge fan of the "full DI" approach suggested by @HaoK and @brockallen as it will likely introduce a whole new set of issues without really solving the initial one.

That's pretty vague. The current approach was inherited from katana with tweaks that happened along the way to make it ASP.NET Core ready. I absolutely think we should consider the alternative design and look at the 2 things side by side and decide if we should do anything for the next major release or not. There would still be an authentication middleware in the pipeline that would use these services.

Second issue: how do you support path-specific authentication scenarios with this new design?

You never needed to if you challenged by scheme.

HaoK · 2016-12-09T19:54:11Z

@PinpointTownes if all you want to do is change the default behavior of [Authorize], you can already accomplish that today with the AuthZ: DefaultPolicy, just configure that to target the auth scheme you want.

HaoK · 2016-12-09T19:55:12Z

And that should work even with AutomaticChallenge disabled once a scheme is specified.

kevinchalet · 2016-12-09T19:55:23Z

Or maybe if you're shoving the token into a query string, then yes.

Well, it's almost a "must" in practice when dealing with stoopid JS clients, as we can't control the request headers. It's not really SignalR-specific, tho'.

The good thing is that it's completely standard, so it's not really a problem.

davidfowl · 2016-12-09T20:02:10Z

@PinpointTownes except AFAIK, our middleware doesn't read from the query string (does it?). Also query string limits with JWT tokens.....

kevinchalet · 2016-12-09T20:19:54Z

@HaoK sure, but 1. you can't set the scheme per-path. 2. it's tied to the authorization stack, so you can't implement "optional authentication" actions like you could with Web API 2 and its OWIN/Katana HostAuthenticationFilter, where authorization and authentication were clearly separated:

[HostAuthentication("Bearer")]
public IActionResult Action() {
    if (User.IsAuthenticated) {
        return Json(... complete list...);
    }

    return Json(... partial list...);
}

@davidfowl it does not... by default, but it's super easy to change thanks to the events model. Here's how I do it with the aspnet-contrib validation middleware: https://github.com/aspnet-contrib/AspNet.Security.OpenIdConnect.Samples/blob/master/samples/SignalR/HelloSignalR/Startup.cs#L28 (the JWT bearer has a similar event, but the name differs).

Tratcher · 2016-12-09T20:33:32Z

See #1062 for updated guidance regarding the current APIs. With a little more sugar around AddAuthorization/DefaultPolicy/AddPolicy I think we could completely remove AutomaticChallenge and ChallengeAsync().

Tratcher · 2016-12-09T21:49:07Z

In fact, we could even update the VS templates for 1.0.x to use AuthPolicies rather than AutomaticChallenge.

brockallen · 2016-12-10T18:06:00Z

One thing to recognize here is that really there's only the need one for cookie handler, and one google handler, and one oidc handler ever in DI. What we need multiple of is settings. IOW, we just need one instance of the code, and then possibly multiple named settings/config.

Decoupling the code from the config would be useful, especially for OIDC. I had a customer that used Katana that needed hundreds of the ws-fed middleware, and it wasn't pretty to make that happen. For this redesign if it's possible to decouple those that'd be great.

And then taking that one step further, the ability (at least for the external config like OIDC and eventually ws-fed) if those could be loaded from a "store" style interface. Perhaps by default it's just an in-memory implementation for the common scenarios where there are only one or two, but it allows for the possibility of a database to back the OIDC config.

HaoK · 2016-12-10T18:31:06Z

@brockallen yup that's where I was at too, with a single instance of each type of handler, and a named configuration/settings for each "scheme" in DI.

Once we have that, it would be fairly easy to load the config/settings from another source, since the settings are just normal options, so this would already more or less work with options/config binding.

settings.Configure<AuthenticationOptions>(Config.GetSection("Authentication"));

But we might need to spend some cycles making it easier to read options/configuration from a database for your second step (cc @divega)

davidfowl · 2016-12-10T23:02:33Z

@HaoK @brockallen That's where we were all at but @PinpointTownes isn't on the same page. At this point I think we need to prototype the other system and show key scenarios in both of them, including the ones @PinpointTownes feels strongly about.

Then we can weigh the pros and cons of each.

brockallen · 2016-12-10T23:39:14Z

Yep... who knows -- this all might just prove that what's already here is good enough (with some adjustments). But it's still worth the exercise.

HaoK · 2016-12-10T23:55:47Z

Ok I will start a PR next week with some real working code.

kevinchalet · 2016-12-11T00:09:34Z

That's where we were all at but @PinpointTownes isn't on the same page.

@davidfowl I'm always skeptical when hearing that DI is involved in multi-instances scenarios. Last time you did that, it was to bring back the ConfigureXYZAuthentication extensions without named options support, which led to a very confusing pattern (I've avoided it in all the aspnet-contrib projects because a few people have emailed me to tell me they didn't understand how it was working).

davidfowl · 2016-12-11T00:15:19Z

Take 3, well try it again and all party on the PR.

Tratcher · 2016-12-11T00:18:46Z

@HaoK, this thread has wandered quiet a bit, care to summarize the issues you plan to address in your PR?

HaoK · 2016-12-11T01:16:00Z

I was going to start from the public service area first. Starting with something like: ConfigureAuthentication() => AuthenticationOptions + AuthenticationScheme[Builder] to represent how we would register the name scheme/handler which would be the replacement for today's events + AuthenticationDescription. IAuthenticationSchemeProvider which would be responsible for resolving named schemes and providing the actual handler and configuration. This would be the extensibility point for plugging in stuff like database driven schemes. This would just be the master AuthenticationOptions by default. Also the single UseAuthentication() master middleware that would consume the uber options and dispatch/create the appropriate handlers for a request. And finally removing auth from httpabstractions so moving everything we need into the base auth package.

HaoK · 2016-12-12T00:51:35Z

Sneak preview of what I've distilled from this thread so far, in case there's any early feedback: https://gist.github.com/HaoK/0197141b932bcf45d99dcb5e6eb70374

HaoK · 2016-12-12T20:46:27Z

Started the PR here: #1065

HaoK · 2017-04-13T23:36:19Z

Tracked via #1179

blowdart added breaking-change discussion enhancement labels Dec 7, 2016

blowdart added this to the 2.0.0 milestone Dec 7, 2016

blowdart mentioned this issue Dec 7, 2016

Consider reverting https://github.com/aspnet/Security/pull/982 #1044

Closed

blowdart mentioned this issue Dec 7, 2016

Make it harder (or impossible) to have more than one auth provider have "automatic" challenges #1054

Closed

This was referenced Dec 9, 2016

[Announcement] AutomaticChallenge Usage #1062

Closed

AutomaticChallenge Usage aspnet/Announcements#210

Open

HaoK mentioned this issue Jan 13, 2017

[AuthZ] Review authorize filter behavior again #300

Closed

Eilon added needs design and removed discussion labels Jan 26, 2017

Eilon assigned HaoK Jan 26, 2017

HaoK mentioned this issue Feb 3, 2017

Enable IClaimsTransformer to be specified in DI instead of only options #863

Closed

This was referenced Feb 11, 2017

CodeFlow. AccessDenied error when a logged user directly go to ~/signin openiddict/openiddict-samples#33

Closed

Auth 2.0 Iteration 2 #1113

Closed

HaoK mentioned this issue Apr 13, 2017

The Grand Auth Redesign of 2017 #1179

Closed

13 tasks

HaoK closed this as completed Apr 13, 2017

Redesign automatic authentication choice mechanism #1061

Redesign automatic authentication choice mechanism #1061

Comments

blowdart commented Dec 7, 2016

blowdart commented Dec 7, 2016

leastprivilege commented Dec 7, 2016

brockallen commented Dec 7, 2016 • edited Loading

davidfowl commented Dec 8, 2016

brockallen commented Dec 8, 2016 • edited Loading

davidfowl commented Dec 8, 2016

Tratcher commented Dec 8, 2016

davidfowl commented Dec 8, 2016

HaoK commented Dec 8, 2016 via email

davidfowl commented Dec 9, 2016

HaoK commented Dec 9, 2016

HaoK commented Dec 9, 2016

HaoK commented Dec 9, 2016

davidfowl commented Dec 9, 2016

davidfowl commented Dec 9, 2016

HaoK commented Dec 9, 2016 • edited Loading

brockallen commented Dec 9, 2016

HaoK commented Dec 9, 2016

blowdart commented Dec 9, 2016

muratg commented Dec 9, 2016

davidfowl commented Dec 9, 2016

kevinchalet commented Dec 9, 2016

HaoK commented Dec 9, 2016 • edited Loading

davidfowl commented Dec 9, 2016

HaoK commented Dec 9, 2016 • edited Loading

HaoK commented Dec 9, 2016 • edited Loading

kevinchalet commented Dec 9, 2016

davidfowl commented Dec 9, 2016

kevinchalet commented Dec 9, 2016 • edited Loading

Tratcher commented Dec 9, 2016 • edited Loading

Tratcher commented Dec 9, 2016

brockallen commented Dec 10, 2016 • edited Loading

HaoK commented Dec 10, 2016

davidfowl commented Dec 10, 2016

brockallen commented Dec 10, 2016

HaoK commented Dec 10, 2016 via email

kevinchalet commented Dec 11, 2016

davidfowl commented Dec 11, 2016

Tratcher commented Dec 11, 2016

HaoK commented Dec 11, 2016 via email

HaoK commented Dec 12, 2016

HaoK commented Dec 12, 2016

HaoK commented Apr 13, 2017

brockallen commented Dec 7, 2016 •

edited

Loading

brockallen commented Dec 8, 2016 •

edited

Loading

HaoK commented Dec 9, 2016 •

edited

Loading

HaoK commented Dec 9, 2016 •

edited

Loading

HaoK commented Dec 9, 2016 •

edited

Loading

HaoK commented Dec 9, 2016 •

edited

Loading

kevinchalet commented Dec 9, 2016 •

edited

Loading

Tratcher commented Dec 9, 2016 •

edited

Loading

brockallen commented Dec 10, 2016 •

edited

Loading