azure-activedirectory-identitymodel-extensions-for-dotnet icon indicating copy to clipboard operation
azure-activedirectory-identitymodel-extensions-for-dotnet copied to clipboard

[Bug] ClaimsIdentity Actor not serialized into JWTs

Open gustavdw opened this issue 2 years ago • 8 comments

Which version of Microsoft.IdentityModel are you using? Note that to get help, you need to run the latest version. Im using OpenIddict 3.1.1

Where is the issue?

  • [x ] M.IM.JsonWebTokens
  • [ ] M.IM.KeyVaultExtensions
  • [ ] M.IM.Logging
  • [ ] M.IM.ManagedKeyVaultSecurityKey
  • [ ] M.IM.Protocols
  • [x ] M.IM.Protocols.OpenIdConnect
  • [ ] M.IM.Protocols.SignedHttpRequest
  • [ ] M.IM.Protocols.WsFederation
  • [ ] M.IM.TestExtensions
  • [ ] M.IM.Tokens
  • [ ] M.IM.Tokens.Saml
  • [ ] M.IM.Validators
  • [ ] M.IM.Xml
  • [ ] S.IM.Tokens.Jwt
  • Other (please describe)

Is this a new or an existing app? The app is in production and I haven't upgraded Microsoft.IdentityModel.*, but started seeing this issue

Repro Using OpenIddict, see issue here: https://github.com/openiddict/openiddict-core/issues/1426

Expected behavior The ClaimsIdentity Actor is serialized into the JWT (as an json object with a subset of claims attached to the actor claims identity)

Actual behavior The ClaimsIdentity Actor is ignored

Possible solution None

gustavdw avatar Apr 25 '22 07:04 gustavdw

@gustavdw as @kevinchalet pointed out, IdentityModel is not adding ClaimsIdentity.Actor to the JWT. This is a bug.

brentschmaltz avatar Apr 25 '22 15:04 brentschmaltz

Great, thanks for taking a look, @brentschmaltz.

Note: it's possible this is not the only place that needs an update for ClaimsIdentity.Actor to work correctly: the reading part seems to use ClaimTypes.Actor and not the actort claim the previous IM versions used to use, so I think it won't work correctly when claims mapping is disabled:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/799a162276cb699c6634f12077ab7af0646cf710/src/Microsoft.IdentityModel.JsonWebTokens/JsonWebTokenHandler.cs#L699-L710

It's also interesting to note that OAuth 2.0 delegation/impersonation was standardized in 2020: we now have standard act and may_act claims (formatted as JSON objects): https://datatracker.ietf.org/doc/html/rfc8693/#section-4.1 / https://datatracker.ietf.org/doc/html/rfc8693/#section-4.4. Maybe it's something future versions of IM could leverage?

kevinchalet avatar Apr 25 '22 15:04 kevinchalet

Yeah for context, RFC8693 was what I was trying to implement

gustavdw avatar Apr 26 '22 10:04 gustavdw

@kevinchalet @gustavdw i try and make sure you are tagged on the PR. Thanks for the input.

brentschmaltz avatar Apr 29 '22 17:04 brentschmaltz

@gustavdw @kevinchalet our traditional way of handling this type of issue has been delegates and instance variables on TokenValidationParameters and SecurityTokenDescriptor.

For example, since when we started this work, there was no standard 'role claim': TokenValidationParameters.RoleClaimType

We added a delegate that could set the claim type based on information in the token and an instance variable.

Would that sound like a good approach for validation {ActorClaimType, ActorClaimTypeRetriever} (TokenValidationParameters)? For SecurityTokenDescriptor, perhaps a single property {ActorClaimType}, then actually write the value :-)

For convenience, we could have a settable static DefaultValue, that will set all instances of TokenValidationParameters and SecurityTokenDescriptor.

brentschmaltz avatar May 04 '22 00:05 brentschmaltz

Would that sound like a good approach for validation {ActorClaimType, ActorClaimTypeRetriever} (TokenValidationParameters)? For SecurityTokenDescriptor, perhaps a single property {ActorClaimType}, then actually write the value :-)

Sounds like a good plan! 👍🏻

For convenience, we could have a settable static DefaultValue, that will set all instances of TokenValidationParameters and SecurityTokenDescriptor.

Shared/settable statics are always problematic when you have multiple components that use them in the same app maybe it would be better to wait for actual demand to support this scenario?

kevinchalet avatar May 04 '22 21:05 kevinchalet

@kevinchalet thanks for the feedback, will hold off on the statics for now.

brentschmaltz avatar May 08 '22 20:05 brentschmaltz

Any status update on this?

gustavdw avatar Feb 10 '23 14:02 gustavdw