Remove Obsolete APIs

[singhashish-wpf]

Fixes Issue #3660

Description

WPF is using obsolete APIs. These need to be replaced for 6.0.

WebRequest.WebRequest()
WebRequest.Create(Uri)
Uri.EscapeUriString(string)

Customer Impact

Warnings related to various Obsolete APIs

Regression

No

Testing

Build
CI/CD

Risk

[vishalmsft]

@singhashish-wpf As per comments above, the caller is expecting exception, if cryptographic operations fails. GetRSAPrivateKey won't do that.
Please cross check the consequence of not having the exceptions.

[singhashish-wpf]

Throwing exception if the privatekey is null.
Commit : c43cd8d

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ThomasGoulet73]

There are still some usage of ReliabilityContractAttribute in C++/CLI assemblies: https://github.com/dotnet/wpf/search?l=C%2B%2B&q=4950. I think those should be removed as well if they are removed in C# code. They were probably not found at first because they do not use the same warning code. C++/CLI raise the warning with the code 4950 for usage of APIs with ObsoleteAttribute.

[ryalanms]

Please do not merge this until our testing is completed later today.

[SamBent]

You are now throwing an exception if all three of the Get*Key methods return null.

1. Does cert.PrivateKey also throw in that case? I couldn't find a doc that guaranteed that.
2. Does it throw the same exception, with the same message? If not, this is a breaking change.
3. Does the message need to be localized? If it's visible to users (i.e. not caught by WPF code), then yes.
4. The comment at the head of the method says the keys returned by PrivateKey are different from Get*Key. If answer to (1) is 'no', any code that acts on that difference is now obsolete. Is there such code? (Not visible in this review.)

[ryalanms]

https://docs.microsoft.com/en-us/dotnet/api/system.security.cryptography.x509certificates.x509certificate2.privatekey?view=net-5.0.

For 1, there is no specific language about a null key throwing, but it does say an invalid RSA, DSA, or 'unreadable' key will throw CryptographicException. One indication that it throws if everything is null is that the callers like PackageDigitalSignatureManager.Sign catch but do not handle the null case.

For 2/3, we can leave the 509certificate2.privatekey replacement for RC2 and check in everything else. Or if 509certificate2.privatekey can only throw a single error code in CryptographicException when all three Get*Key methods return null, then it can be replaced now.

For 4, I read the comment as the PrivateKey property was used as a replacement for the Get*PrivateKey methods to intentionally cause an exception, rather than returning null.

[SamBent]

That doc doesn't mention ECDsa keys at all, so I'm not sure whether PrivateKey can be an ECDsa key at all, and if so whether c.PrivateKey throws. The doc could be literally true, or simply out-of-date since ECDsa was added. Have you or Ashish asked the crypto team?

In any case, if we called c.PrivateKey to cause a user-visible exception, we are now throwing that exception ourselves and for compat we must throw the same exception (we do, if doc is correct) with the same message (unlikely), and that message needs localization. Ask the crypto team what message they use, and use SR to localize it. Plenty of examples in our code.

[SamBent]

Same concerns as my remark in XmlDigitalSignatureProcessor.cs. Except this exception is caught 8 lines down, so it's not visible to user.

[ThomasGoulet73]

The try/catch could probably be removed and just set requestAgain to true instead of throwing since none of the Get*Key methods are documented to throw CryptographicException.

[ryalanms]

DRTs and Microsuite tests showed no regressions vs the most recent changes in main.

[SamBent]

Approving the version without crypto changes.