Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ASPNetCore shared framework pulls in System.Drawing.Common but doesn't use it. #42645

Closed
ericstj opened this issue Sep 23, 2020 · 10 comments · Fixed by #50284
Closed

ASPNetCore shared framework pulls in System.Drawing.Common but doesn't use it. #42645

ericstj opened this issue Sep 23, 2020 · 10 comments · Fixed by #50284
Assignees
Labels
area-System.Security Bottom Up Work Not part of a theme, epic, or user story
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Sep 23, 2020

I noticed today that ASPNetCore pulls in System.Drawing.Common. This struck me as odd because I wasn't sure why ASP.NET would need to deal with images. That seems like >1MB of unused code.

I did a bit of digging and found the following:

    Microsoft.AspNetCore.DataProtection & Microsoft.Extensions.Configuration.Xml > System.Security.Cryptography.Xml > System.Security.Permissions > System.Windows.Extensions > System.Drawing.Common

Crypto.Xml references System.Security.Permissions for only the Evidence type. It's exposed in it's public surface area. This type is useless in .NETCore and only used for source compat. This probably should have been omitted from the Crypto.Xml surface area, like we omitted all other uses of Evidence across the stack, however I bet Crypto.Xml was also thought to be legacy due to the spec/security issues with XML encryption. We could push this one type down (and EvidenceBase) to break the dependency between Crypto.Xml and System.Security.Permissions.

System.Security.Permissions references System.Windows.Extensions for only the XamlAccessLevel type. We could push this one type down to break this dependency. Then Permissions could reference only the assembly lower in the stack.

System.Windows.Extensions references System.Drawing.Common because we pushed types down. We can't break this dependency without making a binary breaking change (old code compiled against System.Windows.Extensions needs to resolve the types which have been moved down).

So we have two options to break dependencies here that involve pushing down tiny types. We should consider doing this in 6.0 to reduce the size of ASPNETCore and pull System.Drawing.Common out of it (as well as S.S.P). We just need to decide where to put these types 🤔

/cc @bartonjs @safern

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Drawing untriaged New issue has not been triaged by the area owner labels Sep 23, 2020
@ghost
Copy link

ghost commented Sep 23, 2020

Tagging subscribers to this area: @safern, @tannergooding, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@ericstj
Copy link
Member Author

ericstj commented Sep 23, 2020

So it looks like we initially didn't have the dependency from Crypto.Xml to Permissions. It was added here: dotnet/corefx#23417

@safern
Copy link
Member

safern commented Sep 23, 2020

We could move: System.Security.Policy.Evidence down to System.Security.AccessControl and we would break this dependency.

@safern safern removed the untriaged New issue has not been triaged by the area owner label Sep 23, 2020
@safern safern added this to the 6.0.0 milestone Sep 23, 2020
@ghost
Copy link

ghost commented Sep 24, 2020

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@bartonjs
Copy link
Member

We could move: System.Security.Policy.Evidence down to System.Security.AccessControl and we would break this dependency.

EvidenceBase needs to move along with it, but I had just independently come to the same conclusion. It's not a terribly inappropriate place for it.

@ericstj
Copy link
Member Author

ericstj commented Jan 7, 2021

This just came up and is something we should do, preferably early, in 6.0. Moving types tends to require some babysitting as it flows through repos.

@ericstj ericstj added the Bottom Up Work Not part of a theme, epic, or user story label Jan 7, 2021
@ericstj
Copy link
Member Author

ericstj commented Mar 17, 2021

With dotnet/aspnetcore#30968 this is now fixed.

The following assemblies were removed from ASPNETCore.

  • Microsoft.Win32.SystemEvents
  • System.Drawing.Common
  • System.Security.Permissions
  • System.Windows.Extensions

@Anipik we should remove these from ASP.NET's transport package

@ericstj ericstj self-assigned this Mar 17, 2021
@Anipik
Copy link
Contributor

Anipik commented Mar 19, 2021

sure, let me know if you are not working on this then i can go ahead make the changes

@ericstj
Copy link
Member Author

ericstj commented Mar 19, 2021

@pranavkm @dougbu is it ok to remove these 4 assemblies from the transport package now?

@pranavkm
Copy link
Contributor

Yup. They're gone - aspnet/Announcements#456

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Mar 26, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Mar 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Security Bottom Up Work Not part of a theme, epic, or user story
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants