Azure-activedirectory-identitymodel-extensions-for-dotnet: Problem referencing both v5 and System.IdentityModel

[leastprivilege]

I have a situation where my project references both

"System.IdentityModel.Tokens.Jwt": "5.0.0-rc1-211161024"

and System.IdentityModel from the full .NET Framework.

this gives me errors like:

Error CS0433 The type 'SecurityKey' exists in both 'System.IdentityModel.Tokens, Version=5.0.0.112, Culture=neutral, PublicKeyToken=31bf3856ad364e35' and 'System.IdentityModel, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089'

[brockallen]

You can repo this in the "System.IdentityModel.Tokens.Jwt.Tests" project by adding "System.IdentityModel": "4.0.0.0" as a "frameworkAssemblies" to "dnx451". The final project.json being:

{
    "version": "5.0.0-*",
    "dependencies": {
      "System.IdentityModel.Tokens.Tests": "5.0.0-*"
    },
    "commands": {
        "test": "xunit.runner.aspnet"
    },
    "frameworks": {
      "dnx451": {
        "frameworkAssemblies": {
          "System.IdentityModel": "4.0.0.0"
        }
      },
        "dnxcore50": { }
    },
    "compilationOptions": {
        "allowUnsafe": false,
        "warningsAsErrors": true
    }
}

and try to run the test under dnvm use 1.0.0-rc1-final -r clr.

[kevinchalet]

Hahahaha, I knew I wouldn't be the single one :smile:

It's actually not a bug, just a (bad) design decision: the net451 version doesn't rely on System.IdentityModel (from the full framework) but uses System.IdentityModel.Tokens (the lightweight equivalent of System.IdentityModel). Sadly, since System.IdentityModel and System.IdentityModel.Tokens expose similar types, you can't reference them in the same application.

I chatted with @brentschmaltz on JabbR and sent @blowdart and @Eilon a mail a long time ago to warn them about the implications of such a design, but nothing changed :smile:

Hey Barry,

As a follow-up to my previous tweets, here's a quick recap of my worries with the new IdentityModel framework:

Unlike the other packages used by ASP.NET 5, System.IdentityModel.Tokens provides its own types for dnx451, instead of using type forwarding and relying on the existing System.IdentityModel assembly (shipped with the full .NET framework).

This means that applications targeting dnx451 and referencing the "legacy" System.IdentityModel assembly won't be able to use the existing types with ASP.NET 5, specially when referencing a package that itself references the new IdentityModel (like the OIDC middleware package) ; as the types it exposes will necessarily conflict with the System.IdentityModel assembly.

Of course, I perfectly understand why Brent decided to rewrite everything and start from a new and simpler base. But is this kind of major breaking change really compatible with the whole concept of having a dnx451 target that works with the full .NET framework?

Best regards.

[kevinchalet]

BTW, reverting this design decision is definitely possible.

2 weeks ago, I started working on a POC modifying System.IdentityModel.Tokens to use System.IdentityModel on net451: https://github.com/PinpointTownes/azure-activedirectory-identitymodel-extensions-for-dotnet/commit/705659db58302465a81d2dfd1e5ba4f842837b8e

The annoying part is SecurityKey.Kid: it's a whole new property that doesn't exist in the full framework version. And since SecurityKey doesn't directly expose the SKI, SecurityKey.Kid would have to be removed and replaced by the old SecurityKeyIdentifier, just like in .NET 4.5.

[blowdart]

It's not an asp.net issue, and not under that namespace, so it's not something I have visibiliry or influence into. I agree it's odd, and frankly shouldn't be under system, that's a a hangover from 4.5, but Brent et al own their namespace, so it's their decision.

[leastprivilege]

Changing the namespace would be an easy fix..

Sent from my iPad

On 06 Dec 2015, at 19:36, Barry Dorrans [email protected] wrote:

It's not an asp.net issue, and not under that namespace, so it's not something I have visibiliry or influence into. I agree it's odd, and frankly shouldn't be under system, that's a a hangover from 4.5, but Brent et al own their namespace, so it's their decision.

—
Reply to this email directly or view it on GitHub.

[brentschmaltz]

@leastprivilege @brockallen @PinpointTownes Fully understand the issue. We weren't able to move all of System.IdentityModel to FxCore (open source) so we had to cherry pick what we needed for OIDC and SAML to support asp.net vNext. We changed the OM as somethings didn't make sense (S.IM was designed in 2005). When our SAML-WsFed support returns, we should be closer to supporting the entire S.IM assembly feature set, but a goal is not to fully 'proxy' it. For this reason, it seems reasonable to follow the suggestion from @leastprivilege with a ns / nuget change (as we did with M.IM.Protocols.Oidc). I would like to close on this soon.

[leastprivilege]

Yes - I also think it is time for a new library and break backwards compat with S.IM where needed. Please do that change soon.

[brentschmaltz]

@leastprivilege it's one of our top priorities.

[kevinchalet]

Renaming the library sounds like a good option, but it's really disappointing to see that the Azure AD team is the only one that opted for this approach: the other teams have done an amazing job with the BCL and found the right balance between guaranteeing backwards compatibility and breaking everything.

Updating System.IdentityModel.Tokens to use System.IdentityModel for net451 doesn't mean you have to "copy" what the full framework version does: you're totally free to use different signatures for the CoreCLR version (e.g it's exactly what the CLR team did with X509Certificate2 when they removed PrivateKey and PublicKey.Key).

Not to mention that nothing prevents you from adding new APIs to the full framework version (in .NET 4.6, the age-old RSA base class was updated to offer better Encrypt/Decrypt overloads).

TBH, my POC showed that using System.IdentityModel was actually not really bad and even lead to better stuff in some cases. For instance, replacing ClaimsIdentity by a list of Claim in SecurityTokenDescriptor broke at least one important scenario: supporting Actor serialization.

Why not working on a prototype? If you don't like the final result, simply forget my idea and rename the package.

[kevinchalet]

Not to mention that it has one (great) merit: unifying IdentityModel 4 and 5, and allowing old apps to use the great new things added in IdentityModel 5 and never backported to IdentityModel 4 (for instance, ECDsa signing keys).

[brentschmaltz]

@PinpointTownes It is not the Azure AD team. S.IM needs support from .Net that wasn't going to show up. AsymmetricSignatureDeformatter for example, DSig as another. We tried to line it up, but it wasn't going to happen. We were able to get a new serialization model in S.S.Claims but to move all of S.IM was too big.

About the Actor, you can add your own claim OR open an issue.

[kevinchalet]

@PinpointTownes It is not the Azure AD team. S.IM needs support from .Net that wasn't going to show up. AsymmetricSignatureDeformatter for example, DSig as another. We tried to line it up, but it wasn't going to happen.

But absolutely nothing prevents you from omitting AsymmetricSignatureFormatter/AsymmetricSignatureDeformatter support in the CoreCLR version! It's exactly what I've done in my POC: https://github.com/PinpointTownes/azure-activedirectory-identitymodel-extensions-for-dotnet/blob/705659db58302465a81d2dfd1e5ba4f842837b8e/src/System.IdentityModel.Tokens/SecurityKey.cs

We were able to get a new serialization model in S.S.Claims but to move all of S.IM was too big.

(a serialization model that doesn't work. At all: https://github.com/aspnet/Security/issues/158#issuecomment-123111395 :smile:)
BTW, have you fixed it in .NET 4.6.1?

[brentschmaltz]

@PinpointTownes I will have a look at the ClaimsIdentity issue. If a fix is needed it will be in RC2.

[brentschmaltz]

@leastprivilege We are thinking of changing the ns and nuget package by swapping out System with Microsoft. Users could then use .Net S.IM easier with M.IM.xyz. However, WIF V1 would then be an issue, but that is probably OK.
We are thinking of keeping the same packageid (System.IdentityModel.Tokens.Jwt) as we would like to see all versions of our Jwt nugets easily found. What do you think?

[leastprivilege]

Yes please fix that scenario ASAP.

Keeping the package id seems odd to me.

[brentschmaltz]

@leastprivilege @PinpointTownes putting together both your concerns leads me to the following.
In a perfect world, we would have moved S.IM to FxCore. Tweaked the types, moved additions to 4.6 Desktop etc. The types that are colliding are in S.IM.Tokens. That we can move to M.IM.Tokens. We leave S.IM.Tokens.Jwt as is. This puts us in the position where we can have a 4.0 / 5.0 convergence. When we start our Saml / WsFed support, we should have a clear direction on S.IM.

[kevinchalet]

In a perfect world, we would have moved S.IM to FxCore. Tweaked the types, moved additions to 4.6 Desktop etc.

[leastprivilege]

What I really care about is the best library for JWT for .NET going forward.

I don't care about backward compat, WIF 1.0, legacy desktop CLR, SAML or WS-Fed

[ClaytonPeddy]

I agree 100%, develop the best JWT library for .Net going forward. No interest in pushing legacy forward in this area.

[kevinchalet]

"developing the best JWT library" is a bit vague if you don't mention the most important points for you.

IMHO, IdentityModel's best thing is its formidable abstraction, that allows you to issue and validate security tokens without ever tying your own code to a specific implementation or format (SAML/SAML2, JWT or even... CWT :trollface:). Personally, this is exactly how and why I want to use IdentityModel.

Of course, for those who prefer a simpler and lower-level approach, JwtSecurityTokenHandler has to offer a way to easily generate JWT tokens.

[leastprivilege]

Well - I told you what's not important for me. The rest is.

[kevinchalet]

So basically, you'd like something like jose-jwt?

[brentschmaltz]

OK, working on the PR. Goal is to keep JWT clean leaving the WS* stuff where it belongs :-)
I will ping you guys when the PR is live.

[brentschmaltz]

@PinpointTownes yes a strong clean JOSE layer is a goal.

[leastprivilege]

@PinpointTownes yes - but coreclr, xplat and probably more than one guy behind it.

[leastprivilege]

but yeah preferring that over jack of all trades.

[brentschmaltz]

@leastprivilege we are a little team now :-)
Clean is key.

[brentschmaltz]

@leastprivilege @PinpointTownes @tushargupta51 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/333

[brentschmaltz]

fixed with PR: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/333

[tushargupta51]

@brentschmaltz @leastprivilege @PinpointTownes packages pushed out to nightlies - 212181359.

[JohnGalt1717]

If you don't use System.IdentitityModel.Tokens.Jwt 5.1.2 then now you can't use WindowsAzure.MediaServices. Hence you're forcing, care of this disaster, us to choose between OpenIdConnect and Media Services support which is idiotic.

System.IdentityModel.Tokens, System.IdentityModel.Tokens.Jwt and Microsoft.IdentityModel.Protocol.Extensions needs to be fixed to work properly with .NET 4.x without these issues.

[brentschmaltz]

@JohnGalt1717 why does WindowsAzure.MediaServices require System.IdentityModel.Tokens.Jwt 5.1.2?

[JohnGalt1717]

Because the client it uses requires this stuff. So you have automatic updates happening for nuget packages that break your code without any work around because they removed functionality that is still reported there and didn't provide any alternatives what-so-ever.

And somewhere along the line someone at MS forgot to write an article somewhere explaining the changes from 4.x to 5.x to even just replace the JWT token functionality with the new stuff let alone the outright breaks caused that prevent OpenIdConnect from working anymore.

[brentschmaltz]

@JohnGalt1717 to understand better, do you have a list of assemblies that the app is using?

[brentschmaltz]

@JohnGalt1717 I moved this into the 5.2.0 milestone. Can you provide me with the assemblies you are binding to? Also when you say "automatic updates" is that during build OR runtime?

[JohnGalt1717]

Automatic updates: Open Manage Nuget for Solution, choose to update something that depends upon the new version (5.0+) and it will automatically install these versions borking your code in the process. (And because VS.net 2017 doesn't properly update bin/obj when these nuget packages are changed and you build and the temp directory doesn't get cleared properly either, it took me a day and a half to actually get working code again because every time I fixed/changed something back while discovering the below items I didn't know if it worked or not properly without going through and deleting all bin/obj and the temp folder and restarting visual studio 2017 every time (Yes I've reported the vs.net 2017 issues)

The following either cause an update or have issues:

Microsoft.IdentityModel.Logging (1.1.3 or later)
System.IdentityModel.Tokens.Jwt (5.0 or later)
Microsoft.IdentityModel.Tokens (5.1.2 or later)
Microsoft.IdentityModel.Protocol.Extensions (1.0.4 or later has issues)

(as an aside this is a complete mess with this naming and you should be fixing that too because you have overlapping functionality between these .dlls and the standard System.xxx that are part of the .net framework and it is VERY confusing to try and figure out what you're supposed to use and it is VERY easy to end up cross linking these and causing problems that only crop up at run-time as a result)

[brentschmaltz]

@JohnGalt1717 can you please point me to the WindowsAzure.MediaServices package that you are referring to.

IdentityModel extensions 4.x are not compatible with 5.x. That is the short story. You can't mix and match the parts.

4.x was written as a standard .net451 for OpenIdConnect, WsFed and OAuth support before any CoreFx and .net standard existed.
5.x was written as an identity layer to provide support for the same protocols (although, wsFed is still in the works) for .net451 and netstandard 1.4.

I agree the naming of System.IdentityModel.Tokens.Jwt is a bit confusing with all the others starting with Microsoft.IdentityModel as a way to distinguish them from CoreFx. This assemblies, while supported by Microsoft are not formally part of CoreFx.

[JohnGalt1717]

Sure, but try and use nuget and have it not get updated by accident or block updates to dlls.

The Azure nuget package was updated and it no longer requires V5.1.2 but it did just long enough for me to waste a ton of time.

V5 shouldn't even be in the same nuget package and should never be offered to .NET full. It completely breaks OpenID Connect with no alternative thus is not a replacement for previous versions. This should be fixed at the least by making a new nuget package for it for .net core stuff if it isn't going to have full compatibility or a viable alternative to V4.

[brentschmaltz]

@JohnGalt1717 https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/595 will include a future release of Katana that uses 5.2.0.