search

Azure / azure-sdk-for-net

This repository is for active development of the Azure SDK for .NET. For consumers of the SDK we recommend visiting our public developer docs at https://learn.microsoft.com/dotnet/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-net.
MIT License
5.49k stars 4.81k forks source link

[QUERY] Azure.MixedReality.Authentication transitive dependency on Newtonsoft.Json 10.0.1 #29617

Closed craigktreasure closed 1 year ago

craigktreasure commented 2 years ago

Library name and version

Azure.MixedReality.Authentication

Query/Question

I'm the current maintainer for Azure.MixedReality.Authentication. It currently depends on System.IdentityModel.Tokens.Jwt 5.4.0 to perform JWT validation. That version of System.IdentityModel.Tokens.Jwt also depends on Newtonsoft.Json 10.0.1, which contains a vulnerability that gets flagged by scanners. The version of System.IdentityModel.Tokens.Jwt is dictated here. Newer versions (6.x) of System.IdentityModel.Tokens.Jwt have actually removed the Newtonsoft.Json dependency. Is there any interest in updating the common version, should I override the version I use in Azure.MixedReality.Authentication, or do you have suggestions for a replacement for this functionality?

@weshaggard

Environment

No response

pallavit commented 2 years ago

/cc: @heaths for information.

heaths commented 2 years ago

Are there any other project dependencies on System.IdentityModel.Tokens.Jwt? If not, I would upgrade that to remove the Newtonsoft.Json dependency; otherwise, you can mitigate the vulnerability without upgrading Newtonsoft.Json by setting the MaxDepth of any JsonReader derivative you create. If you don't create a JsonReader in any way, let me know and I can mark the scanner issue as invalid.

craigktreasure commented 2 years ago

I see usage by the following:

I don't create a JsonReader directly, so it's not something I can fix in that way.

Internally, we get scanned by Component Governance and it shouts if you have a dependency on Newtonsoft.Json < 13.0.1. In this case, it would be nice to get rid of it so that downstream projects aren't impacted because of this library.

I'd propose I override the version for my project for now. Thoughts?

heaths commented 2 years ago

I don't recommend overriding the version dependency. It would be best to upgrade the dependency to the minimum version that eliminates the dependency on Newtonsoft.Json. If CI fails, we can instead mark the vulnerability as not used directly.

craigktreasure commented 2 years ago

Can you tell me more about the hesitation with overriding?

heaths commented 2 years ago

I've included you in an internal thread as to why.

heaths commented 2 years ago

But, in general, we don't want SDKs using @OverrideVersion unless absolutely necessary because it breaks central management of dependencies, which is done to improve compatibility between our SDKs and other partners like the Az PowerShell modules.

craigktreasure commented 2 years ago

Per your suggestion, i've updated the common version since I believe we're the only Track 2 SDK shipping a dependency on that library. See #29822.