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

keegan-caruso · 2023-07-20T16:12:03Z

Move aspnetcore to leverage JsonWebToken and JsonWebTokenHandler in .NET 8 preview 7

Updates the JwtBearer, WsFederation and OpenIdConnect authentication handlers to leverage more
performant and resilient token validation. Also enables developers to opt-out of this new improvement
if they choose too in .NET 8.

Description

ASP.NET, and then ASP.NET Core use Microsoft.IdentityModel to validate tokens. A more performant (30%), more resilient and async processing has been developed for three years and is widely used internally in Microsoft. It also offers defense in depth. This PR brings this improvement to ASP.NET Core in .NET 8 preview 7.

Fixes #49469 which also contains the API review

Customer Impact

Customers can expect a 30% performance in token validation, and associated COGS improvements. They also benefit from
resiliency by the usage of a last-known-good cache of the OpenIdConnect metadata.

By default ASP.NET Core uses the new improved token validation. There is no breaking change to the public API, but a behavioral
change in the TokenValidatedContext classes (for the three authentication handlers): the SecurityToken property (of abstract type
SecurityToken) used to be backed by the previous generation JwtSecurityToken, and is now backed by another derived class JsonWebToken. Developers can opt-out by setting a boolean. For details see https://aka.ms/aspnetcore8/security-token-changes

Regression?

Yes
No

Risk

High
Medium
Low

The new classes have been extensively exercised internally in Microsoft for several years.
The PR doesn't bring regression, based on community feedback (see for instance here)

Verification

Manual (required)
Automated

Packaging changes reviewed?

Yes
No
N/A

When servicing release/2.1

Make necessary changes in eng/PatchConfig.props

Co-authored-by: Brent Schmaltz <brentsch@hotmail.com>

remove using System

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* SymmetricSecurityKey needs 32 bytes * Update source-build-externals dependencies --------- Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

* 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 <keegancaruso@microsoft.com>

…/wilson7-consolidated

ghost · 2023-07-20T16:12:10Z

Thanks for your PR, @keegan-caruso. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

ghost · 2023-07-20T16:34:26Z

Hi @keegan-caruso. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

ghost · 2023-07-20T17:16:13Z

Hi @keegan-caruso. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

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

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

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.

agreed it would be a good follow up

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

eerhardt · 2023-07-20T18:18:36Z

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

@@ -103,8 +113,14 @@ public new JwtBearerEvents Events
    /// <summary>
    /// Gets the ordered list of <see cref="ISecurityTokenValidator"/> used to validate access tokens.
    /// </summary>
+    [Obsolete("SecurityTokenValidators is no longer used by default. Use TokenHandlers instead. To continue using SecurityTokenValidators, set UseSecurityTokenValidators to true. See https://aka.ms/aspnetcore8/security-token-changes")]


@Tratcher @halter73 @captainsafia - what do you think about defining a new DiagnosticId for this obsoletion? That way people who are using SecurityTokenValidators, and want to keep using it, don't need to suppress CS0618 (the catch-all obsolete ID), and instead can just globally suppress the specific ASP1234 ID. Then we could do the same in our tests and not need to #pragma warning disable everywhere.

Let’s do this for rc1. @keegan-caruso - can you log a follow up issue for it?

@eerhardt #49558

eerhardt · 2023-07-20T18:24:38Z

src/Security/Authentication/WsFederation/src/WsFederationHandler.cs

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


Why does the WsFederationHandler still cache the configuration in a field, but the JwtBearer doesn't?

See also the conversation at #48966 (comment).

We can make the same change to this handler if you want. It made sense to remove the field in JwtBearer to clean up the code. It was just being read in one place.

Was trying to make less changes to WsFed where possible.

Ok. Let’s make a follow up change in main for this.

BrennanConroy

Mainly nits since I'm not super familiar with the area

BrennanConroy · 2023-07-20T18:40:42Z

src/Security/Authentication/OpenIdConnect/src/Resources.resx

@@ -135,4 +135,10 @@
  <data name="IdTokenCodeMissing" xml:space="preserve">
    <value>Cannot process the message. Both id_token and code are missing.</value>
  </data>
+  <data name="ValidatedSecurityTokenNotJsonWebToken" xml:space="preserve">
+    <value>The Validated Security Token must be of type JsonWebToken, but instead its tye is '{0}'.</value>


Suggested change

<value>The Validated Security Token must be of type JsonWebToken, but instead its tye is '{0}'.</value>

<value>The Validated Security Token must be of type JsonWebToken, but instead its type is '{0}'.</value>

BrennanConroy · 2023-07-20T18:41:02Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectOptions.cs

+    /// <summary>
+    /// Gets or sets the <see cref="TokenHandler"/> used to validate identity tokens.
+    /// <para>
+    /// This will be used instead of <see cref="SecurityTokenValidator"/> if <see cref="UseSecurityTokenValidator"/> is <see langword="false"/>


Suggested change

/// This will be used instead of <see cref="SecurityTokenValidator"/> if <see cref="UseSecurityTokenValidator"/> is <see langword="false"/>

/// This will be used instead of <see cref="SecurityTokenValidator"/> if <see cref="UseSecurityTokenValidator"/> is <see langword="false"/>.

BrennanConroy · 2023-07-20T18:42:56Z

src/Security/Authentication/OpenIdConnect/src/LoggingExtensions.cs

+    [LoggerMessage(56, LogLevel.Error, "Unable to validate the 'id_token', no suitable TokenHandler was found for: '{IdToken}'.", EventName = "UnableToValidateIdTokenFromHandler")]
+    public static partial void UnableToValidateIdTokenFromHandler(this ILogger logger, string idToken);
+
+    [LoggerMessage(57, LogLevel.Error, "The Validated Security Token must be of type JsonWebToken, but instead its type is: '{SecurityTokenType}'", EventName = "InvalidSecurityTokenTypeFromHandler")]


Suggested change

[LoggerMessage(57, LogLevel.Error, "The Validated Security Token must be of type JsonWebToken, but instead its type is: '{SecurityTokenType}'", EventName = "InvalidSecurityTokenTypeFromHandler")]

[LoggerMessage(57, LogLevel.Error, "The Validated Security Token must be of type JsonWebToken, but instead its type is: '{SecurityTokenType}'.", EventName = "InvalidSecurityTokenTypeFromHandler")]

BrennanConroy · 2023-07-20T18:43:18Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

+                }
+                else
+                {
+                    tokenEndpointUser = ValidateToken(tokenEndpointResponse.IdToken, properties, validationParameters, out  tokenEndpointJwt);


Suggested change

tokenEndpointUser = ValidateToken(tokenEndpointResponse.IdToken, properties, validationParameters, out tokenEndpointJwt);

tokenEndpointUser = ValidateToken(tokenEndpointResponse.IdToken, properties, validationParameters, out tokenEndpointJwt);

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

BrennanConroy · 2023-07-20T18:47:08Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

+
+        if (validatedToken is not JsonWebToken)
+        {
+            Logger.InvalidSecurityTokenTypeFromHandler(validatedToken?.GetType().ToString());


Change this log method to accept Type? instead of string so we don't allocate the string unless it actually logs.

BrennanConroy · 2023-07-20T18:54:08Z

src/Security/Authentication/OpenIdConnect/src/OpenIdConnectHandler.cs

+                ?? _configuration.SigningKeys;
+        }
+
+        var validationResult = await Options.TokenHandler.ValidateTokenAsync(idToken, validationParameters);


Do the async methods in identity not accept a cancellation token?

BrennanConroy · 2023-07-20T18:55:42Z

src/Security/Authentication/WsFederation/src/WsFederationHandler.cs

+        {
+            Options.ConfigurationManager.RequestRefresh();
+        }
+


Suggested change

eerhardt

The JwtBearer changes look good to me. It would be good to get others approval (for OIDC) before merge.

@halter73

…kages to the latest patch release (7.0.3 & 2.15.2) (#51430) # Update the Microsoft.IdentityModel.* and Microsoft.Identity.Web.* packages to the latest patch release (7.0.3 & 2.15.2) Update the reference to the Microsoft.IdentityModel.* and Microsoft.Identity.Web.* packages so that we don't regress AAD authentication scenarios for web apps. ## Description We've hit an issue with AAD authentication in ASP.NET Core web apps, which was resulting in errors during login. This was due to an issue in the IdentityModel package, for which @halter73 has proposed a fix: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2361 The Identity team has approved the fix and has released a new package to NuGet so that we can update our dependency and **avoid the regression in 8.0**. Please note, that this change will have to include the soure-build change as well: dotnet/source-build-externals#228 Fixes #51005 ## Customer Impact Customers who will try to use AAD authentication for their ASP.NET Core web applications in 8.0 will fail to login. ## Regression? - [x] Yes - [ ] No This was technically an existing bug, which was already in the IdentityModel package, however only after a recent change #49542 the issue has surfaced impacting 8.0 apps. ## Risk - [ ] High - [ ] Medium - [x] Low From our point of view this is a dependency update. And the dependency has taken only a targeted fix to avoid the bug, going through all the necessary validation on the AAD side. ## Verification - [x] Manual (required) - [ ] Automated ## Packaging changes reviewed? - [ ] Yes - [ ] No - [x] N/A ---- ## When servicing release/2.1 - [ ] Make necessary changes in eng/PatchConfig.props --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Stephen Halter <halter73@gmail.com>

Keegan Caruso and others added 20 commits June 23, 2023 10:22

Option to use JsonWebTokenHandler in OpenIdConnectHandler

2260e57

fix sample

61323b8

split event tests

7f45829

update IdentityModel to 6.31.0

507d241

Added JsonWebTokenHandler and TokenHandlers (dotnet#48857)

47ffe32

Co-authored-by: Brent Schmaltz <brentsch@hotmail.com>

adjust for claims mapping

4153b06

remove using System

moved api's to unshipped

7fa2f96

Addressed PR comments

aca45dd

Removed setter.

0402406

Use 7.0.0-preview for identity model libraries

b208837

fix bild break, useTokenHanlders default false.

df64e72

use var for identitymodel versions

9a4cc56

Move new apis to unshipped

a4c116c

Increase key size to 256 bits or HMAC will fail.

e63b5ce

update version of identity model libraries (dotnet#49349)

4501b82

Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

Update Wilson7 branch (dotnet#49491)

583728e

* SymmetricSecurityKey needs 32 bytes * Update source-build-externals dependencies --------- Co-authored-by: Keegan Caruso <keegancaruso@microsoft.com>

Merge branch 'main' into Wilson7

65595cb

Update Wilson7 branch (dotnet#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 <keegancaruso@microsoft.com>

Changes from API review

b7c4701

Merge branch 'keegan-caruso/json-web-handler-oidc' into keegan-caruso…

ce34b0f

…/wilson7-consolidated

keegan-caruso requested review from BrennanConroy, halter73, Tratcher, a team and wtgodbe as code owners July 20, 2023 16:12

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 20, 2023

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

keegan-caruso changed the title ~~Keegan caruso/wilson7 consolidated~~ Update JwtBearer, WsFed, and OIDC handlers to use identity model 7 Jul 20, 2023

Tratcher added the Servicing-consider Shiproom approval is required for the issue label Jul 20, 2023

rbhanda added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jul 20, 2023

eerhardt reviewed Jul 20, 2023

View reviewed changes

eerhardt mentioned this pull request Jul 20, 2023

Update JwtBearer and WsFederation to use updated IdentityModel #48966

Closed

BrennanConroy reviewed Jul 20, 2023

View reviewed changes

Comments from review

7d0bc0b

eerhardt approved these changes Jul 20, 2023

View reviewed changes

keegan-caruso mentioned this pull request Jul 20, 2023

Add diagnostic ID to obsolete in SecurityTokenValidators #49558

Open

1 task

halter73 approved these changes Jul 21, 2023

View reviewed changes

wtgodbe merged commit 2922b05 into dotnet:release/8.0-preview7 Jul 21, 2023

ghost added this to the 8.0-preview7 milestone Jul 21, 2023

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jul 21, 2023

halter73 mentioned this pull request Oct 14, 2023

Fix AadIssuerValidator's handling of trailing forward slashes AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2361

Merged

mkArtakMSFT mentioned this pull request Oct 17, 2023

Update the Microsoft.IdentityModel.* and Microsoft.Identity.Web.* packages to the latest patch release (7.0.3 & 2.15.2) #51430

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

keegan-caruso commented Jul 20, 2023

ghost commented Jul 20, 2023

ghost commented Jul 20, 2023

ghost commented Jul 20, 2023

eerhardt Jul 20, 2023

keegan-caruso Jul 20, 2023

eerhardt Jul 20, 2023

eerhardt Jul 20, 2023

keegan-caruso Jul 20, 2023

eerhardt Jul 20, 2023

keegan-caruso Jul 20, 2023

eerhardt Jul 20, 2023

BrennanConroy left a comment

BrennanConroy Jul 20, 2023

BrennanConroy Jul 20, 2023

BrennanConroy Jul 20, 2023

BrennanConroy Jul 20, 2023

BrennanConroy Jul 20, 2023

BrennanConroy Jul 20, 2023

keegan-caruso Jul 20, 2023

BrennanConroy Jul 20, 2023

eerhardt left a comment


		namespace Microsoft.AspNetCore.Authentication.JwtBearer;

		public class JwtBearerTests_Handler : SharedAuthenticationTests<JwtBearerOptions>

	<value>The Validated Security Token must be of type JsonWebToken, but instead its tye is '{0}'.</value>
	<value>The Validated Security Token must be of type JsonWebToken, but instead its type is '{0}'.</value>

	/// This will be used instead of <see cref="SecurityTokenValidator"/> if <see cref="UseSecurityTokenValidator"/> is <see langword="false"/>
	/// This will be used instead of <see cref="SecurityTokenValidator"/> if <see cref="UseSecurityTokenValidator"/> is <see langword="false"/>.

	[LoggerMessage(57, LogLevel.Error, "The Validated Security Token must be of type JsonWebToken, but instead its type is: '{SecurityTokenType}'", EventName = "InvalidSecurityTokenTypeFromHandler")]
	[LoggerMessage(57, LogLevel.Error, "The Validated Security Token must be of type JsonWebToken, but instead its type is: '{SecurityTokenType}'.", EventName = "InvalidSecurityTokenTypeFromHandler")]

	tokenEndpointUser = ValidateToken(tokenEndpointResponse.IdToken, properties, validationParameters, out tokenEndpointJwt);
	tokenEndpointUser = ValidateToken(tokenEndpointResponse.IdToken, properties, validationParameters, out tokenEndpointJwt);

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

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

Conversation

keegan-caruso commented Jul 20, 2023

Move aspnetcore to leverage JsonWebToken and JsonWebTokenHandler in .NET 8 preview 7

Description

Customer Impact

Regression?

Risk

Verification

Packaging changes reviewed?

When servicing release/2.1

ghost commented Jul 20, 2023

ghost commented Jul 20, 2023

ghost commented Jul 20, 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

BrennanConroy left a 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

eerhardt left a comment

Choose a reason for hiding this comment