Update JwtBearer and WsFederation to use updated IdentityModel #48966

brentschmaltz · 2023-06-22T15:41:01Z

Summary of the changes (Less than 80 chars)

Description

IdentityModel has updated the token validation to be more performant, provide an async model and provide a Last-Known-Good model for preserving metadata that was last known to validate a token for a specific identity provider

{Detail}
The user must opt-out in by setting a boolean, UseTokenHandlers, in JwtBearerOptions or WsFederationOptions.
If not disabled, then a JsonWebToken will be the SecurityToken that is available for further processing by the application instead of a JwtSecurityToken.

halter73

The user must opt in by setting a boolean, UseTokenHandlers

How breaking would it be to default to using the new TokenHandlers? We could in theory make it implicitly opt-in only if you touch the legacy SecurityTokenValidators properties. This could help us avoid adding a new UseTokenHandlers bool to the options.

If we changed the default to rely on JsonWebTokenHandler instead of JwtSecurityTokenHandler and force the app to call a method to use the JwtSecurityTokenHandler, it should be easier to eventually trim away System.IdentityModel.

You mentioned that the event handlers might as cast to JwtSecurityToken in the OnTokenValidated/OnSecurityTokenValidated events. Is that the only other code that could break? Could JwtSecurityToken define an explicit conversion to JsonWebToken to mitigate this?

What exactly do the event handlers that would be broken look like? How would we update code using to use the new JsonWebToken? Is there a migration doc?

I think it might also make sense to parameterize all or most of the JwtBearerTests and WsFederationTest to make sure they work with and without UseTokenHandlers set.

halter73 · 2023-06-22T17:39:41Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs

@@ -15,13 +16,17 @@ namespace Microsoft.AspNetCore.Authentication.JwtBearer;
 public class JwtBearerOptions : AuthenticationSchemeOptions
 {
    private readonly JwtSecurityTokenHandler _defaultHandler = new JwtSecurityTokenHandler();
+    private readonly JsonWebTokenHandler _defaultTokenHandler = new JsonWebTokenHandler();


Suggested change

private readonly JsonWebTokenHandler _defaultTokenHandler = new JsonWebTokenHandler();

private readonly JsonWebTokenHandler _defaultTokenHandler = new JsonWebTokenHandler

{

MapInboundClaims = JwtSecurityTokenHandler.DefaultMapInboundClaims,

};

Right? Otherwise it doesn't align with the default value of _mapInboundClaims. Can we add a test to JwtBearerTests verifying that we default to mapping claims when UseTokenHandlers is configured?

@halter73 thanks for pointing this out, i will have a look

@halter73 About this comment:

How breaking would it be to default to using the new TokenHandlers? We could in theory make it implicitly opt-in only if you touch the legacy SecurityTokenValidators properties. This could help us avoid adding a new UseTokenHandlers bool to the options.

For many users, they will never need to touch SecurityTokenValidators.

Addressed: 507d241

halter73 · 2023-06-22T18:00:10Z

@kevinchalet Do you have any thoughts on this PR?

kevinchalet · 2023-06-22T18:28:29Z

@kevinchalet Do you have any thoughts on this PR?

Using the new JWT stack is definitely an improvement (OpenIddict was one of the very first non-MSFT projects to adopt it and I have only good things to say about it 😄)

Regarding the approach used in this PR, it looks reasonable, as it avoids both a binary/source and behavior change - since the new model is opt-in. That said, the new stack is such an improvement that it should probably be the default option, even if it means it's potentially breaking.

Note: to avoid introducing new properties, you could also update the new IM token handlers to implement ISecurityTokenValidator and use as TokenHandler casts in the authentication handlers to call the async methods.

brentschmaltz · 2023-06-22T19:01:16Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs


    /// <summary>
    /// Initializes a new instance of <see cref="JwtBearerOptions"/>.
    /// </summary>
    public JwtBearerOptions()
    {
        SecurityTokenValidators = new List<ISecurityTokenValidator> { _defaultHandler };
+        // TODO - communicate to IdentityModel team to see if ITokenValidator interface can be created.
+        TokenHandlers = new List<TokenHandler> { _defaultTokenHandler };


@kevinchalet i am thinking that creating the interface ITokenValidator that has a couple of methods instead of the abstract class TokenHandler, this would make it easier for users who currently have an implementation of ISecurityTokenValidator.

Thoughts?

Thoughts?

IM already has TokenHandler, SecurityTokenHandler and ISecurityTokenValidator, which makes using these abstractions already quite painful. I'm not sure adding a 4th one will help 😄

WIF only had SecurityTokenHandler and it was much clearer: unifying everything in the next Wilson major version would be more than welcome 😄

@kevinchalet OK let's stick with TokenHandler then as this was meant to be the replacement for SecurityTokenHandler.

@kevinchalet we could implement ISecurityTokenValidator on JsonWebTokenHandler, but just because we can cast it to TokenHandler doesn't mean that users will not have a runtime break as the SecurityToken will change from JwtSecurityToken to JsonWebToken.

Sure, but that would mean an instance of JsonWebTokenHandler was added to SecurityTokenValidators, which, if you keep making using the new JWT stack opt-in, requires a deliberate action from the developer. And in this case, getting a JsonWebToken instead of a JwtSecurityToken is a logical result.

I'm hoping to make it opt-out.

Addressed: 507d241

@Tratcher default is true, use TokenHandlers.

kevinchalet · 2023-06-22T19:16:16Z

Note: before bumping Wilson, you should triple check that AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2059 is fixed for good as it caused a lot of friction when OpenIddict took a dependency on the latest IM version (to the point I had to re-release a new version with the IM references downgraded):

ilmax · 2023-06-23T07:11:01Z

src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs

+        }
+    }
+
+    private async Task<TokenValidationParameters> SetupTokenValidationParameters()


Suggested change

private async Task<TokenValidationParameters> SetupTokenValidationParameters()

private async Task<TokenValidationParameters> SetupTokenValidationParametersAsync()

Addressed: 507d241

eerhardt · 2023-06-26T19:39:39Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs


    /// <summary>
    /// Initializes a new instance of <see cref="JwtBearerOptions"/>.
    /// </summary>
    public JwtBearerOptions()
    {
        SecurityTokenValidators = new List<ISecurityTokenValidator> { _defaultHandler };
+        // TODO - communicate to IdentityModel team to see if ITokenValidator interface can be created.


Suggested change

// TODO - communicate to IdentityModel team to see if ITokenValidator interface can be created.

From the discussion, it sounds like this can be removed.

Addressed: 507d241

eerhardt · 2023-06-26T19:40:11Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs

@@ -126,15 +136,20 @@ public new JwtBearerEvents Events
    public bool IncludeErrorDetails { get; set; } = true;

    /// <summary>
-    /// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, which is used when determining
-    /// whether or not to map claim types that are extracted when validating a <see cref="JwtSecurityToken"/>.
+    /// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/>which is used when determining


Suggested change

/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/>which is used when determining

/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/> in TokenHandlers, which is used when determining

Addressed: 507d241

eerhardt · 2023-06-26T19:42:38Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs

+    /// The advantage of using the TokenHandlers is:
+    /// <para>There is an Async model.</para>
+    /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>
+    /// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>
+    /// </summary>


Suggested change

/// The advantage of using the TokenHandlers is:

/// <para>There is an Async model.</para>

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

/// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>

/// </summary>

/// </summary>

/// <remarks>

/// The advantage of using the TokenHandlers is:

/// <para>There is an Async model.</para>

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

/// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>

/// </remarks>

Addressed: 507d241

eerhardt · 2023-06-26T19:43:51Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs

+    /// Gets of sets the <see cref="UseTokenHandlers"/> property to control if <see cref="TokenHandlers"/> or <see cref="SecurityTokenValidators"/> will be used to validate the inbound token.
+    /// The advantage of using the TokenHandlers is:
+    /// <para>There is an Async model.</para>
+    /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>


Suggested change

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is faster when validating.</para>

I'm not sure putting specific numbers in our docs is appropriate. The number can change over time, and is specific to a machine configuration.

Addressed: 507d241

eerhardt · 2023-06-26T19:47:12Z

src/Security/Authentication/WsFederation/src/WsFederationOptions.cs

+        set
+        {
+            _tokenHandlers = value ?? throw new ArgumentNullException(nameof(TokenHandlers));
+        }
+    }


Suggested change

set

{

_tokenHandlers = value ?? throw new ArgumentNullException(nameof(TokenHandlers));

}

}

}

I know the existing SecurityTokenHandlers property has a setter, but it violates the .NET Design Guidelines:

https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/guidelines-for-collections#collection-properties-and-return-values

❌ DO NOT provide settable collection properties.

Users can replace the contents of the collection by clearing the collection first and then adding the new contents. If replacing the whole collection is a common scenario, consider providing the AddRange method on the collection.

Addressed: 507d241

eerhardt · 2023-06-26T19:48:15Z

src/Security/Authentication/WsFederation/src/WsFederationOptions.cs

@@ -181,4 +205,13 @@ public TokenValidationParameters TokenValidationParameters
    /// </summary>
    [EditorBrowsable(EditorBrowsableState.Never)]
    public new bool SaveTokens { get; set; }
+
+    /// <summary>
+    /// Gets of sets the <see cref="UseTokenHandlers"/> property to control if <see cref="TokenHandlers"/> or <see cref="SecurityTokenHandlers"/> will be used to validate the inbound token.


Suggested change

/// Gets of sets the <see cref="UseTokenHandlers"/> property to control if <see cref="TokenHandlers"/> or <see cref="SecurityTokenHandlers"/> will be used to validate the inbound token.

/// Gets or sets whether <see cref="TokenHandlers"/> or <see cref="SecurityTokenHandlers"/> will be used to validate the inbound token.

Addressed: 507d241

eerhardt · 2023-06-26T19:48:52Z

src/Security/Authentication/WsFederation/src/WsFederationOptions.cs

+    /// The advantage of using the TokenHandlers is:
+    /// <para>There is an Async model.</para>
+    /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>
+    /// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>
+    /// </summary>


Suggested change

/// The advantage of using the TokenHandlers is:

/// <para>There is an Async model.</para>

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

/// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>

/// </summary>

/// </summary>

/// <remarks>

/// The advantage of using the TokenHandlers is:

/// <para>There is an Async model.</para>

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

/// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>

/// </remarks>

These are more appropriate for remarks than summary.

Addressed: 507d241

eerhardt · 2023-06-26T19:59:00Z

src/Security/Authentication/WsFederation/src/WsFederationOptions.cs

+    /// Gets of sets the <see cref="UseTokenHandlers"/> property to control if <see cref="TokenHandlers"/> or <see cref="SecurityTokenHandlers"/> will be used to validate the inbound token.
+    /// The advantage of using the TokenHandlers is:
+    /// <para>There is an Async model.</para>
+    /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>


Suggested change

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is faster when validating.</para>

Addressed: 507d241

eerhardt · 2023-06-26T20:00:32Z

src/Security/Authentication/WsFederation/src/WsFederationOptions.cs

+    /// Gets of sets the <see cref="UseTokenHandlers"/> property to control if <see cref="TokenHandlers"/> or <see cref="SecurityTokenHandlers"/> will be used to validate the inbound token.
+    /// The advantage of using the TokenHandlers is:
+    /// <para>There is an Async model.</para>
+    /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>


"The default token handler is" is kind of incorrect, since there are 3 default TokenHandlers. Maybe just a slight re-wording would help:

Suggested change

/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

/// <para>The default JWT token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

Addressed: 507d241

eerhardt · 2023-06-26T20:02:09Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs

+    /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>
+    /// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>
+    /// </summary>
+    public bool UseTokenHandlers { get; set; }


Suggested change

public bool UseTokenHandlers { get; set; }

public bool UseTokenHandlers { get; set; } = true;

+1 for making this the default.

Addressed: 507d241

This appears to have been reverted. Was that intentional?

@eerhardt yes, upon reflection, this would be breaking for some users.
@Tratcher would like to see this 'true' by default, but we have time to change this decision if we choose.

@brentschmaltz was the default set to true?

ghost · 2023-07-07T15:01:17Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no breaking changes are introduced, please leave an /azp run comment here to rerun the CI pipeline and confirm success before merging the change.

Co-authored-by: Brent Schmaltz <[email protected]>

remove using System

brentschmaltz · 2023-07-11T17:49:24Z

/azp run

azure-pipelines · 2023-07-11T17:49:30Z

No commit pushedDate could be found for PR 48966 in repo dotnet/aspnetcore

brentschmaltz · 2023-07-11T18:48:04Z

/azp run

azure-pipelines · 2023-07-11T18:48:20Z

Azure Pipelines successfully started running 3 pipeline(s).

eerhardt · 2023-07-11T21:23:17Z

I see SignalR tests failing with:

2023-07-11T19:23:28.0125349Z System.ArgumentOutOfRangeException: IDX10720: Unable to create KeyedHashAlgorithm for algorithm 'HS256', the key size must be greater than: '256' bits, key has '128' bits. (Parameter 'keyBytes')
2023-07-11T19:23:28.0125375Z    at Microsoft.IdentityModel.Tokens.CryptoProviderFactory.ValidateKeySize(Byte[] keyBytes, String algorithm, Int32 expectedNumberOfBytes)
2023-07-11T19:23:28.0125400Z    at Microsoft.IdentityModel.Tokens.CryptoProviderFactory.CreateKeyedHashAlgorithm(Byte[] keyBytes, String algorithm)
2023-07-11T19:23:28.0125427Z    at Microsoft.IdentityModel.Tokens.SymmetricSignatureProvider.CreateKeyedHashAlgorithm()
2023-07-11T19:23:28.0125457Z    at Microsoft.IdentityModel.Tokens.DisposableObjectPool`1.CreateInstance()
2023-07-11T19:23:28.0126961Z    at Microsoft.IdentityModel.Tokens.DisposableObjectPool`1.Allocate()
2023-07-11T19:23:28.0127018Z    at Microsoft.IdentityModel.Tokens.SymmetricSignatureProvider.GetKeyedHashAlgorithm(Byte[] keyBytes, String algorithm)
2023-07-11T19:23:28.0127232Z    at Microsoft.IdentityModel.Tokens.SymmetricSignatureProvider.Sign(Byte[] input)
2023-07-11T19:23:28.0127260Z    at Microsoft.IdentityModel.JsonWebTokens.JwtTokenUtilities.CreateEncodedSignature(String input, SigningCredentials signingCredentials)
2023-07-11T19:23:28.0127288Z    at System.IdentityModel.Tokens.Jwt.JwtSecurityTokenHandler.WriteToken(SecurityToken token)
2023-07-11T19:23:28.0127316Z    at Microsoft.AspNetCore.SignalR.Tests.Startup.GenerateToken(HttpContext httpContext) in /_/src/SignalR/server/SignalR/test/Startup.cs:line 102
2023-07-11T19:23:28.0127343Z    at Microsoft.AspNetCore.SignalR.Tests.Startup.<Configure>b__3_1(HttpContext context) in /_/src/SignalR/server/SignalR/test/Startup.cs:line 92
2023-07-11T19:23:28.0127373Z    at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext) in /_/src/Http/Routing/src/EndpointMiddleware.cs:line 64
2023-07-11T19:23:28.0127512Z    at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context) in /_/src/Security/Authorization/Policy/src/AuthorizationMiddleware.cs:line 138
2023-07-11T19:23:28.0127544Z    at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context) in /_/src/Security/Authentication/Core/src/AuthenticationMiddleware.cs:line 75
2023-07-11T19:23:28.0127721Z    at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application) in /_/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs:line 676
2023-07-11T19:23:28.0127749Z ===================
2023-07-11T19:23:28.0127779Z   Stack Trace:
2023-07-11T19:23:28.0127809Z      at Microsoft.AspNetCore.SignalR.Tests.VerifyNoErrorsScope.Dispose() in D:\a\_work\1\s\src\Shared\SignalR\VerifyNoErrorScope.cs:line 47
2023-07-11T19:23:28.0127838Z    at Microsoft.AspNetCore.SignalR.Tests.InProcessTestServer`1.DisposeAsync() in D:\a\_work\1\s\src\Shared\SignalR\InProcessTestServer.cs:line 157
2023-07-11T19:23:28.0127865Z    at Microsoft.AspNetCore.SignalR.Tests.InProcessTestServer`1.DisposeAsync() in D:\a\_work\1\s\src\Shared\SignalR\InProcessTestServer.cs:line 164
2023-07-11T19:23:28.0129174Z    at Microsoft.AspNetCore.SignalR.Tests.EndToEndTests.AuthorizedHubConnectionCanConnect() in /_/src/SignalR/server/SignalR/test/EndToEndTests.cs:line 682
2023-07-11T19:23:28.0129224Z --- End of stack trace from previous location ---

brentschmaltz · 2023-07-12T03:30:48Z

/azp run

azure-pipelines · 2023-07-12T03:31:01Z

Azure Pipelines successfully started running 3 pipeline(s).

brentschmaltz · 2023-07-12T03:32:45Z

@BrennanConroy @eerhardt the reason the SignalR test failed is due to a security fix in IdentityModel where the HMAC must have a key of the proper size.

This speaks to the importance of integrated tests before IdentityModel ships a new version otherwise our customers will hit such issues.

eng/Versions.props

Co-authored-by: Keegan Caruso <[email protected]>

* SymmetricSecurityKey needs 32 bytes * Update source-build-externals dependencies --------- Co-authored-by: Keegan Caruso <[email protected]>

* Update Wilson7 branch Default to using new handlers Changes from API review * Handle obsolete members * Handle obsolete members in tests * Add aka.ms link --------- Co-authored-by: Keegan Caruso <[email protected]>

BrennanConroy · 2023-07-20T03:50:44Z

/backport to release/8.0-preview7

github-actions · 2023-07-20T03:50:56Z

Started backporting to release/8.0-preview7: https://github.com/dotnet/aspnetcore/actions/runs/5606679712

github-actions · 2023-07-20T03:51:28Z

@BrennanConroy backporting to release/8.0-preview7 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: update IdentityModel to 6.31.0
Applying: Added JsonWebTokenHandler and TokenHandlers (#48857)
Applying: adjust for claims mapping remove using System
Applying: moved api's to unshipped
Applying: Addressed PR comments
Applying: Removed setter.
Applying: fix bild break, useTokenHanlders default false.
Applying: Increase key size to 256 bits or HMAC will fail.
Applying: update version of identity model libraries (#49349)
Applying: Update Wilson7 branch (#49491)
Using index info to reconstruct a base tree...
M	eng/Version.Details.xml
M	eng/Versions.props
M	src/SignalR/clients/csharp/Client/test/FunctionalTests/Startup.cs
M	src/SignalR/clients/ts/FunctionalTests/Startup.cs
M	src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/SignalR/common/Http.Connections/test/HttpConnectionDispatcherTests.cs
Auto-merging src/SignalR/clients/ts/FunctionalTests/Startup.cs
Auto-merging src/SignalR/clients/csharp/Client/test/FunctionalTests/Startup.cs
Auto-merging eng/Versions.props
CONFLICT (content): Merge conflict in eng/Versions.props
Auto-merging eng/Version.Details.xml
CONFLICT (content): Merge conflict in eng/Version.Details.xml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0010 Update Wilson7 branch (#49491)
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2023-07-20T03:51:29Z

@BrennanConroy an error occurred while backporting to release/8.0-preview7, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

eerhardt · 2023-07-20T15:23:58Z

src/Security/Authentication/test/JwtBearerTests_Handler.cs

+
+namespace Microsoft.AspNetCore.Authentication.JwtBearer;
+
+public class JwtBearerTests_Handler : SharedAuthenticationTests<JwtBearerOptions>


Is there any way to refactor these tests so they can be shared with the existing JwtBearerTests? For example having a base test class with the tests in them?

May be something to think about as a follow-up refactoring.

eerhardt · 2023-07-20T15:29:42Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs

+    /// The advantage of using TokenHandlers are:
+    /// <para>There is an Async model.</para>
+    /// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is faster than a <see cref="JwtSecurityTokenHandler"/>.</para>
+    /// <para>There is an ability to make use of a Last-Known-Good model for metadata that protects applications when metadata is published with errors.</para>


I wonder if we should rethink these doc comments as they might not make full sense now that we've inverted the boolean to be UseSecurityTokenValidators.

Maybe we should add why someone would want to use SecurityTokenValidators as well.

eerhardt · 2023-07-20T15:32:57Z

src/Security/Authentication/JwtBearer/src/JwtBearerOptions.cs

@@ -126,15 +142,20 @@ public new JwtBearerEvents Events
    public bool IncludeErrorDetails { get; set; } = true;

    /// <summary>
-    /// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, which is used when determining
-    /// whether or not to map claim types that are extracted when validating a <see cref="JwtSecurityToken"/>.
+    /// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/> in TokenHandlers which is used when determining


Suggested change

/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/> in TokenHandlers which is used when determining

/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/> in TokenHandlers, which is used when determining

eerhardt · 2023-07-20T15:43:26Z

src/Security/Authentication/JwtBearer/src/JwtBearerHandler.cs

+            if (Options.ConfigurationManager != null)
+            {
+                // GetConfigurationAsync has a time interval that must pass before new http request will be issued.
+                var configuration = await Options.ConfigurationManager.GetConfigurationAsync(Context.RequestAborted);


Previously we would cache the _configuration on the handler. However, we don't cache it anymore, and instead will always call GetConfigurationAsync. Is this OK? Will it cause any sort of performance issue?

GetConfigurationAsync should do its own caching, and handlers are re-created per request anyways. It looks like this is the only place config is read?

Correct, ConfigurationManager does its own caching

eerhardt · 2023-07-20T18:26:13Z

Closing in favor of #49542.

ghost · 2023-07-20T18:26:19Z

Hi @eerhardt. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

bbhxwl · 2023-11-26T01:59:21Z

IDX10720

I want to be compatible with the token of my Net6 program, 128 bits. How should I set it in Net8?

ghost · 2023-11-26T01:59:28Z

Hi @bbhxwl. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

brentschmaltz requested review from a team, Tratcher and wtgodbe as code owners June 22, 2023 15:41

dotnet-issue-labeler bot added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 22, 2023

halter73 requested a review from eerhardt June 22, 2023 17:36

halter73 reviewed Jun 22, 2023

View reviewed changes

brentschmaltz commented Jun 22, 2023

View reviewed changes

ilmax reviewed Jun 23, 2023

View reviewed changes

eerhardt reviewed Jun 26, 2023

View reviewed changes

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 7, 2023

Brent Schmaltz and others added 4 commits July 7, 2023 11:28

update IdentityModel to 6.31.0

507d241

Added JsonWebTokenHandler and TokenHandlers (#48857)

47ffe32

Co-authored-by: Brent Schmaltz <[email protected]>

adjust for claims mapping

4153b06

remove using System

moved api's to unshipped

7fa2f96

brentschmaltz force-pushed the Wilson7 branch from 247a1f6 to 7fa2f96 Compare July 7, 2023 18:31

Brent Schmaltz added 3 commits July 8, 2023 10:10

Addressed PR comments

aca45dd

Removed setter.

0402406

fix bild break, useTokenHanlders default false.

df64e72

ghost removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 11, 2023

Increase key size to 256 bits or HMAC will fail.

e63b5ce

brentschmaltz requested a review from BrennanConroy as a code owner July 12, 2023 03:22

jennyf19 reviewed Jul 16, 2023

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

update version of identity model libraries (#49349)

4501b82

Co-authored-by: Keegan Caruso <[email protected]>

jennyf19 mentioned this pull request Jul 17, 2023

Move aspnetcore to leverage JsonWebToken and JsonWebTokenHandler #49469

Closed

captainsafia and others added 3 commits July 18, 2023 08:43

Update Wilson7 branch (#49491)

583728e

* SymmetricSecurityKey needs 32 bytes * Update source-build-externals dependencies --------- Co-authored-by: Keegan Caruso <[email protected]>

Merge branch 'main' into Wilson7

65595cb

Update Wilson7 branch (#49524)

f22eba9

* Update Wilson7 branch Default to using new handlers Changes from API review * Handle obsolete members * Handle obsolete members in tests * Add aka.ms link --------- Co-authored-by: Keegan Caruso <[email protected]>

eerhardt reviewed Jul 20, 2023

View reviewed changes

keegan-caruso mentioned this pull request Jul 20, 2023

Update JwtBearer, WsFed, and OIDC handlers to use identity model 7 #49541

Merged

eerhardt mentioned this pull request Jul 20, 2023

Update JwtBearer, WsFed, and OIDC handlers to use identity model 7 #49542

Merged

11 tasks

eerhardt closed this Jul 20, 2023

BrennanConroy deleted the Wilson7 branch November 26, 2023 03:05

	private async Task<TokenValidationParameters> SetupTokenValidationParameters()
	private async Task<TokenValidationParameters> SetupTokenValidationParametersAsync()

	/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/>which is used when determining
	/// Gets or sets the <see cref="MapInboundClaims"/> property on the default instance of <see cref="JwtSecurityTokenHandler"/> in SecurityTokenValidators, or <see cref="JsonWebTokenHandler"/> in TokenHandlers, which is used when determining

	/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>
	/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is faster when validating.</para>

	/// Gets of sets the <see cref="UseTokenHandlers"/> property to control if <see cref="TokenHandlers"/> or <see cref="SecurityTokenHandlers"/> will be used to validate the inbound token.
	/// Gets or sets whether <see cref="TokenHandlers"/> or <see cref="SecurityTokenHandlers"/> will be used to validate the inbound token.

	/// <para>The default token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>
	/// <para>The default JWT token handler is a <see cref="JsonWebTokenHandler"/> which is 30 % faster when validating.</para>

	public bool UseTokenHandlers { get; set; }
	public bool UseTokenHandlers { get; set; } = true;


		namespace Microsoft.AspNetCore.Authentication.JwtBearer;

		public class JwtBearerTests_Handler : SharedAuthenticationTests<JwtBearerOptions>

Update JwtBearer and WsFederation to use updated IdentityModel #48966

Update JwtBearer and WsFederation to use updated IdentityModel #48966

Conversation

brentschmaltz commented Jun 22, 2023 • edited Loading

Description

halter73 left a comment

Choose a reason for hiding this comment

halter73 Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

halter73 commented Jun 22, 2023

kevinchalet commented Jun 22, 2023

brentschmaltz Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentschmaltz Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinchalet commented Jun 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 7, 2023

brentschmaltz commented Jul 11, 2023

azure-pipelines bot commented Jul 11, 2023

brentschmaltz commented Jul 11, 2023

azure-pipelines bot commented Jul 11, 2023

eerhardt commented Jul 11, 2023

brentschmaltz commented Jul 12, 2023

azure-pipelines bot commented Jul 12, 2023

brentschmaltz commented Jul 12, 2023

BrennanConroy commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

github-actions bot commented Jul 20, 2023

eerhardt Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

eerhardt Jul 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Jul 20, 2023

ghost commented Jul 20, 2023

bbhxwl commented Nov 26, 2023

ghost commented Nov 26, 2023

brentschmaltz commented Jun 22, 2023 •

edited

Loading

halter73 Jun 22, 2023 •

edited

Loading

brentschmaltz Jun 22, 2023 •

edited

Loading

brentschmaltz Jun 22, 2023 •

edited

Loading

eerhardt Jul 20, 2023 •

edited

Loading

eerhardt Jul 20, 2023 •

edited

Loading