Skip to content
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

Updated System.IdentityModel.Tokens.Jwt and Microsoft.IdentityModel.JsonWebTokens to latest to address CVEs #53

Merged
merged 2 commits into from
Feb 26, 2024

Conversation

chgl
Copy link
Contributor

@chgl chgl commented Jan 13, 2024

What issue does this PR address?

Resolves CVE-2024-21319

Important: Any code or remarks in your Pull Request are under the following terms:

If You provide us with any comments, bug reports, feedback, enhancements, or modifications proposed or suggested by You for the Software, such Feedback is provided on a non-confidential basis (notwithstanding any notice to the contrary You may include in any accompanying communication), and Licensor shall have the right to use such Feedback at its discretion, including, but not limited to the incorporation of such suggested changes into the Software. You hereby grant Licensor a perpetual, irrevocable, transferable, sublicensable, nonexclusive license under all rights necessary to incorporate and use your Feedback for any purpose, including to make and sell any products and services.

(see our license, section 7)

@sievajetnamdar
Copy link

sievajetnamdar commented Feb 8, 2024

Can we merge this already? Im waiting for this fix😄 @josephdecock

@leastprivilege
Copy link
Member

You can simply update the package version in your host. No need to wait for other libraries to update their dependencies.

@carlin-q-scott
Copy link

@leastprivilege Yes, we can do that. And I did. But there's 10s of thousands of consumers of this package. Wouldn't it be more efficient for your company to address the vulnerable dependency? It has been over a month since this PR was created for a CVE 6.8 vulnerability.

@brockallen
Copy link
Member

There is precedence for our approach: aspnet/AspNetKatana#522 (comment)

@carlin-q-scott
Copy link

Yes, people do that because we can't wait months for the package maintainer to merge their PR.

@brockallen
Copy link
Member

brockallen commented Feb 14, 2024

The browbeating tone of your message makes it difficult to have an unimpassioned and productive conversation. Please keep it professional.

Back on topic, can you show other examples from Microsoft where they have done such a change? We've taken their approach and guidance on this topic for years. In our experience this is not practical to patch every time a such a dependency changes, especially given the various dependency chains. But if there's new and/or updated guidance, we're more than happy to consider it.

@goldsam
Copy link

goldsam commented Feb 15, 2024

Isn't this why dependabot is a thing? Just click the merge button and tada🪄

Some companies don't use a dependabot and will be exposed to vulnerabilities. As Uncle Ben once said, "with great power comes great responsibility." Lesser known is quote from Aunt May: "Responsible young men update their vulnerable package dependencies." 😉

@goldsam goldsam mentioned this pull request Feb 15, 2024
@brockallen
Copy link
Member

I appreciate everyone's passion here, but we're in a difficult position here. I'll try to explain below.

Isn't this why dependabot is a thing? Just click the merge button and tada🪄

Unfortunately, it's not that simple. Given the historic issues with diamond dependency problems specifically related to code quality and frequent breaking changes in minor and patch updates to Wilson, we took the approach a long time ago to only reference the exact versions of downstream dependencies that were the same ones as targeted by the base version of .NET and/or ASP.NET we were targeting (e.g. .NET 6 or .NET 8).

This is why, in addition to the guidance we sought from Microsoft about on how to handle these issues, and the fact that hosts using our library have a solution to getting the updated version of the dependency, this hasn't been the highest priority.

We also have not had the time to actually verify that the newer version hasn't broken something else (as they have often done in the past).

So hopefully this provides some context. We do have time on our upcoming schedules to do feature work here, and this will be one of the topics we look into.

Thanks again.

@josephdecock josephdecock merged commit e70d4cc into DuendeSoftware:main Feb 26, 2024
5 checks passed
@chgl chgl deleted the updated-deps-to-address-cve branch February 26, 2024 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants