Investigate WindowsDesktop dependencies on System.Security.Permissions so it can be safely omitted

[dagood]

While trying to remove System.Security.Permissions for https://github.com/dotnet/core-setup/issues/7290, some closure validation errors came up:

Assembly 'System.Xaml' is missing dependency 'System.Security.Permissions'
Assembly 'System.Configuration.ConfigurationManager' is missing dependency 'System.Security.Permissions'
Assembly 'System.Security.Cryptography.Xml' is missing dependency 'System.Security.Permissions'
Assembly 'WindowsBase' is missing dependency 'System.Security.Permissions'
Assembly 'System.Windows.Forms.Design.Editors' is missing dependency 'System.Security.Permissions'
Assembly 'System.Drawing' is missing dependency 'System.Security.Permissions'
Assembly 'System.DirectoryServices' is missing dependency 'System.Security.Permissions'

Some may be caused by having stale dependencies (https://github.com/dotnet/core-setup/issues/7343), but not all (dotnet/core-setup#7324 (comment)):

That's not the only cause for this. Some of these other dependencies are concerning. I suspect we'll need to suppress this, since even once everything is fixed WindowsBase will have typeforwards to System.Security.Permission, but we need to examine all the references before suppressing to find out if there is other work to do to safely omit this.

Once we're sure this is safe (not sure if Core-Setup should be tracking that in particular), we can remove the dependency on System.Security.Permissions and add:

    <IgnoredReference Condition="'$(PackageTargetRuntime)' == ''" Include="System.Security.Permissions" />

/cc @ericstj @vatsan-madhavan

[vatsan-madhavan]

Re System.Xaml, WindowsBase: they forward types to s.s.permissions. Those are the only references they have left. We want those TypeForwardedTo attributes to become dangling references.

[vatsan-madhavan]

/cc @ojhad,@rladuca

[ericstj]

This list seems like it contains implementation dependencies as well. I'll look at the CoreFx dependencies on S.S.P and report back the usage.

@vatsan-madhavan have you reviewed the DirectoryServices dependency? That seems a bit unusual from PresentationUI.

[vatsan-madhavan]

ave you reviewed the DirectoryServices dependency? That seems a bit unusual from PresentationUI.

There is a People picker UI in PresetationUI that uses DirectoryServices.

[vatsan-madhavan]

This is what I'm seeing:

• System.Security.Cryptography.Xml.dll
  • System.Security.Policy.Evidence, System.Security.Permissions, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51
• System.DirectoryServices.dll
  • System.Security.Permissions.ResourcePermissionBase, System.Security.Permissions, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51
  • System.Security.Permissions.PermissionState, System.Security.Permissions, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51
• System.Configuration.ConfigurationManager.dll
  • System.Security.PermissionSet, System.Security.Permissions, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51
• System.Windows.Forms.Design.Editors.dll
  • System.Security.Permissions.PermissionSetAttribute, System.Security.Permissions, Version=4.0.2.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51

[vatsan-madhavan]

System.Drawing.dll is a type-forwards only assembly. I'm sure allowing its dependency on System.Security.Permissions to become a dangling-dependency will be ok.

[ericstj]

• System.Windows.Forms.Design.Editors

  • was cleaned up in https://github.com/dotnet/winforms/pull/1397/files.

• System.Configuration.ConfigurationManager

  • Public API dependency on PermissionSet and targets NETStandard. https://github.com/dotnet/corefx/blob/e6a5951fb68a09598f1fc1826a94de212385c649/src/System.Configuration.ConfigurationManager/ref/System.Configuration.ConfigurationManager.cs#L1385
  • In .NETCore3.0 we moved this type down to System.Runtime.Extensions, so we could cross-compile ConfigurationManager to avoid the dependency.

• System.Security.Cryptography.Xml

  • Public API dependency on Evidence: https://github.com/dotnet/corefx/blob/e6a5951fb68a09598f1fc1826a94de212385c649/src/System.Security.Cryptography.Xml/ref/System.Security.Cryptography.Xml.cs#L119-L120
  • Type still lives in S.S.P, would need to be pushed down to avoid the dependency. Pushing down is somewhat risky.

• System.DirectoryServices

  • Public API that depends on permissions: https://github.com/dotnet/corefx/blob/e6a5951fb68a09598f1fc1826a94de212385c649/src/System.DirectoryServices/ref/System.DirectoryServices.cs#L263-L270
  • Cannot push this down without effectively pushing down majority of CAS.
  • Less bad since this dependency is implementation only, DirectoryServices isn't exposed in WindowsDesktop's references.

[ericstj]

What are folks thoughts on leaving System.Security.Permissions in the ref-pack but making folks use a Reference item in order to get it?

[vatsan-madhavan]

What are folks thoughts on leaving System.Security.Permissions in the ref-pack but making folks use a Reference item in order to get it?

Seems reasonable to me. We should probably do a test pass for WinForms & WPF to make sure nothing breaks because of the missing S.S.Permissions (we did some testing already in WPF a few days back to validate this fact by manually removing S.S.Permissions manually from the runtime pack).

Reference item in order to get it
Is this work in already (support for Reference items for Framework assemblies) ?

Given repos below core-setup lock down on Thu 7/25, should we hold this change for start of Preivew 9 (i.e., until Fri, 7/26)?

[ericstj]

We should probably do a test pass for WinForms & WPF to make sure nothing breaks because of the missing S.S.Permissions

I'm not suggesting it be removed from run-time, so run-time behavior shouldn't change, only reference.

[vatsan-madhavan]

I'm not suggesting it be removed from run-time, so run-time behavior shouldn't change, only reference.

So retain it in both ref and runtime packs, but not include it in any profiles?

I'm good with this proposal.

[ericstj]

So it looks like omitting it from the framework list does this:

Then if I click that helper it just brings up the add-reference dialog without doing anything. Manually adding a simple reference doesn't fix it either, so I guess this isn't a great option, if we leave it out might as well omit it completely from references (what @dagood was originally doing).

@nguerrera @dsplaisted in case they have any opinion here.

[dagood]

There's also ReferencedByDefault="false" (new), but I'm not sure how it's different from having no profiles. Maybe it's unrelated, but I asked about what makes it different in https://github.com/dotnet/core-setup/issues/7218 FYI.

[nguerrera]

What are folks thoughts on leaving System.Security.Permissions in the ref-pack but making folks use a Reference item in order to get it?

There's also ReferencedByDefault="false"

This is what ReferencedByDefault="false" implies. However, the ability to reference them individually isn't implemented yet, or planned to be implemented in 3.0. I don't think we intended to use that for things that are not leaf nodes in the reference closure. We do not expect to have 'You must add a reference to X' errors when consuming things in the default references. That is a pretty bad experience.

[nguerrera]

I'm not sure how it's different from having no profile

No profile means something very different: if the entire shared framework is referenced, then include it, but there are no subsets which include it. Integration is like this because it pulls in both WPF and WindowsForms. You have to pull in both to use it.

[ericstj]

We do not expect to have 'You must add a reference to X' errors when consuming things in the default references. That is a pretty bad experience.

Right, we don't have this today. We do have this when you consume a desktop library.

I believe the extent of this issue if we were to omit S.S.P would be the EncryptedXml API I showed above, the DocumentEvidence property on the same type, and the System.Configuration.Internal.IInternalConfigHost.GetRestrictedPermissions method. These aren't exactly show-stopper APIs. It may be acceptable.

We could do the work to more selectively cut this stuff. It would look like cross-compiling ConfigurationManager for netcoreapp3.0 so it doesn't reference S.S.P, pushing down Evidence (to System.Runtime.Extensions or similar) and cross-compiling Crypto.Xml for netcoreapp3.0, then pushing down the permission types from DirectoryServices to S.S.P and allowing the dangling typeforwards. If we did all of this then I think we could maintain our existing precedent for only having dangling type-forwards from reference assemblies. I just can't convince myself that risk/reward of this level of changes is appropriate for this point in the release.

[vatsan-madhavan]

I just can't convince myself that risk/reward of this level of changes is appropriate for this point in the release

+1. This sounds like a lot for 3.0 and out of scope for 3.1.

If the functionality for individual references is coming in 3.1, then can we live with this until 3.1?

Or are we now thinking that SSP should just stay in, and we can reconsider removal in .NET 5?

[RussKie]

Is this still a thing?

[ericstj]

Yes, please remove System.Security.Permissions from WindowsDesktop.

[vatsan-madhavan]

WPF continues to rely on/use XamlAccessLevel and XamlLoadPermission because these were exposed in .NET 4.x public contracts and in turn .NET Core 3.x needed to maintain the same contracts as well.

At some point, WPF would have to break those contracts and remove those S.S.P dependencies - and only then this work can be completed.

[ericstj]

We could push types into System.Security.Permissions then leave a dangling typeforward. It's not all that unlike mscorlib.dll with dangling type-forwards; only WPF also happens to put type-defs in WindowsBase. Would this impact WPF runtime?

[vatsan-madhavan]

WPF does more than passively expose these types; it uses XamlAccessLevel and XamlLoadPermission in runtime (esp. the former)

https://github.com/dotnet/wpf/search?q=xamlaccesslevel

For e.g., XamlObjectWriter continues to respect XamlAccessLevel

src/Microsoft.DotNet.Wpf/test/DRT/DrtXaml/DrtXaml/Tests/AdvXamlFeatureTests.cs#L1046-L1053

            // Set local assembly on the XOW also (to access internal Ctors)
            XamlObjectWriterSettings xowSettings = new XamlObjectWriterSettings();
            xowSettings.AccessLevel = System.Xaml.Permissions.XamlAccessLevel.AssemblyAccessTo(localAssembly);


            XamlObjectWriter objWriter = new XamlObjectWriter(xamlReader.SchemaContext, xowSettings);
            XamlServices.Transform(xamlReader, objWriter);
            object o = objWriter.Result;
            Test.Elements.Object10 root = (Test.Elements.Object10)o;

Ultimately, this would likely require some refactoring of DynamincMethodRuntime and its antecedents:

        internal DynamicMethodRuntime(XamlRuntimeSettings settings, XamlSchemaContext schemaContext,
            XamlAccessLevel accessLevel)
            : base(settings, true /*isWriter*/)
        {
            Debug.Assert(schemaContext != null);
            Debug.Assert(accessLevel != null);
            _schemaContext = schemaContext;
            _localAssembly = Assembly.Load(accessLevel.AssemblyAccessToAssemblyName);
            if (accessLevel.PrivateAccessToTypeName != null)
            {
                _localType = _localAssembly.GetType(accessLevel.PrivateAccessToTypeName, true /*throwOnError*/);
            }
        }

[dreddy-work]

adding @pchaurasia14 / @singhashish-wpf to follow up on this.