-
Notifications
You must be signed in to change notification settings - Fork 382
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
JSON object as an act
claim not handled properly
#1516
Comments
Further investigation: I'm adding JToken token = JToken.FromObject(delegationClaim1);
jwtPayload.Add("act", token); And then while debugging |
@mdrgeniebelt you hit an issue where we have moved a clone of Newtonsoft internally. I am working through some sample code and am finding multiple issues with supporting your scenario. |
@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. |
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 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. |
@mdrgeniebelt @brockallen i pushed a slightly different solution where the you can make use of the ability to specify the 'json serializer'
|
@brockallen I assume the extensibility worked for you? |
I did not try it yet. |
@brockallen we would like to make sure this works, do you have an example of the 'address' scenario you mention above? |
Just so you have it: https://openid.net/specs/openid-connect-core-1_0.html#AggregatedExample I'll test when I can. |
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. |
Ok, don't worry about this then. We're just working around this by #ifdef around .NET Core 3.1 vs .NET 5. |
Sadly seems like updating to .net 5 and updating one of the authentication packages resulted in I tried adding
In my application |
Yes this "internalizing JSON.NET" was a horrible decision, it breaks code in many scenarios. |
@leastprivilege it is unfortunate that we needed to replace the external Newtonsoft package. @brockallen noted the extensibility points are static globals. |
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... |
@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. I am closing #1580 . |
I understand that. But "internalizing" doesn't necessarily mean making the types
I am glad to hear that ;)
Not sure what that means |
@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. |
I'm also facing the same issue. Is there a release target for this fix? |
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 How jwt looks like after decoding it on jwt.io:
When received on the service side and decoded by jwt bearer handler the result looks more or less like this: Claims list (key, value)->
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). |
@brentschmaltz any news on this one? Tried version |
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. |
@mdrgeniebelt @leastprivilege @brockallen we are hoping to have this implemented before the end of year. Our performance tests have shown a 30% increase in validations using this model. We are also delaying the creation of the ClaimsIdentity until asked for. |
Ok then...performance was actually never a concern of mine...but sounds exciting. |
@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 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 |
AFAIK this is all fixed in v5+ |
We're currently on I'll tinker a bit more with |
yea sorry - I meant IdentityServer v5 |
Alright - then we're talking about Duende IdentityServer, right? |
That is correct. |
For the Which makes the actor claim basically unusable when there are nested act claims since (at least as far as I can see) there is no way to access any data from the internal tokenValidationResult.SecurityToken as JsonWebToken).TryGetPayloadValue<Dictionary<string, object>>("act", out var actor)
// Actor is Dictionary but the nested act claims are still JObject
tokenValidationResult.Claims.TryGetValue("act", out actor)
// Actor is JObject (but the internal one) And the suggestion
Does not seem to have any effect (which makes sense given the code I suppose, unless I'm missing something. The only thing that seems to be working is: (with as an example MethodInfo dynMethod = originalActor.GetType().GetMethod("ToObject",
BindingFlags.Public | BindingFlags.Instance, new[] { typeof(Type) });
var result = dynMethod.Invoke(nestedActor, new object[] { typeof(Dictionary<string,object>) }); But that feels kind of icky. EDIT: For if you just have to make it work. public static class ActorClaimHelper
{
public static bool TryGetActorClaim(this JsonWebToken token, out Dictionary<string, object> originalActor)
{
if (token.TryGetPayloadValue<Dictionary<string, object>>(CalcasaClaimNames.Actor, out originalActor))
{
var finalActor = originalActor;
while (finalActor.ContainsKey(CalcasaClaimNames.Actor))
{
if (finalActor[CalcasaClaimNames.Actor] is Dictionary<string, object> actDict)
{
finalActor = actDict;
continue;
}
if (finalActor[CalcasaClaimNames.Actor].GetType().Name == "JObject")
{
MethodInfo dynMethod = finalActor[CalcasaClaimNames.Actor].GetType().GetMethod("ToObject", BindingFlags.Public | BindingFlags.Instance, new[] { typeof(Type) });
if (dynMethod == null)
{
break;
}
if (dynMethod.Invoke(finalActor[CalcasaClaimNames.Actor], new object[] { typeof(Dictionary<string, object>) }) is Dictionary<string, object> newAct)
{
finalActor[CalcasaClaimNames.Actor] = newAct;
finalActor = newAct;
}
else
{
break;
}
}
else
{
break;
}
}
return true;
}
return false;
}
} |
Closing this as we dropped Newtonsoft in 7.x. |
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:The result of running this code is a JWT token like this:
e30.eyJuYmYiOjE1OTc4MTg4MTMsImV4cCI6MTU5NzgxODkzMywiaXNzIjoiaHR0cDovL2xvY2FsaG9zdDo1MDAxIiwiYWN0Ijp7IkNsaWVudElkIjpbXSwiQWN0b3IiOltdfX0.
And inspecting it on jwt.io yields following result:
For some reason both
CientId
andActor
are empty arrays.Tested with:
.NET Core 3.1
System.IdentityModel.Tokens.Jwt 6.7.1
The text was updated successfully, but these errors were encountered: