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

Question

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

benleduc opened this issue 5 years ago · 19 comments

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:

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!

Answer 1 · 2019-09-25T15:19:41.000Z

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

Answer 2 · 2019-09-25T15:22:25.000Z

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

Answer 3 · 2019-09-25T15:31:46.000Z

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?

Answer 4 · 2019-09-27T13:00:18.000Z

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.

Answer 5 · 2019-10-16T08:54:12.000Z

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

Answer 6 · 2019-10-16T10:51:32.000Z

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

Answer 7 · 2020-05-29T12:54:57.000Z

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?

Answer 8 · 2020-06-01T13:28:07.000Z

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

Answer 9 · 2020-09-10T09:46:56.000Z

@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).

Answer 10 · 2020-09-10T10:25:36.000Z

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

Answer 11 · 2020-09-10T12:00:54.000Z

@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.

Answer 12 · 2020-09-10T12:20:12.000Z

@aiwangmicrosoft FYI

Answer 13 · 2020-09-16T08:22:25.000Z

@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).

Answer 14 · 2020-09-18T07:48:13.000Z

@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

Answer 15 · 2020-10-30T18:57:49.000Z

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".

Answer 16 · 2020-11-02T09:38:47.000Z

@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

Answer 17 · 2020-11-02T13:11:19.000Z

Sure, let's fix it in MSAL.

Answer 18 · 2020-11-30T13:41:03.000Z

@bgavrilMS : did we fix it in MSAL?

Answer 19 · 2020-11-30T13:41:33.000Z

Closing as transfered to MSAL.