[API Proposal]: remove dependency on System.Drawing.Common from System.Configuration.ConfigurationManager

[abelykh0]

Background and motivation

.Net 6 introduced a breaking change in the Windows compatibility (see https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only). The motivation is to make System.Runtime.Caching to not require dependency on "not recommended" System.Drawing.Common (via System.Windows.Extensions).

API Proposal

Somehow hide the dependency on System.Drawing.Common from System.Runtime.Caching, even if System.Drawing.Common it is used internally.

API Usage

N/A

Alternative Designs

No response

Risks

Backward compatibility

[ghost]

Tagging subscribers to this area: @dotnet/area-system-drawing
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

.Net 6 introduced a breaking change in the Windows compatibility (see https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only). The motivation is to make System.Runtime.Caching to not require dependency on "not recommended System.Drawing.Common (via System.Windows.Extensions).

API Proposal

Somehow hide the dependency from System.Runtime.Caching, even if System.Drawing.Common it is used internally.

API Usage

N/A

Alternative Designs

No response

Risks

Backward compatibility

Author:	abelykh0
Assignees:	-
Labels:	api-suggestion, area-System.Drawing, untriaged
Milestone:	-

[abelykh0]

Please look at #62649 for more comments. It is closed, because it is not possible to remove dependency on System.Drawing.Common from System.Windows.Extensions.

[ericstj]

Here the dependency is coming from

runtime/src/libraries/System.Runtime.Caching/src/System.Runtime.Caching.csproj

Line 81 in 53d5288

<ProjectReference Include="$(LibrariesProjectRoot)System.Configuration.ConfigurationManager\src\System.Configuration.ConfigurationManager.csproj" />

System.Configuration.ConfigurationManager -> System.Security.Permissions -> System.Windows.Extensions -> System.Drawing.Common

The dependency from ConfigurationManager on down is public API, so it cannot be broken without impacting binary compatibility. The dependency between Caching and ConfigurationManager appears to be implementation only, so this could be hidden with PrivateAssets="Compile".

@abelykh0 -- you may not want to expose a dependency on System.Runtime.Caching either. It was ported for .NETFramework compatibility and its dependencies reflect that. Any reason you can't mark it with PrivateAssets="Compile" at the point of reference to Caching? Are you exposing it in your public API?

[abelykh0]

@abelykh0 -- you may not want to expose a dependency on System.Runtime.Caching either. It was ported for .NETFramework compatibility and its dependencies reflect that.

What is the correct assembly to use instead of System.Runtime.Caching? We need only Linux and Windows support.

[ericstj]

Here's what the docs say. cc @davidfowl @StephenMolloy

Microsoft.Extensions.Caching.Memory/IMemoryCache (described in this article) is recommended over System.Runtime.Caching/MemoryCache because it's better integrated into ASP.NET Core. For example, IMemoryCache works natively with ASP.NET Core dependency injection.

Use System.Runtime.Caching/MemoryCache as a compatibility bridge when porting code from ASP.NET 4.x to ASP.NET Core.

Here's some interesting discussion from when that doc was created: dotnet/AspNetCore.Docs#8478 (comment)

[abelykh0]

Thanks! I will consider migrating then.

[AraHaan]

Here is a fun one: System.Configuration.ConfigurationManager -> System.Security.Permissions -> System.Security.AccessControl (which the package reference could be removed on the package version of System.Security.Permissions)

Why? because I run into this issue with SqlClient referencing S.C.CM 5.0.0, which references S.S.P 5.0.0 which references S.S.AC 5.0.0 which is a part of the runtime and ref packs so it should be using the version from there since S.S.P is not in the base runtime for some reason (along with S.C.CM). This happens on my local build of SqlClient where I added an .NET 6 target to it to reduce dependencies.

I guess an option would be to see if we can remove the dependency of S.S.P in S.C.CM.

[AraHaan]

Edit: it seems S.S.P is for Code Access Security which can be removed.

[AraHaan]

cc: @dotnet/area-system-configuration Would it be ok if I open an PR to remove S.S.P from S.C.CM?

[ghost]

Tagging subscribers to this area: @dotnet/area-system-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

.Net 6 introduced a breaking change in the Windows compatibility (see https://docs.microsoft.com/en-us/dotnet/core/compatibility/core-libraries/6.0/system-drawing-common-windows-only). The motivation is to make System.Runtime.Caching to not require dependency on "not recommended" System.Drawing.Common (via System.Windows.Extensions).

API Proposal

Somehow hide the dependency on System.Drawing.Common from System.Runtime.Caching, even if System.Drawing.Common it is used internally.

API Usage

N/A

Alternative Designs

No response

Risks

Backward compatibility

Author:	abelykh0
Assignees:	-
Labels:	api-suggestion, area-System.Configuration, area-System.Runtime.Caching
Milestone:	-

[NickCraver]

For what it's worth, we're getting questions about this due to CVEs in System.Drawing.Common when we're using performance counters upstream (System.Diagnostics.PerformanceCounter => System.Configuration.ConfigurationManager) - admittedly this dependency chain just going by names doesn't make a lot of sense to me either, but it's present even in the latest versions: StackExchange/StackExchange.Redis#2283

[AraHaan]

Perhaps one should comb through all of the libraries in this repo for code that uses CAS (Code Access Security) so they can be removed (and their dependency on S.S.P) so that way they do not use a feature that afaik are no-ops anyway.

If they reference a nuget package version of the libraries from this repository in .NET Framework apps, then it could be conditionally only to use it from those frameworks, but I feel that it should be the only place where CAS apis should be called (under #if NETFRAMEWORK).

[ericstj]

We've already removed nearly all usage of System.Security.Permissions in the dotnet/runtime libraries.

The two cases remaining are DirectoryServices and ConfigurationManager and these are for public API.

We could try to push the public API down into System.Security.Permissions, type forward to it, then keep the dependency on System.Security.Permissions private (IOW dangling from Package graph perspective). This would allow for retaining binary compatibility with existing API but trimming S.S.P out of the package graph. We do have some precedent for this in that we have dangling dependencies for typeforwards elsewhere (eg: inbox mscorlib) however we haven't done so in packages.

The challenge with pushing types down might be that those types might rely on other types which we can't push down. Dangling dependencies are OK for type-forwards since you only need those types if they're actually used, but if the types are used in other scenarios then it gets harder since you don't want to introduce exceptions in working scenarios.

[steveharter]

Per offline discussion with @buyaa-n it appears that the reference in CM to SSP is due to System.Configuration.Internal.DelegatingConfigHost.GetRestrictedPermissions() and System.Configuration.Internal.IInternalConfigHost.GetRestrictedPermissions() both of which are in the "internal" namespace and not actually called from the framework's libraries.

We could then just remove those two methods. I haven't actually tested the removal of SSP however to ensure these are the only references.

[ericstj]

I put together a quick prototype of this. DirectoryServices can push types down without breaking any API. All dangling references are for typeforwards and none of those types are used by the implementation.

Configuration manager cannot push down any types. As @steveharter mentioned, it's single reference to System.Security.Permissions is due to an interface method and it's implementations. There are no callers to this inside the assembly, however there are implementations. We may be able to get away with just leaving a dangling reference for this if the JIT is sufficiently lazy in compiling that method (and loading S.S.P).
ericstj@3426ddd attempts this. I didn't test it thoroughly to determine if it was viable.

Also as @steveharter suggests we might just remove the member from the interface - that would be minimally breaking -- only to callers of the method or explicit implementers (which would be very rare, especially since it's designated internal).
ericstj@4d0c588 does this.

[0xced]

I've just been hit by the System.Windows.Extensions → System.Drawing.Common dependency too.

I'm trying to migrate to FastReport.Core.Skia which has replaced System.Drawing.Common with the Skia graphics engine. In order to ease migration, FastReport.Core.Skia includes an API almost identical to the System.Drawing.Common API (through the FastReport.SkiaDrawing package). It is even using the System.Drawing namespace (that was probably not a great idea but here we are)! Consequently it is not possible to have a dependency on System.Drawing.Common, else compilation errors ensue:

[CS0433] The type 'FontFamily' exists in both 'FastReport.SkiaDrawing, Version=2022.3.0.0, Culture=neutral, PublicKeyToken=406e1f4c3c8ef97e' and 'System.Drawing.Common, Version=6.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51'

But unfortunately, the project I'm working on depends on System.Drawing.Common through an unexpected chain starting from a database driver!

graph
Oracle.ManagedDataAccess.Core --> System.DirectoryServices --> System.Security.Permissions --> System.Windows.Extensions --> System.Drawing.Common

Loading

Fortunatley, it is possible to get rid of the System.Drawing.Common dependency with a simple MSBuild trick:

<Target Name="RemoveSystemDrawingCommon" AfterTargets="ResolveAssemblyReferences">
  <ItemGroup>
    <ReferencePath Remove="@(ReferencePath)" Condition="%(ReferencePath.NuGetPackageId) == 'System.Drawing.Common'" />
  </ItemGroup>
</Target>

I'm pretty sure that the Oracle database driver does not depend on System.Drawing.Common at runtime so removing it should not cause any issue.

[AraHaan]

Sadly, DirectoryServices seems like it does. However, I think that package might be in the same boat with S.C.CM as well.

I also feel like S.S.P as an assembly should be deprecated and made to throw when anything tries to call any of it's APIs (since they are no-op anyways. This would then ensure that anyone that relies on S.S.P would be able to find areas that use it in their code and remove it from their non-.NET Framework targets. S.W.E on the other hand I feel is impossible to do that though because many important things depend on it.

[danmoseley]

Came up with internal customer, they depend on a package depending on StackExchange.Redis --> System.Diagnostics.PerformanceCounter --> System.Configuration.ConfigurationManager --> System.Security.Permissions --> System.Windows.Extensions --> System.Drawing.Common and thus needing an unexpected update. So another +1 for any snip that can be done to this chain.

[steveharter]

... We may be able to get away with just leaving a dangling reference for this if the JIT is sufficiently lazy in compiling that method (and loading S.S.P).
ericstj@3426ddd attempts this. I didn't test it thoroughly to determine if it was viable.

Also as @steveharter suggests we might just remove the member from the interface - that would be minimally breaking -- only to callers of the method or explicit implementers (which would be very rare, especially since it's designated internal).
ericstj@4d0c588 does this.

The first approach depends on behavior that may be nondeterministic and requires testing. The second approach, being fully deterministic and cleaner, seems preferred to me at this point (I don't know if there are external implementors of IInternalConfigHost).

[danmoseley]

@marklio has the ability to scan nuget for implementors of IInternalConfigHost if you reach out.

[CumpsD]

We hit this today with Microsoft.Data.SqlClient when we were trying to eliminate System.Drawing.Common from our dependencies

[AraHaan]

Let me see if I can take a stab at this and find other usages of S.S.P and S.D.C in the 2 dependencies mentioned and see if they can be completely removed without breaking public facing API. And if it does break, I think it should be ok for breakage in .NET 8 for sake of API cleanup of deprecated no-op feature usage (or at least have the removed parts be specific to NETFRAMEWORK).

[AraHaan]

The methods in S.C.CM can be put under #if NETFRAMEWORK it seems as it only is used in 3 places (internally but exposed publicly). An analyzer can also be added to also error when they use internal apis from S.C.CM as well (like what efcore does but instead of warning it is a hard error).

After I removed those methods (except for when used under .NET Framework), the code compiled.

With this, it should now be possible to then ship S.C.CM with (S.S.C.PD aka system.security.cryptography.protecteddata), and other dependencies of S.C.CM in the shared framework as well from .NET 8 onwards. Diagnostics.EventLog I am sure is already in the shared framework so it would only be an additional 2 libraries added to it. With all of this, it would add 3 total libraries to the shared framework, System.Diangostics.EventLog, System.Security.Cryptography.ProtectedData, and System.Configuration.ConfigurationManager. So overall a good change for everyone.

Edit: PerformanceCounter also works with the removed reference in S.C.CM and the removed members as well so that can also be added to the shared framework as well.

[AraHaan]

And I done the same for System.DirectoryServices, however it comes with the following changes that could be breaking for some:

diff (ref):

+ #if NETFRAMEWORK
    public sealed partial class DirectoryServicesPermission : System.Security.Permissions.ResourcePermissionBase
+ #else
+     public sealed class DirectoryServicesPermission
+ #endif
    {
        public DirectoryServicesPermission() { }
        public DirectoryServicesPermission(System.DirectoryServices.DirectoryServicesPermissionAccess permissionAccess, string? path) { }
        public DirectoryServicesPermission(System.DirectoryServices.DirectoryServicesPermissionEntry[]? permissionAccessEntries) { }
+ #if NETFRAMEWORK
        public DirectoryServicesPermission(System.Security.Permissions.PermissionState state) { }
+ #endif
        public System.DirectoryServices.DirectoryServicesPermissionEntryCollection? PermissionEntries { get { throw null; } }
    }

diff (src):

    [Obsolete(Obsoletions.CodeAccessSecurityMessage, DiagnosticId = Obsoletions.CodeAccessSecurityDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+ #if NETFRAMEWORK
    public sealed class DirectoryServicesPermission : ResourcePermissionBase
+ #else
+     public sealed class DirectoryServicesPermission
+ #endif
    {
        public DirectoryServicesPermission() { }
        public DirectoryServicesPermission(DirectoryServicesPermissionEntry[]? permissionAccessEntries) { }
+ #if NETFRAMEWORK
        public DirectoryServicesPermission(PermissionState state) { }
+ #endif
        public DirectoryServicesPermission(DirectoryServicesPermissionAccess permissionAccess, string? path) { }
        public DirectoryServicesPermissionEntryCollection? PermissionEntries { get; }
    }

Results: Changes can be trivially made to S.C.CM with little breaking changes to public api (I consider API in the internal namespace to be internal implementation details and so users using it should expect breaking changes to happen at any time there), as for S.DS I cannot say as much so I can open PR to S.C.CM but as for S.DS it would need to go through api review and see if it can be approved for the breaking changes (and becomes a documented breaking change about the removed ctor to DirectoryServicesPermission and that when not using .NET Framework that it no longer derives from ResourcePermissionBase so then users of that can update their code when they realize that their code might no longer compile under .NET 8. With .NET 8 being the next LTS release I think it's acceptable for this kind of breaking change as the feature behind it was obsolete anyway so it's not like they are not warned beforehand that things in obsolete apis might not break.

Note: The changes to S.DS would need to be done in a PR separate from the changes to S.C.CM.

cc: @steveharter

[ericstj]

For DirectoryServices we can do this in a less breaking way if we just move DirectoryServicesPermission into System.Security.Permission, then add a type-forward from DirectoryServicesPermission to System.Security.Permission and put PrivateAssets="All" on the project reference so that we leave the dependency out of the nuspec. That's what I prototyped in this change: ericstj@3426ddd#diff-c7413ea7bade71198dd11d098dc4707989ecdf075fa31c65ea9afcb2cfc61092R13

IMO this is less breaking and it's an acceptable dangling dependency. We have similar dangling dependencies for typeforwards in other compatibility shims (mscorlib, system, system.core, etc)

Ideally, we should also try to remove System.Security.Permissions from the WindowsDesktop shared framework as part of this, however there may be more references that need to be chased down and eliminated.

runtime/src/libraries/NetCoreAppLibrary.props

Line 245 in 156c373

System.Security.Permissions;

https://github.com/dotnet/windowsdesktop/blob/58e59b5930d519cdd7fa020562e08c68b7e51122/pkg/windowsdesktop/sfx/Microsoft.WindowsDesktop.App.Ref.sfxproj#L31

[AraHaan]

I agree, I personally think that S.S.P should be removed entirely from everything with how most of that assembly is either stubs that do nothing at all or minor things that can probably be moved into corelib (I think moving some of it to corelib would be even better).

[steveharter]

Closing based on #82259 and #82453.