Move KeyDerivation to use the built in Rfc2898 SHA2* support in .NET Core 2.0

[aspnet-hello]

From @blowdart on Monday, August 28, 2017 10:55:59 AM

https://docs.microsoft.com/dotnet/api/system.security.cryptography.rfc2898derivebytes now supports SHA256, SHA384 and SHA512. We could remove the code from data protection and just use the framework support.

Copied from original issue: aspnet/DataProtection#272

[aspnet-hello]

From @natemcmaster on Friday, September 1, 2017 10:41:44 AM

Looks like a good opportunity to delete code (a favorite pastime). Before we do, however, we should ensure we don't regress behavior by checking our test coverage.

[aspnet-hello]

From @GrabYourPitchforks on Thursday, September 28, 2017 12:22:02 PM

Use caution that you don't cause a perf regression when you do this. The code as currently written determines which platform it is running on and calls into the fastest implementation for the platform. This is especially important when we're talking about password hashing, which defaults to 10,000 iterations. You probably don't want to take the CPU cost of 10,000 p/invoke calls in a tight loop if you can get away with just one. (Of course, this all depends on how the Rfc2898DeriveBytes class is written.)

[aspnet-hello]

From @blowdart on Friday, October 13, 2017 12:03:43 PM

@GrabYourPitchforks do you think we should do this the other way around, and move the data protection version to core?

[aspnet-hello]

From @GrabYourPitchforks on Friday, October 13, 2017 1:46:41 PM

@blowdart that would be ideal since you could bring the perf gains to the core class. It looks like Rfc2898DeriveBytes has some custom caching logic for results so you'd have to make sure that you don't break the scenario where somebody calls GetBytes multiple times.

[aspnet-hello]

From @Tornhoof on Thursday, October 19, 2017 3:26:59 AM

Atleast on my Windows machine, the Dataprotection version is still ~25% faster than the pbkdf2 version from .NET Core:

BenchmarkDotNet=v0.10.9, OS=Windows 10 Redstone 1 (10.0.14393)
Processor=Intel Xeon CPU E5-1620 0 3.60GHz, ProcessorCount=8
Frequency=3507177 Hz, Resolution=285.1296 ns, Timer=TSC
.NET Core SDK=2.0.2
  [Host]     : .NET Core 2.0.0 (Framework 4.6.00001.0), 64bit RyuJIT
  DefaultJob : .NET Core 2.0.0 (Framework 4.6.00001.0), 64bit RyuJIT

Method	Iterations	Mean	Error	StdDev
Rfc2898DeriveBytesSHA512	1000	1.273 ms	0.0034 ms	0.0032 ms
KeyDerivationPBKDF2SHA512	1000	1.019 ms	0.0025 ms	0.0024 ms
Rfc2898DeriveBytesSHA512	10000	12.652 ms	0.0160 ms	0.0142 ms
KeyDerivationPBKDF2SHA512	10000	10.143 ms	0.0361 ms	0.0301 ms
Rfc2898DeriveBytesSHA512	100000	128.595 ms	0.8824 ms	0.8254 ms
KeyDerivationPBKDF2SHA512	100000	102.741 ms	0.9762 ms	0.9131 ms

Apparently there are quite a few performance improvements in there, last time I checked against the vanilla implementation (modified to support SHA512) on netfx, the dataprotection version was pretty much twice as fast.

As for source:

using System.Security.Cryptography;
using System.Text;
using BenchmarkDotNet.Attributes;
using Microsoft.AspNetCore.Cryptography.KeyDerivation;

namespace PBKDF2Benchmark
{
    public class HashBenchmark
    {
        [Params(1000, 10000, 100000)]
        public int Iterations { get; set; }
        private static readonly byte[] _password = Encoding.UTF8.GetBytes("Hello World");
        private static readonly byte[] _salt = GenerateSalt();

        private static byte[] GenerateSalt()
        {
            using (var random = new RNGCryptoServiceProvider())
            {
                var result = new byte[64];
                random.GetBytes(result);
                return result;
            }
        }

        [Benchmark]
        public byte[] Rfc2898DeriveBytesSHA512()
        {
            var rfc = new Rfc2898DeriveBytes(_password, _salt, Iterations, HashAlgorithmName.SHA512);
            return rfc.GetBytes(64);
        }
        [Benchmark]
        public byte[] KeyDerivationPBKDF2SHA512()
        {
            var key = KeyDerivation.Pbkdf2("Hello World", _salt, KeyDerivationPrf.HMACSHA512, Iterations, 512 / 8);
            return key;
        }
    }
}

[aspnet-hello]

From @natemcmaster on Friday, October 20, 2017 9:24:59 AM

Thanks for the numbers. I'll take a look at these more closely before we change implementations.

[blowdart]

OK this does need to be the other way around. @GrabYourPitchforks do you want to create an issue in .NET Core and drive this?

[GrabYourPitchforks]

I opened https://github.com/dotnet/corefx/issues/26796 to track improving perf of Rfc2898DeriveBytes. No guarantees at the moment because I'm not sure what constraints will be placed on us when making changes or exposing Span in public API surface.

[natemcmaster]

I'm seeing something similar in perf on Windows. On macOS/Linux, it goes the other way though. @blowdart what do you think about starting with switching the Linux/macOS implementation first? Windows is still faster and allocates less.

https://github.com/aspnet/DataProtection/blob/namc/bench/benchmarks/PBKDF2Benchmarks/HashBenchmarks.cs

Windows x64:

Method	Iterations	Mean	Error	StdDev	Op/s	Allocated
Rfc2898DeriveBytesSHA512	1000	1,067.7 us	43.66 us	65.35 us	936.565	896 B
KeyDerivationPBKDF2SHA512	1000	864.6 us	15.71 us	23.52 us	1,156.558	128 B
Rfc2898DeriveBytesSHA512	10000	10,595.7 us	369.94 us	553.72 us	94.378	896 B
KeyDerivationPBKDF2SHA512	10000	8,630.5 us	284.45 us	425.74 us	115.869	129 B
Rfc2898DeriveBytesSHA512	100000	102,029.7 us	1,527.41 us	2,286.16 us	9.801	896 B
KeyDerivationPBKDF2SHA512	100000	83,219.7 us	1,423.79 us	2,131.06 us	12.016	132 B

macOS 10.13

Method	Iterations	Mean	Error	StdDev	Op/s	Gen 0	Allocated
Rfc2898DeriveBytesSHA512	1000	2.483 ms	0.3441 ms	0.5150 ms	402.674	-	888 B
KeyDerivationPBKDF2SHA512	1000	2.552 ms	0.2142 ms	0.3207 ms	391.864	3.9063	176504 B
Rfc2898DeriveBytesSHA512	10000	21.696 ms	0.6027 ms	0.9021 ms	46.091	-	888 B
KeyDerivationPBKDF2SHA512	10000	23.534 ms	1.1734 ms	1.7563 ms	42.491	62.5000	1760504 B
Rfc2898DeriveBytesSHA512	100000	221.153 ms	8.2108 ms	12.2895 ms	4.522	-	888 B
KeyDerivationPBKDF2SHA512	100000	235.548 ms	9.2109 ms	13.7865 ms	4.245	750.0000	17600504 B

Debian 9

Method	Iterations	Mean	Error	StdDev	Op/s	Gen 0	Allocated
Rfc2898DeriveBytesSHA512	1000	1.147 ms	0.0150 ms	0.0224 ms	871.911	-	840 B
KeyDerivationPBKDF2SHA512	1000	1.231 ms	0.0129 ms	0.0193 ms	812.291	17.7734	176456 B
Rfc2898DeriveBytesSHA512	10000	11.280 ms	0.1232 ms	0.1844 ms	88.651	-	840 B
KeyDerivationPBKDF2SHA512	10000	12.794 ms	0.6271 ms	0.9386 ms	78.164	187.5000	1760456 B
Rfc2898DeriveBytesSHA512	100000	117.387 ms	3.3823 ms	5.0625 ms	8.519	-	840 B
KeyDerivationPBKDF2SHA512	100000	125.631 ms	1.4265 ms	2.1351 ms	7.960	1937.5000	17600456 B

[blowdart]

That'd be fine by me

[natemcmaster]

Interesting, looks like we may not be able to do this. It would break apps that use a salt with less than 8 bytes.

 System.ArgumentException : Salt is not at least eight bytes.
  at System.Security.Cryptography.Rfc2898DeriveBytes..ctor(Byte[] password, Byte[] salt, Int32 iterations, HashAlgorithmName hashAlgorithm)