AzureAD / azure-activedirectory-library-for-dotnet

ADAL authentication libraries for .net
http://aka.ms/aaddev
MIT License
357 stars 214 forks source link

.NETFramework 4.5 nuget package has a dependency but the assembly doesn't reference those dependencies #1659

Closed benleduc closed 3 years ago

benleduc commented 4 years ago

Which Version of ADAL are you using ? Note that to get help, you need to run the latest preview or non-preview version For MSAL, please log issues to https://github.com/AzureAD/microsoft-authentication-library-for-dotnet ADAL 5.2.2

Which platform has the issue? net45

What authentication flow has the issue? Not relevant

Other? - please describe;

Is this a new or existing app? a. The app is in production, and I have upgraded to a new version of ADAL -->

Repro Issue: The net45 Adal assembly doesn't reference any nuget package related references but the nuget package defines 2 dependencies System.Net.Http (>= 4.3.4) and System.Private.Uri (>= 4.3.1) that get installed when installing the Adal nuget package. Since the Adal package references the .net framework version of System.Net.Http (4.0.0.0), it creates issues when executing the code which tries to load the System.Net.Http from the nuget package instead of the one from the .net framework.

Repro steps: 1) Install latest nuget package 5.2.2 https://www.nuget.org/packages/Microsoft.IdentityModel.Clients.ActiveDirectory/5.2.2 into a net45 project.

Not relevant

Expected behavior No nuget packages references added.

Actual behavior System.Net.Http (>= 4.3.4) and System.Private.Uri (>= 4.3.1) nuget packages installed and causing issues when executing the code.

Possible Solution Remove the nuget package dependencies for the net45 target.

Additional context/ Logs / Screenshots Add any other context about the problem here, such as logs and screebshots. Logging is described at https://github.com/AzureAD/azure-activedirectory-library-for-dotnet/wiki/Logging-in-ADAL.Net. Don't post logs containing PII on GitHub!

image

image

jmprieur commented 4 years ago

@bgavrilMS : do you have an idea? should we have filters depending in the platforms?

bgavrilMS commented 4 years ago

We had to specify those explicit versions because the default Net.Http and Private.Uri have security issues.

jmprieur commented 4 years ago

I understant that @bgavrilMS : but should we reference the DLL on .NET FW, and use the NuGet packages in the other platforms? or are we doing the right things?

bgavrilMS commented 4 years ago

Looking into this. I believe that on .NET classic we should not have to do this, because .NET will pick up this DLL from the GAC and it is the user's responsability to have a .NET runtime installed (Windows will probably have updated the runtime to ensure System.Private.Uri.dll is not affected by any security issues).

I am looking at solutions for .net core and .net standard.

jmprieur commented 4 years ago

@bgavrilMS ; Do we agree that this one if fixed?

bgavrilMS commented 4 years ago

Yes, I removed this from .net 45 since it does not affect it.

simendsjo commented 4 years ago

Hi, is this really fixed?

The nuget library still references the nuget package of System.Net.Http for netframework45, which causes problems for us.

I notice the group for net45 in the csproj file doesn't reference the version, but there exists a reference further down in the file which references this version, could that be the problem?

bgavrilMS commented 4 years ago

Hi @simendsjo - ADAL is on a deprecation path and we recommend you to use MSAL instead https://github.com/AzureAD/microsoft-authentication-library-for-dotnet

c-s-n commented 4 years ago

@bgavrilMS I really suggest to reopen this ticket. The System.Net.Http story with its out-of-band package is a nightmare and causes a lot of assembly binding problems. See the official recap on Github.

Basically, you should remove the all-target NuGet package reference, since you correctly already reference the framework version for net45 and already reference the package for netstandard1.3.

I also don't think the ADAL deprecation argument is valid, since Microsoft.Azure.Services.AppAuthentication depends on it, which in turn is depended on by the Azure SDK for .NET (e.g. WindowsAzure.ServiceBus and Microsoft.Azure.ServiceBus). Also your argument in #1617 doesn't seem to still apply? At least I found NSubstitute to not use System.Net.Http, and Microsoft.Azure.KeyVault to only use it for NetStandard (which you should treat separately, see above).

jmprieur commented 4 years ago

@c-s-n : the Azure SDK now uses MSAL.NET. See https://www.nuget.org/packages/Azure.Identity/

c-s-n commented 4 years ago

@jmprieur looking at the official Azure SDK page, e.g. for Service Bus, Azure.Messaging.ServiceBus is still in preview. The current Microsoft.Azure.ServiceBus depends on Microsoft.Azure.Services.AppAuthentication, which in turn depends on Microsoft.IdentityModel.Clients.ActiveDirectory.

jmprieur commented 4 years ago

@aiwangmicrosoft FYI

volleyms commented 4 years ago

@jmprieur: any chance that the reference to the System.Net.Http NuGet package is removed or that you introduce a net472 target which does not contain it... in the short term? The current situation makes it impossible to build cleanly for .NET Framework 4.7.2, cause unnecessarily including the System.Net.Http NuGet package (which can finally be overridden with binding redirects, but nevertheless), and even worse, makes MS Build build the including project each time again though successfully built before cause it struggles with conflicting dependencies all the time (which finally raises local building/testing/debugging times inacceptably).

jmprieur commented 4 years ago

@volleyms : we won't update ADAL.NET any longer. You are now advised to move to MSAL.NET: https://techcommunity.microsoft.com/t5/azure-active-directory-identity/update-your-applications-to-use-microsoft-authentication-library/ba-p/1257363

simendsjo commented 3 years ago

You say you don't maintain it, but a new version was shipped June 30 with the same bug. We've patched the nuspec and added it to our own local cache to avoid the bug, but the next time you push an updated version and we're upgrading, we'll get the same problems all over again.

Is there a problem with fixing this bug? For all libraries I've encountered with this bug, the fix is to remove the reference for netframework to avoid this million dollar mistake. God knows how many hours people are wasting on the System.Net.Http issue, and as long as you don't fix the package, it will continue to be a problem. Proprietary closed software like EPiServer is using this corrupt package, which makes it really difficult to just "use the new library".

jmprieur commented 3 years ago

@henrik-me @bgavrilMS do we want to reopen this issue (to be sure to address this if we ever provide a hotfix of ADAL.NET in the future?) cc: @jennyf19 @trwalke @neha-bhargava

bgavrilMS commented 3 years ago

Sure, let's fix it in MSAL.

jmprieur commented 3 years ago

@bgavrilMS : did we fix it in MSAL?

jmprieur commented 3 years ago

Closing as transfered to MSAL.