Add missing members to EncryptedXml. #23417

vladimir-kazakov · 2017-08-19T13:29:56Z

While importing EncryptedXml into Mono (mono/mono#5273), it was noticed that its following members are missing:

This PR adds them. The implementation is taken from .NET Framework.

The behavior of the EncryptedXmlTest.DecryptData_CipherReference_InvalidUri test was changed. As far as I understand, both .NET Core and .NET Framework must behave the same way. According to the documentation, the SecurityManager.GetStandardSandbox must throw if the evidence is null. Previously, .NET Core wasn't throwing in this case, so this was changed as well.

ViktorHofer · 2017-08-19T13:32:50Z

src/System.Security.Permissions/tests/SecurityManagerTests.cs

+        [Fact]
+        public void GetStandardSandbox_WhenEvidenceIsNull_ThrowsArgumentNullException()
+        {
+            Assert.Throws<ArgumentNullException>(() => SecurityManager.GetStandardSandbox(null));


use AssertExtensions to also check for the name of the argument thrown (nameof(evidence))

ViktorHofer · 2017-08-19T13:33:49Z

src/System.Security.Cryptography.Xml/tests/EncryptedXmlTest.cs

-                    Assert.Throws<ArgumentNullException>(decrypt);
-                else
-                    Assert.Throws<CryptographicException>(decrypt);
+                Assert.Throws<ArgumentNullException>(() => exml.DecryptData(ed, aes));


same here. Check for name argument too.

ViktorHofer · 2017-08-19T13:35:57Z

src/System.Security.Cryptography.Xml/tests/EncryptedXmlTest.cs

@@ -47,28 +45,47 @@ public override void GenerateKey()
            }
        }

+        private readonly Encoding _defaultEncoding = Encoding.UTF8;


ViktorHofer · 2017-08-19T13:36:05Z

src/System.Security.Cryptography.Xml/tests/EncryptedXmlTest.cs

+        private const CipherMode DefaultCipherMode = CipherMode.CBC;
+        private const PaddingMode DefaultPaddingMode = PaddingMode.ISO10126;
+        private const string DefaultRecipient = "";
+        private readonly XmlResolver _defaultXmlResolver = null;


ViktorHofer · 2017-08-19T13:43:00Z

I hope you are aware that Evidence is stubbed out in .NET Core: https://github.com/dotnet/corefx/blob/986ffdb1053d334b862f96b53c67ca3bae222219/src/System.Security.Permissions/src/System/Security/Policy/Evidence.cs

I guess you aren't using our sources where Evidence is defined (System.Security.Permissions)?

ViktorHofer · 2017-08-19T13:44:37Z

src/System.Security.Cryptography.Xml/src/System/Security/Cryptography/Xml/EncryptedXml.cs

+        {
+            // maybe a network stream, make sure we allow just what is needed!!
+            PermissionSet ps = SecurityManager.GetStandardSandbox(_evidence);
+            ps.PermitOnly();


this will throw in .NET Core:

corefx/src/System.Security.Permissions/src/System/Security/PermissionSet.cs

Line 41 in 986ffdb

public void PermitOnly() { throw new PlatformNotSupportedException(SR.PlatformNotSupported_CAS); }

@danmosemsft what's the strategy for code that checks if permission is granted? I guess it's intentional that PermitOnly wasn't stubbed out silently instead of throwing?

If this code path is reached, which means the user passed an Evidence in the constructor and therefore _evidence is not null an exception is still throw in Core.

ViktorHofer · 2017-08-19T13:55:50Z

src/System.Security.Permissions/tests/System.Security.Permissions.Tests.csproj

@@ -26,6 +26,7 @@
    <Compile Include="SecurityElementTests.cs" />
    <Compile Include="HostSecurityManagerTests.cs" />
    <Compile Include="HostProtectionTests.cs" />
+    <Compile Include="SecurityManagerTests.cs" />


I know this isn't related to your change but please sort the files as we were supposed to do before we got lazy ;)

ViktorHofer · 2017-08-19T13:56:46Z

src/System.Security.Permissions/tests/SecurityManagerTests.cs

+
+namespace System.Security.Permissions.Tests
+{
+    public class SecurityManagerTests


Also include a positive test?

vladimir-kazakov · 2017-08-19T20:30:52Z

I reverted all changes related to System.Security.Permissions. It seems that in order to download a cipher value from a URI, this has to be implemented in .NET Core in a different way than it was done in .NET Framework. This is more like a topic for another PR. In this one, the goal is to simply add missing members to EncryptedXml, so that it would be compatible with .NET Framework and Mono.

Speaking of Mono, currently it's not able to download cipher values from URIs as well, so not having an implementation for this case doesn't really change anything.

ianhays · 2017-08-21T18:35:04Z

It looks like we originally had these but removed them in ac17228#diff-8f0821d6d517dbaeb3868dab123e4238 as a part of #14662 because EncryptedXml..ctor(XmlDocument, Evidence)` was removed as a loosely CAS-ish notion.

And @bartonjs 's response: On the one hand, I think the type exists (System.Security.Permissions). On the other hand, it's dead code in this implementation... so... I'm content. We can add it back later if we decide that we need 100% API compatibility.

So the question then would be if it's worth adding these back even though they're empty variables and whether it would help or hinder in the long run since we're not using them anywhere.

bartonjs · 2017-08-21T19:21:26Z

@vladimir-kazakov Is your goal here to make Mono no longer have a distinct implementation and just use the package from Core?

danmoseley · 2017-08-21T20:13:36Z

System.Diagnostics.TraceSource.Tests is AV'ing.

vladimir-kazakov · 2017-08-21T21:39:49Z

@bartonjs, the goal is to directly use .cs files from .NET Core in Mono. It looks like this: https://github.com/mono/mono/blob/master/mcs/class/System.Security/System.Security.dll.sources#L115. In other words, the code is the same, but frameworks are different.

According to comments in issues and PRs from this repository related to System.Security.Cryptography.Xml, people were discussing whether there should be a new API or the straight port from .NET Framework. If I understood everything correctly, the "new API" idea didn't receive much support due to its complexity. The decision was made to make a 100% API-compatible port of what .NET Framework has. Changes in this PR are adding missing APIs that I found while trying to use the code from .NET Core in Mono. Although I'm talking about Mono, these changes also make .NET Core compatible with .NET Framework, because Mono must be fully compatible with .NET Framework.

So the question then would be if it's worth adding these back even though they're empty variables and whether it would help or hinder in the long run since we're not using them anywhere.

@ianhays, empty right now, but perhaps someone in the future will decide to implement missing functionality. Someone who was using it in .NET Framework and who needs it in either .NET Core or Mono. But this is the future. Currently, only missing APIs are needed (without implementation) in order to make it possible to reuse code from .NET Core in Mono. Right now, the implementation is not important, because both .NET Core and Mono don't have the implementation of missing functionality (the ability to download cipher values from URIs). In the future, though, I'd expect this functionality to be implemented, in order to be fully compatible with .NET Framework.

danmoseley · 2017-08-26T06:32:09Z

@dotnet-bot test this please (no log left)

bartonjs · 2017-08-28T16:19:14Z

Seems fine to me. I'm happy with getting code reuse out of the effort.

@ericstj System.Security.Cryptography.Xml is a standalone package that has a netfx facade... is there anything tricky we need to do here when changing the ref to include members that netfx has that we didn't provide in the last stable package version?

ericstj · 2017-08-28T16:25:25Z

@ericstj System.Security.Cryptography.Xml is a standalone package that has a netfx facade... is there anything tricky we need to do here when changing the ref to include members that netfx has that we didn't provide in the last stable package version?

So long as they are all present in the supported version of desktop then, you can update the ref and keep it building for all targeted frameworks (make sure you don't redist anything). Note that desktop isn't the only thing to think about when you expose more API from one of these. Also consider Mono/Xamarin to understand if they have the API as well. If you are exposing new types it's good to let folks like @marek-safar know so that they can ensure that they update the facades.

Contrast that to adding members that didn't exist in desktop: you'd have to make those additions exclusive to NETCoreApp.

ianhays

The changes LGTM assuming we want the api exposed for all targetgroups.

karelz · 2017-09-06T16:44:24Z

@dotnet-bot test Windows x64 Debug Build

karelz · 2017-09-06T16:44:51Z

@bartonjs can you please help get it through while @ianhays is OOF?

bartonjs · 2017-09-06T17:47:50Z

@dotnet-bot Test Windows x64 Debug Build please

joperezr · 2017-09-11T17:42:56Z

This PR added a line that broke our builds. https://github.com/dotnet/corefx/pull/23417/files#diff-e86e9429bdfda04e0301ba8bd13932ebR13

Ref projects should have P2P references as opposed to just Reference items, since we can't really ensure that all dependencies have been built yet.

joperezr · 2017-09-11T17:49:47Z

I created #23946 to fix it.

…d-xml-missing-apis Add missing members to EncryptedXml. Commit migrated from dotnet/corefx@46579e4

dnfclas added the cla-already-signed label Aug 19, 2017

ViktorHofer reviewed Aug 19, 2017

View reviewed changes

ViktorHofer added the area-System.Security label Aug 19, 2017

Add missing members to EncryptedXml.

de5dfce

danmoseley requested a review from krwq August 19, 2017 21:38

karelz assigned vladimir-kazakov, krwq, ianhays and bartonjs Aug 20, 2017

ianhays approved these changes Aug 28, 2017

View reviewed changes

bartonjs merged commit 46579e4 into dotnet:master Sep 7, 2017

vladimir-kazakov deleted the encrypted-xml-missing-apis branch September 7, 2017 20:57

kingces95 mentioned this pull request Sep 11, 2017

Fixing build break #23946

Merged

karelz added this to the 2.1.0 milestone Oct 11, 2017

bartonjs removed their assignment Jan 29, 2018

ericstj mentioned this pull request Sep 23, 2020

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

Closed

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#23417 from vladimir-kazakov/encrypte…

3ef94df

…d-xml-missing-apis Add missing members to EncryptedXml. Commit migrated from dotnet/corefx@46579e4

Add missing members to EncryptedXml. #23417

Add missing members to EncryptedXml. #23417

Uh oh!

Conversation

vladimir-kazakov commented Aug 19, 2017

Uh oh!

ViktorHofer Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Aug 19, 2017

Uh oh!

ViktorHofer Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Aug 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

ViktorHofer Aug 19, 2017

Choose a reason for hiding this comment

Uh oh!

vladimir-kazakov commented Aug 19, 2017

Uh oh!

ianhays commented Aug 21, 2017

Uh oh!

bartonjs commented Aug 21, 2017

Uh oh!

danmoseley commented Aug 21, 2017

Uh oh!

vladimir-kazakov commented Aug 21, 2017

Uh oh!

danmoseley commented Aug 26, 2017

Uh oh!

bartonjs commented Aug 28, 2017

Uh oh!

ericstj commented Aug 28, 2017

Uh oh!

ianhays left a comment

Choose a reason for hiding this comment

Uh oh!

karelz commented Sep 6, 2017

Uh oh!

karelz commented Sep 6, 2017

Uh oh!

bartonjs commented Sep 6, 2017

Uh oh!

joperezr commented Sep 11, 2017

Uh oh!

joperezr commented Sep 11, 2017

Uh oh!

Uh oh!

ViktorHofer Aug 19, 2017 •

edited

Loading

ViktorHofer Aug 19, 2017 •

edited

Loading