Improve Code Coverage for System.Security.Cryptography.Xml (66.3%)

[krwq]

Here are the main areas which need improvement:

• [x] SignedXml.CheckSignature(X509Certificate2 certificate, bool verifySignatureOnly) - currently no coverage - sample code should be sufficient for now
• [x] System.Security.Cryptography.Xml.XmlDecryptionTransform - https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/System.Security.Cryptography.Xml_XmlDecryptionTransform.htm
• [x] System.Security.Cryptography.Xml.SymmetricKeyWrap - https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/System.Security.Cryptography.Xml_SymmetricKeyWrap.htm
• [ ] System.Security.Cryptography.Xml.KeyInfoClause - https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/System.Security.Cryptography.Xml_KeyInfoClause.htm
• [X] System.Security.Cryptography.Xml.EncryptedXml - https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/System.Security.Cryptography.Xml_EncryptedXml.htm - only most common scenarios
• [x] System.Security.Cryptography.Xml.EncryptionMethod - https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/System.Security.Cryptography.Xml_EncryptionMethod.htm
• [ ] System.Security.Cryptography.Xml.CanonicalXmlEntityReference - https://ci.dot.net/job/dotnet_corefx/job/master/job/code_coverage_windows/Code_Coverage_Report/System.Security.Cryptography.Xml_CanonicalXmlEntityReference.htm - at minimum one or two tests triggering use of this class

Guidelines:

• focus on writing tests which provide most value (rarely used scenarios can be targeted later)
• focus on public entry points

cc: @anthonylangsworth @tintoy @peterwurzinger

[StanislavUshakov]

The 2nd item (System.Security.Cryptography.Xml.XmlDecryptionTransform) is done in PR 17546 from 52% to 93% link to coverage report I will continue writing tests.

[StanislavUshakov]

The 3rd point (System.Security.Cryptography.Xml.SymmetricKeyWrap) is done in PR 17599

[StanislavUshakov]

@krwq @bartonjs Hi, I'm working on tests for EncrypteXml, found like there's a small issue in RoundtripSample1 EncryptedXmlTest.cs

edata.KeyInfo.AddClause(new KeyInfoEncryptedKey(ekey));
edata.KeyInfo = new KeyInfo();

Looks like the KeyInfo ibject is being overwritten by the empty one. I can fix it alongside with my added tests.

[krwq]

@StanislavUshakov thank you for spotting this! If that is a simple fix then you may fix it either together or separately. If it will resurface some bug in the product please do it separately.

[StanislavUshakov]

5th point is done in PR 17684 Overall coverage is 68.2% for System.Security.Cryptography.Xml Also I've removed

edata.KeyInfo = new KeyInfo();

from RoundtripSample1

[StanislavUshakov]

About 6th point and CanonicalXmlEntityReference. Right now CreateEntityReference method from CanonicalXmlDocument is not called. This happens because of the following. There are 2 different types how XmlTextReaderImpl class can handle references:

public enum EntityHandling
    {
        // Expand all entities. This is the default in XmlValidatingReader. No nodes with NodeType EntityReference will be returned. 
        // The entity text is expanded in place of the entity references.
        ExpandEntities = 1,

        // Expand character entities only and return general entities as nodes (NodeType=XmlNodeType.EntityReference, Name=the name of the entity).
        // Default in XmlTextReader. You must call ResolveEntity to see what the general entity expands to.
        ExpandCharEntities = 2,
    }

And XmlTextReaderImpl has 2 constructors: 1 - First sets _entityHandling to ExpandCharEntities 2 - Second sets _entityHandling to ExpandEntities The first constructor is called for all XmlDocument.Load methods (accepting Stream, filename, TextReader) except public virtual void Load(TextReader txtReader) In the constructors of ExcCanonicalXml we call XmlDocument.Load(TextReader txtReader) which means that all references will be resolved automatically resulting in not calling method creating CanonicalXmlEntityReference. We can:

1. Add a flag indicating how we what to deal with the references and construct XmlTextReader instead of XmlReader.Create or pass Stream to XmlDocument.Load
2. Leave as is without any breaking changes, code coverage for CanonicalXmlEntityReference will be 0%..

[krwq]

@StanislavUshakov - I haven't investigated this code path too much but can we possibly make SignedXml create us an instance of CanonicalXmlDocument (possibly some API returns it) and then manually create entity reference by simply calling an API?

If we can't: Is this also true for netfx? (not being able to test that) If it is possible to do it on netfx we need to figure out what is the diff and fix that. If we could never create an instance of that class then we should not test it for 2.0 (that would be a new feature which is not a target for 2.0) and for post 2.0 we should figure out if this is something we should support or not - do you possibly know of any E2E scenarios we would be missing?

[jaedson-barbosa]

Hi guys, you can get a version of that project that works in a UWP app here: https://github.com/JaedsonBarbosa/corefx/tree/BigOptimization/src/System.Security.Cryptography.Xml/UWP

[garg000dhruv]

@krwq First-time contributor here. Not sure if this issue is still being worked on but if not, I'd like to tackle it.

[krwq]

Hello @garg000dhruv, thanks for offering your help. I believe no one else is working on this at the moment so you can go ahead and start 😄 Please let me know if you need any help

[KindOfANiceGuy]

@krwq @garg000dhruv First time contributor here, is this issue still being worked on actively? I've had some experience with some of these classes at my current job. I'd like to contribute if possible.

[krwq]

@KindOfANiceGuy I'm not aware of anyone working on it at the moment and I believe it's free to being picked up.

Please make sure to read https://github.com/dotnet/corefx/#reporting-security-issues-and-security-bugs before sending out PRs/filing issues 😄

[KindOfANiceGuy]

@krwq A few questions.

1. It appears from the discussion above that at one point, there was some debate around the usablity/testability of the CanonicalXmlEntityReference class. Did this ever get resolved? From what I can see, the scenario that was described above is still very much the case.

2. Regarding KeyInfoClause, this is an abstract class where the default implimentation of internal virtual XmlElement GetXml(XmlDocument xmlDocument) is never called. Several classes inherit from KeyInfoClause, and all override the default implementation. Is there another way to test this that you can think of? For now, it's more or less unreachable code from the tests project.

Looking forward to your thoughts. 😄

[krwq]

@KindOfANiceGuy I haven't touched this code in a while so my memory might be a bit fuzzy

1. We haven't had any conclusion on that - I'd recommend trying out if you can find a way to test it but if you don't then we can leave it as is
2. What you say essentially suggests that this is a dead code (non public API with nothing using it) - I'd recommend changing it to abstract method and remove the body and see if the code still works - if something breaks that should tell you how to hit that code path and if not then we have less code

[bartonjs]

FWIW, I don't think that the internal virtual can be made internal abstract, because then out-of-assembly derived types would be unable to provide a method for it. But it could be internal virtual WhateverTheRestOfTheSignatureIs() { Debug.Fail($"Internal types are expected to override {nameof(TheMethod)}"); throw new NotImplementedException(); }

[krwq]

@bartonjs I thought this was entirely internal type, in that case it might be possible to test this method by simply inheriting from this class and touching something which calls it

[KindOfANiceGuy]

@krwq I had that same idea, but it would still be impossible to define an out-of-assembly derived type that called the base implementation of the method in question, right?

[krwq]

@KindOfANiceGuy I thought there was something touching that code already in there but now looking at the implementation there is nothing really interesting to test there so it's fine to skip

[KindOfANiceGuy]

@krwq Alright. I'll continue investigating the CanonicalXmlEntityReference class and see if I can come up with anything.

[KindOfANiceGuy]

@krwq So it would appear that the CanonicalXmlEntityReference class is completely encapsulated. There's no API we can access from outside the assembly. Following your suggestion from above, I compared it with the version in the .NET Framework. It didn't look like there was very much difference in the way it had been implemented, and is basically internal all the way up the chain. So looks like this is more or less untestable in the current implementation.

[deeprobin]

@krwq Can you update the links if this issue is still relevant? ci.dot.net is not reachable

[bartonjs]

It's still relevant, but we no longer have code coverage runs that are automatic and update a website.

System.Security.Cryptography.Xml.CanonicalXmlEntityReference apparently has literally 0 coverage.

C:\git\bartonjs\runtime\src\libraries\System.Security.Cryptography.Xml\tests>dotnet msbuild /t:Test /p:Coverage=true

Let it run... then open the report

>start C:\git\bartonjs\runtime\artifacts\bin\System.Security.Cryptography.Xml.Tests\Debug\net7.0\report\index.html

Once you have the browser window open adding tests and rerunning coverage will just update that report, and you can just hit F5/refresh.