Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Review and add more commonly used password hashing algorithms for importing users #2422

Closed
6 tasks done
ravikiran438 opened this issue Apr 24, 2022 · 11 comments
Closed
6 tasks done
Labels
feat New feature or request. stale Feedback from one or more authors is required to proceed.

Comments

@ravikiran438
Copy link

Preflight checklist

Describe your problem

As per earlier discussion here 605 and the subsequent implementation 2256 its possible now to import user credentials with passwords hashed using the following 3 algorithms.

  1. Argon2
  2. PKBDF2
  3. BCrypt

But most systems still use legacy hashing algorithms and need a way to support importing of user credentials as sending a recovery link to hundreds of thousands of users could be bothersome to some of the users.

Describe your ideal solution

Solution 1: We may want to review and add few more commonly used algorithms to facilitate easy migration from other systems. As an example Auth0 provides the following hashing algorithms.

And Firebase Authentication allows importing user credentials hashed with the following algorithms
BCRYPT, SCRYPT, STANDARD_SCRYPT, HMAC_SHA512, HMAC_SHA256, HMAC_SHA1, HMAC_MD5, MD5, SHA512, SHA256, SHA1, PBKDF_SHA1, PBKDF2_SHA256.

I am particularly blocked by MD5 but would love to see some of the above implemented for ease of other users. Especially scrypt which is used by Firebase.

Solution 2: Provide a webhook that can be configured to be used as one time password hash verifier much like Okta does which will enable importing users from systems using uncommon algorithms, double hashing and custom practices.

Sorry to plug references from other IdP websites.

Workarounds or alternatives

Only workaround possible is to send recovery link

Version

Ory Cloud

Additional Context

No response

@ravikiran438 ravikiran438 added the feat New feature or request. label Apr 24, 2022
@aeneasr
Copy link
Member

aeneasr commented Apr 24, 2022

Great idea! Contributions welcomed :)

@jonas-jonas
Copy link
Member

@aeneasr is this issue still up-for-grabs and relevant? Would love to take a look at it.

I skimmed the existing parts of the credential import code and read through #605 and it seems a bit too much to support all of the mentioned algorithms, as there would need to be encoding and decoding logic for every algorithm. I thought about starting with scrypt, as it was specifically mentioned here and then going on from there and providing PRs for each algorithm. Would that work for you?

@aeneasr
Copy link
Member

aeneasr commented May 29, 2022

That sounds great  @jonas-jonas !

@jonas-jonas
Copy link
Member

Alright, I had a look at this.

Firebase is not using scrypt directly, but a fork, meaning Firebase passwords can not be verified using the standard scrypt library AFAICT. To support this algorithm for password imports with the current strategy in kratos, a golang implementation of firebase-scrypt is needed. A third-party module exists, but it's not an official SDK. (But since the official SDK is written in C it might be possible to use a cgo based solution here.)

A different method would be to verify the account on login by using Firebase's API, but that would require users to keep their Firebase project up-and-running potentially indefinitely. So not great either. Maybe we can provide a hook here that can be configured in some way to do this?

(FYI @vinckr since we talked about this yesterday.)

@aeneasr
Copy link
Member

aeneasr commented Jun 8, 2022

Phew, that sounds complicated. I would suggest to park Firebase import for now. Do you have some links for reading into the topic?

@vinckr
Copy link
Member

vinckr commented Jun 8, 2022

Thanks for sharing Jonas!
I had an inkling that it was not going to be super easy...

A different method would be to verify the account on login by using Firebase's API, but that would require users to keep their Firebase project up-and-running potentially indefinitely.

I think a "just-in-time" migration for the unsupported algos would be the best long-time solution here. Some users have implemented their own workarounds for this, but ideally we have something out of the box down the line.

@jonas-jonas
Copy link
Member

Phew, that sounds complicated. I would suggest to park Firebase import for now. Do you have some links for reading into the topic?

Yup, thought so as well.
The firebase documentation on exporting users is located at: https://firebase.google.com/docs/cli/auth that page links to their fork of scrypt: https://github.com/firebase/scrypt. Unfortunately, I have not really found anything else on the topic.

I think a "just-in-time" migration for the unsupported algos would be the best long-time solution here. Some users have implemented their own workarounds for this, but ideally we have something out of the box down the line.

I am not sure how the custom hooks for kratos work (yet), but I think either using one of the existing login hooks or providing a new migrated password login hook might be a good solution to cater to every one using some obscure algorithm here. Alongside that we could still provide the current mechanism for well known/defined hash mechanisms with good Golang APIs (MD4/5, standard Scrypt, etc.). WDYT?

I am still looking at supporting MD5 with the current mechanism though I am a bit swamped right now, so I don't have a lot of time.

MD5 does not officially support the PHC format, which is mentioned here: https://www.ory.sh/docs/kratos/concepts/credentials/username-email-password#hashed-password-format
That means, that we need a different way of recognizing hashes, instead of matching the hash-strings using the PHC. There is already an issue at #1378, which would entail a change to the schema, to incorporate the hash type and other metadata about the password. OR we "force" the MD5 hash into the format by just adding $md5$ in front of the hash, this would be "quick and dirty". Again WDYT?

@luxaritas
Copy link

luxaritas commented Mar 7, 2023

I'm personally interested in support for phpass and Drupal's "version" of phpass ($S$/phpass with sha512 and U$S$/phpass with md5 rehashed with phpass with sha512).

While I can't fully commit to doing this myself, I may be interested. I don't know of any "reliable" go module which would provide this - presumably that means inlining a port of the algorithm directly? Would such a PR be accepted, or would that create issues (and if so would there be another suggested option)?

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this issue Jun 30, 2023
It is now possible to import scrypt-hashed passwords.

See ory#2422
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this issue Jun 30, 2023
@tristankenney
Copy link
Contributor

tristankenney commented Sep 20, 2023

Hello friends! I'm interested in having a bash at implementing HMAC support (if it's welcome). I'm assuming a good place to start would be referring to Auth0's options.

I'm assuming a tokenized string format similar to MD5, perhaps something like:
$hmac-<algo>$<hash>$<key>

So, a SHA256 of password with a key of key would look like:
$hmac-sha256$TUL7n/yNfQokVClDi0vHPbEAehZwJqCgxqdPpY6Ohso=$a2V5

Happy for feedback, otherwise I might see if I can throw a PR up for feedback

@knguyen0125
Copy link

@tristankenney HMAC support would be great for us

aeneasr pushed a commit that referenced this issue Oct 12, 2023
The basic format is `$hmac-<hashfunction>$<base64 encoded hash>$<base64 encoded key>`:

```
# password = test; key=key; hash function=sha
$hmac-sha1$NjcxZjU0Y2UwYzU0MGY3OGZmZTFlMjZkY2Y5YzJhMDQ3YWVhNGZkYQ==$a2V5
```

See #2422
Copy link

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers for a year. That does not imply that the issue has no merit! If you feel strongly about this issue

  • open a PR referencing and resolving the issue;
  • leave a comment on it and discuss ideas on how you could contribute towards resolving it;
  • leave a comment and describe in detail why this issue is critical for your use case;
  • open a new issue with updated details and a plan for resolving the issue.

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneously you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️

@github-actions github-actions bot added the stale Feedback from one or more authors is required to proceed. label Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request. stale Feedback from one or more authors is required to proceed.
Projects
None yet
Development

No branches or pull requests

7 participants