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

Obsolete unnecessary cryptographic derived types #46934

Closed
bartonjs opened this issue Jan 13, 2021 · 2 comments · Fixed by #52303
Closed

Obsolete unnecessary cryptographic derived types #46934

bartonjs opened this issue Jan 13, 2021 · 2 comments · Fixed by #52303
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@bartonjs
Copy link
Member

Background and Motivation

Many of the cryptographic algorithms have an inheritance hierarchy to allow for distinct implementations to be used. For example, in .NET Framework SHA256Managed uses a fully managed implementation, SHA256CryptoServiceProvider uses Windows CAPI, and SHA256Cng uses Windows CNG.

In .NET (nee Core) these types are all always backed by a single native implementation (using Windows CNG, Apple Security.Framework, or OpenSSL (as appropriate)), so there's no reason to prefer any one type over another.

  • Hash algorithms: All derived types use the same underlying implementation.
  • Symmetric cryptography: AesCng and TripleDESCng both allow opening a persisted/named key via type-specific constructors. No other derived types have type-specific options, and all implementations for a particular algorithm on a given platform have the same underlying implementation.
  • Asymmetric cryptography: The different providers have mild observable differences, so this issue doesn't apply to them.

Notably, the .NET (nee Core) reference assemblies already has these types marked as [EditorBrowsable(Never)].

All modern code should instantiate these algorithms via the Create() static method on the algorithm type (e.g. SHA256.Create()) (except, as noted, for the persisted key use case for AesCng and TripleDESCng).

Proposed API

namespace System.Security.Cryptography.Algorithms
{
+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class AesCryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class AesManaged
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class DESCryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class MD5CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class RC2CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA1Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA1CryptoServiceProvider
    {
    }


+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA256Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA256CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA384Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA384CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA512Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA512CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class TripleDESCryptoServiceProvider
    {
    }
}
@bartonjs bartonjs added area-System.Security api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 13, 2021
@bartonjs bartonjs added this to the 6.0.0 milestone Jan 13, 2021
@ghost
Copy link

ghost commented Jan 13, 2021

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

Issue Details

Background and Motivation

Many of the cryptographic algorithms have an inheritance hierarchy to allow for distinct implementations to be used. For example, in .NET Framework SHA256Managed uses a fully managed implementation, SHA256CryptoServiceProvider uses Windows CAPI, and SHA256Cng uses Windows CNG.

In .NET (nee Core) these types are all always backed by a single native implementation (using Windows CNG, Apple Security.Framework, or OpenSSL (as appropriate)), so there's no reason to prefer any one type over another.

  • Hash algorithms: All derived types use the same underlying implementation.
  • Symmetric cryptography: AesCng and TripleDESCng both allow opening a persisted/named key via type-specific constructors. No other derived types have type-specific options, and all implementations for a particular algorithm on a given platform have the same underlying implementation.
  • Asymmetric cryptography: The different providers have mild observable differences, so this issue doesn't apply to them.

Notably, the .NET (nee Core) reference assemblies already has these types marked as [EditorBrowsable(Never)].

All modern code should instantiate these algorithms via the Create() static method on the algorithm type (e.g. SHA256.Create()) (except, as noted, for the persisted key use case for AesCng and TripleDESCng).

Proposed API

namespace System.Security.Cryptography.Algorithms
{
+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class AesCryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class AesManaged
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class DESCryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class MD5CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class RC2CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA1Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA1CryptoServiceProvider
    {
    }


+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA256Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA256CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA384Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA384CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA512Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA512CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class TripleDESCryptoServiceProvider
    {
    }
}
Author: bartonjs
Assignees: -
Labels:

api-ready-for-review, area-System.Security

Milestone: 6.0.0

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 13, 2021
@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Jan 13, 2021
@bartonjs
Copy link
Member Author

bartonjs commented Jan 15, 2021

Video

Looks good as proposed.

namespace System.Security.Cryptography.Algorithms
{
+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class AesCryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class AesManaged
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class DESCryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class MD5CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class RC2CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA1Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA1CryptoServiceProvider
    {
    }


+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA256Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA256CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA384Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA384CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA512Managed
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class SHA512CryptoServiceProvider
    {
    }

+   [Obsolete(someID)]
    [EditorBrowsable(EditorBrowsableState.Never)]
    public partial class TripleDESCryptoServiceProvider
    {
    }
}

@bartonjs bartonjs 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 Jan 15, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 5, 2021
@ghost ghost closed this as completed in #52303 May 11, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2021
This issue was closed.
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.

1 participant