[API Proposal]: Obsolete RSACryptoServiceProvider.Decrypt,Encrypt with fOAEP

[vcsjones]

Background and motivation

Inspired by #113555. I don't think we are quite in a position to obsolete RSACryptoServiceProvider entirely, but it would be nice if it were possible.

In lieu of that, we might want to consider obsoleting

• public byte[] Encrypt(byte[] rgb, bool fOAEP)
• public byte[] Decrypt(byte[] rgb, bool fOAEP)

With the recommendation to replace them with

• public byte[] Encrypt(byte[] data, RSAEncryptionPadding padding)
• public byte[] Decrypt(byte[] data, RSAEncryptionPadding padding)

The primary motivating factor here is that fOAEP: true means "OAEP SHA-1". Since this will always mean SHA-1, and SHA-1 is discouraged now, the right API should always be the one that accepts an RSAEncryptionPadding so that OAEP algorithm is not defaulted to SHA-1.

In other words, the bool is making the OAEP/MGF-1 hash algorithm non-apparent.

Developers would be expected to replace

• Decrypt(data, fOAEP: true) ➡️ Decrypt(data, RSAEncryptionPadding.OaepSHA1)
• Decrypt(data, fOAEP: false) ➡️ Decrypt(data, RSAEncryptionPadding.Pkcs1)
• Encrypt(data, fOAEP: true) ➡️ Encrypt(data, RSAEncryptionPadding.OaepSHA1)
• Encrypt(data, fOAEP: false) ➡️ Encrypt(data, RSAEncryptionPadding.Pkcs1)

API Proposal

namespace System.Security.Cryptography;

public partial sealed class RSACryptoServiceProvider {
+   [Obsolete("Encrypt and Decrypt on RSACryptoServiceProvider that accept a boolean value for OAEP are obsolete. Use an overload that accepts RSAEncryptionPadding.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
    public byte[] Decrypt(byte[] rgb, bool fOAEP);
+   [Obsolete("Encrypt and Decrypt on RSACryptoServiceProvider that accept a boolean value for OAEP are obsolete. Use an overload that accepts RSAEncryptionPadding.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
    public byte[] Encrypt(byte[] rgb, bool fOAEP);
}

API Usage

None.

Alternative Designs

If there is a "better" obsoletion for RSACryptoServiceProvider that we can pursue, I would be in favor of that.

Risks

No response

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

[KalleOlaviNiemitalo]

The EncryptedXml.EncryptKey and EncryptedXml.DecryptKey methods take bool useOAEP parameters, but they do not have alternative methods with RSAEncryptionPadding parameters, so I don't think they can be obsoleted yet.

There is also RsaProtectedConfigurationProvider.UseOAEP, but it currently throws PlatformNotSupportedException (#19838).

[bartonjs]

EncryptedXml is itself obsolete (whether or not we've painted it that way), so it not having an alternative doesn't really inhibit this proposal.

[bartonjs]

Regarding "man, if only we could deprecate RSACryptoServiceProvider"... we should obsolete all of the constructors that don't take a CspParameters. If you're not using it for Win32 interop you're holding it wrong.

That doesn't mean we shouldn't also obsolete these members, because some API might be stuck with accepting one of these as a parameter, and might have decided to leave special RSACSP code around.

[vcsjones]

Regarding "man, if only we could deprecate RSACryptoServiceProvider"... we should obsolete all of the constructors that don't take a CspParameters.

I thought about it, but backed away from the idea in case someone does

RSACryptoServiceProvider rsa = new();
rsa.ImportParameters(...);
CspKeyContainerInfo ci = rsa.CspKeyContainerInfo;
string containerName = ci.KeyContainerName;
// pass containerName to some API that is expecting an imported key in to that container

[bartonjs]

Bah! That type annoys me 😄

[vcsjones]

Just a suggestion, but nothing pressing; so I will mark as future for now.

[bartonjs]

Video

• Looks good as proposed, use the next available diagnostic ID.
• Let's make sure the docs say a) these methods are bad, b) what to do as equivalent calls, and c) that those calls are themselves probably not good.

namespace System.Security.Cryptography;

public partial sealed class RSACryptoServiceProvider {
+   [Obsolete("Encrypt and Decrypt on RSACryptoServiceProvider that accept a boolean value for OAEP are obsolete. Use an overload that accepts RSAEncryptionPadding.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
    public byte[] Decrypt(byte[] rgb, bool fOAEP);
+   [Obsolete("Encrypt and Decrypt on RSACryptoServiceProvider that accept a boolean value for OAEP are obsolete. Use an overload that accepts RSAEncryptionPadding.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
    public byte[] Encrypt(byte[] rgb, bool fOAEP);
}