Provide Argon2id overloads to pass password hashes as strings #15
Conversation
|
This is a good idea. Thanks @ektrah! I'll try to take a look tomorrow if I get time. |
| @@ -43,6 +57,14 @@ public static bool VerifyHash(ReadOnlySpan<byte> hash, ReadOnlySpan<byte> passwo | |||
| return crypto_pwhash_str_verify(hash, password, (ulong)password.Length) == 0; | |||
| } | |||
|
|
|||
| public static bool VerifyHash(string hash, ReadOnlySpan<byte> password) | |||
| { | |||
| Validation.NotNull(nameof(hash), hash); | |||
There was a problem hiding this comment.
Please change this to Validation.NotNullOrEmpty().
There was a problem hiding this comment.
Actually, it's probably better to do Validation.NotNull() and then Validation.SizeBetween() with the MinHashSize and MaxHashSize constants.
| @@ -54,10 +76,29 @@ public static bool NeedsRehash(ReadOnlySpan<byte> hash, int iterations, int memo | |||
| return ret == -1 ? throw new FormatException("Invalid encoded password hash.") : ret == 1; | |||
| } | |||
|
|
|||
| public static bool NeedsRehash(string hash, int iterations, int memorySize) | |||
| { | |||
| Validation.NotNull(nameof(hash), hash); | |||
There was a problem hiding this comment.
Please change this to Validation.NotNullOrEmpty().
There was a problem hiding this comment.
Again, it's probably better to do Validation.NotNull() and then Validation.SizeBetween() with the MinHashSize and MaxHashSize constants.
| bool rehash = Argon2id.NeedsRehash(h, iterations, memorySize); | ||
| Assert.IsFalse(rehash); | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing ComputeHash_String_Invalid, not that there's much to test.
There was a problem hiding this comment.
There are also no NeedsRehash tests.
|
Thank you for making changes. I'll try to review this next weekend. |
|
The PR etiquette depends. If someone really wants their code merged into your project, you can expect them to put in some effort to make sure the code style is followed, full unit tests are included, etc. In this case, I just wanted to make a suggestion, but not without providing some sample code to show what I mean. It's up to you whether you like the suggestion or not, and if so, whether you want to build on the sample code or implement the suggestion in a different way, which of course means some effort on your part. I'm glad you liked my suggestion. 😸 I would perhaps even deprecate the old overloads, since there isn't much saved when passing a buffer instead of a string and the API would be come simpler. I guess most API users will use the string overloads. For |
Yes, I might do that. There's probably not much use for them.
This is an interesting point. However, I did it this way to prevent Argon2i hashes being verified since this library deliberately only supports Argon2id. As long as someone is using this library for computing the hashes, the method shouldn't throw. |
It can be a bit tricky to convert the password hash correctly between
Span<byte>andstring, so it might be helpful to provide overloads in theArgon2idclass that do the conversion for the user.The unsafe code can be replaced with
Marshal.AllocHGlobal,Marshal.PtrToStringAnsi, andMarshal.FreeHGlobalin atry ... finally ...if preferred.