JSON object as an `act` claim not handled properly

[mdrgeniebelt]

Hello :)

So we've been implementing the delegation flow for our microservices and following the specification (still draft) we found out that we can provide the chain of delegation in the act claim (RFC link) which should be a json object.

We're using IdentityServer as a IdP and they have easy way of creating new custom grants and generating proper JWT tokens. The issue they we're facing though right now is that it seems that JwtPayload class is not handling it properly. So below is a test code I've created to show you the problem:

class Program
{
	static void Main(string[] args)
	{
		var jwtPayload = new JwtPayload("http://localhost:5001", null, null, DateTime.UtcNow, DateTime.UtcNow.AddMinutes(2));
		
		var delegationClaim1 = new DelegationActorClaim("client1", string.Empty);
		var delegationClaim2 = new DelegationActorClaim("client2", JsonSerializer.Serialize(delegationClaim1));
		var delegationClaim3 = new DelegationActorClaim("client3", JsonSerializer.Serialize(delegationClaim2));
		var delegationClaim4 = new DelegationActorClaim("client4", JsonSerializer.Serialize(delegationClaim3));

		var claim = delegationClaim4.ToClaim();
		
		// jwtPayload.AddClaim(claim);
		jwtPayload.Add("act", JToken.FromObject(delegationClaim1));
		
		var jwtHeader = new JwtHeader();

		var jwt = new JwtSecurityToken(jwtHeader, jwtPayload);
		
		var handler = new JwtSecurityTokenHandler();
		var result = handler.WriteToken(jwt);

		
		Console.ReadKey();
	}
}

public class DelegationActorClaim
{
	[JsonPropertyName("sub")]
	public string ClientId { get; set; } = null!;
	[JsonPropertyName("act")]
	public DelegationActorClaim? Actor { get; set; }

	public DelegationActorClaim() {}

	public DelegationActorClaim(string clientId, string? previousActor)
	{
		ClientId = clientId;
		if (string.IsNullOrWhiteSpace(previousActor))
		{
			return;
		}

		Actor = JsonSerializer.Deserialize<DelegationActorClaim>(previousActor);
	}

	public Claim ToClaim()
	{
		return new Claim("act", JsonSerializer.Serialize(this), "json");
	}
}

The result of running this code is a JWT token like this: e30.eyJuYmYiOjE1OTc4MTg4MTMsImV4cCI6MTU5NzgxODkzMywiaXNzIjoiaHR0cDovL2xvY2FsaG9zdDo1MDAxIiwiYWN0Ijp7IkNsaWVudElkIjpbXSwiQWN0b3IiOltdfX0.

And inspecting it on jwt.io yields following result:

{
  "nbf": 1597818813,
  "exp": 1597818933,
  "iss": "http://localhost:5001",
  "act": {
    "ClientId": [],
    "Actor": []
  }
}

For some reason both CientId and Actor are empty arrays.

Tested with: .NET Core 3.1 System.IdentityModel.Tokens.Jwt 6.7.1

[mdrgeniebelt]

Further investigation:

I'm adding JToken object to the JwtPayload

JToken token = JToken.FromObject(delegationClaim1);
jwtPayload.Add("act", token);

And then while debugging public virtual IEnumerable<Claim> Claims from JwtPayload I can see that its type is JObject instead and this is why it's treated in a different way than it is supposed to be treated:

[brentschmaltz]

@mdrgeniebelt you hit an issue where we have moved a clone of Newtonsoft internally. We haven't moved to System.Text.Json yet.

I am working through some sample code and am finding multiple issues with supporting your scenario. Wanted to let you know we are working on a workaround for now...

[brentschmaltz]

@mdrgeniebelt i added some sample code here: https://github.com/brentschmaltz/CodeSnips/blob/d25dc28b4d2af139e74f08feea31bc4c25b87069/src/JwtTokens/CreateToken.cs#L110 that will produce the correct json. I see there is a lot of work to have the ClaimsIdentity have the 'Actor' chained properly. We use "ClaimTypes.Actor' that doesn't map to the json spec you reference.

[brockallen]

The short of it is though that we need an easy way to take json and have it serialize properly. I think the simplest example is the address claim type from OIDC.

BTW, this is the major blocker why IdentityServer was unable to upgrade to the current version of "System.IdentityModel.Tokens.Jwt". We have to pin against version 5.6.0 because of this regression in 6x.

[brentschmaltz]

@mdrgeniebelt @brockallen i pushed a slightly different solution where the you can make use of the ability to specify the 'json serializer'

        JsonExtensions.Serializer = JsonConvert.SerializeObject;
        JsonExtensions.Deserializer = JsonConvert.DeserializeObject;

see: https://github.com/brentschmaltz/CodeSnips/blob/9ed9c141035a87f671e716b5318a7017009c3438/src/JwtTokens/CreateToken.cs#L114

[brentschmaltz]

@brockallen I assume the extensibility worked for you?

[brockallen]

I did not try it yet.

[brentschmaltz]

@brockallen we would like to make sure this works, do you have an example of the 'address' scenario you mention above?

[brockallen]

Just so you have it: https://openid.net/specs/openid-connect-core-1_0.html#AggregatedExample

I'll test when I can.

[brockallen]

@brockallen we would like to make sure this works, do you have an example of the 'address' scenario you mention above?

Finally tested this, and it did work. I hadn't thought about what your suggest meant until now and I realize these are more static variables, so the chances of my use case globally affecting anything else in the hosting app is high.

Are these exposed as instance members somewhere instead? That will help isolate the effect, because these changes are causing other errors elsewhere in our codebase.

[brockallen]

Are these exposed as instance members somewhere instead? That will help isolate the effect, because these changes are causing other errors elsewhere in our codebase.

Ok, don't worry about this then. We're just working around this by #ifdef around .NET Core 3.1 vs .NET 5.

[mdrgeniebelt]

Sadly seems like updating to .net 5 and updating one of the authentication packages resulted in System.IdentityModel.Tokens.Jwt update to 6.7.1 which broke this again :(

I tried adding

JsonExtensions.Serializer = JsonConvert.SerializeObject;
JsonExtensions.Deserializer = JsonConvert.DeserializeObject;

In my application Startup class but seems that it doesn't fix it. I'm not sure where the issue is exactly, we're also using IdentityServer4 version 4.1.1

[mdrgeniebelt]

After downgrading these packages and setting System.IdentityModel.Tokens.Jwt explicitly back to 5.6.0 it works again..

[leastprivilege]

Yes this "internalizing JSON.NET" was a horrible decision, it breaks code in many scenarios.

[brentschmaltz]

@leastprivilege it is unfortunate that we needed to replace the external Newtonsoft package. Our position was and is, given our purpose in the auth stack, we need to be very careful about which assemblies we trust.

@brockallen noted the extensibility points are static globals. We will work on a better extensibility model.

[leastprivilege]

replace the external Newtonsoft package.

Not sure how this is related to trust. The fact that your public API is exposing the internalized JSON objects in various places is just non-sense. What are we supposed to do with these internal types then? Globally switching the the serializer is (while very much in the spirit of this library) also not very useful...

[brentschmaltz]

@leastprivilege the idea is we want control over what assemblies we link to. We only take dependencies on .net assemblies, no third parties. We have had issues with backcompat, acknowledged we have caused some of our own issues, some was unavoidable some was not. In any event, from 6.x forward we have a strong mandate not to break anyone. We are looing to fix this in 6.8.1 and we can wait for this fix.

I am closing https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/1580 . We need to address that also.

[leastprivilege]

we want control over what assemblies we link to

I understand that. But "internalizing" doesn't necessarily mean making the types internal - because that is causing the main issues here.

from 6.x forward we have a strong mandate not to break anyone

I am glad to hear that ;)

we can wait for this fix

Not sure what that means

[brentschmaltz]

@leastprivilege If we expose the internal Json objects, then we have a whole lot of support, we are looking for a different way thinking that .net Core is the way of the future and supporting the internal version of newtonsoft will take resources.

"we can wait for this" ... imcomplete grammar, what i meant to write was:

We have a strong desire to release 6.8.1 and we had not considered including this fix. We need fixes in 6.8.1 mostly for SignatureProviderCaching fixes, we will consider either delaying 6.8.1 OR wait for 6.8.2.

[joshuafonseca]

I'm also facing the same issue. Is there a release target for this fix?

[mdrgeniebelt]

Funny thing is that recently I've tried updating all nuget packages in our solution and it seemed that this issue somehow has been resolved....until the service that is also issuing tokens received an http request with jwt and after deserializing the jwt both scope and aud claims instead of containing multiple values with proper scopes/audiences contained an empty array, for example:

How jwt looks like after decoding it on jwt.io:

...
'aud' : ['aud1', 'aud2'],
'scope': ['scope1', 'scope2', 'scope3']
...

When received on the service side and decoded by jwt bearer handler the result looks more or less like this:

Claims list (key, value)->

...
'aud': [],
'aud': [],
'scope': [],
'scope': [],
'scope': []
...

Honestly I haven't had time to investigate it properly yet but it seems like we won't be able to update jwt nuget package for a while (which also blocks us from upgrading other oidc packages since these depend on it).

[mdrgeniebelt]

@brentschmaltz any news on this one? Tried version 6.10.2 and the issue is still there :(

[leastprivilege]

Yes - it is pretty annoying that this doesn't get fixed. The related issues around JSON objects and the discovery endpoint prevents customers from upgrading to .NET 5.

[brentschmaltz]

@mdrgeniebelt @leastprivilege @brockallen we are hoping to have this implemented before the end of year. The plan is to use System.Text.Json, hydrate the JsonWebToken once from a utf-8 into binary and provide properties for high use properties such as: Issuer, Audiences, etc and JsonWebToken.TryGetPayloadValue<...>( propertyName). We can then remember such values and only deserialize once for those properties.

Our performance tests have shown a 30% increase in validations using this model. Properties can be accessed in constant time as opposed to the ClaimsIdentity that runs a list. For tokens with a large number of properties, this makes a difference.

We are also delaying the creation of the ClaimsIdentity until asked for.

[leastprivilege]

Ok then...performance was actually never a concern of mine...but sounds exciting.

[mdrgeniebelt]

@leastprivilege @brockallen while we're waiting for this to be changed I wonder if there is a way to change how IdS is reading jwt? I created a 'temporary hack' for reading JWT (custom handler, we read claims manually for now) for our API - because we really had to upgrade System.IdentityModel.Tokens.Jwt to latest version (other packages do depend on it also...).

It was fine as long as we were using reference tokens when calling IdS endpoints but now we're testing external apps that use JWT and it fails because even though JWT contains openid scope, when we call connect/userinfo endpoint it fails with 403 (scopes is an empty array when read) 😞

[leastprivilege]

AFAIK this is all fixed in v5+

[mdrgeniebelt]

We're currently on 6.14.1 version of System.IdentityModel.Tokens.Jwt and 4.1.2 of IdentityServer4 and it doesn't work 😞

I'll tinker a bit more with JsonExtensions.Deserializer from Jwt package but I'm not even sure if that is the issue anymore 🤷‍♂️

[leastprivilege]

and 4.1.2 of IdentityServer4

yea sorry - I meant IdentityServer v5

[mdrgeniebelt]

Alright - then we're talking about Duende IdentityServer, right?