-
Notifications
You must be signed in to change notification settings - Fork 401
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
Breaking changes introduced from Microsoft.IdentityModel.JsonWebTokens 7.3.1 to 7.4.0 #2514
Comments
I don't know if this is the same thing, but we also see a breaking change from 7.3.1 to 7.4.0:
This code works fine with 7.3.1, fails on 7.4.0 with the above message:
|
We also see breaking change when updating from 7.3.1 to 7.4.0
|
It seems the The following test works on 7.3.1, but fails on 7.4.0. [Test]
public void OpenIdConnectConfiguration_FromJson_Should_Work()
{
var json = @"{""issuer"": ""http://localhost"",""jwks_uri"": ""http://localhost""}";
var config = OpenIdConnectConfiguration.Create(json);
config.JwksUri.Should().Be("http://localhost");
} |
We have the same issue. Specifically we are seeing the "Cannot redirect to the authorization endpoint, the configuration may be missing or invalid." exception after the upgrade. Is there a workaround? |
@chris-briddock @rmmason @rlf @yborektsioglou looking into it. @rlf thanks for the repo. |
@yborektsioglou it looks like we are not obtaining the security keys. |
@rmmason @christophwille this error doesn't seem related to security keys, but redirecting to the OIDC authorization endpoint. I will look there. |
@rmmason do you have a stack trace? |
@christophwille can you provide us with a little bit of additional source code so i can run the repo? |
That is about as much as I can show... underlying (hidden by another level) is actually Salesforce authN which has a specific issue in that the defaults for Scope and ResponseType need to be set as shown below: services.AddAuthentication(opt =>
{
opt.DefaultScheme = CookieAuthenticationDefaults.AuthenticationScheme;
opt.DefaultChallengeScheme = OpenIdConnectDefaults.AuthenticationScheme;
})
.AddCookie(CookieAuthenticationDefaults.AuthenticationScheme, options =>
{
options.SlidingExpiration = true;
options.ExpireTimeSpan = TimeSpan.FromMinutes(20);
})
.AddOpenIdConnect(OpenIdConnectDefaults.AuthenticationScheme, opt =>
{
opt.Authority = "our-authority";
opt.ClientId = "our-clientid";
opt.ResponseType = OpenIdConnectResponseType.Code; // IdToken not supported in our case
opt.Scope.Clear(); // ctor adds 'profile' too, but that is not supported by our case
opt.Scope.Add("openid");
opt.CallbackPath = "/api/login/callback"; |
Hi @brentschmaltz, Thanks for looking in to this for us. Stack trace is as follows: at Microsoft.AspNetCore.Authentication.OpenIdConnect.OpenIdConnectHandler.d__19.MoveNext() This is actually something one of my colleagues is working on. I'm a bit tied up at the moment but I'll attempt to recreate a repro project as soon as I get a moment. |
Hi @brentschmaltz, Thanks for looking into this. We're hitting the token endpoint. I'm a bit tied up with other stuff at the moment but I see if I can provide more info |
I've tested this with Duende IdentityServer. I can confirm that upgrading a simple MVC code flow client to use Wilson 7.4.0 breaks OpenID Connect login.
I used a debugger to check the values and it passes Repro is available at https://github.com/DuendeSoftware/IdentityServer/tree/anders/wilson-7.4.0. Run hosts\main project and then run clients\MvcCode on the same time. Try clicking "Secure" in the client app and login (alice/alice) to make it fail. |
I encountered this upgrading to ASP.NET Core 8. Disappointing bug. It appears the cause is the OpenIdConnectConfigurationSerializer here is too eagerly advancing to the next JSON token after reading a value in the document body. So for example with document: {
"issuer": "https://localhost",
"authorization_endpoint": "https://localhost/connect/authorize",
...
} After the issuer value is read, the current token is the "authorization_endpoint" property when it shouldn't be. The while loop advances to the next token which becomes the authorization endpoint value, not the property name. Please add more thorough tests!...Lost a full day tracking down NuGet upgrade path and root cause. |
@AndersAbel long time, thanks for the repo. |
@dahovey can tell us the version of all IdentityModel assemblies you are using? |
@brentschmaltz Actually I added an explicit reference to version
|
@brentschmaltz Thanks for your assistance |
you need to unlist v7.4.0. It causes a lot of issues. Then bump the major version and publish again. |
We also see a problem with |
Can you share the transitive dependencies?
|
It's indeed a packages mismatch issue: openiddict/openiddict-core#2033 (comment). The issue was introduced by 051d164: you changed the internal implementation of a static helper in The thing is, if users only update It's not the first time we're seeing bugs caused by changes in your internal helpers and it's getting a bit ridiculous at this point. You should really consider embedding your helpers in each assembly to avoid such issues, as I had suggested last time: #2059 (comment) |
Same issue here Seems pretty clear the issue is this
Updating Microsoft.IdentityModel.Protocols.OpenIdConnect to 7.4.0 fixes the issue. Why is the document deserialized manually? surely this can't be any more secure? And bugs like this just highlight why you shouldn't do manual deserialization. (I've removed all the non-Microsoft, non-System packages from the output...)
|
@AndersAbel thanks for the great repo. When i ran your app, i saw the error as you said. Below would pull in 7.4.0 (latest) and also pull Microsoft.IdentityModel.Token 7.4.0, which will cause parsing of OpenIdConfiguration to fail as Microsoft.IdentityModel.Protocols.OpenIdConnect will be at version 7.0.0. On the host app, I clicked on 'discovery document' now we have recent copy of IdentityServer metadata, we will add another unit test alongside Google and AzureAD to ensure against regressions of IdentityServer. |
@m-wild you can see that you have different versions of Microsoft.IdentityModel.Tokens (7.4.0), Microsoft.IdentityModel.Protocols (7.0.0) and Microsoft.IdentityModel.Protocols.OpenidConnect (7.0.0). The low-level serialization primitives are in M.IM.Tokens. |
@kevinchalet i agree with your comment on #2059 (comment) However, last year i was not able to work (health issues) and may have missed your comment. The good news is we are going to add a regression test with IdentityServers metadata. |
@brentschmaltz yep, understand that is the fix. It's disappointing that this version coupling isn't expressed by the Nuget package dependencies. |
Thanks everyone for all your input and help with root causing this. We will look into analyzers to help detect this issue as early as possible. |
@m-wild we are going to fix the references. As Jennyf19 mentioned, we are going to get on this. |
I agree with @m-wild. Setting the Nuget package dependency to be an exact match between the IdentityModel packages would be the preferred solution. |
No worries. I hope you're doing well now 😃
More coverage is always good, but in this case, I'm not sure a classical unit test would have caught that since it requires a package versions mismatch that you wouldn't have in a typical tests project. I really like the Roslyn analyzers approach. Let's hope it will materialize before the next breaking changes made to your internal helpers 😄 |
I'm running against this issue when I was upgrading from 7.0.3 to 7.5.1 (yes I know, quite an upgrade in one step) along with other NuGets, after this is started to have |
I just ran into this issue as well and wasted time debugging. How frustrating. |
For me it did not solve the issue |
@mr-davidc I misread your comment, I thought you were referring to |
…thing is provided by application overrides. Note: 7.3.1 of various Tokens libraries used for good measure because of AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2514
I've just hit this problem for the second time in a couple of months. I'm confused (and frustrated):
|
I believe I hit a similar issue but in version 6. For us we are probably going to update Microsoft.aspnetcore.authentication.JwtBearer to 6.0.33 (the most recent for version 6) which installs version 6.35.0 of Microsoft.identitymodel.protocols.openidconnect, system.identitymodel.tokens.jwt and Microsoft.identitymodel.jsonwebtokens and seems to work on our dev site. It sounds like a similar versioning issue anyway. |
I am using version 7.3.1 in production and dependabot has upgraded from 7.3.1 to 7.4.0 but there is breaking changes.
Please could you take a look at this: https://github.com/chris-briddock/ChristopherBriddock.Identity/pull/62
https://dev.azure.com/chris1997/ChristopherBriddock.Identity/_build/results?buildId=798&view=logs&j=7e6a3fb7-dbfe-5169-4db8-92b72295ba6c&t=45683e7c-5c24-5fa1-4844-9f376a3fcc8a&l=702
Expected behavior
All tests pass.
Actual behavior
Tests that require this library fail.
The text was updated successfully, but these errors were encountered: