feat: allow import peppered/salted password hashes #2946

[ci42]

Related issue(s)

#2946

Checklist

• I have read the contributing guidelines.
• I have referenced an issue containing the design document if my change
  introduces a new feature.
• I am following the
  contributing code guidelines.
• I have read the security policy.
• I confirm that this pull request does not address a security
  vulnerability. If this pull request addresses a security vulnerability, I
  confirm that I got the approval (please contact
  security@ory.sh) from the maintainers to push
  the changes.
• I have added tests that prove my fix is effective or that my feature
  works.
• I have added or changed the documentation.

Further Comments

[CLAassistant]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #3057 (4711e09) into master (a17bcb8) will increase coverage by 0.01%.
The diff coverage is 82.85%.

❗ Current head 4711e09 differs from pull request most recent head 76a861c. Consider uploading reports for the commit 76a861c to get more accurate results

@@            Coverage Diff             @@
##           master    #3057      +/-   ##
==========================================
+ Coverage   77.88%   77.90%   +0.01%     
==========================================
  Files         320      320              
  Lines       20474    20543      +69     
==========================================
+ Hits        15946    16003      +57     
- Misses       3327     3335       +8     
- Partials     1201     1205       +4     

Impacted Files	Coverage Δ
hash/hash_comparator.go	85.05% <82.85%> (-0.57%)	⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[zepatrik]

Thanks for putting in the effort. however we need to fix a few things. It is also not a good idea to mix new features with refactoring in one PR, so I would suggest you either split this in two PRs, or we don't do the refactoring at all.
For pepper secrets, I think we need to include the possibility to rotate the secret as well. It is not cheap to do, as we need to hash the password multiple times, but there is no other way to update peppers or increase the secret entropy without a rotation mechanism, so this is a requirement IMO.

[aeneasr]

See also feedback from #2946 (comment)

[ci42]

If the pepper changed over time (not uncommon), we can import passwords anyways

I added support for importing passwords with different pepper values.

[ci42]

@aeneasr @zepatrik
I have completely rewritten the PR. The new hash comparison function is now similar to the existing hash comparison functions. t now supports various old hashing methods (md5, sha1, sha256, sha512) as well as optional salts, peppers and a work factor (number of iterations). The old hash itself is hashed with argon2id for improved security (e.g. offline attacks in case of a database leak).

All parameters are encoded in the hash string. The hash string has the form:
$legacy$<legacy hash parameters>$<argon2id in phc format> - where the legacy hash parameters are encoded as follows
alg=<md5|sha1|sha256|sha512>,format=<base64 encoded format, e.g: '{PEPPER}-{PWD}-{SALT}'>,i=<number of iterations>,(salt=<base64 encoded salt>,pepper=<base64 encoded pepper>)

An example using a salted and peppered md5 in the format {SALT}::{PWD}::{PEPPER}with 3 iterations: $legacy$alg=md5,format=e1NBTFR9Ojp7UFdEfTo6e1BFUFBFUn0,i=3,salt=7aCA,pepper=/v7//w$argon2id$v=19$m=65536,t=2,p=2$YuYcJII0fm8UXm8XiCqimA$xzUihMR5G40U3Gf2CmTfNhXDYTtva6ykbjGnfQnWCac

[zepatrik]

Looks good now, thanks for revisiting!
I just have some minor suggestions, nothing critical.

[ci42]

@zepatrik Thanks for the review. I've included your suggested changes, added some more tests and rebased the PR onto the current master.

By the way, I made a PR with some minor hash_comparator.go refactorings. Maybe you can have a look at it as well.

[zepatrik]

Nice 👍