Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move aspnetcore to leverage JsonWebToken and JsonWebTokenHandler #49469

Closed
jennyf19 opened this issue Jul 17, 2023 · 2 comments
Closed

Move aspnetcore to leverage JsonWebToken and JsonWebTokenHandler #49469

jennyf19 opened this issue Jul 17, 2023 · 2 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer NativeAOT
Milestone

Comments

@jennyf19
Copy link
Contributor

jennyf19 commented Jul 17, 2023

Background and Motivation

Improvements in JsonWebToken and JsonWebTokenHandler have been made in Microsoft.IdentityModel, which include a 30% performance improvement over JwtSecurityToken which is currently used in ASP.NET today. In later versions of Microsoft.IdentityModel 7.x (before .NET8 RC1), we will enable AOT support by having fully trimmable assemblies in Microsoft.IdentityModel and remove the dependency of Newtonsoft, enabling a smaller dll for AOT.

Microsoft.IdentityModel offers two generations of JSON web token (JWT) handling, which are in two assemblies:

  • System.IdentityModel.Tokens.Jwt is the old generation. Notable types are JwtSecurityToken and JwtSecurityTokenHandler. This is the assembly currently used by ASP.NET Core.

  • Microsoft.IdentityModel.Tokens.JsonWebToken is the next generation. It offers JsonWebToken and JsonWebTokenHandler with:

    • Improved performance (30%)

    • Better resilience: IdentityModel will fetch and maintain the OIDC metadata and uses its last known good state (repair item from March 2020 outage)

    • Defense in depth: IdentityModel provides additional AAD key issuer validation protection

    • Support for async token validation, returning a TokenValidationResult rather than throwing

Microsoft.IdentityModel also has two abstractions for Token Handlers:

  • ISecurityTokenValidator is the old generation.
  • TokenHandler (abstract class) is the next generation and offers asynchronous token validation.

The following assemblies have a dependency on JwtSecurityToken, or JwtSecurityTokenHandler:

  • Microsoft.AspNetCore.Authentication.JwtBearer,
  • Microsoft.AspNetCore.Authentication.OpenIdConnect,
  • Microsoft.AspNetCore.Authentication.WsFederation,
  • Microsoft.AspNetCore.Authentication

Proposed API

We introduce a new boolean, UseTokenHandlers, in the JwtBearer and WsFederation options to enable developers to decide whether the access token validation will be done with the new TokenHandlers (more performant, resilient and async) or with the legacy SecurityTokenValidators. We also expose the list of TokenHandlers used to validate the token. Developers can decide to add their own TokenHandlers (token type) for each protocol (JwtBearer/WsFed). By default JwtBearerOptions.TokenHandlers contains an instance of the JsonWebTokenHandler and WsFederationOptions.TokenHandlers contains handlers for SAML1, SAML2, and JsonWebTokenHandler.

PR for reference

Additions in JwtBearer:

namespace Microsoft.AspNetCore.Authentication.JwtBearer;

public class JwtBearerOptions: AuthenticationSchemeOptions
{
+   public IList<TokenHandler> TokenHandlers { get; }
+   public bool UseTokenHandlers { get; set; } = true;
}

Additions in WsFederation:

namespace Microsoft.AspNetCore.Authentication.WsFederation;

public class WsFederationOptions: RemoteAuthenticationOptions
{
+   public IList<TokenHandler> TokenHandlers { get; }
+   public bool UseTokenHandlers { get; set; } = true;
}

Additions in OpenIdConnect:
We introduce a new boolean, UseTokenHandler, in the OpenIdConnect options to enable developers to decide whether the ID token validation will be done with the new TokenHandler (more performant, resilient and async) or with the legacy SecurityTokenValidator.

PR for reference

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectOptions: RemoteAuthenticationOptions
{
+   public TokenHandler TokenHandler { get; }
+   public bool UseTokenHandler { get; set; } = true;
+   public bool MapInboundClaimsTokenHandler {get; set;}
}

Usage Examples

By default, ASP.NET Core in .NET 8 uses the new TokenHandler. If a developer wants to use the legacy validators, they can set UseTokenHandlers = false.

services.Configure<JwtBearerOptions>(JwtBearerDefault.AuthenticationScheme, options => { options.UseTokenHandlers = false; });

If a developer wants to have their own TokenHandler, they can add it to the list of TokenHandlers:

services.Configure<JwtBearerOptions>(JwtBearerDefault.AuthenticationScheme, options => { options.TokenHandlers.Add( new MyTokenHandler()); });

Alternative Designs

Alternative designs were discussed with @Tratcher, @eerhardt, @halter73 . We went with Option A_1, but the alternatives discussed were:

Option A_1:
Have the same assemblies as today with a dual dependency on System.IdentityModel.Tokens.Jwt and Microsoft.IdentityModel.Tokens.JsonWebToken, and offer both interfaces (Jwt for compatibility, whereas the processing is done with JsonWebToken). In practice, note that the Jwt Wilson assembly already depends on the JsonWebToken Wilson assembly, so there would not be any additional dependencies than today.
Additionally, to leverage the new generation of Wilson assemblies without breaking changes, both ISecurityTokenValidator and TokenHandler members would have to be in ASP.NET core’s surface area.
Validation would need to be done on the options such that when using a new generation of Jwt classes, the new generation of the TokenHandlers would also need be used.

Option A_2:
Duplicate the current ASP.NET Core assemblies, and have a new generation (let’s name them Microsoft.AspNetCore.Authentication.JwtBearer2. Microsoft.AspNetCore.Authentication.OpenIdConnect2, Microsoft.AspNetCore.Authentication.WsFederation2, Microsoft.AspNetCore.Authentication2 for now, until we have a better name), and have these new generation depend only on JsonWebToken, and only expose these concepts. Letting the old generation leverage only Jwt. Additionally, these new assemblies would only rely on TokenHandler and drop support for ISecurityTokenValidator
In practice, we could, for this Option A2, use the same codebase, but add the files of the old projects as links in the new project, with conditional projects)

Option A_3:
Breaking changes in ASP.NET to rely only on the Microsoft.IdentityModel.Tokens.JsonWebToken assembly for Jwts and only TokenHandler for token handler abstractions.
In practice, these breaking changes should only affect users that leverage the extensibility features. We need to understand how large of a population this would affect.

Also gathering customer feedback in the GitHub discussion in Microsoft.IdentityModel repo and the two above mentioned PRs.

Risks

When setting UseTokenHandlers or UseTokenHandler to true, the SecurityToken passed in the context of the TokenValidated event needs to be downcast to JsonWebToken instead of JwtSecurityToken for users who were already doing this, which is not a common scenario, but for more advanced users. Mitigation for the risk is to have an implicit operator.

Initial feedback on 7.0.0-preview of Microsoft.IdentityModel from @kevinchalet: "FYI, I tested the 7.0.0-preview packages with OpenIddict and haven't seen any particular regression. Good job"

@jennyf19 jennyf19 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 17, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jul 17, 2023
@ghost ghost added the NativeAOT label Jul 17, 2023
@jennyf19
Copy link
Contributor Author

  • Microsoft.IdentityModel.Tokens.Jwt is the old generation. Notable types are JwtSecurityToken and JwtSecurityTokenHandler. This is the assembly currently used by ASP.NET Core.

System.IdentityModel.Tokens.Jwt you mean?

Yes! Great catch! Have fixed, thanks for pointing that out @cakescience

@halter73
Copy link
Member

halter73 commented Jul 18, 2023

API Review Notes:

  • There are no changes to Microsoft.AspNetCore.Authentication
  • SecurityTokenValidators is the name of the existing IList<ISecurityTokenValidator>. This will now be disabled by default.
    • Can we make the disabled IList throw if it is modified for clearer errors?
      • Is it possible that old libraries use it? Wouldn't you want to know? But would it be hard to workaround.
      • We should make it throw.
    • Should we obsolete the old API?
      • It does still work, so we should obsolete it with a warning, not an error.
  • There is no API change to JwtBearerEvents or other events, but some people do downcast TokenValidatedContext.SecurityToken to JwtSecurityToken while it will now be a JsonWebToken which does not implement, e.g. https://github.com/AzureAD/microsoft-identity-web/blob/07f896d427f52552c0fa5b18a463f1dc0c9dadce/src/Microsoft.Identity.Web/WebApiExtensions/MicrosoftIdentityWebApiAuthenticationBuilder.cs#L102. It will now be
    • We would like it if there was an explicit conversion from JwtSecurityToken to JsonWebToken.
  • What do we think about making UseTokenHandlers default to true?
    • Opting out should be possible to opt out by setting UseTokenHandlers via named options even if you didn't add the auth hander directly.
    • Defaulting to true potentially makes trimming easier, but if someone sets it to true manually, neither code path will be trimmed.
  • Do we want a method that only allows opting out?
    • This would have a small benefit if someone manually resets UseTokenHandlers to true, but we don't think it's big because Newtonsoft will be removed from JwtSecurityToken logic.
    • Properties can be set from config which is nice, but anyone doing that won't get trimming.
    • @captainsafia assures that we don't bind to all of JwtBearerOptions automatically and it's a hand-rolled artisanal binding.
  • Can we make the property false by default by calling it something else?
    • UseSecurityTokenValidators? Yes.
  • Are we worried we will have to change the default back to the old API because it's too breaking?
    • We're not too worried, and we could change the default if necessary
  • Do we need MapInboundClaimsTokenHandler? Can we continue to use MapInboundClaims and have it map to both? It's intended to do the same thing.
    • It could make trimming trickier, but hopefully not impossible.

API Approved!

namespace Microsoft.AspNetCore.Authentication.JwtBearer;

public class JwtBearerOptions : AuthenticationSchemeOptions
{
+   [Obsolete("SecurityTokenValidators is no longer used by default. Use TokenHandlers instead. To continue using SecurityTokenValidators, set UseSecurityTokenValidators to true.")]
    public IList<ISecurityTokenValidator> SecurityTokenValidators { get; }

+   public IList<TokenHandler> TokenHandlers { get; }
+   public bool UseSecurityTokenValidators { get; set; }
}

namespace Microsoft.AspNetCore.Authentication.WsFederation;

public class WsFederationOptions : RemoteAuthenticationOptions
{
+   [Obsolete("SecurityTokenValidators is no longer used by default. Use TokenHandlers instead. To continue using SecurityTokenValidators, set UseSecurityTokenValidators to true.")]
    public IList<ISecurityTokenValidator> SecurityTokenValidators { get; }

+   public IList<TokenHandler> TokenHandlers { get; }
+   public bool UseSecurityTokenValidators { get; set; }
}

namespace Microsoft.AspNetCore.Authentication.OpenIdConnect;

public class OpenIdConnectOptions : RemoteAuthenticationOptions
{
+   [Obsolete("SecurityTokenValidator is no longer used by default. Use TokenHandler instead. To continue using SecurityTokenValidators, set UseSecurityTokenValidator to true.")]
    public ISecurityTokenValidator SecurityTokenValidator { get; set; }

+   public TokenHandler TokenHandler { get; set; }
+   public bool UseSecurityTokenValidator { get; set; }
}

This is not for ASP.NET Core, but in System.IdentityModel.Token.Jwt we'd like to see an explicit conversion. Edit: This doesn't work because the declared type is just SecurityToken not JwtSecurityToken.

namespace System.IdentityModel.Tokens.Jwt;

public class JwtSecurityToken : SecurityToken
{
        // Edit: This was suggested as an addition earlier. I'm now putting a '-' instead of a '+' to make it extra clear not to add it.
-      public static explicit operator JsonWebToken(JwtSecurityToken token);
}

@halter73 halter73 added api-approved API was approved in API review, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jul 18, 2023
@mkArtakMSFT mkArtakMSFT added this to the 8.0-preview7 milestone Jul 19, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer NativeAOT
Projects
None yet
Development

No branches or pull requests

5 participants
@halter73 @eerhardt @jennyf19 @mkArtakMSFT and others