AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.05k stars 396 forks source link

[Bug] Well-known openId configuration parsing is causing IDX1050 errors #2523

Open AlexandreBossard opened 6 months ago

AlexandreBossard commented 6 months ago

Which version of Microsoft.IdentityModel are you using? Identity.Models 7.4

Where is the issue?

After updating to the latest 7.4 release, all my app tests failed due to invalid jwt token authentication. reverting to 7.3.1 works as expected. No code change between the package upgrade has been made.

We use asymmetric jwt token signature with a custom mock identity server that provide the /.well-known and jwks_uri endpoints. We notice that, even so the well-known endpoint is called when validating a token, the jwks_uri one is not.

Repro Here is the exact endpoint code that worked previously:

app.MapGet("/.well-known/openid-configuration", () =>
            new
            {
                Issuer = issuerUrl,
                jwks_uri = new Uri(new(issuerUrl), ".well-known/jwks.json").ToString(),
            });

which produce the following

$ http --format json.sort_keys:false http://localhost:5001/.well-known/openid-configuration
HTTP/1.1 200 OK
Content-Type: application/json; charset=utf-8
Date: Wed, 13 Mar 2024 12:10:06 GMT
Server: Kestrel
Transfer-Encoding: chunked

{
    "issuer": "http://localhost:5001",
    "jwks_uri": "http://localhost:5001/.well-known/jwks.json"
}

All seems good for all I know.

Now watch this! If I swap the 2 lines above like this:

app.MapGet("/.well-known/openid-configuration", () =>
            new
            {
                jwks_uri = new Uri(new(issuerUrl), ".well-known/jwks.json").ToString(),
                Issuer = issuerUrl,
            });

I get

$ http --format json.sort_keys:false http://localhost:5001/.well-known/openid-configuration
HTTP/1.1 200 OK
Content-Type: application/json; charset=utf-8
Date: Wed, 13 Mar 2024 11:54:19 GMT
Server: Kestrel
Transfer-Encoding: chunked

{
    "jwks_uri": "http://localhost:5001/.well-known/jwks.json",
    "issuer": "http://localhost:5001"
}

And everything works ! 2 things I get from that:

Expected behavior Asymmetric Token validation to work regardless of the json key order.

Actual behavior It fails to parse the json document and do not fetch keys.

Possible solution I've pinned the dependency to the last known working version the 7.3.1

Additional context / logs / screenshots / links to code

dotnet is great, it allows us to specify the key order of the serialized json. But that's not the case of Python for exemple. Also, Json object property serialization order is, sadly, unspecified. You must not rely on a specific order for parsing / validation.

I see a lot of code change around json parsing in the last release (e.g. https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/commit/051d164e3c025a0d7276f1d6acf38c902a4893fc ). I suspect a regression slipped in there.

dolly22 commented 6 months ago

Check if you have consistent versions of referenced Microsoft.IdentityModel.* packages. I think my issue was the same - every second property in configuration json was ignored. In my case the problem was mixed versions of Microsoft.IdentityModel.Tokens/7.4.0 vs Microsoft.IdentityModel.Protocols.OpenIdConnect/7.1.2.

AlexandreBossard commented 6 months ago

Awesome! It has fixed it !

dotnet add package Microsoft.IdentityModel.Protocols.OpenIdConnect --version 7.4.0

It is now a direct dependency instead of a transitive one. I have no way to infer what pulls it, due to the lack of tooling.

Now, if this is a problem that cause regression, a packaging fix still needs to be done, somewhere.

johnjardinemd commented 6 months ago

Check if you have consistent versions of referenced Microsoft.IdentityModel.* packages. I think my issue was the same - every second property in configuration json was ignored. In my case the problem was mixed versions of Microsoft.IdentityModel.Tokens/7.4.0 vs Microsoft.IdentityModel.Protocols.OpenIdConnect/7.1.2.

Thank you for this.

I was using packages:

    <PackageReference Include="Microsoft.IdentityModel.JsonWebTokens" Version="7.3.1" />
    <PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="7.3.1" />

    <PackageReference Include="Duende.AccessTokenManagement" Version="2.1.0" />
    <PackageReference Include="Duende.AccessTokenManagement.OpenIdConnect" Version="2.1.0" />

And previously didn't have Microsoft.IdentityModel.Tokens or Microsoft.IdentityModel.Protocols.OpenIdConnect installed. When I upgraded my Microsoft.IdentityModel.JsonWebTokens and System.IdentityModel.Tokens.Jwt packages today to 7.4.0, I encountered runtime errors.

Installing the latest versions of Microsoft.IdentityModel.Tokens and Microsoft.IdentityModel.Protocols.OpenIdConnect resolved erverything.

EDIT: Like @AlexandreBossard, only Microsoft.IdentityModel.Protocols.OpenIdConnect was required.

jennyf19 commented 6 months ago

@AlexandreBossard looks like this issue is resolved?

AlexandreBossard commented 6 months ago

Yes but no. There is packaging issue somewhere. If the Microsoft.IdentityModel.JsonWebTokens broke ABI somewhere, it should be reflected in the packaging dependencies (I doubt nuget is capable of doing that), or at least in the release note. Automatic updating tool will keep triggering that bug and create time sink for unaware developers.

AlexandreBossard commented 6 months ago

So, after looking a bit more in to this, Microsoft.AspNetCore.Authentication.JwtBearer 8.0.2 pulls Microsoft.IdentityModel.Protocols.OpenIdConnect 7.1.2, and everything else as indirect dependency. My issue was having a Microsoft.IdentityModel.JsonWebTokens direct dependency, that in this instance seems to have silently breaks runtime ABI.

If that's the case 7.4.0 probably should have been a new major release.

Also, AspNetCore 8 did not update their dependency on Microsoft.IdentityModel.* since 7.1.2, so if I want the last and best version, I only need a direct dependency on Microsoft.IdentityModel.Protocols.OpenIdConnect.

This is convoluted and not documented.

KieranFoot commented 6 months ago

I just came across this error with a client and rolling back to Microsoft.IdentityModel.JsonWebTokens @ 7.1.3 fixed it for me.

olegd-superoffice commented 6 months ago

So, if I understand it correctly, Microsoft.IdentityModel.Protocols.OpenIdConnect 7.1.2 has version constraint on System.IdentityModel.Tokens.Jwt (>= 7.1.2) but it should also have constraint (< 7.4)

AlexandreBossard commented 6 months ago

Yes, Microsoft.IdentityModel.Protocols.OpenIdConnect@7.1.2 does not work with Microsoft.IdentityModel.JsonWebTokens@7.1.4. But, IIUC, you would have to release an Microsoft.IdentityModel.Protocols.OpenIdConnect@7.1.3 to fix the dependency constraints.

A-Stapleton commented 5 months ago

We had a similar issue, adding a direct reference to the *.Protocols.OpenIdConnect package solved the issue. The breaking change was here I think https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/2508/files probably just needs a note in the release notes about the breaking change and what to do

martinb69 commented 5 months ago

I think this PR may have solved the issue: https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/2548

olegd-superoffice commented 5 months ago

@martinb69 Thanks for this. But it looks like the changes from this PR are only included in Microsoft.IdentityModel.Protocols.OpenIdConnect@7.5.1 while the current latest version of Microsoft.AspNetCore.Authentication.OpenIdConnect@8.0.3 is still referencing Microsoft.IdentityModel.Protocols.OpenIdConnect@7.1.2 so Microsoft.IdentityModel.Protocols.OpenIdConnect@7.5.1 still needs to be referenced directly to make it work?

AlexandreBossard commented 2 weeks ago

This is still an issue, that broke our dependency update routine today (we keep creating project missing that non obvious dependency). Any news ?

pmaytak commented 1 week ago

The solution is being worked on in #2513. Expected to go into the next release.