Wilson 7x (perf improvements in .NET 8) Discussion Forum ⚒️

[jennyf19]

The team is hard at work to bring:

• a 30% performance improvement in ASP.NET Core 8 by switching from JwtSecurityTokenHandler to JsonWebTokenHandler without causing breaking changes to ASP.NET Core.
• a fully trimmable M.IdentityModel SDK for native AOT.
• Async token validation, returning a TokenValidationResult rather than throwing.

There will, however, be breaking changes between M.IdentityModel 6x and M.IdentityModel 7x, which are documented in a wiki coming soon.

You can follow the changes in the dev7x branch and we will be shipping previews packages to NuGet by end of month (June 2023).

Your feedback is much appreciated and needed so that we ensure we meet your needs and have a solid experience for you. Please leave comments, questions, feedback in the discussion thread and we will address them as quickly as possible.

[brentschmaltz]

@leastprivilege @kevinchalet @brockallen @blowdart @davidfowl @eerhardt @Tratcher
FYI

[brentschmaltz]

@kevinchalet our goal is maintain existing behavior, while dropping net45 and using System.Text.Json.
Using System.Text.Json will be breaking as some values will be a JsonElement instead of a POCO instance as Newstonsoft will deserialize into .net types such as string, int, bool if possible. System.Text.Json will produce JsonElement's except for 'null'.

This will show up in places like OpenIdConnectAn example is OpenIdConnectConfiguration.AdditionalData, where currently .net types could be the value in the [key, value] pair of the dictionary[string, object].

We will be switching to using the utf8Reader and utf8Writers for better control and are leaning towards not maintaining back compatibility (creating POCO's as before) as we want to be in sync with Json.Deserialize[T].

[kevinchalet]

This will show up in places like OpenIdConnectAn example is OpenIdConnectConfiguration.AdditionalData, where currently .net types could be the value in the [key, value] pair of the dictionary[string, object].

Hopefully this shouldn't affect the basic OIDC/JWT middleware scenarios in Katana.

My biggest concern is that folks should be able to use OpenIddict in Katana apps with the OIDC/JWT middleware packages also referenced and registered in the OWIN pipeline. If all these pieces are able to migrate to Wilson 7.x or stay on 6.x but are not affected by a breaking change, this shouldn't be a problem 😃

We will be switching to using the utf8Reader and utf8Writers for better control and are leaning towards not maintaining back compatibility (creating POCO's as before) as we want to be in sync with Json.Deserialize[T].

Using the low-level readers (or JsonDocument/JsonElement which is also a low-level way of traversing the JSON document) is the approach used in OpenIddict and it works really well. It's certainly more work that just (de)serializing some JSON to a POCO, but it's more efficient and way more flexible 😃

[Tratcher]

while dropping net45

What are the new TFMs? Katana is still net45.

[brentschmaltz]

@Tratcher TFM's are 461, 462, 472, netstandard 2.0, net6.0, net8.0

[brentschmaltz]

@kevinchalet i agree with you about using utf8Reader / utf8Writer being more flexible.
We used the same model with XmlReaders and XmlWriters when parsing SAML.

In our SDK that must support multiple TFM's using this approach will allow us to provide consistent exception handling and messaging across multiple TFM's.

[brockallen]

Will ASP.NET Core 8's OIDC and JWT handlers switch, and if so then when (if they've not already)?

[brentschmaltz]

We are working with the asp.net team to make the changes to OIDC, WsFed and JWT bearer, in a non-breaking way.
The hope is to get the first changes into preview6 so that can give us enough time for feedback.

[brockallen]

ok ty

[brockallen]

What preview do you expect this to be in of ASP.NET, please? Also, this will be the new default, or will there need to be something explicit/manual to opt-in?

[jennyf19]

@brockallen we are aiming for .NET 8-preview 7. Haven't decided on default or opt-in, which would you prefer?

[jennyf19]

7.0.0-preview is now on NuGet if you want to try and provide feedback.

[kevinchalet]

FYI, I tested the 7.0.0-preview packages with OpenIddict and haven't seen any particular regression. Good job 👍🏻

[jennyf19]

Thanks for the quick feedback @kevinchalet !

[jmprieur]

@brockallen:

• this would be in ASP.NET for .NET 8 (rc1, or earlier, depending on what the ASP.NET Core team wishes)
• we are still discussing if this will be opt-in or by default. How would you like it to be, yourself?

[brockallen]

this would be in ASP.NET for .NET 8 (rc1, or earlier, depending on what the ASP.NET Core team wishes)

Please update this thread when that is known and/or released, please.

we are still discussing if this will be opt-in or by default. How would you like it to be, yourself?

Not sure if I have an opinion. I just need to know to support our customers since I suspect they will come to us with questions/issues.

[jennyf19]

@brockallen & @leastprivilege please let us know if/when you have time to try 7.1.0-preview and if you hit any issues. Thank you.

[jennyf19]

@cakescience any feedback from your end?

[ghost]

I tested the version 7.1.0-preview on some personal projects and didn't notice any particular regression. You worked hard!

[jennyf19]

Thanks for the feedback @cakescience ! Btw, we are going to align our versioning on aspnetcore, and correct semantic versioning, so the next preview (ETA is 8/31) will be 7.0.0-preview2 and we will delist 7.1.0-preview from NuGet. Just a heads-up.

[jennyf19]

@cakescience we are not going to release 7.0.0-preview2 today. Might be end of week or next instead. Waiting for some more work to go in first.

[kevinchalet]

Thanks for the feedback @cakescience ! Btw, we are going to align our versioning on aspnetcore, and correct semantic versioning, so the next preview (ETA is 8/31) will be 7.0.0-preview2 and we will delist 7.1.0-preview from NuGet. Just a heads-up.

Out of curiosity, do you plan on aligning the versions only for the previews or forever? Since it's part of .NET, ASP.NET Core has a quite slow release pace (one major release a year, no intermediate minor version and a few patches to fix only the most critical bugs), so it would be sad to see IM releases slow down 😓

[jennyf19]

Thanks for asking! I wasn't very clear. Our semantic versioning is not correct for the previews. We currently do 1.0.0-preview, 1.0.1-preview, 1.1.0-preview, and then 1.1.1 (first non-preview), for example. Whereas we should do 1.0.0-preview1, 1.0.0-preview2, 1.0.0-preview3, 1.0.0.

We will align with aspnetcore on the sematic versioning, not on release cycle. We will continue to release IdentityModel as often as needed to ensure we deliver value to our customers. Hope that clarifies things.

[kevinchalet]

Hope that clarifies things.

It's now crystal clear, thanks! 👍🏻

(the new versioning scheme makes a lot more sense, indeed!)

[Cyberboss]

EDIT: Sorry this appears to be an issue with Microsoft.AspNetCore.Authentication.JwtBearer as updating this library exclusively does not reproduce the issue.

Fixed in this commit tgstation/tgstation-server@651c810. Essentially, just updated to setting JwtBearerOptions.MapInboundClaims = false; (which wasn't an option in older ASP.NET Core versions I believe) instead of calling JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear();.

---

Updating to .NET Preview 8, specifically Microsoft.AspNetCore.Authentication.JwtBearer to 8.0.0-preview.7.23375.9 pulled this forward from the latest stable to 7.0.0-preview. Now, I no longer have a JwtRegisteredClaimNames.Sub principal in my TokenValidatedContext. Is there documentation to help with this?

Setup:

// configure bearer token validation
services
	.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
	.AddJwtBearer(jwtBearerOptions =>
	{
		// this line isn't actually run until the first request is made
		// at that point tokenFactory will be populated
		jwtBearerOptions.TokenValidationParameters = tokenFactory.ValidationParameters;
		jwtBearerOptions.Events = new JwtBearerEvents
		{
			// Application is our composition root so this monstrosity of a line is okay
			// At least, that's what I tell myself to sleep at night
			OnTokenValidated = ctx => ctx
				.HttpContext
				.RequestServices
				.GetRequiredService<IClaimsInjector>()
				.InjectClaimsIntoContext(
					ctx,
					ctx.HttpContext.RequestAborted),
		};
	});

JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear();

tokenFactory.ValidationParameters:

ValidationParameters = new TokenValidationParameters
{
	ValidateIssuerSigningKey = true,
	IssuerSigningKey = new SymmetricSecurityKey(signingKeyBytes),

	ValidateIssuer = true,
	ValidIssuer = assemblyInformationProvider.AssemblyName.Name,

	ValidateLifetime = true,
	ValidateAudience = true,
	ValidAudience = typeof(TokenResponse).Assembly.GetName().Name,

	ClockSkew = TimeSpan.FromMinutes(securityConfiguration.TokenClockSkewMinutes),

	RequireSignedTokens = true,

	RequireExpirationTime = true,
};

ClaimsInjector:

public async Task InjectClaimsIntoContext(TokenValidatedContext tokenValidatedContext, CancellationToken cancellationToken)
{
	ArgumentNullException.ThrowIfNull(tokenValidatedContext);

	// Find the user id in the token
	var userIdClaim = tokenValidatedContext.Principal.FindFirst(JwtRegisteredClaimNames.Sub);
	if (userIdClaim == default)
		throw new InvalidOperationException("Missing required claim!"); // Hitting this error

	long userId;
	try
	{
		userId = Int64.Parse(userIdClaim.Value, CultureInfo.InvariantCulture);
	}
	catch (Exception e)
	{
		throw new InvalidOperationException("Failed to parse user ID!", e);
	}

        ...

[eerhardt]

@Cyberboss - can you open an issue for this? Note that Microsoft.AspNetCore.Authentication.JwtBearer in 8.0.0-preview.7.23375.9 updated to use the new IdentityModel JwtSecurityTokenHandler, which may have changed behavior here.

[Cyberboss]

Here? Or in https://github.com/dotnet/aspnetcore?

[eerhardt]

I guess I would start here. We can transfer it to aspnetcore if a change needs to be made there.

[Cyberboss]

#2230

[brentschmaltz]

The issue was: JwtSecurityTokenHandler.DefaultInboundClaimTypeMap.Clear();
asp.net is using JsonWebTokenHandler by default now.

So Cyberboss solved the issue with JsonWebTokenHandler.DefaultInboundClaimTypeMap.Clear();

For JwtBearer you can also use:
JwtBearerOptions.MapInboundClaims = false

[brentschmaltz]

@kevinchalet @brockallen @leastprivilege

We want to discuss the differences of serialization of a JsonWebToken in 7 project.
When creating a JWT, the JsonWebTokenHandler is sometimes passed a Dictionary<string, object>.
In our 6.x release we used a Newtonsoft JObject to serialize the Dictionary.
In 7 we cannot use JsonSerializer.Serialize<Dictionary<string,object>> and support the goal of AOT for asp.net users.

Preview4 will support collections {list, collection, enumeration, dictionary} where the 'object' resolves to C# type {int, bool, string, datetime, JsonElement, different variations of numbers}.

Dictionary<string,object>{"a",new List{"str1",new Dictionary<string,string>{{"key1","value1"}}}}
Will serialize as: {"a":["str1": {"key1":"value1"}]}
JsonWebToken.GetPayloadValue<IList>("a"), will return IList.

Consider: class MyClass{}
With Dictionary<string, object>{{"MyClass",new MyClass()}}
Will serialize as: {"MyClass" :MyClass.ToString()}

Preview4 recognizes JsonElement so if MyClass was serialized into a JsonElement that was added to the Dictionary, it would be serialized.
JsonWebToken.GetPayloadValue("MyClass") would return a JsonElement.

We are very interested if the community sees this as a blocker for asp.net 8.

Do you think we will need extensibility such as callbacks for unknown types in 7?

[devlie]

@brentschmaltz @kevinchalet

We are using v7.1.2 and this is a horrible breaking change, IMO - it's silent and deadly. Our services started to mysteriously fail last week, and we spent whole day hunting this one down. Our service references System.IdentityModel.Tokens 6.x and has following code:

var hashes = List<MyHash>{ new MyHash { Digest = "foo" } };
var tokenPayload = new JwtPayload
{ 
   { "Hashes", hashes }
};

Some other dependencies silently brought in 7.1.2 and the payload went from { "hashes": "foo" } to { "hashes": "MyHash" } which is unusable to token receiver. Good thing our token isn't encrypted, else this would have taken even longer to debug.

[kevinchalet]

@devlie I mentioned at some point that using .ToString() wasn't a great idea (for this exact reason), but heh 😆

Doesn't seem like a great idea to me: if a type is too complex to be serialized/not supported, an exception should be thrown instead of trying to use .ToString() (in most cases, the result will be terrible anyway 🤣)

[kevinchalet]

Preview4 recognizes JsonElement so if MyClass was serialized into a JsonElement that was added to the Dictionary, it would be serialized.
JsonWebToken.GetPayloadValue("MyClass") would return a JsonElement.

I personally don't mind having to directly use JsonElement, but these questions will need to be addressed first: #2244 (comment).

Consider: class MyClass{}
With Dictionary<string, object>{{"MyClass",new MyClass()}}
Will serialize as: {"MyClass" :MyClass.ToString()}

Doesn't seem like a great idea to me: if a type is too complex to be serialized/not supported, an exception should be thrown instead of trying to use .ToString() (in most cases, the result will be terrible anyway 🤣)

[mgroetan]

Today, I updated my ASP.NET Core MVC web API project running with OIDC under .NET 7 to use the latest 7.0.2 version of the "System.IdentityModel.Tokens.Jwt" NuGet package.
Up to that point, I had been using the 6.32.3 version.

Now every web API request fails with this:

System.InvalidOperationException: IDX20803: Unable to obtain configuration from: '[PII of type 'System.String' is hidden. For more details, see https://aka.ms/IdentityModel/PII.]'.
 ---> System.TypeLoadException: Could not load type 'Microsoft.IdentityModel.Json.JsonConvert' from assembly 'Microsoft.IdentityModel.Tokens, Version=7.0.2.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'.
   at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectConfigurationRetriever.GetAsync(String address, IDocumentRetriever retriever, CancellationToken cancel)
   at System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[TStateMachine](TStateMachine& stateMachine)
   at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectConfigurationRetriever.GetAsync(String address, IDocumentRetriever retriever, CancellationToken cancel)
   at Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectConfigurationRetriever.Microsoft.IdentityModel.Protocols.IConfigurationRetriever<Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectConfiguration>.GetConfigurationAsync(String address, IDocumentRetriever retriever, CancellationToken cancel)
   at Microsoft.IdentityModel.Protocols.ConfigurationManager`1.GetConfigurationAsync(CancellationToken cancel)

I don't have a direct dependency towards the mentioned "Microsoft.IdentityModel.Json.JsonConvert" package, rather it's just a transitive dependency of "System.IdentityModel.Tokens.Jwt", which you'd think should be unproblematic to resolve.

Through debugging, I also get this variant:

System.InvalidOperationException: IDX20803: Unable to obtain configuration from: '[PII of type 'System.String' is hidden. For more details, see https://aka.ms/IdentityModel/PII.]'.
   at Microsoft.IdentityModel.Protocols.ConfigurationManager`1.GetConfigurationAsync(CancellationToken cancel)
   at Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerHandler.HandleAuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.JwtBearer.JwtBearerHandler.HandleAuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.AuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticateAsync(HttpContext context, String scheme)
   at Microsoft.AspNetCore.Authentication.AuthenticationHandler`1.AuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationService.AuthenticateAsync(HttpContext context, String scheme)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddlewareImpl.Invoke(HttpContext context)

I see that @Cyberboss has already mentioned earlier that there's a potential bearer access token validation issue with the "Microsoft.AspNetCore.Authentication.JwtBearer" NuGet package mentioned in the latter stack trace.
We've already been using the 7.0.10 version of that for some time, and also upgraded this to 7.0.11 today, at the same time.

I've debugged the access token validation configuration logic, but nothing bad happens there. The error occurs once the control is given over to ASP.NET Core.

I've tested both of these options, suggested earlier:

var accessToken = authorization[1];
var handler = new JwtSecurityTokenHandler();
var securityToken = (JwtSecurityToken)handler.ReadToken(accessToken);

handler.MapInboundClaims = false;

and:

protected static void SetCommonAccessTokenValidationOptions(JwtBearerOptions options)
{
    // Note: These ARE the defaults, but included for added readability
    options.TokenValidationParameters.RequireSignedTokens = true;
    options.TokenValidationParameters.ValidateIssuer = true;
    options.TokenValidationParameters.ValidateAudience = true;
    options.TokenValidationParameters.ValidateLifetime = true;

    // Restrict to the signature algorithm that Azure AD uses
    options.TokenValidationParameters.ValidAlgorithms = new[] { JwsAlgorithm.RS256.ToString() };

    options.MapInboundClaims = false;
}

None of these changes make any difference whatsoever.
Please advice on what I'm doing wrong.

More complete original code:

services.AddAuthentication(options =>
    options.DefaultScheme = JwtBearerDefaults.AuthenticationScheme)
    .AddPolicyScheme(JwtBearerDefaults.AuthenticationScheme, "Bearer policy scheme", options =>
        options.ForwardDefaultSelector = c =>
        {
            string authHeader = c.Request.Headers["Authorization"];
            var authorization = authHeader?.Split(' ');

            // If there's an authentication request header, inspect the accompanying access token
            if (authorization?.Length >= 2 && authorization[0].ToLower() == JwtBearerDefaults.AuthenticationScheme.ToLower())
            {
                var accessToken = authorization[1];
                var handler = new JwtSecurityTokenHandler();
                var securityToken = (JwtSecurityToken)handler.ReadToken(accessToken);

                if (securityToken.Issuer == nativeAuthenticationIssuer && securityToken.Audiences.Any(aud => aud == nativeAuthenticationAudience))
                {
                    const string authScheme = AuthenticationSchemes.Native;
                    return authScheme;
                }
[...]
    .AddJwtBearer(AuthenticationSchemes.Native, options =>
    {
        SetCommonAccessTokenValidationOptions(options);

        options.Authority = nativeAuthenticationAuthority;
        options.TokenValidationParameters.ValidIssuer = nativeAuthenticationIssuer;
        options.TokenValidationParameters.ValidAudience = nativeAuthenticationAudience;

        options.Events = new JwtBearerEvents
        {
            OnTokenValidated = _ => Task.FromResult(0),
            OnAuthenticationFailed = OnAuthenticationFailedForImplicitFlow
        };
    })
[...]
protected static void SetCommonAccessTokenValidationOptions(JwtBearerOptions options)
{
    // Note: These ARE the defaults, but included for added readability
    options.TokenValidationParameters.RequireSignedTokens = true;
    options.TokenValidationParameters.ValidateIssuer = true;
    options.TokenValidationParameters.ValidateAudience = true;
    options.TokenValidationParameters.ValidateLifetime = true;

    // Restrict to the signature algorithm that Azure AD uses
    options.TokenValidationParameters.ValidAlgorithms = new[] { JwsAlgorithm.RS256.ToString() };
}

[brentschmaltz]

@mgroetan @cakescience i am seeing issues with asp.net 7 and IdentityModel 7.
We have tested IdentityModel7 with asp.net 8.
Thanks for the code snippet, that really helps.

[mgroetan]

You can check #2294 (reply in thread)

Thx, @cakescience. I was just about to add the essence of that as a reply myself, from trial and error, after a hunch about double checking the current dependency tree.
So, in short, upgrading to the latest version of "Microsoft.IdentityModel.Protocols.OpenIdConnect" (7.0.2) fixed my problem. I didn't even have to mess about with that "MapInboundClaims" property.

That (".OpenIdConnect") package is not something I had a direct dependency on, though.
Rather, it's just a transitive dependency of "Microsoft.AspNetCore.Authentication.JwtBearer".
For the latter package (of version 7.0.11), its requirement towards "Microsoft.IdentityModel.Protocols.OpenIdConnect" is a measly 6.15.1, though, which was the version I ended up getting, and which does not work in this context.
That doesn't quite make sense to me, given all what we've now found out. Shouldn't its (".JwtBearer") latest version have a (".OpenIdConnect") 7.0.x as its minimum requirement? Then I (and others in the referenced issue) wouldn't have had this problem in the first place...?

[brentschmaltz]

@mgroetan it is important to have all the versions of IdentityModel to be the same version.

[brentschmaltz]

@Tratcher @mgroetan we should open an issue in asp.net.

[Tratcher]

It should already be using the 7.0 version:
https://github.com/dotnet/aspnetcore/blob/da6c314d76628c1f130f76ed3e55f1d39057e091/eng/Versions.props#L260
https://github.com/dotnet/aspnetcore/blob/da6c314d76628c1f130f76ed3e55f1d39057e091/src/Security/Authentication/JwtBearer/src/Microsoft.AspNetCore.Authentication.JwtBearer.csproj#L13C25-L13C72

@mgroetan, what version of the JwtBearer package are you using?

[Tratcher]

https://www.nuget.org/packages/Microsoft.AspNetCore.Authentication.JwtBearer/8.0.0-rc.2.23480.2#dependencies-body-tab

[brentschmaltz]

@Tratcher i see asp.net 8 is using 7+ :-)

[Tratcher]

Yes

[brentschmaltz]

@mgroetan we have a work item to mark each nuget of IdentityModel to require an exact version of the other packages.
[7.0.3], for example.