Add ToXmlString and FromXmlString implementations to RSA and DSA. #37593

bartonjs · 2019-05-10T18:32:55Z

The ToXmlString implementations produce output identical to .NET Framework.

The FromXmlString implementations are based on XDocument in Core, vs a
custom parser in Framework. Additionally, the FromXmlString in Core can
read values which (per the xmldsig spec) removed any leading zero-value
bytes, whereas the Framework version can't.

No ToXmlString or FromXmlString is being added for ECDsa or
ECDiffieHellman, because these types have always thrown in .NET Framework.
The equivalent functionality was provided by an overload on ECDsaCng (and
ECDiffieHellmanCng) that took a format-type enum (with only one member
defined in it). Since that's not portable, and telemetry has never
seen a caller of that method, they are being left as PNSE.

Fixes #23686.

The ToXmlString implementations produce output identical to .NET Framework. The FromXmlString implementations are based on XDocument in Core, vs a custom parser in Framework. Additionally, the FromXmlString in Core can read values which (per the xmldsig spec) removed any leading zero-value bytes, whereas the Framework version can't. No ToXmlString or FromXmlString is being added for ECDsa or ECDiffieHellman, because these types have always thrown in .NET Framework. The equivalent functionality was provided by an overload on ECDsaCng (and ECDiffieHellmanCng) that took a format-type enum (with only one member defined in it). Since that's not portable, and telemetry has never seen a caller of that method, they are being left as PNSE.

filipnavara · 2019-05-13T15:11:22Z

I wonder why you chose to use XDocument and custom writers as opposed to XmlReader and XmlWriter which seem to be more suitable.

bartonjs · 2019-05-13T15:29:47Z

I wonder why you chose to use XDocument ... as opposed to XmlReader

The reader is (seems to be) a lot harder to work with via reflection (where enums have to be imported or copied).

I wonder why you chose to use ... custom writers as opposed to ... XmlWriter

Once there isn't really element nesting or attributes or anything that needs to be escaped, the writer is just a version of StringBuilder that has to be reflected, so it doesn't really provide value.

src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAXml.cs

krwq · 2019-05-14T04:36:41Z

src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs

+                Exception exception = Assert.ThrowsAny<Exception>(
+                    () => rsa.FromXmlString(
+                        @"
+<RSAKeyV


nit: extra enters

krwq · 2019-05-14T04:38:05Z

src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/RSA/RSAXml.cs

+                Exception exception = Assert.ThrowsAny<Exception>(
+                    () => rsa.FromXmlString(
+                        @"
+<RSAKeyV


I'm wondering if this is something we should fix

What kind of fix? Adding a namespace might break someone who is expecting it to not have one. The wrong namespace, and Base64Binary vs CryptoBinary (which, if fixed, would break import back in netfx) are just characteristics of these methods as deviations from the spec.

I think the design assumed it was being rendered inside an element that already declared the implicit inherited namespace.

fine with me, although I think if you try to nest it inside such document (using XML APIs) it will explicitly remove that namespace though

krwq · 2019-05-14T04:47:48Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/XmlKeyHelper.cs

+                {
+                    Type xDocument =
+                        Type.GetType(
+                            "System.Xml.Linq.XDocument, System.Private.Xml.Linq, Version=4.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51");


can you depend on System.Xml.XDocument instead of this? (it should forward to System.Private.Xml.Linq)

No. System.Private.Xml touches networking, networking touches crypto, so I can't touch System.Private.Xml.

Is that only runtime dependency though? https://github.com/dotnet/corefx/blob/master/src/System.Xml.ReaderWriter/ref/System.Xml.ReaderWriter.csproj touches only System.Net.Primitives

krwq · 2019-05-14T04:51:24Z

why is the reflection necessary? (I'm fine with using StringBuilder instead of XmlWriter as a workaround since all of the values seem to be known and encoded as base64)

krwq

LGTM codewise (with few tiny comments). Not sure if reflection is necessary though, if possible I'd prefer if we just use one of the old contracts (System.Xml.ReaderWriter/System.Xml.XDocument) - is there some layering issue there? cc: @ericstj

stephentoub · 2019-05-14T13:30:06Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/XmlKeyHelper.cs

+                Buffer.BlockCopy(ret, 0, ret, shift, written);
+                ret.AsSpan(0, shift).Clear();
+                return ret;
+            }


I'm not fully understanding the logic here. What's the purpose of sizeHint?

RSA.ImportParameters, and DSA.ImportParameters, have explicit requirements on the relationships of the sizes of the parameters.

For RSA: P, Q, DP, DQ, InverseQ must have lengths equal to half (round up) of Modulus and D must have the same length as Modulus.
For DSA: G and Y have to match P, X has to match Q.

stephentoub · 2019-05-14T13:35:09Z

src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/XmlKeyHelper.cs

+                if (!_enumerator.MoveNext())
+                {
+                    idx = -1;
+                    _enumerator = _enumerable.GetEnumerator();


So we loop through the whole enumerable many times?

In happy degenerate cases: just once. But every missing optional property requires checking all the values. Basically I modelled it after how it was already working in NetFX; but I had thought (and am now thinking again) that a one-loop-into-a-switch might be better.

bartonjs · 2019-05-15T04:48:24Z

Hm, it does look like I can reference System.Xml.ReaderWriter from System.Security.Cryptography.Algorithms... I feel like that used to be a cyclic dependency build error.

There's definitely a runtime problem:

System.Private.Xml references System.Net.Requests
System.Net.Requests references System.Security.Cryptography.X509Certificates
System.Security.Cryptography.X509Certificates references System.Security.Cryptography.Algorithms

I haven't made it as far as exploring if I can actually run the code... but: is using the facades like this stable? What we're really doing here is inducing a cyclic dependency, does the loader handle it OK? Deterministically?

I'm torn between pursuing the correctness of using XmlReader for reading Xml and sticking with the runtime reflection (cyclic) dependency, since that's a dependency model I've seen work for years.

bartonjs · 2019-05-15T05:54:01Z

In the interest of getting to other bugs, I'm going to merge this as-is, and I opened #37664 to see if we can avoid reflection.

bartonjs added the area-System.Security label May 10, 2019

bartonjs added this to the 3.0 milestone May 10, 2019

bartonjs self-assigned this May 10, 2019

bartonjs requested review from krwq, GrabYourPitchforks and stephentoub May 10, 2019 18:33

bartonjs added 9 commits May 10, 2019 12:57

Merge remote-tracking branch 'dotnet/master' into asymm_tofrom_xml

524fa7c

Fix messaging on RSA XML D mismatch

80e28c5

Make tests that use RSA16384 not run on macOS (no OS support)

a48d773

Support alternate D value for 1032 and 16384 write tests

a8009b7

Disable RSAXml.FromNonsenseXml pending 37595

802b861

Make DSAXml.FromNonsenseXml accept the different provider exceptions

3792ea6

RSAXml Missing_ tests should use ThrowAny

2034098

Also disable RSA FromNonsenseXml on macOS

0edc95a

Add missing condition on TestWrite16384Parameters

9b2ca80

jingsunsql added this to In progress in .NET Core impacting internal partners via automation May 13, 2019

jingsunsql removed this from In progress in .NET Core impacting internal partners May 13, 2019

jingsunsql added this to In progress in .NET Core impacting internal partners via automation May 13, 2019

krwq reviewed May 14, 2019

View reviewed changes

src/Common/tests/System/Security/Cryptography/AlgorithmImplementations/DSA/DSAXml.cs Show resolved Hide resolved

krwq reviewed May 14, 2019

View reviewed changes

krwq approved these changes May 14, 2019

View reviewed changes

stephentoub reviewed May 14, 2019

View reviewed changes

stephentoub approved these changes May 14, 2019

View reviewed changes

bartonjs merged commit 271138b into dotnet:master May 15, 2019

.NET Core impacting internal partners automation moved this from In progress to Done May 15, 2019

bartonjs deleted the asymm_tofrom_xml branch May 15, 2019 05:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ToXmlString and FromXmlString implementations to RSA and DSA. #37593

Add ToXmlString and FromXmlString implementations to RSA and DSA. #37593

bartonjs commented May 10, 2019

filipnavara commented May 13, 2019

bartonjs commented May 13, 2019

krwq May 14, 2019

krwq May 14, 2019

bartonjs May 14, 2019

krwq May 14, 2019

krwq May 14, 2019 •

edited

bartonjs May 14, 2019

krwq May 14, 2019

krwq commented May 14, 2019

krwq left a comment

stephentoub May 14, 2019

bartonjs May 14, 2019

stephentoub May 14, 2019 •

edited

bartonjs May 14, 2019

bartonjs commented May 15, 2019

bartonjs commented May 15, 2019

Add ToXmlString and FromXmlString implementations to RSA and DSA. #37593

Add ToXmlString and FromXmlString implementations to RSA and DSA. #37593

Conversation

bartonjs commented May 10, 2019

filipnavara commented May 13, 2019

bartonjs commented May 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krwq commented May 14, 2019

krwq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub May 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartonjs commented May 15, 2019

bartonjs commented May 15, 2019

krwq May 14, 2019 •

edited

stephentoub May 14, 2019 •

edited