search

tintoy / corefx

This repo contains the .NET Core foundational libraries, called CoreFX. It includes classes for collections, file systems, console, XML, async and many others.
https://docs.microsoft.com/dotnet/
MIT License
1 stars 1 forks source link

[System.Security.Cryptography.Xml] Test plan for public classes #3

Open tintoy opened 7 years ago

tintoy commented 7 years ago

Part of dotnet/corefx#4278.

The goal is to write automated tests for the System.Security.Cryptography.Xml namespace such that it can be incorporated into the corefx libraries.

There is no articulated quality bar, like a code coverage percentage. However, there is an implicit high bar for the quality set for corefx. This is security sensitive so additional testing of possible attacks and ensuring no foreseeable vulnerabilities is required.

The tests should ensure the code complies with the spec. It should also revisit the changes introduced in MS16-035. Details on the spec and security advisory will be added later.

There is no deadline or other requirements.

Please contact @tintoy or @anthonylangsworth if you have questions. Participation by others is welcome and encouraged. To avoid duplication of effort, please reach out to @tintoy and @anthonylangsworth before doing so.

Methodology

Use the feature/xml-crypto/tests branch as a starting point for your work; the projects in this branch have been modified so that tests can be built and run.

To run tests:

  1. Build CoreFx on Windows.
  2. Load the solution file src\System.Security.Cryptography.Xml\System.Security.Cryptography.Xml.sln in Visual Studio 2015 SP 1 or later, set the System.Security.Cryptograpy.Xml.Tests project as the startup project and hit Ctrl+F5.

Tests:

  1. must be written using xUnit, as per the standard for .Net core.
  2. must follow the C# Coding style guidelines.
  3. should ensure the corefx version follows the behavior of the existing System.Security.Cryptography.Xml namespace. If this is not possible, such as from missing dependencies or platform differences, please raise this for further discussion with @tintoy and @anthonylangsworth.
  4. must be committed to the branch via pull request so it can be reviewed. The goal is to get multiple eyes on the tests to ensure we meet our goal and share understanding.
  5. must pass.

Test Plan

Creation of automated tests follows two phases:

  1. Writing tests that cover all exposed public classes and methods. The goal is to ensure all exposed methods and properties of all classes work.
  2. Drill into use of SignedXml to ensure it meets the standard, effectively integration and security tests. This builds upon the foundation of the previous phase with the confidence that the supporting classes work.

Phase 1 (Unit Testing)

The following table lists the public classes exposed by the System.Security.Cryptography.Xml namespace. These will be checked off once the tests are completed and reviewed.

As stated above, if you wish to contribute, please reach out to @tintoy and @anthonylangsworth to avoid duplication of effort.

Phase 2 (Spec and Integration Testing)

TBA once we pass phase 1.

Questions

tintoy commented 7 years ago

Hey @anthonylangsworth - as suggested by @steveharter, Mono has some tests we might be able to include (especially for SignedXml).

BTW, if we do include them, he has specific headers that need to be added to the files in question (license-related).

anthonylangsworth commented 7 years ago

I saw that, @tintoy . I missed the link provided and asked him about them. As you can see, I have also asked what happens if we expand on them.

peterwurzinger commented 7 years ago

Hey there! As already mentioned, I'd like to get my hands dirty with this too :-) So if you agree I would like to write some Tests for DataObject, just to get an easy start into this framework-testing-thing. I'm familiar with xunit, but only applied to web application testing, so testing a library is new to me.

EDIT: Sorry, missed the ping @tintoy @anthonylangsworth

anthonylangsworth commented 7 years ago

Hi @peterwurzinger . Thanks for helping out. Please go ahead. I have added your name to the list above.

Jaedson33 commented 7 years ago

Hi, what can I do to help you? Because I really need to use that in my project!

tintoy commented 7 years ago

Let me have a chat to Anthony - we should be able to split up the work now.

Jaedson33 commented 7 years ago

If the mono tests work well will we need to create tests to those classes?

anthonylangsworth commented 7 years ago

Once we have the mono tests working, let's review this list. The mono tests are good but there are still lots of gaps.

For example, focusing on the issue of the moment, there was only a single test for each of ToXmlString and FromXmlString on DSA and RSA. We should have a much broader coverage, handling missing and extraneous XML elements.

That said, staging is also an option. We could implement a basic set of tests, merge them into .Net Core then follow up with an expanded set of tests. We can discuss this with the .Net Core team but it is there decision, not ours.

krwq commented 7 years ago

@tintoy @anthonylangsworth @peterwurzinger I'll start working on fixing the tests now - I'll take a look into all PlatformNotSupportedException failures first

anthonylangsworth commented 7 years ago

@krwq Thanks for that. Maybe it is worth creating a branch and all of us contributing to that so we can use each other's code instead of waiting for approved PRs to master. I'll create one tonight or over the weekend if no one jumps in first.

krwq commented 7 years ago

@anthonylangsworth - I'm hoping to fix all the mono tests before weekend 😄

anthonylangsworth commented 7 years ago

@krwq For me, the weekend starts in about 8 hours. :-)

Either way, I'll want to clean up the class names (Test.cs vs Tests.cs), flesh out the tests (lots of basic cases missing) and convert the Assert statements to use Xunit features where applicable (e.g. remove AssertCrypto.cs. I also want to cover the cases in mentioned in the security advisory.

krwq commented 7 years ago

@anthonylangsworth @tintoy as I started fixing one of the tests I'm seeing it is different than the original mono test where original one is closer to working - were there many changes to original code? i.e. SignedXmlTest.SymmetricMACTripleDESSignature (Assert.Throws was added where I do not see any exception being thrown - even on full .NET)

@anthonylangsworth we should refrain from any refactoring for now - let's make it work first and after that we can focus on refactoring :-) it's 4PM Thursday here

anthonylangsworth commented 7 years ago

@krwq Regarding SymmetricMACTripleDESSignature, I am pretty sure that was one of the mono ported tests (you can tell by the header). I have not worked on it and I am pretty sure no one else has either. I'll have a look at it later today if you like. Otherwise, I'd go ahead and make whatever changes you think are necessary.

@krwq Regarding my second paragraph, I am only referring to the tests, not refactoring. Sorry if that was unclear. In summary, I want to clean up things while this namespace has some focus.

tintoy commented 7 years ago

@anthonylangsworth I see what he means though - old version (when first added to the repository) was:

SignedXml signedXml = MSDNSample ();

// Compute the signature.
byte[] secretkey = Encoding.Default.GetBytes ("password");
MACTripleDES hmac = new MACTripleDES (secretkey);
signedXml.ComputeSignature (hmac);

new version is:

SignedXml signedXml = MSDNSample();

// Compute the signature.
byte[] secretkey = Encoding.Default.GetBytes("password");
using (HMAC hmac = HMACSHA256.Create())
{
    hmac.Key = secretkey;
    Assert.Throws<CryptographicException>(() => signedXml.ComputeSignature(hmac));
}
tintoy commented 7 years ago

(the first diff is very noisy because of code-style changes required by mono vs corefx (e.g. the spacing before parentheses, but I've copied and pasted to be sure)

anthonylangsworth commented 7 years ago

@krwq @tintoy Sorry. I think that one was me. The previous code did not compile. Too many test cases. They all merge into one another after a while.

tintoy commented 7 years ago

I know what you mean :-)

krwq commented 7 years ago

@anthonylangsworth no worries - I feel you :-) I wouldn't start any renames or clean ups even in tests without having them working first (unless it makes it easier to fix test).

I'll be sending PRs before leaving work so you can always pick up from wherever I left. To pull changes from my PR you can use: git fetch corefxremote pull/<PRnumber>/head:local-branch-name

I usually end before 2h from now (not sure what time that is in Sydney)

For SymmetricMACTripleDESSignature - are you sure:

            using (HMAC hmac = new HMACSHA256())
            {
                hmac.Key = secretkey;
                //..
             }

is equivalent to new MACTripleDES (secretkey); ? I'm fairly new to crypto library so that might be a stupid question

krwq commented 7 years ago

@tintoy @anthonylangsworth for the private branch - you can send PRs to my branch I can send joint PR in the end: https://github.com/krwq/corefx/tree/signed-xml-fixing-tests-1

I haven't had much progress as I started doing some reading on some of those algorithms to get some more context on it

anthonylangsworth commented 7 years ago

@krwq I think that may have been a copy and paste issue on my part. A Triple DES MAC (the last block of a Triple DES CBC encryption) is different to a HMAC e.g. SHA256 using a secret salt. I probably missed it in the sea of failing tests. So saying, we probably want to eventually retire the Triple DES MAC as it is considered very weak, if not broken.

krwq commented 7 years ago

@anthonylangsworth - I'll recheck internally if that's why the type is not available (sounds likely). In either case it still might be valid to make validation against TripleDES and obsolete only signing - I'll check with guys what's the plan

Starting tomorrow I'll be fixing errors similar to:

System.NotImplementedException : http://www.w3.org/2000/09/xmldsig#dsa-sha1

I'm seeing only one actually but it sounds like we might have some test gap

@anthonylangsworth - I think all the errors which look like:

      Assert.Throws() Failure
      Expected: typeof(System.Security.Cryptography.CryptographicException)

might be similar mistake to 3DES - I think this is a good place to figure out for you - you can send a separate PR to corefx or put it in my branch (it will be easier to push it quicker since I'm in the same time zone)

peterwurzinger commented 7 years ago

Hi

@krwq I made some changes especially to DSAKeyValueTests, since some of the tests were just wrong (e.g. testing a wrong Schema). You may encounter some differences to the original code there.

krwq commented 7 years ago

@peterwurzinger where did you put your change?

@anthonylangsworth @tintoy we can safely remove MACTripleDES tests for now - I got info that we got really low usage of them, not recommended and poorly supported on different platforms (3DES stays though, only MAC3DES is gone at least for now)

anthonylangsworth commented 7 years ago

Thanks, @krwq . That makes sense. At best, MACTripleDES would be for backwards compatibility. There is no reason to use it how with salted hashes, e.g. HMAC256. The latter are faster and have fewer collisions.

peterwurzinger commented 7 years ago

@krwq These changes are already included in the merged monotest-branch. I made them about a week ago already

krwq commented 7 years ago

@peterwurzinger could you point me to the commit or file? I can't seem to be able to find it. For me it looks like those tests weren't changed since 2009: https://github.com/mono/mono/commits/master/mcs/class/System.Security/Test/System.Security.Cryptography.Xml/DSAKeyValueTest.cs

I think we will have to manually port them over here

krwq commented 7 years ago

@tintoy @anthonylangsworth: SignElementWithoutPrefixedNamespace looks like an interesting one - I got no clue right now. I probably won't be making any commits over the weekend unless there is broken build - I will be monitoring github though - if you want you could help me with some of those tests

tintoy commented 7 years ago

One or the other of us will probably be able to figure it out (or fail trying) 😉

krwq commented 7 years ago

Ok, I think we need to get rid of those files - they cause my trouble than they solve and we have already spend enough time on this - what do you think? 😉 (unless you know how to properly escape it then)

tintoy commented 7 years ago

We can just use a custom XmlResolver - pretty sure that'll cover >90% of the scenarios I've seen.

krwq commented 7 years ago

I dont' have any energy left for it 😜 I only did a small commit but each issue was fairly unique and required some reading (code/learning about algorithms etc). I hope we can get all of them passing (except for ones which also fail on net45 - those we can skip but we need to verify they fail for the same reason)

tintoy commented 7 years ago

I don't know how you managed to get through as many as you did in a single sitting - it's impressive :)

We'll put some fresh eyes on it soon enough.

peterwurzinger commented 7 years ago

@krwq These two commits affected DSAKeyValueTest.cs https://github.com/tintoy/corefx/commit/9ab96f4a206ba50490064b0259781c12a2cac00d https://github.com/tintoy/corefx/commit/d96bc788f1e86125015535f4632119325cb5d6a6

anthonylangsworth commented 7 years ago

@krwq @tintoy @peterwurzinger I have (hopefully) resolved the concurrency issues with https://github.com/krwq/corefx/pull/1.

krwq commented 7 years ago

@peterwurzinger @anthonylangsworth are those two commits already merged then? I think we merged that whole branch

peterwurzinger commented 7 years ago

They were included in the monotest-branch, so in my opinion they should already be merged.