ReadOnlySpan<byte> overloads for Rfc2898DeriveBytes constructors.

[MichalPetryka]

Background and Motivation

Rfc2898DeriveBytes constructors currently only accept a string or a byte array for the password and a byte array for the salt. In some cases a programmer might want to have them stored in a Span, passing them to the API requires a ToArray call which bloats the code and is an useless performance cost due to the constructors copying the data again.

Proposed API

namespace System.Security.Cryptography
{
     public class Rfc2898DeriveBytes : DeriveBytes {
+        public Rfc2898DeriveBytes(ReadOnlySpan<byte> password, ReadOnlySpan<byte> salt, int iterations)
+        public Rfc2898DeriveBytes(ReadOnlySpan<byte> password, ReadOnlySpan<byte> salt, int iterations, HashAlgorithmName hashAlgorithm)
+        public Rfc2898DeriveBytes(string password, ReadOnlySpan<byte> salt)
+        public Rfc2898DeriveBytes(string password, ReadOnlySpan<byte> salt, int iterations)
+        public Rfc2898DeriveBytes(string password, ReadOnlySpan<byte> salt, int iterations, HashAlgorithmName hashAlgorithm)
     }

Usage Examples

ReadOnlySpan<byte> salt = SomeMethodThatReturnsMemory().Span;
var rfc = new Rfc2898DeriveBytes("some password", salt);

Alternative Designs

None that I know of.

Risks

None that I know of.

[ghost]

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

Rfc2898DeriveBytes constructors currently only accept a string or a byte array for the password and a byte array for the salt. In some cases a programmer might want to have them stored in a Span, passing them to the API requires a ToArray call which bloats the code and is an useless performance cost due to the constructors copying the data again.

Proposed API

namespace System.Security.Cryptography
{
     public class Rfc2898DeriveBytes : DeriveBytes {
+        public Rfc2898DeriveBytes(ReadOnlySpan<byte> password, ReadOnlySpan<byte> salt, int iterations)
+        public Rfc2898DeriveBytes(ReadOnlySpan<byte> password, ReadOnlySpan<byte> salt, int iterations, HashAlgorithmName hashAlgorithm)
+        public Rfc2898DeriveBytes(string password, ReadOnlySpan<byte> salt)
+        public Rfc2898DeriveBytes(string password, ReadOnlySpan<byte> salt, int iterations)
+        public Rfc2898DeriveBytes(string password, ReadOnlySpan<byte> salt, int iterations, HashAlgorithmName hashAlgorithm)
     }

Usage Examples

ReadOnlySpan<byte> salt = SomeMethodThatReturnsMemory().Span;
var rfc = new Rfc2898DeriveBytes("some password", salt);

Alternative Designs

None that I know of.

Risks

None that I know of.

Author:	MichalPetryka
Assignees:	-
Labels:	api-suggestion, area-System.Security, untriaged
Milestone:	-

[vcsjones]

Hmm. A couple of thoughts.

1. For password this might be feasible or at least an improvement. Even in the case of the byte[] we defensively copy the input here:

   runtime/src/libraries/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/Rfc2898DeriveBytes.cs

   Line 54 in fb001e6

   _password = password.CloneByteArray();

   So at the very least if the caller is forced to allocate a copy, then we copy it again. If there was a Span input, one of those allocations could be avoided.

2. We might be able to avoid allocating for the password at all if we switch to IncrementalHash.OpenHmac instead of using the HMAC base.

3. salt is exposed as a property of type byte[] and is also part of the object state. So we can't get down to zero allocations, but we could remove one in the similar case of point one.

That said, I think if allocations and performance are important to PBKDF2, a one-shot "no streaming" API would be better time spent focused on, instead of the stateful one that this class exposes.

[bartonjs]

The only thing I'd maybe change is public Rfc2898DeriveBytes(string password, ReadOnlySpan<byte> salt) getting spanified to public Rfc2898DeriveBytes(ReadOnlySpan<char> password, ReadOnlySpan<byte> salt), which would allow for things like reading a password into a stackalloc buffer and then passing a slice of that buffer in here.

That said, I think if allocations and performance are important to PBKDF2, a one-shot "no streaming" API would be better time spent focused on, instead of the stateful one that this class exposes.

I thought we had a proposal for that already. Looks like it was just a discussion point of #24897 (one of the benefits of the one-shot method is we can just call system implementations).

[GrabYourPitchforks]

TBH I agree with Kevin's comment above. I'd like to steer people away from creating an instance of this type + calling instance methods, in favor of calling the static one-shot. The instance members of this type have some weird internal logic which doesn't properly follow the RFC if called multiple times. The static one-shot properly follows the RFC. If we're going to steer people toward the one-shots, that's where our investments should be.

[bartonjs]

The instance members of this type have some weird internal logic which doesn't properly follow the RFC if called multiple times.

I know PasswordDeriveBytes (PBKDF1) has some non-RFC behavior, but IIRC Rfc2898DeriveBytes (PBKDF2)'s only weirdness is that it's streaming.

[MichalPetryka]

Static methods that'd take spans would also be fine, should I close this then?

[bartonjs]

Static methods that'd take spans would also be fine, should I close this then?

Yeah, we can continue the discussion on #24897, maybe you'd like to actually write a proposed API shape so we can mark it as ready for review. Without thinking very much:

partial class Rfc2898DeriveBytes
{
    public static byte[] DeriveKey(ROS<byte> passwordBytes, ROS<byte> salt, HashAlgorithmName hashAlgorithm, int iterationCount, int size);
    public static byte[] DeriveKey(ROS<char> password, ROS<byte> salt, HashAlgorithmName hashAlgorithm, int iterationCount, int size);
    public static byte[] DeriveKey(string password, ROS<byte> salt, HashAlgorithmName hashAlgorithm, int iterationCount, int size);

    public static void DeriveKey(ROS<byte> passwordBytes, ROS<byte> salt, HashAlgorithmName hashAlgorithm, int iterationCount, Span<byte> destination);
    public static void DeriveKey(ROS<char> password, ROS<byte> salt, HashAlgorithmName hashAlgorithm, int iterationCount, Span<byte> destination);
    public static void DeriveKey(string password, ROS<byte> salt, HashAlgorithmName hashAlgorithm, int iterationCount, Span<byte> destination);
}

But we can pick that apart over there.

[bartonjs]

maybe you'd like to actually write a proposed API shape

@vcsjones already did. ::whistles innocently::.

OK, sounds like we don't have anything left for this one, so I'm going to go ahead and hit close.