Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider obsoleting RSA.EncryptValue, RSA.DecryptValue #73969

Closed
reflectronic opened this issue Aug 15, 2022 · 8 comments · Fixed by #76514
Closed

Consider obsoleting RSA.EncryptValue, RSA.DecryptValue #73969

reflectronic opened this issue Aug 15, 2022 · 8 comments · Fixed by #76514
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@reflectronic
Copy link
Contributor

reflectronic commented Aug 15, 2022

RSA.EncryptValue and RSA.DecryptValue have been obsolete and non-working since .NET Framework 4.6 (and were removed initially from .NET Core). A comment in the .NET reference source suggests that the members should have been marked obsolete long ago, but weren't because for .NET Framework compatibility reasons. Since .NET doesn't have these issues anymore, adding these obsoletions should now be straightforward.

namespace System.Security.Cryptography;

public partial class RSA : AsymmetricAlgorithm
{
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    public virtual byte[] EncryptValue(byte[] rgb);
    
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    public virtual byte[] DecryptValue(byte[] rgb);
}

public partial class RSACryptoServiceProvider : RSA
{
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    public override byte[] EncryptValue(byte[] rgb);
    
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    public override byte[] DecryptValue(byte[] rgb);
}
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 15, 2022
@ghost
Copy link

ghost commented Aug 15, 2022

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

Issue Details

RSA.EncryptValue and RSA.DecryptValue have been obsolete and non-working since .NET Framework 4.6 (and were removed initially from .NET Core). A comment in the .NET reference source suggests that the members should have been marked obsolete long ago, but weren't because for .NET Framework compatibility reasons. Since .NET doesn't have these issues anymore, adding these obsoletions should now be straightforward.

namespace System.Security.Cryptography;

public partial class RSA : AsymmetricAlgorithm
{
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException.")]
    public virtual byte[] EncryptValue(byte[] rgb);
    
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException.")]
    public virtual byte[] DecryptValue(byte[] rgb);
}

public partial class RSACryptoServiceProvider : RSA
{
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException.")]
    public override byte[] EncryptValue(byte[] rgb);
    
+   [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException.")]
    public override byte[] DecryptValue(byte[] rgb);
}
Author: reflectronic
Assignees: -
Labels:

area-System.Security

Milestone: -

@bartonjs bartonjs added this to the 8.0.0 milestone Aug 15, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 15, 2022
@bartonjs
Copy link
Member

Huh, I thought we had already done this. But, apparently not.

@bartonjs bartonjs added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Aug 15, 2022
@bartonjs
Copy link
Member

RSA.EncryptValue and RSA.DecryptValue have been obsolete and non-working since .NET Framework 4.6

Actually, they've never worked for built-in providers. They were abstract in .NET Framework 1.0 and RSACryptoServiceProvider threw a NotSupportedException. RSAPKCS1KeyExchangeFormatter is the only thing that hinted at what the method pair was supposed to do, namely the raw RSA public key operation and raw RSA private key operation.

All we did in 4.6 was change abstract to virtual+throw so we didn't have to keep declaring override+throw for RSACng (4.6), RSAOpenSsl (.NET Core 1.0) and RSASecurityTransforms (.NET Core 2.0, non-public)

@teo-tsirpanis
Copy link
Contributor

@reflectronic can you reword the message to guide the users towards using Encrypt and specifying an RSAEncryptionPadding?

@terrajobst
Copy link
Member

terrajobst commented Aug 16, 2022

Video

  • Since the method (and all derivatives) always throw, we should also hide it.
namespace System.Security.Cryptography;

public partial class RSA : AsymmetricAlgorithm
{
    [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public virtual byte[] EncryptValue(byte[] rgb);
    
    [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public virtual byte[] DecryptValue(byte[] rgb);
}

public partial class RSACryptoServiceProvider : RSA
{
    [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public override byte[] EncryptValue(byte[] rgb);
    
    [Obsolete("RSA.EncryptValue and DecryptValue are not supported and throw NotSupportedException. Use RSA.Encrypt and RSA.Decrypt instead.")]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public override byte[] DecryptValue(byte[] rgb);
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Aug 16, 2022
@vcsjones
Copy link
Member

@reflectronic were you interested in implementing this?

@bartonjs
Copy link
Member

@danmoseley What's your interest in taking some extra obsoletions in RC1? (The methods have thrown NotImplementedException in inbox types since they were introduced in .NET Framework 1.0, so it's as low of potential usage as we could ever really hope to expect 😄)

@deeprobin
Copy link
Contributor

If you want I can do that as well.
/cc @reflectronic @danmoseley

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 2, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Oct 5, 2022
@dotnet dotnet locked as resolved and limited conversation to collaborators Nov 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants