Handle RsaSha1 properly in .NET Core 2.0

[timotei]

Description

There are some issues on .NET's side about the
missing functionality of RSACryptoServiceProvider
ToXmlString/FromXmlString:

• https://github.com/dotnet/corefx/issues/23686
• Rsa ToXmlString is not supported on .NET Core 2.0 dotnet/core#874

So, let's do it ourselves, and if in the future we'll
get it in the .NET itself, we can remove this code

Purpose

This pull request is a:

• Bugfix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)

@alexeyzimarev Would be awesome if we could get a release with this fix, right now we can't use the library in a .NET Core 2.0 & OAuth 1 app .

[timotei]

I see two failing tests, but they are not really related to anything I have changed...

[alexeyzimarev]

Can you make FromXmlString an extension method for the RSACryptoServiceProvider and put in to a separate file? You will need to call it differently, like FromXmlStringTemp since such method already exists there. Then we won't need to have pragma condition around the method, it will be optimised away by the compiler and the call would look nicer.

[timotei]

@alexeyzimarev I am not sure how we can get rid of the pragma condition, since we only have one assembly. Unless we always use that extension method? Or put the pragma in the extension method?

Extracted the method into an extensions class, and moved the pragma there, so in OAuthTools it looks like a simple call

[mjwsteenbergen]

Timotei, thank you for your PR! Some thinks worry me though:

• SHA1 is inherently unsafe. Do we want to support this unsafe behaviour as a library?
• If we want to provide backwards compatibility:
  • The first rule of crypto is: "Never implement your own crypto"
    • The code that was implemented has not been proven to correctly work
      • You say that the code came from this. The account connected to the account that made this, is barely active on GitHub and we can therefore not establish their intention or knowledge.
      • The code from the original issue has the same problem
      • Do you have any proof, any tests that prove that this crypto works and does not have any vulnerabilities?
  • We can use bouncy castle as this is a widely used crypto library

This is a library that is used by multiple companies in productions and we must be do everything to make sure that their data is protected and safe using this library. This change does the opposite.

[alexeyzimarev]

@mjwsteenbergen I actually disagree here:

• The PR provides value since it fixes something that is not yet implemented in .NET Core but is implemented in full framework. And, according to issues referenced, Microsoft plans to implement missing functionality and then we can remove this code.

• Sha1 is already supported. This PR just fixes it for one of the supported platforms.

• RestSharp for always been known to have no dependencies and it provides huge value since people do not need to think about versions of these dependencies and we don't need to ILMerge anything.

• The change does not compromise any part of security since Sha1 just does not work with .NET Core right now. So this means that any existing user of the library either does not use Sha1 in .NET Core or is having this issue. It cannot be like they are using Sha1 in .NET Core and this PR makes is less secure, this is just not the case.

[alexeyzimarev]

@mjwsteenbergen the referenced issues provide similar solutions and it seems no one from Microsoft or outside have complained: https://github.com/dotnet/corefx/issues/23686

All implementations that I can find look very similar, like here dazinator/rhino-licensing@1524fc1

[timotei]

@mjwsteenbergen It seems Alexey already responded similarly to what I wanted to say.

The issue with Bouncy Castle is that it's not fully supported on .NET Core either, for example it doesn't have some of the RSA utilities. Besides this, I was cautious not to introduce another library as dependency.

[mjwsteenbergen]

• The PR provides value since it fixes something that is not yet implemented in .NET Core but is implemented in full framework. And, according to issues referenced, Microsoft plans to implement missing functionality and then we can remove this code.
• Sha1 is already supported. This PR just fixes it for one of the supported platforms.

Sure, I can totally see that there is a usecase for supporting RSA-SHA1

• RestSharp for always been known to have no dependencies and it provides huge value since people do not need to think about versions of these dependencies and we don't need to ILMerge anything.

I also totally agree that less dependencies is better.

• So this means that any existing user of the library either does not use Sha1 in .NET Core or is having this issue.

True, but has nothing to do with if this implementation is secure.

The change does not compromise any part of security since Sha1 just does not work with .NET Core right now.

Right now we are saying that this library does not support SHA-1 for dotnet-core, because msft is not supporting it yet. With this pr we want to start supporting it. To do this we need to make sure that it works and is secure. I have not seen any proof of this. There are 0 tests that prove this. There are 0 people qualified looking at this. This is what worries me.

All implementations that I can find look very similar, like here dazinator/rhino-licensing@1524fc1

Yes, because they took it from that same issue. They even mention it in the original issue.

it seems no one from Microsoft or outside have complained

With many issues this can be a valid strategy, but with crypto I would highly recommend the opposite strategy: seeing if someone gives the ok on the code. How do you know that the guy from the original issue didn't just copied the code without looking carefully at it? How do you know everyone that found this issue didn't just copy the code without writing an exhausting test-suite to test if the implementation is correct? We can't and therefore I would highly recommend that a lot of testing is to be done before this pr is merged.

[alexeyzimarev]

I agree that we need tests. @timotei can you add tests to the PR?

[timotei]

@mjwsteenbergen I agree that I can add some tests, but this is not (so much) crypto-oriented as it is reading an XML document and parsing the relevant information out of it :)

@alexeyzimarev Sure.

[timotei]

@alexeyzimarev @mjwsteenbergen

I added a test that verifies that we get the same result as the .NET-based one. I think nothing can beat that, right? :)

Also, it seems that generating a wrong RsaParameters (maybe didn't put enough effort in it) triggeres an exception when we try to use them