Stateless password hash derivation with header specificity

[michaeljgray-sfdc]

This pull request closes #12 by exposing a static method to perform header based and stateless encoding for passwords. I exposed the SaltLength field as a public static readonly field instead of private const to ensure that callers know the buffer size in a deterministic way; the reason for readonly over const is to prevent recompilation of large dependencies if/when the value changes in the future. This pull request also adds an optimization to reduce frequent re-allocation of the salt buffer for each encoding operation performed by instance methods.

This also adds the .vs directory to .gitignore to prevent Visual Studio client settings from being included.

[viniciuschiele]

Why is _salt a class attribute? This will make Encode not thread-safe anymore.

[michaeljgray-sfdc]

Hi @viniciuschiele! Thank you for the comment. The Encode method wasn't thread safe to begin with because RandomNumberGenerator.GetBytes is not thread safe. A general practice most follow is to make the caller responsible for resource locking for thread safety related to individual methods, unless the class itself is running multiple threads internally or using overlapping callbacks of some kind. The exception to this rule is usually for methods marked static, which should ensure their own thread safety. Let me know if you have any other feedback. Thanks!

[viniciuschiele]

Hi, based on this, RNGCryptoServiceProvider is thread-safe and RandomNumberGenerator.Create() also returns a RNGCryptoServiceProvider by default based on that.
My concern is that this change can potentially break existing apps, that's why I think this shouldn't be part of this PR.

[michaeljgray-sfdc]

Thank you for the clarification. After some research based on your comment, I see the issue now. I agree on the point of it breaking existing apps. Even though this varies by implementation, it shouldn't cause a behavioral change that breaks existing consumers of the library who use older versions of .NET, since you still support them. I'll go ahead and remove that change and return it to the original with a new commit tomorrow.

I did want to point out that in .NET Core and .NET Framework, the implementation now just invokes Windows API's BCryptGenRandom on Windows and OpenSSL's GetRandomBytes on Unix, so new consumers should not rely on thread safety of this method in modern codebases.

I really appreciate your thoughtful response and for exercising care around breaking your users. Thank you again and you can expect the commit tomorrow.

[viniciuschiele]

Thanks for providing the links to the source code, for me both implementations seem thread-safe and I couldn't find anything to say otherwise.

I agree that this may vary based on the implementation, I wonder if it would be better to instantiate a new RandomNumberGenerator every time the Encode is called.