The Grand Auth Redesign of 2017

[HaoK]

Latest PRs:

• Options Options 2.0 Iteration 3 - Breaking change edition: Takeover IOptions Options#179
• HttpAbstractions: Auth 2.0: Add Auth Abstractions, Core HttpAbstractions#798
• Security: Iteration 4 - Using Auth 2.0/Options 2.0 packages #1170
• MVC: Update MVC to Auth 2.0 Mvc#6131
• Identity: Auth/Options 2.0 + Identity Identity#1119
• Music Store: Update to Auth 2.0 MusicStore#780
• SignalR: SignalR => Auth 2.0 SignalR#406
• IdentityService: https://github.com/aspnet/IdentityService/pull/63
• Templates: Templates => Auth 2.0  dotnet/templating#624

Fixes

• Dynamic scheme handler support: Add/Remove/Configure Authentication Middleware at Runtime #1132
• Named/multi tennant options Middleware Extensibility: Support "Multi-tenancy" In AuthenticationOptions (and more) #35
• RIP automatic Redesign automatic authentication choice mechanism #1061 Automatic Cookies challenge overridden by Windows Authentication since 1.1.0 HttpSysServer#284
• Signout does not throw in 2.0 Signout for google throws #1039

To be continued in: #1190

@Eilon @muratg @Tratcher @ajcvickers @blowdart

[RickBlouch]

I have read through the history for much of the Auth 2 issues and have browsed the code in the hoak/auth2 branch trying to get an early understanding of the new system. I realize it's still a work in progress so I may be too early and if that's the case just let me know, but since the "Named/multi tenant options #35 " is marked as close I'm hoping you can point me in the right direction.

Right now I am using something very similar to this post where the request pipeline is being forked for each tenant (based on hostname) and I am able to create tenant specific authentication middleware (among other tenant specific middleware setup) configurations based on tenant specific values retrieved from our database. More specifically we have scenarios where some tenants have one or two OIDC middleware configured and others may not have any.

I can see in the new codebase that this will not work the same way with Auth 2 and that's fine, but I couldn't figure out based on reading the various issues and browsing the new codebase exactly how I will replicate the above functionality. Can you point me in the right direction?

[HaoK]

There won't be any way to fork things since there's only a single service collection, but you should be able to register each using the tenant specific name similar to how the cookie name is being set in that post you mention, if you don't know the tenants at startup, can add dynamically schemes via the ISchemeProvider.AddScheme https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Authentication.Abstractions/IAuthenticationSchemeProvider.cs#L56:

   services.AddGoogleAuthentication("Google-<tennant>", options => { });

You'll also have to challenge/authenticate using the new tenant specific names as opposed to just always "Google", but I imagine that shouldn't be too big of a change.

Does that sound like it will work for your scenario?

[CoskunSunali]

I have a scenario where I implement the things exactly the way @RickBlouch does.

The multiple-schemes solution does not sound like the best scenario - if I don't get it wrong.

The way @RickBlouch and I implement the things makes each tenant able to access only the services specific to itself but the way you suggest by schemes solving the issue requires each tenant to have access to all schemes even if they don't relate to it.

What is more, it requires (if I am not wrong) manually invoking the challenge/authenticate methods when needed as opposed to having a single scheme for each tenant's own pipeline using the same scheme name and having it automatically handled.

@HaoK , I have not really looked through the changes you made but the main complaint about the auth configuration was that we had to configure some of the things too early in the app life cycle. For instance we still did not know which tenant was being requested (lazy initialization) but we had to configure the token endpoint using a specific URL instead of tenant specific services and token endpoints based on the tenant host names.

We discussed the same thing with @PinpointTownes at aspnet-contrib/AspNet.Security.OpenIdConnect.Server#107 shortly. The issue does not provide much information but it should give you an idea. Otherwise I invite @PinpointTownes to this discussion.

The following quotation from @PinpointTownes's comments describes the issue very well, in my opinion.

Sadly, this won't work, as AuthenticationMiddleware calls IOptions<>.Options directly in its constructor (where you can't access the current request, since there's simply none 😄) and stores it as a singleton: https://github.com/aspnet/Security/blob/dev/src/Microsoft.AspNet.Authentication/AuthenticationMiddleware.cs#L26-L34

To support multi-tenancy, AuthenticationMiddleware's inheritors would also have to update their constructor, to avoid accessing the options instance too early. I guess we'd need a ValidateOptions directly in AuthenticationMiddleware to delay options validation.

[HaoK]

So far we have only enabled dynamically adding/removing schemes since its is the first building block needed for multi-tenancy.

To keep each tenant's ability to challenge "Google"/"Facebook" with the new stack, you should be able to accomplish this by replacing some combination of IAuthenticationService/IAuthenticationSchemeProvider/IAuthenticationHandlerProvider to resolve the appropriate tenant specific instance using the request.

The main idea was to make auth service based rather than middleware, so there should be a lot more extensibility/flexibility now

[RickBlouch]

There won't be any way to fork things since there's only a single service collection, but you should be able to register each using the tenant specific name similar to how the cookie name is being set in that post you mention, if you don't know the tenants at startup, can add dynamically schemes via the ISchemeProvider.AddScheme https://github.com/aspnet/HttpAbstractions/blob/dev/src/Microsoft.AspNetCore.Authentication.Abstractions/IAuthenticationSchemeProvider.cs#L56:

services.AddGoogleAuthentication("Google-", options => { });

Forking the pipeline was a pretty clean way to do things so I'm sad to see it go; this feels like a step backwards in that regard. From top to bottom, my pipeline could be unique per tenant which was great in terms of isolation. We deal with PCI and HIPAA so the more isolation the better. It's also not realistic to load all tenants at startup time, I can't imagine that would be the case for very many enterprise apps.

Just to confirm what you are suggesting, in order to dynamically add these schemes on the fly, I'm guessing I will have to have a step in my tenant resolution middleware that makes a call to GetSchemeAsync and checks if a scheme exists with my tenant based scheme name "Google-*****". If not, call AddScheme.

Does that sound like what you are anticipating? Will AddScheme be adding additional dependencies to my DI container with every call? At 1000-2000 tenants will that be a problem?

You'll also have to challenge/authenticate using the new tenant specific names as opposed to just always "Google", but I imagine that shouldn't be too big of a change.

I can inject an object that has tenant context so yes this can happen pretty easily, just something extra I need to remember to do that I didn't have to with a forked pipeline.

await _httpContextAccessor.HttpContext.Authentication.SignInAsync("Google-<tenant>", new ClaimsPrincipal(id), authInfo.Properties);

Does that sound like it will work for your scenario?

I don't think I have a good enough understanding of the new way to make that decision yet. I'm going to see if I can evaluate IAuthenticationService/IAuthenticationSchemeProvider/IAuthenticationHandlerProvider next.

[HaoK]

That sounds right at this point, that said, we will likely spend some cycles to have a 'blessed' way that we encourage for multi-tenancy, so things should get better. Make sure to be involved in that issue/PR when we start on that work. cc @blowdart

[HaoK]

Initial wave of PRs pushed to security/MVC/Identity/MusicStore, tests all passed locally so hopefully no 💥

[kevinchalet]

Otherwise I invite @PinpointTownes to this discussion.

@CoskunSunali haha, thanks!

Multi-tenancy was indeed mentioned a few times during the "Auth 2.0" design: @HaoK was in favor of a "multiple scheme handlers" approach while I was more inclined to think that it was not really scalable and that the best approach would be to have a single scheme handler with potentially an infinity of options, resolved dynamically (per request):
#1113 (comment) #1113 (comment)

While I haven't tested (yet), I believe the approach I mentioned in the ASOS topic should now be possible, since options validation and initialization are now delayed. That said, I don't think the experience is ideal (yet), because a few important things are still missing (e.g async support in the options stack).

I'll start porting ASOS to ASP.NET Core 2.0 in the next few days, so I'll have a chance to test whether things work correctly, even for multi-tenant scenarios 😄

[RickBlouch]

@PinpointTownes I would be interested to know your findings with the approach you mentioned in that link. I also believe a single scheme approach with options resolved per request is the cleanest option all around. Please let me know where I can follow your progress / results of your testing. I'm not in position right now to be able to play with the 2.0 bits unfortunately.

[HaoK]

@PinpointTownes I think it should be easier with the new 2.0 stack to implement single handler with infinity options resolved per request if you desire. Its also possible we end up there for the built in auth handlers as well

[CoskunSunali]

@HaoK Do I get that right that you the AuthorizeAttribute of MVC will iterate through the scheme collection and return true as soon as one of the schemes is able to validate the principle/identity?

https://github.com/aspnet/Mvc/pull/6131/files#diff-fbf649a01b0521186bc5d75f07c20d8f

If I am wrong, how do you imagine the AuthorizeAttribute working in a multi-tenant scenario?

Apart from that, does any of the changes you made so far really avoid us using the UsePerTenant<Tenant> implementation we have at #35 (comment) ? If so, can you please be so kind to lead me to the exact file change where I can see the reasons behind the scene?

[HaoK]

Well the long answer for authorize is it is evaluating a policy's requirements against a ClaimsPrincipal, which is assembled using a union of all of the ClaimsPrincipals returned from the schemes specified in the policy.

So for multi-tenant scenarios, the question really is what does the policy look like there, and that depends on how the authentication schemes are setup. If there's a single logical "Google" then the policy can just ask for that. If there's a different scheme per tenant, i.e. "Google-#135", then you likely will have to plug in a tennant-aware AuthorizationPolicyProvider that turns [Authorize("Google")] into really asking for the "Google-#135" scheme.

All that said, revisiting AuthZ is next up, so all of this is subject to change real soon (next few weeks)

The Auth 2.0 changes make the UsePerTenant a bit moot, unless you have a different service container per tenant, since there's a single IAuthenticationSchemeProvider right now.

[CoskunSunali]

@PinpointTownes, @HaoK

Multi-tenancy was indeed mentioned a few times during the "Auth 2.0" design: @HaoK was in favor of a "multiple scheme handlers" approach while I was more inclined to think that it was not really scalable and that the best approach would be to have a single scheme handler with potentially an infinity of options, resolved dynamically (per request)

I certainly agree. You cannot really initialize the whole tenant instances as part of the first incoming request in an enterprise environment. Imagine having 200 tenants on a server and the server just getting up. Consider the server getting its first request made by a random visitor. Making him wait to initialize 200 tenants all at once and then serving him his request does not sound like a great idea. (I know what @HaoK implemented does not require us to initialize 200 tenants all at once so that is not about what he did/does.)

I definitely don't want my tenants to be able to access the authorization scheme of each other. I would certainly like them to be isolated from each other - the way it should be.

So all in all, if all of these changes are being made, we have to consider scenarios where tenants are dynamically resolved, initialized, re-initialized and even un-initialized at times.

@HaoK, Let me try to give you some concrete examples.

Resolve and initialize a tenant

Happens when a request to that tenant comes in for the first time. Not when you are starting up the Asp.Net app, not when you are configuring the services or the request pipeline. After all of these phases passed. Out of the blue. I might even add a new tenant configuration to my database and it should be ready to be initialized as soon as one makes a request to that tenant's host name.

A tenant - in my humble opinion - is not really a tenant unless it can have its own collection of services and its own service provider (I handle this using some dirty way of cloning the original IServiceCollection and IServiceProvider services). Consider your tenants being able to contain plugins. One tenant might have services related to it registered and others not.

These services should be isolated from each other and sometimes not based on the service. Let's say, if you have a service registered as a singleton, it has cases where it really is a singleton and is shared among the tenant's own service provider. However, there are cases where a singleton is a singleton within the scope of a tenant.

A tenant - again in my humble opinion - is not really a tenant unless it can have its own request pipeline. Again consider your tenants being able to contain plugins. One tenant might have a middleware registered within its request pipeline while others not.

Re-initialize a tenant

Happens when one calls tenant.Reload(), let if be for having a new plugin added to the tenant or even having that tenant's host name configured by the owner of the tenant. Could also be in case one removes a plugin from the tenant. So that the tenant's service collection, service provider and request pipeline gets all created from the scratch.

Technically speaking, re-initializing is basically removing the tenant from a concurrent list of initialized tenants and having it initialized with the first upcoming request.

Un-initialize a tenant

Happens when a tenant is removed from the data store (be it a database or a configuration file) and that should really leave all other tenants intact and should not cause all other tenants to re-initialize.

Technically speaking, un-initializing is basically removing the tenant from a concurrent list of initialized tenants (of course disposing its services, etc for the sake of scaling and performance).

Back to Asp.Net

@HaoK, no pun intended for you. You are at least trying it. Please don't take anything below this line personal. I don't know if it is just me but 80% of the projects I have developed/lead/architected myself in the last 20 years have always required the multi-tenancy scenario. I personally - maybe @PinpointTownes remembers - fought for having Asp.Net Core support the multi-tenancy scenarios even back in 2015. Please see dotnet/aspnetcore#743 (comment).

With all these breaking changes - I should now ask those team members to come and cover our times/efforts - and also yours to fix the things that should be well thought initially before the version 1.0 came out.

So many people around me and around the communities complain about the lack of multi-tenancy support in Asp.Net Core framework, also the breaking changes and being have to re-write many things with every release of Asp.Net Core, there is not much to say.

I personally think that the way Asp.Net Core applications are being bootstrapped at the moment is not exactly multi-tenant friendly. @HaoK has no fault in that and my intention is not to point fingers, otherwise I can name a few people who immediately refused my multi-tenancy requests by saying we have other things to do and we will look into that in the future.

I understand the team here develops a "framework" and not a "real app". Meaning, I am aware of the fact that they cannot support all scenarios which developers may or may not need. However, I consider multi-tenancy to be a must have when it comes to a framework using which you develop web apps.

For instance, I don't know who designed it initially, but having the authorization options being have to configured within ConfigureServices was the worst idea ever since invention of computers. We don't have a request, we don't know what host name, no tenant, no service instances, nothing. All you have is the hosting environment, its primitive service provider containing a few services and the logger factory.

Another instance, the built-in service provider. We cannot even create a child container (we can create just a scope) on demand. Even if we hack the hell out of it and somehow create a child container, we cannot add services to or remove from it. Even the constructor of the out of the box ServiceProvider is internal, we always have to use the IServiceProviderFactory which is fine but its Build method does not allow us to provide it with a new collection of child services, etc. I know you can plugin your own IServiceProviderFactory but that requires you to configure one more thing - the IHostBuilder and I think that is a bit of too much. Funny thing is that I read the community asking for child containers but - without pointing finger - some people say we don't see the need for it and still seal classes here and there. Hey, this is a framework we have to extend!

Honestly speaking, I have been developing Asp.Net since version 1 beta 1 (2001?) and I believe I have some idea about what to expect from a framework, I still cannot suggest my clients use Asp.Net Core. It is almost version 2.0 coming out and in my opinion, I still consider it a product in its beta stages. That is mostly because of the incredible amount of breaking changes being announced with every single push to the repo. There is no backwards-compatibility if you update your packages - and you always end-up being have to, to take advantage of a new feature being introduced. Anyone here remembers Microsoft Solutions Framework? MSF states that you should not - and cannot - introduce breaking changes after an RC version is released. Is it just me remembering a million breaking changes being introduced after the RC versions of Asp.Net Core 1.0 were released? Forget about MSF, how in the world a product can get to RC if it is going to have a major re-write of the API and functionality? It is Release Candidate after all.

I think I can sit here and spend my time writing and waste your time reading how important it is for real-world-apps to have multi-tenancy support out of the box for the next 2 days.

To summarize the things in a single sentence: The current implementation of Asp.Net Core does not support multi-tenancy as a first-class citizen and it will not be able to unless the service container and request pipeline can be configured based on a tenant.

When it comes to multi-tenancy in Asp.Net Core, all we are doing here is finding dirty workarounds, unfortunately.

Long live tenants!

[blowdart]

@davidfowl @DamianEdwards so they can have a read, as ideally this goes way beyond identity.

[blowdart]

@CoskunSunali So how are you identifying tenants in your setup? Host name? path? Something else. I think part of the problem (aside from Hao being limited to identity here) is that everyone wants different ways to identify tenants, so a list of some concrete ones in use would be a good start.

[RickBlouch]

@blowdart I can't speak for @CoskunSunali but we are doing hostname tenant resolution. That being said, isn't that just be an implementation detail that should be an abstraction?

You should take a look at SaasKit which cleanly allows for container and pipeline isolation. this sample is pretty simple and shows pipeline forking with a custom tenant resolver that happens to use hostname to identify tenants but could be based on anything in the request.

[CoskunSunali]

@blowdart Thanks for your attention! I can certainly agree that this goes beyond identity and that is exactly why I kept repeating that my words should not be taken personally by @HaoK at all. Should I actually create a new issue in the MVC repo and post my loooong comment as a new issue there?

Regarding your question, it is the host name 100% of the time. I have, personally, never heard of anyone building tenants based on something else. That might be me, I cannot say no one ever would.

Based on my understanding, it does not matter either. The resolution of the tenant is not a part of the framework's job. Tenant is just a concept here and it literally means an entity which corresponds to a request and has its own service container as well as its own request pipeline.

In my case, I find the tenant by its host name (ConcurrentDictionary<string, ITenant>) and pass its own IServiceProvider instance to the RequestServicesFeature of the HTTP context. I also know what middlewares its request pipeline consists of and I invoke them when a request comes into that tenant.

All of the following examples also use host name to resolve the tenant.

https://andrewlock.net/loading-tenants-from-the-database-with-saaskit-in-asp-net-core/
http://benfoster.io/blog/asp-net-5-multitenancy
http://stackoverflow.com/questions/43114075/how-to-implement-multi-tenant-functionality-in-asp-net-core

[HaoK]

I can respond a bit to the auth 2.0 specific pieces, so we definitely are going to support the ability to dynamically add/remove auth schemes, so at a minimum tenants will be able to add/remove specific auth schemes during their load/unload.

But I also wouldn't be surprised if some of the default Auth services would need to be tweaked/replaced as well.

[CoskunSunali]

@RickBlouch and I posted at the same time and wrote the same things.

The resolution of the tenant is just an implementation detail. The problem here is the built in service container and request pipeline being too tied to the startup (bootstrapping?) of the application.

[kevinchalet]

Please let me know where I can follow your progress / results of your testing.

@RickBlouch sure! I'll post more details here.

@HaoK has no fault in that and my intention is not to point fingers, otherwise I can name a few people who immediately refused my multi-tenancy requests by saying we have other things to do and we will look into that in the future.

@CoskunSunali let me guess... @blowdart ?

A tenant - in my humble opinion - is not really a tenant unless it can have its own collection of services and its own service provider (I handle this using some dirty way of cloning the original IServiceCollection and IServiceProvider services). Consider your tenants being able to contain plugins.

What you describe is what I personally call "heavy multi-tenancy". It definitely has interesting pros - it doesn't require supporting multi-tenancy in every component because everything is strongly isolated at the service and middleware level and nothing is shared between tenants - but also cons, as it has a performance impact when you have thousands of tenants up at the same time.

If you like this approach, I really encourage you to take a look at Orchard Core, as it implements multi-tenancy exactly the way you want: tenants are constructed dynamically and can be stopped or restarted separately. Services are not shared between tenants and the tenants pipelines are completely isolated, but you can "import" services defined at the host level when needed (feel free to take a look at Orchard's OpenIddict module to see how we share the data protection services exposed by the host while guaranteeing perfect isolation at the crypto level by spanning tenants-specific sub-protectors: https://github.com/OrchardCMS/Orchard2/blob/master/src/OrchardCore.Modules/Orchard.OpenId/Startup.cs)

For this type of multi-tenancy, I don't think the security stack should do something special (everything should be handled at a higher level, pretty much like what Orchard Core does).

The option I suggested is actually a lot more lightweight, as services would be shared across tenants (only options would be resolved at runtime and would differ between tenants).

@HaoK's approach - registering and resolving as many scheme handlers as needed - was probably somewhere between these two.

Ideally, ASP.NET Core 2.0 should support these 3 approaches, or at least make them "not too painful" to implement.

[kevinchalet]

/cc @sebastienros

[JohnGalt1717]

All I want to do is be able to remove an openidconnect endpoint that was added with .AddOpenIdConnect and then do another .AddOpenIdConnect in a controller based on a secured request from admin. That's it.

Same for .AddWsFederation for SAML2.

I don't think it needs to be any more complex than that. Let us add/remove at runtime not just during setup and life would be good because we wouldn't have to restart our api's just to add a new sso provider. I don't need anything else because I know programmatically when one to invoke.

[Tratcher]

@JohnGalt1717 you can do that. See this sample: https://github.com/aspnet/AspNetCore/tree/5a6b6c90b6e32ecab4975c546228cc63199f168b/src/AuthSamples/samples/DynamicSchemes

[JohnGalt1717]

@Tratcher how would one add standards WsFederation or OpenIdConnect without having to implement the whole thing themselves?

From what I can tell from the sample which doesn't implement anything really, is that I'd have to do the entire thing manually this way.

Any help would be greatly appreciated. Basically I need to be able to do exactly what AddWsFederation and AddOpenIdConect do on the AuthenticationBuilder but in a controller method.

Thanks!

[Tratcher]

It is a little lower level code than you do in Startup, but you can re-use all of the implementation code.
Compare the sample here.

For OIDC it would be something like this:

_schemeProvider.AddScheme(new AuthenticationScheme("OIDC", "MyProvider", typeof(OpenIdConnectHandler)));
_optionsCache.TryAdd("OIDC", new OpenIdConnectOptions { ClientId = ... });

[JohnGalt1717]

@Tratcher Sorry for the delay in trying this. What I'm not understanding is how to pass the options (_optionsCache) to the handler.

I.e. I set it up as above but the _schemProvider.AddScheme doesn't get passed the options so I don't know how it looks them up based on the name of the AuthenticationScheme and since every one of the OpenIdConnectHandlers will be different I'm at a loss.

Thanks!

[Tratcher]

That will get mapped internally based on the scheme name ("OIDC" in this example).

[JohnGalt1717]

Ok, but then I get this:

InvalidOperationException: Provide Authority, MetadataAddress, Configuration, or ConfigurationManager to OpenIdConnectOptions
Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectOptions.Validate()
Microsoft.AspNetCore.Authentication.RemoteAuthenticationOptions.Validate(string scheme)
Microsoft.AspNetCore.Authentication.AuthenticationHandler.InitializeAsync(AuthenticationScheme scheme, HttpContext context)
Microsoft.AspNetCore.Authentication.AuthenticationHandlerProvider.GetHandlerAsync(HttpContext context, string authenticationScheme)
Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

here's how I'm adding it:

` SchemeProvider.AddScheme(new AuthenticationScheme(authMethod.Id.ToString(), authMethod.Name, typeof(OpenIdConnectHandler)));
var oidcOptions = new OpenIdConnectOptions
{

								Authority = authMethod.Type == AuthTypes.Azure ? authMethod.Audience : authMethod.IdPUrl,
								ClientId = authMethod.ClientId,
								ClientSecret = authMethod.ClientSecret,
								ResponseType = "code id_token",
								CallbackPath = "/signin-oidc",
								Resource = authMethod.Type == AuthTypes.Azure ? authMethod.ClientId : Configuration.Issuer,

#if (DEBUG)
Prompt = "login",
#endif
TokenValidationParameters = new TokenValidationParameters
{
ValidateIssuer = false
}
};
oidcOptions.Scope.Clear();
oidcOptions.Scope.Add("openid");
oidcOptions.Scope.Add("profile");
oidcOptions.Scope.Add("email");

							OpenIdOptionsCache.TryAdd(authMethod.Id.ToString(), oidcOptions);

`

This works fine at startup but fails with the above. (we use the Id as the name to differentiate each one)

And the TryAdd returns true and the Authority is being set to a valid value so the error message doesn't make much sense.

I also tried to validate the options that I created and it fails with the same error message but again, Audience is set, so there shouldn't be an issue. What am I doing wrong? (Looks like from other posts the ConfigurationManager is null but I can't figure out what I should be setting it to)

Thanks for your help!

[Tratcher]

@HaoK ?

[HaoK]

Usually errors like this where you have configured things but its not finding them are due to issues with the name of the options being specified, make sure you are specifying the same name everywhere, some of the overloads that don't take a scheme name are using a default scheme which is probably not the one you are configuring.

[JohnGalt1717]

@HaoK I checked and I'm passing it everywhere the same way.

Note that my OpenIdConnectOptions.Validate() throws the same error before it is ever tried to be used and it appears that it's failing because of ConfigurationManager = null which I don't know what it should be set to and clearly it isn't being set when the optionsCache.TryAdd is being called nor later in the process as a result of me doing this manually.

(In general this should be way easier with the same extension methods that work at startup also working later in the process but for right now I just need to get this working.)

[HaoK]

Oh its probably because the Configurationmanager is given a default value here normally: https://github.com/aspnet/AspNetCore/blob/master/src/Security/src/Microsoft.AspNetCore.Authentication.OpenIdConnect/OpenIdConnectPostConfigureOptions.cs#L71

You would need to do something similar when you are manually adding your own schemes

[Tratcher]

Wait, PostConfigure doesn't run? That's pretty broken.

[HaoK]

Not sure what you mean, post configure is only registered when you use the built in methods on the scheme you register, its not going to run on all names since there would be no way to turn it off then. You have full and total control when you are adding your own schemes like this.

[Tratcher]

(In general this should be way easier with the same extension methods that work at startup also working later in the process but for right now I just need to get this working.)

He's right, this experience is very incomplete if we can't even produce working sample code for a real auth handler.

[HaoK]

To be fair, there is no experience, we only added the extensibility points to make it possible to implement multi-tennant scenarios completely on your own via wholesale replacement. We never targeted actually supporting it with our higher level apis, because its just not something most of the current code can easily support

[JohnGalt1717]

OK, well this should be a to-do to make this usable in the real world and expand out the example code so that it actually would work (as it is right now it doesn't)

Now I've added this:

oidcOptions.ConfigurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(oidcOptions.MetadataAddress, new OpenIdConnectConfigurationRetriever(), new HttpDocumentRetriever(oidcOptions.Backchannel) { RequireHttps = oidcOptions.RequireHttpsMetadata } );

Which is basically right out of the code example above.

Which gets this error:

IDX10000: The parameter 'httpClient' cannot be a 'null' or an empty object.
Parameter name: httpClient

[JohnGalt1717]

And if I use the overload that doesn't pass the HttpDocumentRetriever I get the following error:

{"Errors":[{"Description":"Object reference not set to an instance of an object.\r\n at Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectHandler.WriteNonceCookie(String nonce)\r\n at Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectHandler.HandleChallengeAsync(AuthenticationProperties properties)\r\n at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.ChallengeAsync(AuthenticationProperties properties)\r\n at Microsoft.AspNetCore.Authentication.AuthenticationService.ChallengeAsync(HttpContext context, String scheme, AuthenticationProperties properties)\r\n at Microsoft.AspNetCore.Mvc.ChallengeResult.ExecuteResultAsync(ActionContext context)\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeResultAsync(IActionResult result)\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResultFilterAsyncTFilter,TFilterAsync\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResultExecutedContext context)\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeResultFilters()\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeNextResourceFilter()\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Rethrow(ResourceExecutedContext context)\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeFilterPipelineAsync()\r\n at Microsoft.AspNetCore.Mvc.Internal.ResourceInvoker.InvokeAsync()\r\n at Microsoft.AspNetCore.Builder.RouterMiddleware.Invoke(HttpContext httpContext)\r\n at Microsoft.AspNetCore.ResponseCompression.ResponseCompressionMiddleware.Invoke(HttpContext context)\r\n at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.Invoke(HttpContext context)","ErrorType":"System.NullReferenceException","Property":"Microsoft.AspNetCore.Authentication.OpenIdConnect","Url": … }

(and the other override that takes an httpclient also throws.)

So one override will NEVER work. And the other 2 overrides fail with the above error message. And that's just openIdConnect. God knows what's going to happen when I try WSFederation for SAML2.

[Tratcher]

@JohnGalt1717 you're right, this approach is unusable.

@HaoK we should take that sample down.

[HaoK]

The sample is demonstrating the ability to add and remove schemes per request, and how to plug in options for those. Its goal was not to be a working multitenant sample for the OIDC handler, there's a separate issue talking about multi-tenant, but not necessarily OIDC here dotnet/aspnetcore#4126

[JohnGalt1717]

@HaoK the sample doesn't work and never could work and doesn't actually demonstrate what you'd actually have to do to make it work.

Thus it is worse than useless. I'd suggest the people responsible for this pr actually go and use that sole to create an azure openidconnect endpoint and make it work. Don't you can't .

[HaoK]

So there's a mismatch of expectations I think here, the existing auth handlers and related stack in Security (including OIDC/OAuth) are not built at all to make it easy to do multi-tenant. The only goal for the 2.0 auth changes was to preserve the ability to do it yourself via replacing most of the options + potentially the core authentication pieces as well.

This sample was only meant to demonstrate how you can add and remove schemes and their options per request at the Abstractions layer (https://github.com/aspnet/AspNetCore/tree/master/src/Http/Authentication.Abstractions/src) as opposed to the Security layer (which is not really designed for this and would require a lot of work to enable)

[HaoK]

I'll add a paragraph in the readme stating that this sample isn't meant to be a solving the entire multi-tenant auth scenario (and point at the relevant other issues for those approaches).

[JohnGalt1717]

That's fine and all and yet I still can't make this work. Please advise what needs to happen in my code to make this actually work.

And your sample again can never work because of bugs and because of incompleteness. And I suspect if you actually flushed out the sample you'd see how woefully inadiquate the work that's been done on this is.

[dazinator]

Hello. I was wondering, instead of making Api changes, why not solve multitenancy by forking the middleware pipeline, and container / IServiceProvider, per tenant. That approach allows all api's to remain the same, as it solves the multitenancy problem at a higher level. For example: https://github.com/dazinator/Dotnettency/blob/develop/README.md
Do you think forking middleware pipeline and containers is overkill? Or is there some other criticisms we should be aware of with this approach? Most multitenant systems also end up with other customisation per tenant which could touch all levels of the stack, from what middleware should run, to what implementation of a specific service to inject, to what views / pages should be rendered (i.e mvc / razor), to what files should be visible (one tenant shouldnt be allowed to access anothers), to what database should be used etc. It seems more degrading to the stack (to me!) to make all these services support named options in the name of multitenancy, than it does to solve it higher up in the stack!

[HaoK]

We aren't planning on addressing multitenancy for 3.0, we were just adding a more fleshed out multi-tenant auth sample that uses some of the Security stack, there was a PR open in AuthSamples and it will eventually get checked into once Mondo repo is in a good place again, its here:

https://github.com/aspnet/AuthSamples/pull/44/files

[JohnGalt1717]

In the meantime, I'm still completely blocked because of at least one bug in .net core that needs to be fixed.

Can someone give a snippet of a working openidconnect handler being added after the fact please?

[JohnGalt1717]

@HaoK that pr doesn't actually address the primary use case in multi tenancy. In the VAST majority of cases tenants will need sso with either openidconnect or saml2 (wsfederstion as Ms likes to call it)

The samples should be focused on adding and removing those at runtime dynamically, not some custom authentication protocol that people would be insane to attempt given how complex sso is to do right.

[JohnGalt1717]

So I've gotten further with openIdConnect.

I'm able to get it to go to the openIdconnect endpoint and the user logs in there and then I get this:

Exception: Correlation failed.

Show raw exception details
Exception: An error was encountered while handling the remote login.
Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler.HandleRequestAsync()
Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Cors.Infrastructure.CorsMiddleware.InvokeCore(HttpContext context)
Microsoft.AspNetCore.StaticFiles.StaticFileMiddleware.Invoke(HttpContext context)
Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

OpenIdConnect setup:

var dataProtector = DataProtection.CreateProtector(typeof(OpenIdConnectHandler).FullName, authMethod.Id.ToString(), "v1");

								var oidcOptions = new OpenIdConnectOptions
								{
									MetadataAddress = authMethod.IdPUrl,
									Authority = authMethod.Audience,
									ClientId = authMethod.ClientId,
									ClientSecret = authMethod.ClientSecret,
									ResponseType = "code id_token",
									CallbackPath = "/signin-oidc",
									BackchannelTimeout = TimeSpan.FromMinutes(5),
									Resource = authMethod.Type == AuthTypes.Azure ? authMethod.ClientId : Configuration.Issuer,
									
#if (DEBUG)
									Prompt = "login",
									RequireHttpsMetadata = false,
#else
									RequireHttpsMetadata = true,
#endif
									TokenValidationParameters = new TokenValidationParameters
									{
										ValidateIssuer = false,
										ValidAudience = authMethod.ClientId
									},
									DataProtectionProvider = DataProtection,
									StateDataFormat = new PropertiesDataFormat(dataProtector),
									StringDataFormat = new SecureDataFormat<string>(new StringSerializer(), dataProtector)
								};
								oidcOptions.Scope.Clear();
								oidcOptions.Scope.Add("openid");
								oidcOptions.Scope.Add("profile");

								oidcOptions.Backchannel = new HttpClient(oidcOptions.BackchannelHttpHandler ?? new HttpClientHandler());
								oidcOptions.Backchannel.DefaultRequestHeaders.UserAgent.ParseAdd("Microsoft ASP.NET Core OpenIdConnect handler");
								oidcOptions.Backchannel.Timeout = oidcOptions.BackchannelTimeout;
								oidcOptions.Backchannel.MaxResponseContentBufferSize = 1024 * 1024 * 10; // 10 MB

								oidcOptions.ConfigurationManager = new ConfigurationManager<OpenIdConnectConfiguration>(oidcOptions.MetadataAddress, new OpenIdConnectConfigurationRetriever(), new HttpDocumentRetriever(oidcOptions.Backchannel) { RequireHttps = oidcOptions.RequireHttpsMetadata });

Saml2 Setup: (Which gives Unsolicited logins are not allowed.)

								var wsOptions = new WsFederationOptions
								{
									BackchannelTimeout = TimeSpan.FromMinutes(5),
									MetadataAddress = authMethod.IdPUrl,
									Wtrealm = authMethod.Audience,
									RequireHttpsMetadata = false,
									SaveTokens = true,
									TokenValidationParameters = new TokenValidationParameters
									{
										ValidAudiences = new string[] { authMethod.Audience, $"spn:{authMethod.Audience}" },
										ValidateIssuerSigningKey = string.IsNullOrEmpty(authMethod.Certificate),
										IssuerSigningKey = string.IsNullOrEmpty(authMethod.Certificate) ? null : new X509SecurityKey(KeyVaultService.GetCertificateFromString(authMethod.Certificate))
									},
									DataProtectionProvider = DataProtection,
								};

								wsOptions.Backchannel = new HttpClient(wsOptions.BackchannelHttpHandler ?? new HttpClientHandler());
								wsOptions.Backchannel.DefaultRequestHeaders.UserAgent.ParseAdd("Microsoft ASP.NET Core WsFederation handler");
								wsOptions.Backchannel.Timeout = wsOptions.BackchannelTimeout;
								wsOptions.Backchannel.MaxResponseContentBufferSize = 1024 * 1024 * 10; // 10 MB

								wsOptions.SignOutScheme = wsOptions.SignInScheme;

								var dataProtector = wsOptions.DataProtectionProvider.CreateProtector(typeof(WsFederationHandler).FullName, authMethod.Id.ToString(), "v1");
								wsOptions.StateDataFormat = new PropertiesDataFormat(dataProtector);

								wsOptions.ConfigurationManager = new ConfigurationManager<WsFederationConfiguration>(wsOptions.MetadataAddress, new WsFederationConfigurationRetriever(), new HttpDocumentRetriever(wsOptions.Backchannel) { RequireHttps = wsOptions.RequireHttpsMetadata });
								result = WsFederationOptionsCache.TryAdd(authMethod.Id.ToString(), wsOptions);

Anyone have any suggestions on what I'm missing from my manual options? Seems like they're both related and once getting through this it should work.