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.25k stars 4.58k forks source link

Consider removing dependency on System.Security.Cryptography.ProtectedData #45217

Closed m-redding closed 1 month ago

m-redding commented 1 month ago

Currently we have a dependency on version 4.7.0 of System.Security.Cryptography.ProtectedData in Packages.Data.Props. Upgrading this to 6.0.0 results in warnings, because these APIs only work on Windows operating systems, and they added these warnings in .NET 5. From what I can tell, even though we aren't getting warnings in 4.7.0, they are still not safe for use with non-Windows operating systems.

We should consider removing this dependency because it's only used in these two places:

Related

jsquire commented 1 month ago

The main reason that this assembly is used is for the local encryption of the test environment file for local runs, which is only done on Windows. I think that's what the TestFramework tests are checking. Since this is a critical part of the developer loop for most Azure SDK developers, I think we want to leave this in the test framework and ignore the warnings. @JoshLove-msft: Anything there that you disagree with?

@christothes or @schaabs would be the folks with the best insight on the MsalCacheReader.

christothes commented 1 month ago

I don't recall when we ever used MsalCacheReader, but it seems unreferenced as @m-redding mentioned. Seems like we could remove it.

christothes commented 1 month ago

Here's a PR that moves the reference to test projects only and removes it from Identity https://github.com/Azure/azure-sdk-for-net/pull/45232 . I didn't upgrade the version, this is just to validate the movement.

m-redding commented 1 month ago

@christothes Thank you - that would work great!