scottbrady91 / ScottBrady91.AspNetCore.Identity.Argon2PasswordHasher

ASP.NET Core Identity IPasswordHasher implementation using Argon2
https://www.scottbrady91.com/ASPNET-Identity/Improving-the-ASPNET-Core-Identity-Password-Hasher
MIT License
22 stars 7 forks source link

Remove trailing 0x00 byte sequence, making the hash PostgreSQL-friendly. #4

Closed mstgelais closed 3 years ago

mstgelais commented 3 years ago

Please let me know if you'd like me to add some tests or anything else. Tested in a .Net 5 application with npgsql, and it works great.

Thank you for creating this lib!

scottbrady91 commented 3 years ago

Thanks for the PR 🙂

Did you see the various issues linked off of issue #2? Did those fixes not resolve this? Or maybe there's been a regression?

mstgelais commented 3 years ago

Yes, I did take a look at the issue. I was getting the exact same error message from npgsql. While searching for a solution, somebody was suggesting using String.Replace(). I thought using TrimEnd() was safer as it won't accidentally replace useful occurrences of the faulty bytes. To be honest, I don't know enough about hashing to take the best decision here, so I went with what I thought would limit bad side-effects.

scottbrady91 commented 3 years ago

Okay, thanks for confirming 🙂

I'd like to take a look into this a bit more and understand the issue, but I probably won't get to this for a couple of weeks. Are you okay with waiting? Otherwise, I'm happy to release a preview version based on this PR if it is a blocker.

mstgelais commented 3 years ago

No problem for me. For now, I've built your lib from sources with my little adjustment included. My project is brand new and won't be in production before next year. I'll let you know if I find anything on the subject that could help you / us better understand the problem. Thanks for your time!

EraYaN commented 3 years ago

It seems like you just get the full buffer from libsodium (which according to the docs is zero-terminated), so they can most likely be dropped. Although this might be a libsodium-core issue, in getting the cstring into .NET.

https://github.com/adamcaudill/libsodium-net/blob/master/libsodium-net/PasswordHash.cs#L282

That line I think. One needs to only parse the string up to the actual first \0 character.

The lastest version of libsodium-core has this fixed, but only for .NET standard 1.6 I think: https://github.com/tabrath/libsodium-core/blob/master/src/Sodium.Core/Utilities.cs#L242

The best thing would for us be a preview or release version, but I can do a fork internally just fine.

This issue needs an extra push: https://github.com/tabrath/libsodium-core/issues/46

scottbrady91 commented 3 years ago

I've finally had a chance to look into this. Sorry for the delay.

I've written a failing unit test for the issue, which your PR fixes.

scottbrady91 commented 3 years ago

Released in 1.4.0

mstgelais commented 3 years ago

I've finally had a chance to look into this. Sorry for the delay.

I've written a failing unit test for the issue, which your PR fixes.

Good news! Thanks a adding and a test and accepting the PR. No problem for the delay. :-D