Add extractParams for Argon2, PBKDF2, and Scrypt

[blackheaven]

I'm tackling #55

[Vlix]

The bcrypt variant would be extractParams :: PasswordHash Bcrypt -> Maybe Int which would return the cost parameter (if the hash is a valid bcrypt hash)

[blackheaven]

Not sure why, but Bcrypt struggles to decode base64 hashKey

[Vlix]

extractParams for bcrypt doesn't need to do anything with the hash/salt, it just needs to get the cost.

(The reason base64 isn't working, is because bcrypt has it's own de-/encode table which is sortof like base64, but weird and unique. So you don't even have to try.)

---

When if the bcrypt function is updated and works as expected, I'll do a full review to see if anything can be improved, documentation is correct/sufficient and if all the version bumps and changelog additions are in place 👍

[Vlix]

Looks good! Thank you for your effort! I'd like to add some tests as well, if you're up for it?

If you want to just be done with this PR, that's fine too, I'll add the tests when I get some time to do so and we'll merge/upload after that.

[Vlix]

I've also just realized the Bcrypt's extractParams would also work on a lot of non-bcrypt hashes, so it might be a good idea to also check that the _version part is actually a valid bcrypt version: 2, 2a, 2x, 2y, 2b (even though this implementation does not accept 2 and 2x, for the purpose of providing the correct parameter, they should be accepted)

And also that the first parameter when splitting on '$' is actually an empty string (meaning the hash indeed started with a '$')

Again, if you don't feel like doing this, I can finish up the PR if you want to.

[blackheaven]

For the tests, I'm willing to do it, but I need some guidance (I already adapted some tests, but I have no idea on new relevant tests).

[Vlix]

Ok, that's great!

The additions to the test are ok, but I'd rather you add another parameter that will be the expected -Params of that algorithm, so that you don't isJust, but === expectedParams for example.

I'd also like to add a few property tests to check the following:

• newSalt x === length salt
• roundtrip of "unsafePad64 <-> unsafeRemovePad64" (this one more because I noticed we don't have one yet)

Those together with what you already added in the tests should be enough for this addition, I think.

[Vlix]

The newSalt test can be something like the following:

testProperty "newSalt <-> length salt" $
  \i -> ioProperty $ do
    let n = fromIntegral (i :: Int16)
    Salt salt <- newSalt n
    pure $ Data.ByteString.length salt === n

(casting the i to Int16, just so it won't generate HUEG bytestrings)

EDIT: On second thought... Word16 might make more sense, since negative amounts of bytes makes no sense.

[blackheaven]

I did, however:

• Testing newSalt/unsafePad64/unsafeRemovePad64 forced me to expose Internal
• I didn't get right unsafePad64 <-> unsafeRemovePad64 roundtrip
• Converting isJust broke Bcrypt/Scrypt tests, I would guess that it's due to algorithme structure, but I'm not sure

[Vlix]

Ah, right... now I remember why we didn't have newSalt or unsafePad64 tests 🤦 We don't want to expose the Internal module. (Please reverse this change, I'll think about something on my own time) And you can remove the Data.Password.Bcrypt.defaultParams. It's just an Int 🤷

---

Can you give the error that you get when the bcrypt/scrypt tests break? Are those the doctests or the tasty tests?

Now lastly, I'd just like to get an extra copy of every first testCorrectPassword with (completely) different parameters, so there's 2 tests with completely different parameters that still get parsed correctly.

[blackheaven]

Here are the failing tests (tasty):

  bcrypt                                                                                                              
    Bcrypt (hashPassword):                                     FAIL (0.31s)
      *** Failed! Falsified (after 1 test):                                                                           
      ""                         
      Nothing /= Just 4                                                                                               
      Use --quickcheck-replay=788736 to reproduce.
      Use -p '$0=="Password.bcrypt.Bcrypt (hashPassword)"' to rerun this test only.
...
bcrypt
    Bcrypt (hashPasswordWithSalt):                             FAIL (0.28s)
      *** Failed! Falsified (after 1 test):                                                                           
      ""                         
      Salt {getSalt = "\NUL\SOH\NUL\NUL\NUL\NUL\SOH\NUL\SOH\NUL\SOH\NUL\SOH\NUL\NUL\SOH"}
      Nothing /= Just 4          
      Use --quickcheck-replay=40683 to reproduce.
      Use -p '/Bcrypt (hashPasswordWithSalt)/' to rerun this test only.
...
  scrypt
    Scrypt (hashPasswordWithSalt):                             FAIL (0.03s)
      *** Failed! Falsified (after 1 test):
      ""
      Salt {getSalt = "\SOH\SOH\NUL\SOH\NUL\NUL\SOH\SOH\SOH\NUL\NUL\SOH\NUL\NUL\SOH\SOH"}
      Just (ScryptParams {scryptSalt = 16, scryptRounds = 8, scryptBlockSize = 8, scryptParallelism = 1, scryptOutputLength = 64}) /= Just (ScryptParams {scryptSalt = 32, scryptRounds = 8, scryptBlockSize = 8, scryptParallelism = 1, scr
yptOutputLength = 64})
      Use --quickcheck-replay=940239 to reproduce.
      Use -p '/Scrypt (hashPasswordWithSalt)/' to rerun this test only.

PBKDF2 has already multiple calls

[Vlix]

I wonder why testCorrectPassword hasn't been giving this error before? 🤔 Do these errors disappear when you remove the .&&. extractParamsF hpw === Just defaultParams part that was newly added?

I wonder why scrypt is giving back a salt length of 16 when defaultParams has 32... and the doctests confirm the salt is indeed 32 bytes base64-encoded. Weird. I might look at this more closely this weekend.

[blackheaven]

Yep, it works without the condition

[Vlix]

Oh, wait, the scrypt is failing because the Arbitrary instance of Salt at the bottom just hardsets it to 16 bytes 🤦

[Vlix]

Ýou'll have to set the saltLength of testsParams8Rounds in the "Scrypt (hashPasswordWithSalt)" test to 16. The 32 byte test is already done in testCorrectPassword, so that's fine.

I also get the feeling the testCorrectPassword and testWithSalt functions need a pass /= "" ==> condition to not have bcrypt flip out. It's the reason the testIncorrectPassword has a similar condition as well; bcrypt with an empty string password does some weird things IIRC.

[blackheaven]

I think there's a real bug here.

I have tried to change Internal properties (using arbitraryPrintableChar for the Salt generator), and it stills find counter examples.

[Vlix]

Thank you for all the effort and patience 🎉

[blackheaven]

Thanks for helping me, it was great.