[release/6.0] Bring back System.Security.AccessControl package

[ViktorHofer]

Fixes #54341

This issue was discovered in dotnet/aspnetcore#33571 (comment) by @dougbu, @wtgodbe and @ericstj.
Note that this change won't be ported into main as the System.Security.AccessControl package won't ship after 6.0 anymore as the library is now part of the shared framework. The reason why this package needs to be brought back for 6.0 is that aspnetcore depends on S.S.Cryptography.Xml which either depends on S.S.AccessControl or S.S.Permissions. As the latter is undesirable to reference, the former is chosen as a dependency. Going forward, S.S.Cryptography.Xml will continue to reference the last shipped 6.0.* version of S.S.AccessControl and doesn't require it to be built live.

Customer Impact

Customers transitively reference System.Security.Permissions (Legacy) with its large closure of assemblies when consuming System.Security.Cryptography.Xml (non-legacy) or some of the AspNetCore packages which expose it. When referencing matching https://www.nuget.org/packages/Microsoft.AspNetCore.DataProtection/6.0.0-preview.7.21378.6 to what is in the shared framework, that assembly will be dropped, yet SSP and it’s closure will go app-local and increase the app's foot print.

Testing

Automated testing via a) the PackageTesting SDK component that checks if assets are compatible with each other and b) the repo local testPackages infrastructue that restore the generated packages on tfms and checks if the packages are consumable.
Manually tested as well by creating the package and validating its output.

Risk

Low - The package already existed before and the automated package testing covers the consumption of the package on the consumer's end.

[ghost]

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

Issue Details

---

Fixes #54341

@ericstj as we don't want this in main (7.0), I'm not back porting anything but directly targeting 6.0-rc1. What bar do we apply to this change? Is this something for Tactics or M2 approval?

Author:	ViktorHofer
Assignees:	-
Labels:	area-Infrastructure-libraries, new-api-needs-documentation
Milestone:	-

[ericstj]

@ericstj as we don't want this in main (7.0)

You might want some of this in main. The parts that get System.Security.Permissions out of our references for the other crypto packages. Agreed that will look different and will require the final 6.0 packages, please file a follow up issue to do that.

What bar do we apply to this change?

For RC1 we need tactics approval, for RC2 it's M2 approval. Apply the bug bar from @jeffhandley's mail. Make sure to add the template to the PR.

[danmoseley]

RC1 bar high now. Probably if you back port it would be to RC2. Would need template.

[ViktorHofer]

RC1 bar high now. Probably if you back port it would be to RC2. Would need template.

Yes, absolutely. I plan to re-target to RC2 when the change is ready.

[joperezr]

NIT: It might be good to test at least locally with the new PackageValidation logic how the diff looks like since this specific suppression will go away and instead you'll get the actual diffs after following type forwards.

[danmoseley]

Approved for release/6.0: significant regression since 5.0 of footprint of aspnet shared framework. Per discussion offline, we've tested as best we can. Please address any feedback. also verify the result when it reaches aspnetcore repo (perhaps that will happen first in main)

[dougbu]

@ViktorHofer with this, we'll be able to remove our previous workarounds for RC2❔ Should we eliminate our Microsoft.Win32.SystemEvents, System.Drawing.Common, System.Security.Permissions, or System.Windows.Extensions references going forward❔ We reference them mainly to ensure they do not end up in Microsoft.AspNetCore.App.* nor our targeting packs.

Separately, should we expect (and allow) System.Security.AccessControl to land in our targeting packs again❔ If yes, will we need Microsoft.Win32.Registry and System.Security.Principal.Windows again too❔ (We use 5.0.0 versions of the later two elsewhere and I expect that'll continue, right❔)

[ericstj]

LGTM. It might make sense to review a package diff for any project touched here just to make sure you understand the changes and they are what you'd expect. You can also do a diff of S.S.ACL with the last version before the removal, so that it gets the same treatment as other pkgproj->csproj conversions.

[vitek-karas]

For the linker issue: I think we should just not try to trim these generated not-supported assemblies. While we're probably going to fix this particular issue in the linker, there are other potential issues with these type of assemblies (as pointed out in dotnet/linker#2238 (comment)).

[ViktorHofer]

LGTM. It might make sense to review a package diff for any project touched here just to make sure you understand the changes and they are what you'd expect.

If we are going to request nuspec / package diffs for any change that we might make in the future to packages we should invest in tooling that helps with that. At the moment this is a very manual process and takes time. I would imagine that PackageValidation could generate a nuspec diff based on the baseline version vs the just build package.

You can also do a diff of S.S.ACL with the last version before the removal, so that it gets the same treatment as other pkgproj->csproj conversions.

I compared the packages (including content and nuspec diffs) but didn't upload a diff (as mentioned, there's no automation available). Let me know if you want me to upload one, which I can most definitely do.

For the linker issue: I think we should just not try to trim these generated not-supported assemblies. While we're probably going to fix this particular issue in the linker, there are other potential issues with these type of assemblies (as pointed out in dotnet/linker#2238 (comment)).

Thanks Vitek. We have been trimming PNSEs for a quite a while now (as we don't differentiate between the "kind" of assembly) and we are close to shipping release/6.0. Do you think we should stop doing that even for 6.0?

[ericstj]

I don’t need to see the diffs personally, just suggesting to test this as it can be helpful when making these types of changes and it was something we did in the past, especially when we wanted to communicate the risk level of a large change. The mechanism we used to use for this was to just rename the intermediate nuspec folder, apply the change, then build packages again, and diff the folders. It was a low cost check that only took ~5 minutes and often helped quickly illustrate the amount of change to the shipping product.

[ViktorHofer]

It was a low cost check that only took ~5 minutes and often helped quickly illustrate the amount of change to the shipping product.

Let me share the process that I followed during the "pkgproj -> NuGet Pack task" migration. Let me say that generating and providing the diffs was actually what took the longest.

When I touch a package I usually verify packaging changes by doing the following:

1. Generate the package and with it implicitly run PackageValidation.
2. Open the baseline package (i.e. the last released preview / or the last nightly) in NuGet Package Explorer.
3. Open the generated package in NuGet Package Explorer
4. Diff the nuspec, especially the dependencies section and the package content.
5. Optional: Generate and upload the nuspec diff

I personally use NuGet Package Explorer as it provides additional value i.e. checking for symbol files, sourcelink and compiler optimizations being present. The reason why I prefer a baseline package over a live build is that it lets me compare the applied change against the last shipped state.

[vitek-karas]

We have been trimming PNSEs for a quite a while now (as we don't differentiate between the "kind" of assembly) and we are close to shipping release/6.0. Do you think we should stop doing that even for 6.0?

I don't think this is something we should try to play with in 6 - as you said - too late...

[ericstj]

@dougbu I think you can remove

https://github.com/dotnet/aspnetcore/blob/6a65d74163f985b1c685335e7f3902f88cd9f3f7/src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.csproj#L123-L130
https://github.com/dotnet/aspnetcore/blob/6a65d74163f985b1c685335e7f3902f88cd9f3f7/eng/Dependencies.props#L82-L86
https://github.com/dotnet/aspnetcore/blob/6a65d74163f985b1c685335e7f3902f88cd9f3f7/src/Framework/App.Ref/src/Microsoft.AspNetCore.App.Ref.csproj#L71-L78

In release/6.0. You'll be able to do the same in main once main is able to pick up the System.Security.AccessControl package we're building 6.0.

[dougbu]

You'll be able to do the same in main once main is able to pick up the System.Security.AccessControl package we're building 6.0.

Do you mean this change has not been merged into 'main' in this repo❔ I don't see it there but want to make sure this isn't about dependency flow into dotnet/aspnetcore.

[ericstj]

For the effect of this change to occur in main we need to update our reference here

runtime/src/libraries/System.Security.Cryptography.Xml/src/System.Security.Cryptography.Xml.csproj

Line 134 in 0becc99

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

To use the 6.0.0 System.Security.AccessControl package. @ViktorHofer can you file an issue for that if you don't have one? Not sure that will happen naturally.

[ericstj]

Actually, that makes me realize that this part might have been missed here. @ViktorHofer -- can you check that the crypto package that ASP.NET uses was updated to reference S.S.AcessControl instead of Permissions?