AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.05k stars 396 forks source link

Add Async pattern for TokenValidation #468

Open MicahZoltu opened 8 years ago

MicahZoltu commented 8 years ago

There are a number of places where JWTSecurityTokenHandler calls into user provided methods that may be doing some IO (e.g., database or remote calls) but they do so in a synchronous way.

One example of such a place is with JWT signature validation. The user can provide their own SignatureValidator or IssuerSigningKeyResolver. The act of validating a signature or resolving signing keys may require an interaction with an external server.

For a real-world example, Google rotates its signing keys regularly (rumor is daily) and you can get the latest public key in JWK format here. Unfortunately, it is not possible to know when they are going to roll their keys so an application that is attempting to validate Google issued JWTs will need to hit that endpoint to retrieve the latest signing key. The application could be optimized to cache the signing keys but that is still an async operation and it is entirely up to Google as to how often they rotate their keys (they could start rotating them for every request if they wanted).

Another real-world example once again with Google's authentication, Google supports sending the token to here for validation rather rather than doing the validation locally. If one wants to use this mechanism then every token validation would be a remote call.

Unfortunately, I recognize that the current API is very async un-friendly and the interfaces it implements are also not async-friendly but I wanted to get this filed anyway in hopes that at some point things can be improved to support modern asynchronous use cases.

brentschmaltz commented 8 years ago

@Zoltu I agree with you that off-box validation is a real scenario. We should have an async pattern up and down the call graph to realize this pattern. We stopped at metadata retrieval.

brentschmaltz commented 7 years ago

We will need to add an new return type: ValidationResult so the 'out' param is not needed.

brentschmaltz commented 7 years ago

Include this issue with this fix. https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/566

brentschmaltz commented 7 years ago

Modified title to reflect that this is general in nature across all token types. Perhaps in SecurityTokenHandler

shagenhokie commented 6 years ago

Do we have an ETA on this?

brentschmaltz commented 6 years ago

@shagenhokie not yet

bugproof commented 5 years ago

@brentschmaltz the method that handles jwt authentication is async so I think it shouldn't be that difficult to add? https://github.com/aspnet/Security/blob/beaa2b443d46ef8adaf5c2a89eb475e1893037c2/src/Microsoft.AspNetCore.Authentication.JwtBearer/JwtBearerHandler.cs#L107-L128

It's a limitation if you want to implement token invalidation and call redis or any other db asynchronously inside the handler: https://stackoverflow.com/questions/52476325/how-to-invalidate-tokens-after-password-change/52476727#52476727

Because ValidateToken token uses out parameter and async methods can't be used with out parameter that would be a breaking change, ISecurityTokenValidator would need a method which returns Task anyway. The best workaround to avoid breaking change I can think of is:

public async Task<(ClaimsPrincipal, SecurityToken)> ValidateTokenAsync(string token,
    TokenValidationParameters validationParameters)
{
    var claimsPrincipal = base.ValidateToken(token, validationParameters, out var validatedToken); 
    return (claimsPrincipal, validatedToken);
}

public override ClaimsPrincipal ValidateToken(string token, TokenValidationParameters validationParameters,
    out SecurityToken validatedToken)
{
    var tuple = ValidateTokenAsync(token, validationParameters).Result;
    validatedToken = tuple.Item2;
    return tuple.Item1;
}
brentschmaltz commented 5 years ago

@fragilethoughts I agree we need async, particularly with delegation to validation services, such as KeyVault and Google. Not only do we need ValidateTokenAsync, we need async on our Crypto such as SignatureProviders and KeyWrapProviders.

This is a feature we are thinking about for 5.3.1, but realistically it may be later. The other issue is we want to remove references to any libraries other than those shipped by Microsoft, such as JSON.net. With many JwtSecurityToken and JwtSecurityTokenHandler API's using JSON.net, we created a new assembly M.IM.JsonWebTokens (free from JSON.net) where ValidateToken returns a structure TokenValidationResult and can have easily have an async version. We are free to invent here.

bugproof commented 5 years ago

Sounds good

dhjf commented 5 years ago

Is the plan to create an async version of JwtSecurityTokenHandler, so it would be possible to call WriteToken asynchronously? It would help a lot, being able to go all async using IdentityServer4 for signing access tokens.

mcthuesen commented 5 years ago

Any status on this?

We have this issue as well where we need to do an http request to an HSM to sign access tokens. Our solution right now is to use WebClient.cs instead of the recommended way of doing http requests using HttpClient.cs. HttpClient does not support synchronous requests.

brentschmaltz commented 5 years ago

@mcthuesen We haven't moved forward much on this, but it is important for a number of reasons. We are not sure if this fits into our 5.x release or will need to wait for 6.0. Our 6.0 release fits with CORE 3.0.

mfeingol commented 5 years ago

Speaking on behalf of Azure DevOps, we'd love to have an async variant of the AudienceValidator delegate.

LeroyK commented 4 years ago

Any idea when this will be available?

brentschmaltz commented 4 years ago

@LeroyK at the earliest in the 1st qtr of 2020 for a full stack. However if Async Delegates were available would that satisfy a pent up need?

That may be simpler and we could stage the deliverable. Most likely, we would put the additions into JsonWebTokenHandler.

@mfeingol @mcthuesen @bugproof @shagenhokie @dhjf @MicahZoltu

Looking for feedback.

daniel-munch-cko commented 4 years ago

Just came across this, was there any follow-up? I'm mostly interested in async versions of SignatureProvider.Sign and SignatureProvider.Verify

brentschmaltz commented 4 years ago

@daniel-munch-cko we are working on shipping 6.x where we have a number of internal fixes being validated. Once that is out, we will address this as a lot of interest has been expressed. Particularly where crypto operations are performed out-of-process.

stebet commented 4 years ago

This would be extremely helpful for our case where we have an ecosystem of services that communicate with each others APIs using JWT tokens, so they need to fetch signing keys on demand as they are rotated.

cyberhck commented 4 years ago

@stebet that's exactly my usecase as well, it sucks having to do .Result on async method

stebet commented 4 years ago

@stebet that's exactly my usecase as well, it sucks having to do .Result on async method

Shhhh... you'll invoke @davidfowl....

brentschmaltz commented 4 years ago

@cyberhck @stebet @daniel-munch-cko yes we hear you. We have some internal work to do with our 6.x branch (our dev is ahead of dev5x). We do not want to backport this work, so the order of items is 6.x public, coordinating with asp.net, identityserver to ensure no breaking issues. Then we can address these long standing issues.

bugproof commented 4 years ago

Kinda funny to see it open since 2016.

brentschmaltz commented 4 years ago

@bugproof fair enough. When opened the win was seen as a programming convenience, in a large number of cases, as almost all crypto operations were inproc and synchronous. That has changed as it is now common for part of the inbound validation for on out-of-proc or web request to be invoked.

Mordahlhuilhulh commented 4 years ago

Hello @brentschmaltz , Just wants to be sure that there isn't any new update regarding the async versions of SignatureProvider.Sign and SignatureProvider.Verify?

brentschmaltz commented 4 years ago

@Mordahlhuilhulh not yet ...

Mordahlhuilhulh commented 4 years ago

@brentschmaltz Thanks for your quick replay. Do you know when the async version is planned?

cyberhck commented 3 years ago

@Mordahlhuilhulh it looks like it's planned for V6 looking at assigned milestone.

mchandschuh commented 3 years ago

This might help anyone looking to do async JWT validation.

I ended up adding a new interface that also implements ISecurityTokenValidator. This allows me to add my custom validator to the JwtBearerOptions.TokenValidators collection and in my version of JwtBearerHandler (see #29917), I check to see if I'm working w/ the synchronous or asynchronous abstraction and act accordingly. The nice part about this approach is that it's a non-breaking change. The not so nice part is the potential for type checks, which I suppose could be hidden away behind an extension method, but still somewhat tacky.

public interface IAsyncSecurityTokenValidator : ISecurityTokenValidator
{
    /// <summary>
    /// Validates a token passed as a string using <see cref="TokenValidationParameters"/>
    /// </summary>
    ValueTask<(ClaimsPrincipal principal, SecurityToken validatedToken)> ValidateTokenAsync(
        string securityToken,
        TokenValidationParameters validationParameters
        );
}

public class CustomAsyncSecurityTokenValidator : JwtSecurityTokenHandler, IAsyncSecurityTokenValidator
{
    // optionally override the synchronous method
    // NOTE: You shouldn't let this actually be called, but it's better that this is called and the real validation happens
    //       rather than running the default `JwtSecurityTokenHandler.ValidateToken` code. You'll need to add custom
    //       IAuthenticationHandler to call the async method appropriately - see `JwtBearerAsyncValidationHandler` in #29917
    public override ClaimsPrincipal ValidateToken(
        string token,
        TokenValidationParameters validationParameters,
        out SecurityToken validatedToken
        )
    {
        ClaimsPrincipal principal;
        (principal, validatedToken) = ValidateTokenAsync(token, validationParameters).AsTask().GetAwaiter().GetResult();
        return principal;
    }

    public ValueTask<(ClaimsPrincipal principal, SecurityToken validatedToken)> ValidateTokenAsync(
        string securityToken,
        TokenValidationParameters validationParameters
        )
    {
        // do async stuff here
    }
}
stebet commented 3 years ago

This might help anyone looking to do async JWT validation...

This is a workaround that does semi-work, but the sync-over-async implementation is risky as it might cause threadpool starvation on a high-traffic implementation.

We really need a proper async implementation here, but thanks for this workaround :)

mchandschuh commented 3 years ago

This might help anyone looking to do async JWT validation...

This is a workaround that does semi-work, but the sync-over-async implementation is risky as it might cause threadpool starvation on a high-traffic implementation.

We really need a proper async implementation here, but thanks for this workaround :)

If you look at the linked issue it also includes a custom authentication handler implementation that does a type check to see if the token validator is an IAsyncSecurityTokenValidator, so it's async all the way down. There shouldn't be any issues w/ this unless I'm misunderstanding something.

EDIT: Turns out I put the wrong code in the linked issue. I've updated it to properly invoke the IAsyncSecurityTokenHandler:

if (validator is IAsyncSecurityTokenValidator asyncValidator)
{
    (principal, validatedToken) = await asyncValidator.ValidateTokenAsync(token, validationParameters).ConfigureAwait(false);
    if (principal == null || validatedToken == null)
    {
        throw new SecurityTokenValidationException("The provided token failed validation.");
    }
}
else
{
    principal = validator.ValidateToken(token, validationParameters, out validatedToken);
}
HarelM commented 3 years ago

I'm having an issue when I start the server and it stops responding to requests. The token validation I'm doing is against a third party server (openstreetmap in my case). I ended up here as I'm concerned what will happen if a lot of users will try to use the server at once. What I'm doing can be seen here: https://github.com/IsraelHikingMap/Site/blob/751a30b9c88e3e6dfb52d11605663c45dcc1deac/IsraelHiking.Web/OsmAccessTokenValidator.cs#L56 Which now feels like a great way to get the server stuck... but I'm not sure I know how to properly solve this due to this limitation...

goldsam commented 3 years ago

I don't see it mentioned above, but you can currently provide async token validation via the Events property of JwtBearerOptions. This might provide a stop-gap solution for some users until a proper async ISecurityTokenValidator API is implemented:

public void ConfigureServices(IServiceCollection services)
{
    services.AddAuthentication()
        .AddJwtBearer(jwtBearerOptions =>
        {
            jwtBearerOptions.Events.OnMessageReceived = async (context) =>
            {
                // do some async validation stuff...

                // on happy path:
                context.Principal = ...
                context.Properties = ...;
                context.Success();

                // or sad path:
                context.Fail(...);
            };
        });
}
HarelM commented 3 years ago

@goldsam Thanks for the info! This seems to be an interesting solution. This means I don't need to derive from ISecurityTokenValidator too, right? Simply supply the relevant code in this section...? Is there an easy way to avoid sending the request to the third party validator when the same user sends two requests at almost the same time besides locking? which is not advised anymore as far as I understand...? (not talking about caching the response in order to avoid future responses, I mean almost at the same time when the cache does not contain this information - you can see the ugly code I wrote above in the link which just feels wrong now...)

HarelM commented 3 years ago

Alongside the OnMessageRecieved stuff does the following article and LockProvider referenced there would solve the deadlock possibility here: https://www.ryadel.com/en/asp-net-core-lock-threads-async-custom-ids-lockprovider/ https://github.com/Darkseal/LockProvider/blob/master/LockProvider.cs For me it seems like the right direction, but I'm not sure If I'm not missing anything related to deadlocks, threads, locks etc...

goldsam commented 2 years ago

@HarelM I think locking is a bit out of the scope of this issue, but perhaps you might consider a solution that uses Lazy together with ConcurrentDictionary or a cache of some sort. LazyCache might be exactly the solution you are looking for that uses this approach.

HarelM commented 2 years ago

@goldsam Thanks for the info! I've implemented the fix according to the article above, it solved the hang. This lazy cache you referenced is interesting, it might allow me to remove some of the code I added and simplify it.

rmandvikar commented 2 years ago

Any eta on this? Without this, same workaround where I'm having to do a blocking call when the cache ttls out or is empty.

HarelM commented 2 years ago

@goldsam Thanks for this great suggestion! I have incorporated the lazy cache package in my code and it looks a lot cleaner right now. Thanks!

bugproof commented 2 years ago

One of the reasons I stopped using JWT tokens altogether 😄 5+ years for a small change. We might see this change in the next 5 years.

brentschmaltz commented 2 years ago

@bugproof it's not a small change and we are close to starting this as it fit well with this PR, which reaches out for metadata in the call graph.

Consider changes related to this PR https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/1779/files

ricavir11 commented 2 years ago

Hi @brentschmaltz, do you have an ETA for this ? Our Azure App Service application is struggling because of this :/

bugproof commented 2 years ago

@ricavir11 probably in 5 years from now. Don't see a reason why they couldn't just breaking change it. That wouldn't hurt anyone. Just add an announcement that it's a breaking change like they usually do with many other things.

I completely stopped using anything "Identity" that's coming with ASP.NET Core (along with IdentityUser, IdentityRole and default auth middlewares). I've learned it's a bloated and hard to configure and customize mess. Rolled my own user + auth. Completely dropped usage of JWT tokens as well. Use it only if you care about 3rd party login openid/oauth stuff. Auth solution that comes with ASP.NET Core is just really bad IMHO. JWTs shouldn't be used for user sessions as well - which is a mistake I made early on due to how much hype there was around it (you can't have stateless auth with a stateful backend)

ricavir11 commented 2 years ago

@bugproof, I hope this will not be so long 🤦‍♂️ ! I'm using it along with aspnetzero framework and it is working well for low traffic apps. As soon as you get more hits to the server, things become bad ! I'm having huge waiting timeouts just to validate tokens .... like 20s or even more. Adding more CPU or RAM does not change the problem. I really don't understand why this issue has not been put in the top priority of Microsoft team.

brentschmaltz commented 2 years ago

@bugproof there are a couple of items ahead of this. I believe we will get this in 2022

benmccallum commented 2 years ago

Got an email from Auth0 today that some of our signing keys are old and it's best practice that they are rotated out. (You rotate in a "next" to become "current" and then will effectively have two keys that are active until you can fully revoke the old one once you're happy all the JWTs signed with it are expired).

So we need a way our apps can fall through at runtime failure (with current known keys) and go grab the latest from Auth0 asyncly, then try "one more time". Source.

It'd be great if this was a scenario that was easily supported + documented. i.e. it'll be great to have the token grabbing support async, that'll unblock some scenarios; but a further feature to setup a recall of IssuerSigningKeyResolver and a retry of validation once would be incredible!

Right now I think the only choice we have is to rotate the old one, then immediately force our apps to grab the latest keys again somehow. Is there anything that can be done in an event (that's async) now, that would support grabbing the new keys in a way that'd loop back for another attempt at validation?

brentschmaltz commented 2 years ago

@benmccallum @ricavir11 we added the first part of our Async effort in this PR: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/1810

@benmccallum This PR may interest you: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/1797

Enabling the feature requires TokenValidationParameters.BaseConfigurationManager to be set.

When enabled the following logic will apply:

If JWT has a 'kid' and if the JWT is not {expired, some other cases, ...}, reach out and get new metadata and try again. If the new metadata still results in a failure, the last metadata that resulted in a successful validation was 'remembered' as LastKnownGood and will be re-used for a default of 1 hour. This protects against the publication of bad metadata.

The feature can be disabled and will only work if TokenValidationParameters.BaseConfigurationManager is set, so existing users will not see any effects.

We are working with the asp.net team to get this into asp.net 7.0

rmandvikar commented 2 years ago

@brentschmaltz That's a good start, and thanks for the update. What's the plan/eta to make JwtSecurityTokenHandler.ValidateTokenAsync() method fully async? Currently, it's just a wrapper around the sync ValidateToken() method.

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/adc8d59d4aaae502f77fd8f758e4f020045f69eb/src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs#L1654-L1675

brentschmaltz commented 2 years ago

@rmandvikar i know it has been on the list for a while and we finally inched towards this gap.

We have a couple of items before we can think about getting this into a release. This is mainly about having Asp.Net 7.0 using 6.x as the default and use JsonWebTokenHandler / JsonWebToken

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1726 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1654 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1426 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1197 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1160

Using System.Text.Json, that does away with all our splitting and translating from string -> bytes -> string as in: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1805

mendhak commented 2 years ago

Right now I think the only choice we have is to rotate the old one, then immediately force our apps to grab the latest keys again somehow. Is there anything that can be done in an event (that's async) now, that would support grabbing the new keys in a way that'd loop back for another attempt at validation?

@benmccallum would that mean simply redeploying the applications, so the caches are cleared?

benmccallum commented 2 years ago

@mendhak , yea, or via some admin screen that triggers it, but it's not exactly a clean solution.