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

[release/6.0 Cleanup remaining SSP references #36248

Merged
merged 3 commits into from
Sep 9, 2021

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Sep 7, 2021

  • follow up to e58a6c2
  • remove references to System.Security.Permissions or its closure
    • only mentions are in eng/PackageOverrides.txt and eng/PlatformManifest.txt
    • those files will remain unused until we update them in the run-up to 6.0.1
  • still indirectly pick up package refs for SSP and its closure but that does not impact targeting pack content

nit: Only need to exclude System.Net.Quic and System.Security.Cryptography.Pkcs from @(_AvailableRuntimeRefAssemblies)

  • assemblies are not in @(ReferencePathWithRefAssemblies), just the transport package

@dougbu dougbu added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Sep 7, 2021
@dougbu dougbu requested review from ViktorHofer, ericstj and a team September 7, 2021 21:14
@dougbu dougbu requested a review from Pilchie as a code owner September 7, 2021 21:14
@wtgodbe
Copy link
Member

wtgodbe commented Sep 7, 2021

those files will remain unused until we update them in the run-up to 6.0.1

Can you file an issue for this?

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again LGTM assuming the outputs look good

@ViktorHofer
Copy link
Member

and System.Security.Cryptography.Pkcs from @(_AvailableRuntimeRefAssemblies)

How is Pkcs getting into the closure? Is it part of the transport pack?

@dougbu
Copy link
Member Author

dougbu commented Sep 7, 2021

Can you file an issue for this?

No need. The 6.0.1 build won't work w/o the updated files 😀 This is part of what we should document for major version branding updates too.

How is Pkcs getting into the closure? Is it part of the transport pack?

We build the entire closure now that 0b0bed3 is in.

My point was different in any case: @(_AvailableRuntimeRefAssemblies) lists the files from the internal transport package. We need the ref/ assemblies for QUIC and PKCS to build our stuff but they must not land in our targeting package.

@dougbu
Copy link
Member Author

dougbu commented Sep 7, 2021

Assert.Empty() Failure
Collection: ["System.Net.Quic", "System.Security.Cryptography.Pkcs"]

Need to double-check my @(_AspNetCoreAppPackageOverrides) exclusions and see where these assemblies are coming from. Likely need to undo a part of this change.

@(ReferencePathWithRefAssemblies->WithMetadataValue('Filename', 'System.Security.Cryptography.Pkcs'));
@(ReferencePathWithRefAssemblies->WithMetadataValue('Filename', 'System.Security.Permissions'));
@(ReferencePathWithRefAssemblies->WithMetadataValue('Filename', 'System.Windows.Extensions'));" />
@(ReferencePathWithRefAssemblies->WithMetadataValue('NuGetPackageId', 'Microsoft.NETCore.App.Ref'))" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@(ReferencePathWithRefAssemblies->WithMetadataValue('NuGetPackageId', 'Microsoft.NETCore.App.Ref'))" />
@(ReferencePathWithRefAssemblies->WithMetadataValue('Filename', 'System.Net.Quic'));
@(ReferencePathWithRefAssemblies->WithMetadataValue('Filename', 'System.Security.Cryptography.Pkcs'));
@(ReferencePathWithRefAssemblies->WithMetadataValue('NuGetPackageId', 'Microsoft.NETCore.App.Ref'))" />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A reasonable suggestion given that's what we did before.

But, the intent can be made more obvious by excluding all of $(RuntimeTransportReferenceDirectory)*.dll instead of the @(_ReferencedRuntimeRefAssemblies) subset. The double-exclusion just looks wrong and that's why I wanted to remove it. Will try my approach and fall back to your suggestion if that fails.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I wasn't certain what any of this does, just noticed a place where I thought it might have been accidental.

@ericstj
Copy link
Member

ericstj commented Sep 8, 2021

How is Pkcs getting into the closure? Is it part of the transport pack?

It's a dependency of Crypto.Xml which gets referenced by DataProtection. It's not exposed by any surface area in ASP.NET nor Crypto.Xml which is why it has been be excluded, but if ASP.NET wanted to implement the intent of your changes here dotnet/runtime#53892, @ViktorHofer, they could decide to just expose it. I'm not sure if its the best time to make that change, but something to consider perhaps for 7.0 if it simplifies things. We'd want to get @bartonjs's blessing that exposing Pkcs would be no worse than Crypto.Xml.

@bartonjs
Copy link
Member

bartonjs commented Sep 8, 2021

We'd want to get @bartonjs's blessing that exposing Pkcs would be no worse than Crypto.Xml.

Pkcs we like. We may not have the best of APIs in there, but it's based on tech we're happy with. It's also a library that has seen new types and new functionality added to it since the start of .NET Core, unlike Crypto.Xml.

So, definitely "no worse than" :)

- follow up to e58a6c2
- remove references to System.Security.Permissions or its closure
  - only mentions are in eng/PackageOverrides.txt and eng/PlatformManifest.txt
  - those files will remain unused until we update them in the run-up to 6.0.1
- still indirectly pick up package refs for SSP and its closure but that does not impact targeting pack content

nit: Only need to exclude System.Net.Quic and System.Security.Cryptography.Pkcs from `@(_AvailableRuntimeRefAssemblies)`
  - assemblies are not in `@(ReferencePathWithRefAssemblies)`, just the transport package
- also remove duplication between `@(AspNetCoreReferenceAssemblyPath)` additions (for efficiency)
@dougbu dougbu force-pushed the dougbu/permissions.closure.cleanup/6.0 branch from ca9dd1e to 1be8200 Compare September 9, 2021 00:31
@dougbu
Copy link
Member Author

dougbu commented Sep 9, 2021

We'd want to get @bartonjs's blessing that exposing Pkcs would be no worse than Crypto.Xml.

Pkcs we like. We may not have the best of APIs in there, but it's based on tech we're happy with. It's also a library that has seen new types and new functionality added to it since the start of .NET Core, unlike Crypto.Xml.

So, definitely "no worse than" :)

For 6.0, we ship both Crypto.Pkcs and Crypto.Xml in our shared frameworks but only Crypto.Xml in our targeting packs. Only the native aspnetcorev2_inprocess module (on Windows) and System.Diagnostics.EventLog.Messages (which isn't needed in the targeting pack) are treated similarly to Crypto.Pkcs. So, Crypto.Pkcs is the one "obviously unaligned" assembly here.

We can align things by removing the Crypto.Pkcs exclusion. But, is this worthwhile for our 7.0.x customers if we aren't using Crypto.Pkcs types in our public API❔ If yes, we should first get Crypto.Pkcs into the Microsoft.Internal.Runtime.AspNetCore.Transport package.

- Crypto.Pkcs is **not** present in the transport package
@dougbu dougbu merged commit 888e03a into release/6.0 Sep 9, 2021
@dougbu dougbu deleted the dougbu/permissions.closure.cleanup/6.0 branch September 9, 2021 17:24
@ghost ghost added this to the 6.0-rc2 milestone Sep 9, 2021
dougbu added a commit that referenced this pull request Sep 10, 2021
- `cherry-pick` 888e03a
- remove references to System.Security.Permissions and its closure
  - only mentions are in eng/PackageOverrides.txt and eng/PlatformManifest.txt
  - those files will remain unused until we update them in the run-up to 6.0.1
  - may see some package refs for SSP and its closure elsewhere but this does not impact targeting pack content
- also remove duplication between `@(AspNetCoreReferenceAssemblyPath)` additions (for efficiency)

nit: Only need to exclude System.Net.Quic from `@(_AvailableRuntimeRefAssemblies)`
  - Crypto.Pkcs is **not** present in the transport package
  - other disallowed entries aren't present in the transport package or our dependency closure
dougbu added a commit that referenced this pull request Sep 22, 2021
- `cherry-pick` 888e03a
- remove references to System.Security.Permissions and its closure
  - only mentions are in eng/PackageOverrides.txt and eng/PlatformManifest.txt
  - those files will remain unused until we update them in the run-up to 6.0.1
  - may see some package refs for SSP and its closure elsewhere but this does not impact targeting pack content
- also remove duplication between `@(AspNetCoreReferenceAssemblyPath)` additions (for efficiency)

nit: Only need to exclude System.Net.Quic from `@(_AvailableRuntimeRefAssemblies)`
  - Crypto.Pkcs is **not** present in the transport package
  - other disallowed entries aren't present in the transport package or our dependency closure
dougbu added a commit that referenced this pull request Sep 23, 2021
* [main] React to dotnet/runtime#57816 (#36155)
- `cherry-pick` e58a6c2
- see dotnet/runtime#57816 (comment)
- also reacting to dotnet/runtime#58731
- undo much of 18a61fb

* [main] Cleanup remaining SSP references (#36248)
- `cherry-pick` 888e03a
- remove references to System.Security.Permissions and its closure
  - only mentions are in eng/PackageOverrides.txt and eng/PlatformManifest.txt
  - those files will remain unused until we update them in the run-up to 6.0.1
  - may see some package refs for SSP and its closure elsewhere but this does not impact targeting pack content
- also remove duplication between `@(AspNetCoreReferenceAssemblyPath)` additions (for efficiency)

nit: Only need to exclude System.Net.Quic from `@(_AvailableRuntimeRefAssemblies)`
  - Crypto.Pkcs is **not** present in the transport package
  - other disallowed entries aren't present in the transport package or our dependency closure
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants