Allow JWT Authorities to be resolved dynamically

atrauzzi commented 3 years ago

I'm currently able to configure CORS dynamically based on the hostname for the current request. Unfortunately similar functionality is missing for configuring the Authority of JwtBearerOptions.

Describe the solution you'd like

I would like JwtBearerOptions to offer something similar to ICorsPolicyProvider except where I can use the token and query the database (or any other services) for advice on what well-known endpoint to use.

Additional context

Please note, I cannot configure these up-front as they are not part of my environment and to update them would require an entire redeploy. Therefore, it's not quite practical to simply have lots of statically defined schemes.

There's some degree of inconsistency and inflexibility given that something arguably similar is available for CORS. Why would one be offered, but not the other?

Suffice it to say, there's no straightforward workaround here. We need some kind of option to dynamically provide these URIs.

blowdart commented 3 years ago

Is this for multi-tenancy, or is it aimed at one authority per instance, just from a database rather than a config? Does it change during runtime, or is it set at startup?

atrauzzi commented 3 years ago

Hey @blowdart :wave:

This would be for multi-tenancy, similar in spirit to how we have to adapt CORS per-tenant.

The URI for the authority would be determined by searching the DB for configured domains for the current host header.
- This means that it might be nice to inject a DbContext as well as potentially other services like a cache abstraction. So something like an IJwtBearerPolicyProvider that I could implement and have control over the constructor.
Authorities would vary during runtime and configuring a new tenant should only require me to modify the data in my database. I shouldn't have to restart or redeploy my application to effect new tenants.

blowdart commented 3 years ago

Multi-tenancy has been a much-asked for feature, which the team has pushed back on, pointing out that most people just want auth multi-tenancy and it would be better served by making auth multi-tenant aware. As the auth owner it's falling to me to look at where the effort should go. Can I ask what else in your app varies according to tenant? (for example, database connection strings, image urls, configuration files, favourite pair of pyjamas etc.), which will help me put together a survey to gather feedback on what we need to do for 6.0.

atrauzzi commented 3 years ago

For what it's worth, I think I understand your rationale and I think it's sound when it comes to "image urls" and "favourite pair of pyjamas". Tenant-scope is something that comes right after request-scope, which is a clean line for anything that happens after the route is matched or the last Microsoft middleware is involved. :wink:

The problem is that ASP.net doesn't have all the extensibility points for devs to hook everything they need - or at least if it does, the mechanisms aren't intuitive or documented well enough to seem like my first-best-choice:tm:

In our application, we vary:

The scheme and host portion of generated URIs from routing for the app itself
The scheme and host portion of generated URIs from routing to other applications
- ie: Deep URIs in our SPA
CORS
JWT Authorities
...I'll update this list if I recall more :crossed_fingers::sweat_smile:

Database connection strings isn't one we've encountered yet, I'd fight to the last moment to steer away from that, but wouldn't rule it out totally over time.

Here's the order I see it all happening:

If I wanted to make my own application-specific Tenant type available in request scope, perhaps I could write a middleware to add the instance to the HTTP request.
- (I follow the advice that you don't bind model instances to DI)
Then, I'd love to write my own services.AddMyAppTenanting() extension method to IServiceCollection which goes through and binds a handful of interfaces that ASP.net offers. Each one would look similar to ICorsPolicyProvider, providing the current request and be able to to have resources in request scope injected.
- This seems like a pattern that could be repeated throughout the whole framework!

It may be that item 1 just needs to be advertised better in docs and guides and that item 2 just requires more things to be configurable in such a fashion?

blowdart commented 3 years ago

Has anyone, Microsoft or otherwise pointed you at orchard which has been our "solution" for a while? If they have, did you look and find it unsuitable?

As and when I put the survey together I'll follow up with a link, in which you could "win" a follow up phonecall from me to chat about it all, if you desire grin

atrauzzi commented 3 years ago

Assuming it's this: https://www.orchardcore.net, this looks quite batteries-included to me.

Overall, I'd worry that there's too much to buy into here versus taking a more granular approach by leveraging sound extensibility in ASP.net.

Even if orchard itself claims to be modular, it's a long road to travel.
From a time-investment standpoint, past experience has taught me that you almost always hit a wall of compromises made to satisfy some feature set. Especially when "CMS" is one of the features involved.
I'm not a luddite when it comes to consuming packages, even outside of Microsoft (I love flurl!). But Maintaining third party dependencies brings with it baggage like keeping up to date with versions, API changes and transitive dependencies that I do not think make sense given how much control we want over the tenanting pipeline.

BTW absolutely, happy to provide more info in a survey.

blowdart commented 3 years ago

Wonderful. I'll put something together with our UX folks.

atrauzzi commented 3 years ago

@blowdart - Sounds great. If I might add one small note, maybe even for your survey -- I think the JwtBearerHandler portion of this needs to be something that gets introduced within the next month. Not a whole year from now for 6.x.

Even as a separate implementation in its own nuget package that is allowed to grow and then graduated into 6.x as the new default.

@terrajobst, if it's alright, I'd like to ping your attention here. I think this is a good minor example as you are studying the relationship between .net and the world of OSS in general. My concern here being that the next major release is an impractically long time to wait for important functionality that is blocking me today.

Locking ASP.net to the same release schedule as .net itself seems to be a regression to the GAC/Service Pack mentality.

I'll move this to twitter with a few more thoughts...

blowdart commented 3 years ago

It can't/won't be a month. We haven't even completed 5.0.

Features needs prioritization against other issues, as well as looking at the time the AAD team has to work on things.

atrauzzi commented 3 years ago

Understood, and for what it's worth, I'm mostly aware of this limitation.

I'm just cross-referencing as I think this it's something that both .net and ASP.net need to address in a bigger picture way.

Although yes, for all intents & purposes, consider me eager to try anything to get through this today as right now I'm kind of dead in the water. Open to suggestions.

blowdart commented 3 years ago

@Tratcher might be aware of some ugly work arounds. He's good like that.

atrauzzi commented 3 years ago

:pray: much appreciated.

sebastienros commented 3 years ago

@atrauzzi

Here is a blog post on how to host a auth server based on OpenIddict with Orchard Core (no CMS), written by @kevinchalet https://kevinchalet.com/2020/10/03/using-the-orchardcore-openid-management-feature-with-an-existing-openiddict-deployment/

Here is a repository with examples of modular or multi-tenant apps using Orchard Core (no CMS) https://github.com/OrchardCMS/OrchardCore.Samples

atrauzzi commented 3 years ago

@sebastienros - I don't plan on using orchard and my openid connect server is a 3rd party solution (FusionAuth).

Is there something specific in these examples that could be useful to me?

Ideally I would like some small boilerplate code that I can keep in my project until such a time that ASP.net has some official option for me to transition to.

atrauzzi commented 3 years ago

So if I was dreaming, there would be some simple way to kick back into the default flow:

namespace MyProject
{
    public class TenantedJwtBearerHandler : JwtBearerHandler
    {
        public TenantedJwtBearerHandler(IOptionsMonitor<JwtBearerOptions> defaultOptions, ILoggerFactory logger, UrlEncoder encoder, ISystemClock clock) 
        : base(defaultOptions, logger, encoder, clock)
        {
        }

        protected override async Task<AuthenticateResult> HandleAuthenticateAsync()
        {
            var host = (string) Context.Request.Headers["host"];

            if (string.IsNullOrEmpty(host))
            {
                throw new Exception("Unable to infer current host from request.");
            }

            var dbContext = Context.RequestServices.GetService<DbContext>();

            if (dbContext == null)
            {
                throw new Exception("Unable to resolve database context.");
            }

            var domain = await dbContext.Domains.ForHost(host).FirstOrDefaultAsync();

            if (domain == null)
            {
                throw new Exception("Unknown tenant.");
            }

            var authority = new Url($"{Context.Request.Scheme}://auth.{domain.Name}");

            var options = new JwtBearerOptions
            {
                RequireHttpsMetadata = false,
                Authority = authority.ToString(),
                SaveToken = true,
                TokenValidationParameters = new TokenValidationParameters
                {
                    ValidateIssuer = false,
                    ValidateAudience = false,
                    RequireAudience = false,
                    RequireSignedTokens = true,
                    RequireExpirationTime = true,
                }
            };

            // todo: This is where I'd love to be able to just throw things back out to the default MS implementation if at all possible.

        }
    }
}

Tratcher commented 3 years ago

The problem is that ASP.net doesn't have all the extensibility points for devs to hook everything they need

It's worse than that. The auth system was not designed for multi-tenancy. The current design assumes you know all of your providers up front and that they don't change (much). It's not a matter of adding extensibility points until it's fully multi-tenant capable, we need a fundamentally new design and new components. That's why we recommend Orchard, it's tackled the multi-tenancy issues at an application scale.

For now you might be able to work around this by overriding JwtBearerHandler.InitializeAsync which is called per request and you can use to create unique options instances. You'd either need to deep copy what OptionsMonitor returns to you before modifying it, or directly highjack the IOptionsMonitor<JwtBearerOptions> service. https://github.com/dotnet/aspnetcore/blob/2dec94a0a6e60e86fc500bd84ad617bba68b9b66/src/Security/Authentication/Core/src/AuthenticationHandler.cs#L140

kevinchalet commented 3 years ago

@atrauzzi you may want to take a look at https://stackoverflow.com/questions/52955238/how-can-i-set-the-authority-on-openidconnect-middleware-options-dynamically/52977687#52977687. It's for the OpenID Connect middleware, but it should work exactly the same way with the JWT bearer handler.

atrauzzi commented 3 years ago

@Tratcher - It looks like I can't override it as it's not declared virtual?

Something I think that would make situations like this a lot easier to work with is if ASP.net used virtual more often. This would help ensure I'm not forced to replace the entire object graph just to change one method. Instead I could extend and override for the one change I need.

(This is not the first time I've encountered this situation.)

Are there any other possible workarounds? I really want to be able to get this working before late 2021 and without taking on orchard -- which is unfortunately not a viable solution for me (and probably most people trying to accomplish this).

@kevinchalet - That looks similar to above where I'm basically replacing every class in the hierarchy, effectively creating a fork. Surely there has to be a less heavy-handed way of dealing with this?

Tratcher commented 3 years ago

Ah, you're right.

@HaoK can you show how to highjack IOptionsMonitor<JwtBearerOptions> to let them return a new instance per request based on the request details (via IHttpContextAccessor?).

kevinchalet commented 3 years ago

@kevinchalet - That looks similar to above where I'm basically replacing every class in the hierarchy, effectively creating a fork. Surely there has to be a less heavy-handed way of dealing with this?

I don't think that replacing a single service - IOptionsMonitor<OpenIdConnectOptions> - can be seen as "replacing every class in the hierarchy", specially since I didn't touch any of the OIDC components in my example.

HaoK commented 3 years ago

I think pinpoint's example (sorry old habits @kevinchalet ) is illustrative, you implement IOptionsMonitor<OpenIdConnectOptions> and take it over to return the multitenant aware instance using the current request. That SO answer is exactly how the extensibility was designed to be used.

atrauzzi commented 3 years ago

Alright, I'll dig into that example more, thank you everyone. Happy to come back to this discussion in several months when changes are being done for authentication handlers. :heart:

atrauzzi commented 3 years ago

I've been working through getting things going with pretty good success. I have tenant-informed Authorities now. :hugs:

I was looking at request performance and I'm noticing that even the simplest are taking ~100ms. I wonder... Is there any way to configure a cache for the JWKS lookups that JwtBearer does? Even if that cache was a few minutes or even seconds, it would go a long way to offset every request having to hit for JWKS?

cc. @Tratcher @HaoK

Tratcher commented 3 years ago

The ones downloaded as part of the metadata using the ConfigurationManager? The ConfigurationManager has built in caching, but if you're re-creating the options then you might be re-creating the ConfigurationManager too. Start by caching the ConfigurationManager. https://github.com/dotnet/aspnetcore/blob/a34dc1a999ac4e4adb40a7c51ac20a8e0d7ed797/src/Security/Authentication/JwtBearer/src/JwtBearerPostConfigureOptions.cs#L29-L64

ghost commented 3 years ago

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

dotnet / aspnetcore

Allow JWT Authorities to be resolved dynamically #26866

Describe the solution you'd like

Additional context