Azure-activedirectory-identitymodel-extensions-for-dotnet: Alg still wrong?

[leastprivilege]

I still get the weird XML identifier for alg in a JWT in RC2. Wasn't that fixed?

[brentschmaltz]

@leastprivilege we removed the outbound mapping.
What are you seeing?

[brockallen]

JWTs have this for the header:

{
  "alg": "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256",
  "kid": "6B7ACC520305BFDB4F7252DAEB2177CC091FAAE1",
  "typ": "JWT"
}

The alg is wrong.

[brentschmaltz]

@leastprivilege , @brockallen it looks so you are looking for the outbound mapping (where we had an equivalence class of algorithms that were mapped to the JWT ns) to be re-installed.
Our thought was, the user set the algorithm on SigningCredentials and we didn't want to mess with it.
But, we are hearing that it was somewhat useful.

[brockallen]

I don't follow. The JWA spec does not list that alg:

https://tools.ietf.org/html/rfc7518#section-3.1

[kevinchalet]

@leastprivilege @brockallen not sure it's worth arguing. I've tried hard and lamentably failed: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/317#issuecomment-163076860.

[brockallen]

Right, but JWTs/id_tokens produced by this library will then fail to validate.

[brentschmaltz]

@brockallen @PinpointTownes that is a good point that we will fail to inter-operate.
We will try and fit this in, outbound mapping only.

[leastprivilege]

Thank god @brockallen discovered this issue. Was a tricky one.

[kevinchalet]

@leastprivilege actually, it's not really new: two ASOS users reported this issue in November and I immediately informed the Azure AD team about the implications of this change:

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r45013780
https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/316

Almost 7 months later, it's still not fixed... but I guess it's probably better to waste time working on the XML documentation or polishing the exception messages rather than fixing this kind of bugs. After all, who cares if the JWT issued by IdentityModel don't use standard-compliant JWA algorithms...

[brentschmaltz]

@PinpointTownes now now, we weren't convinced it was needed. It is up to the user to set the outbound algorithm. But now we agree that it will help.

Exception messages are important to get right.

[brentschmaltz]

@PinpointTownes @brockallen If there are any other nagging items, please speak up so we have a chance to get them in.

[leastprivilege]

@PinpointTownes that was sarcasm - but never mind ;)

[kevinchalet]

@leastprivilege my sarcasm radar must be broken, then :smile:

@brentschmaltz so, is this something you'll fix for RTM?

[brockallen]

@brentschmaltz so, is this something you'll fix for RTM?

If not, then I doubt we can use this library in IdentityServer. Maybe that's by design? :)

[kevinchalet]

FWIW, my PR was ready to be merged: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/317/files. Would you like me to rebase it?

My approach was clear:

• Remove the totally insane mix between XMLDsig and JWA identifiers in SecurityAlgorithms and opt for one standard only. For consistency with IdentityModel 4, I opted for the XMLDsig identifiers.
• Make the appropriate mapping directly in JwtSecurityTokenHandler to avoid returning XMLDsig identifiers (ideally, we should also prevent JWT tokens using a XMLDsig alg header from being validated, but you were opposed to that...).

[brentschmaltz]

@PinpointTownes @brockallen We are going to fix at least outbound for RTM.

We were opposed to restricting inbound alg's as we found that being lenient inbound was beneficial to users. The spec is open for alg's.

[kevinchalet]

The spec is open for alg's.

It is, but I don't think that's a reason to accept XML security algorithms (at least by default).

And hem, it's really urgent to fix your stuff. There are currently many methods where things are totally mixed up (digest/hash algorithms instead of signature algorithms...).

Even your unit tests are impacted by this mess: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/KDev/test/Microsoft.IdentityModel.Protocols.OpenIdConnect.Tests/OpenIdConnectProtocolValidatorTests.cs#L276.

[brentschmaltz]

To that point, our extensibility in OpenIdConnectProtocolValidator is lacking.

[kevinchalet]

To that point, our extensibility in OpenIdConnectProtocolValidator is lacking.

It's so much more than an extensibility issue, actually. I just can't understand why you'd want a digest algorithm to be accepted in the alg header.

[brentschmaltz]

I was referring to L439, where the user may have difficulties creating a custom algorithm as the algorithms are fixed, of course the method is virtual, but not tied to the HashAlgorithmMap.

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/KDev/src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs#L439

I think you are referring to this (also Sha384, Sha512), which is obviously an error.

https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/KDev/src/Microsoft.IdentityModel.Protocols.OpenIdConnect/OpenIdConnectProtocolValidator.cs#L432

[kevinchalet]

which is obviously an error.

An error I reported 7 months ago: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/317/files#r45627961 :smile:

[brentschmaltz]

Is it too late to say Thank-You :blush:

[kevinchalet]

I'm not here for glory ; fix it, and you'll make me happy :smile:

[m-andrew-albright]

I would like this to be fixed as well. This completely derails our ability to validate signed JWTs from our Identity Server.

[brentschmaltz]

@leastprivilege @PinpointTownes @m-andrew-albright https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/447

[kevinchalet]

Fix confirmed, thanks :clap: