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

[API Proposal]: Obsolete all Rfc2898DeriveBytes constructors #97221

Open
vcsjones opened this issue Jan 19, 2024 · 2 comments · May be fixed by #111678
Open

[API Proposal]: Obsolete all Rfc2898DeriveBytes constructors #97221

vcsjones opened this issue Jan 19, 2024 · 2 comments · May be fixed by #111678
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@vcsjones
Copy link
Member

Background and motivation

Rfc2898DeriveBytes implements RFC 2898 (PBKDF2) and offers two ways to use it: create an instance or use static one-shots.

The instance based implementation provides the ability to "stream" bytes back from the KDF. Successive calls to GetBytes do not reset the in-progress operation.

This streaming behavior was never an intended use case of RFC 2898 (or its successor RFC 8018). Further, none of the underlying platforms implement PBKDF2 in a way that supports streaming. So the streaming implementation will always need to be managed on top of HMAC.

Since streaming is the only real reason to instantiate the class instead of using the one-shots, I propose obsoleting the constructors with the recommendation that the one-shot be used.

A slight complication from this is that the class is not sealed. A quick code search however indicates that few folks are regularly deriving from this class.

Since this class already has obsolete constructors, I propose re-obsoleting the current ones under a different SYSLIB since the new obsoletion is "stronger". This will also cause people that suppressed the obsoletion to re-evaluate the suppression.

API Proposal

public partial class Rfc2898DeriveBytes : DeriveBytes
{
-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations, HashAlgorithmName hashAlgorithm);

API Usage

Alternative Designs

No response

Risks

No response

@vcsjones vcsjones added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security labels Jan 19, 2024
@ghost
Copy link

ghost commented Jan 19, 2024

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

Issue Details

Background and motivation

Rfc2898DeriveBytes implements RFC 2898 (PBKDF2) and offers two ways to use it: create an instance or use static one-shots.

The instance based implementation provides the ability to "stream" bytes back from the KDF. Successive calls to GetBytes do not reset the in-progress operation.

This streaming behavior was never an intended use case of RFC 2898 (or its successor RFC 8018). Further, none of the underlying platforms implement PBKDF2 in a way that supports streaming. So the streaming implementation will always need to be managed on top of HMAC.

Since streaming is the only real reason to instantiate the class instead of using the one-shots, I propose obsoleting the constructors with the recommendation that the one-shot be used.

A slight complication from this is that the class is not sealed. A quick code search however indicates that few folks are regularly deriving from this class.

Since this class already has obsolete constructors, I propose re-obsoleting the current ones under a different SYSLIB since the new obsoletion is "stronger". This will also cause people that suppressed the obsoletion to re-evaluate the suppression.

API Proposal

public partial class Rfc2898DeriveBytes : DeriveBytes
{
-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations, HashAlgorithmName hashAlgorithm);

API Usage

Alternative Designs

No response

Risks

No response

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Security

Milestone: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 19, 2024
@bartonjs bartonjs added this to the 10.0.0 milestone Jul 5, 2024
@jeffhandley jeffhandley removed the untriaged New issue has not been triaged by the area owner label Jul 19, 2024
@bartonjs bartonjs added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jan 9, 2025
@terrajobst
Copy link
Member

terrajobst commented Jan 21, 2025

Video

  • Looks good as proposed
namespace System.Security.Cryptography;

public partial class Rfc2898DeriveBytes : DeriveBytes
{
-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(byte[] password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, byte[] salt, int iterations, HashAlgorithmName hashAlgorithm);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize);

-       [Obsolete(Obsoletions.Rfc2898OutdatedCtorMessage, DiagnosticId = Obsoletions.Rfc2898OutdatedCtorDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations);

+       [Obsolete("Creating instances of Rfc2898DeriveBytes is obsolete. Use the static Pbkdf2 method instead.", DiagnosticId = "SYSLIBXXXX", UrlFormat = Obsoletions.SharedUrlFormat)]
        public Rfc2898DeriveBytes(string password, int saltSize, int iterations, HashAlgorithmName hashAlgorithm);
}

@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 Jan 21, 2025
@vcsjones vcsjones linked a pull request Jan 21, 2025 that will close this issue
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jan 21, 2025
@vcsjones vcsjones self-assigned this Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-System.Security in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants