feat: support importing MD5 hashes

[erolkskn]

Related issue(s)

#2716

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 green light (please contact [email protected]) 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.

[codecov]

Codecov Report

Merging #2725 (e2f1a92) into master (2ff34b6) will increase coverage by 0.00%.
The diff coverage is 92.30%.

@@           Coverage Diff           @@
##           master    #2725   +/-   ##
=======================================
  Coverage   76.10%   76.11%           
=======================================
  Files         308      308           
  Lines       18880    18918   +38     
=======================================
+ Hits        14369    14399   +30     
- Misses       3392     3397    +5     
- Partials     1119     1122    +3     

Impacted Files	Coverage Δ
identity/handler_import.go	63.04% <0.00%> (ø)
hash/hash_comparator.go	84.21% <94.73%> (+2.33%)	⬆️
persistence/sql/persister_courier.go	82.82% <0.00%> (-2.03%)	⬇️
session/test/persistence.go	98.12% <0.00%> (-0.94%)	⬇️

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

[hperl]

Hi, and thanks a lot for your PR. It looks really good already, I have just left some comments.

You can improve the coverage by adding more test cases to parse for formats that don't match.

[hperl]

Thanks @erolkskn, LGTM! 🎉
@zepatrik can we merge this now or should we wait for final approval by @aeneasr?

/cc @jonas-jonas

[zepatrik]

Looks good IMO. We will merge it beginning of next week 👍

[aeneasr]

Awesome! This is mergable!

The last missing piece is no adding a bit of docs for this feature :) https://www.ory.sh/docs/kratos/manage-identities/import-user-accounts-identities#bcrypt-pkbdf2-argon2-scrypt-family-hashed-password

We probably need a dedicated section for MD5 as our implementation is not standardized, meaning that we'll need to explain how this works :)

[hugotiburtino]

Beautiful!

[erolkskn]

Oh sorry for this took too long. I couldn't find appropriate time to write down docs. It should be ready in the next few days.

[aeneasr]

Awesome! And there have since been a few merge conflicts - could you please also take a look into those? :)

[erolkskn]

Awesome! And there have since been a few merge conflicts - could you please also take a look into those? :)

Of course :-) Going to look at them as soon as I can today

[aeneasr]

Thank you! We still need to explain this feature in the documentation. Do you need help for that? :)

[erolkskn]

Thank you! We still need to explain this feature in the documentation. Do you need help for that? :)

No help needed now, thank you :-) I'm going to create a PR regarding this on "ory/docs" this evening.

[aeneasr]

awesome!!!

[hperl]

LGTM! Thanks for the good work, @erolkskn!

[erolkskn]

LGTM! Thanks for the good work, @erolkskn!

Your welcome :-)

[graysontvbs]

hi , I am using the "import" command to import the identity, and the password is encrypted using the md5 method. However, I cannot log in correctly. After checking, I found that if the previous password was encrypted using a different programming language, it cannot be properly compared and verified.

This is because the md5 encryption method in Go differs from other programming languages (Go uses bytes). However, in older systems, different programming languages such as PHP or C# may have been used. This prevents me from using those md5 passwords for login.

In summary, it seems that there are two different md5 encryption methods, and it appears that they are not supported at the moment. Can we open a separate issue to address this matter?

[jonas-jonas]

I found that if the previous password was encrypted using a different programming language, it cannot be properly compared and verified.

With which programming language were your passwords hashed before? In theory the implementation in Kratos should follow the spec, but if the passwords were hashed with some custom implementation, that deviates from the spec, Kratos won't be able to verify the passwords.

Please also make sure, to include the salt if a salt was used to hash the passwords.

Can we open a separate issue to address this matter?

Please do!

[graysontvbs]

I think I have reached some conclusions. The unit test cases for this PR are incorrect. Taking the first test case as an example, after encrypting with md5 and base64, the expected result should be MDk4ZjZiY2Q0NjIxZDM3M2NhZGU0ZTgzMjYyN2I0ZjY=, but the test case uses CY9rzUYh03PK3k6DJie09g==.

The reason for this is that the encryption process involves passing the byte[:] result of md5 into the base64 encode function, rather than passing the string result of md5. If you use the following sample code, you can clearly see that the base64 output will be different depending on the input data:

md5Hash := md5.Sum([]byte("test")) // 098f6bcd4621d373cade4e832627b4f6

//use byte[:] to base64 encode (X)
base64Encoded := base64.StdEncoding.EncodeToString(md5Hash[:])
fmt.Println(base64Encoded) //CY9rzUYh03PK3k6DJie09g==

//use string to base64 encode (O)
base64Encoded1 := base64.StdEncoding.EncodeToString([]byte(hex.EncodeToString(md5Hash[:])))
fmt.Println(base64Encoded1) //MDk4ZjZiY2Q0NjIxZDM3M2NhZGU0ZTgzMjYyN2I0ZjY=

I will open an issue, and if anyone is willing, please assist in fixing it. 🥲