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

Add ability to import user credentials #605

Closed
3 tasks
aeneasr opened this issue Jul 27, 2020 · 52 comments · Fixed by #2256
Closed
3 tasks

Add ability to import user credentials #605

aeneasr opened this issue Jul 27, 2020 · 52 comments · Fixed by #2256
Assignees
Labels
feat New feature or request.
Milestone

Comments

@aeneasr
Copy link
Member

aeneasr commented Jul 27, 2020

Is your feature request related to a problem? Please describe.

It should be possible to import user/identity credentials when creating identities using the POST /identities Admin API.

Describe the solution you'd like

There needs to be some type of value (or a dedicated API?) which allows the client to specify the credential. For the credential type, we should be able to work with passwords hashed with

  • BCrypt;
  • Argon2*;
  • PBKDF1/2.

We could also allow other algorithms, such as SHA with rounds or similar methods. We should probably have a list of software platforms and their hashing algorithms to decide what to support. Custom or insecure mechanisms (salted md5?) should probably not be supported in order to keep the code base tidy.

It should also be possible to import OIDC linked accounts.

The idea would be to specify a payload, for example:

{
  "traits": ...
  "import_credentials": [
    {
      "type": "password",
      "hash": "...",
      "algorithm": "bcrypt"
    },
    {
      "type": "oidc",
      "provider": "google",
      "user_id": "123456"
    }
  ]
}

On the next login, the password would then be converted to a secure Argon2 hash and the original hash will be removed from the database.

Describe alternatives you've considered

Instead of providing the key import_credentials there would be a collection of endpoints at /identities/<identity>/credentials which would allow for PUT, POST, DELETE.

@aeneasr
Copy link
Member Author

aeneasr commented Jul 28, 2020

@Jasrags do you have any preference regarding the API? We're currently thinking about having a dedicated endpoint for that which would mean that you need to do two requests. One to POST /identities and then one to POST /identities/<identity>/credentials. We'd probably add a CLI helper to help with this.

@Jasrags
Copy link

Jasrags commented Jul 28, 2020

@aeneasr I think that could work. I don't have any preferences at the moment.

@aeneasr
Copy link
Member Author

aeneasr commented Jul 29, 2020

Sounds good @Jasrags if you have any other troubles with migrating over to ORY Kratos let me know :)

@zcmack
Copy link

zcmack commented Sep 15, 2020

i'll pile on and say that i have a very, very similar use case and that having a dedicated endpoint / two requests would be just fine. this may be out of scope for this issue, but I figure I'll share for context: in an evaluation of FusionAuth this behaved similarly and worked very well, I was able to use their SDK to easily script a migration however the system would not allow for users to have duplicate email addresses by design. this doesn't work so well for a large public site in which having multiple accounts bound to a single email address is perfectly allowable.

@ykuzma1
Copy link

ykuzma1 commented Nov 6, 2020

Also wanted to chime in that it looks like we will need this feature in our use case. We are looking to pre-create a user using a custom admin interface that POSTs to the /identities endpoint so that we can set various user properties prior to their first login.

Current behavior: First POST a new identity. Have that same OIDC user login. The user gets redirected to the registration flow displaying the following error An account with the same identifier (email, phone, username, ...) exists already.. Trying to Continue with Google brings you back to the Google OAuth screen which then brings you back to the registration flow and same identifier error in an endless loop.

@zepatrik
Copy link
Member

zepatrik commented Nov 8, 2020

Can't you send your users to the account recovery flow instead of registration? You can even send them the recovery link by getting it through the admin API (see https://www.ory.sh/kratos/docs/reference/api#create-a-recovery-link).

@lozybean
Copy link

Should consider compare encoded hash in PHC string format.

In golang.org/x/crypto/bcrypt, there is no way to generate encoded hash with salt, nor compare hash and password in PHC string format, and bcrypt.bcrypt is private too.

@jandd
Copy link

jandd commented Jan 3, 2021

Open Source implementations for password upgrades exist in i.e.

Maybe these can give a good starting point for the password upgrade part of this feature.

I think for those legacy systems that have even worse password hashing it would be cool to have a good guide how to implement an own hash implementation. So that a desperate user can build his own Hydra binary with support for such legacy mechanisms. Go plugins would be another idea but they are not available on all platforms.

The password hash prefix that is available in the Spring security world where all but the default password hash are prefixed with {hashtype} might be a good idea too.

@jandd
Copy link

jandd commented Jan 3, 2021

Can't you send your users to the account recovery flow instead of registration? You can even send them the recovery link by getting it through the admin API (see https://www.ory.sh/kratos/docs/reference/api#create-a-recovery-link).

that is a very good idea indeed. Much better then carrying the old password hashes arround.

@aeneasr
Copy link
Member Author

aeneasr commented Jan 4, 2021

There will be some way to upgrade the passwords in ORY Kratos, we'll try to support as many algorithms as possible but obviously can not support all of them. In the case of the latter, recovery links are the way to go or some type of RPC.

@realStandal
Copy link

I have a use-case which I have yet to implement but have none-the-less been working through and think I've ran into something worth noting here for y'all. It's on-subject with the idea of credentials being set during the creation of an identity via the /identities/ endpoint; I did not see any other issue describing this as a feature but this seems to be that place (I'm seeing now a dupe was amidst, c'est la vie).

I'm exploring using Kratos to manage identities in a multi-tenant environment. I plan to deploy it as suggested, and have ran into no issues with this plan so far. In addition, I'm planning to take full advantage to Kratos' claim of treating identities incredibly arbitrarily (it can be a human, IoT device, etc.); again, no issues there. In the end, tenants will have full control over the management (and life-cycle, so to speak) of their Identities. Registration for a particular tenant's instance will be disabled.

In terms of managing an identity in this context, it would be beneficial for the credential of an identity to be set during its creation using the /identities/ end-point in a single request.

Account's used by human's can take advantage of the recovery flow once created, as suggested:

Can't you send your users to the account recovery flow instead of registration? You can even send them the recovery link by getting it through the admin API (see https://www.ory.sh/kratos/docs/reference/api#create-a-recovery-link).

But for IoT devices, how are they to "reset" their password? For this, lets consider it'd be cumbersome and unnecessarily time-consuming to associate each device with an E-Mail (likely non-uniquely) to force it to use reset behavior.

I'm sure I can (and will need to) make sacrifices and do a bit of leg work to get to my outcome, but I just wanted to highlight this use-case in-case it was beneficial. All-in-all, having the ability to set an identities credentials, using the admin endpoints, would be very beneficial. Letting them be set during creation would be ideal; but even having the second end-point would simply require the additional UI-step of creating the Identity then setting its credential's.

First interaction with y'all as the maintainer(s), lovely software thank you :-)

@aeneasr
Copy link
Member Author

aeneasr commented Feb 5, 2021

@realStandal thank you for your thoughts. We definitely want to add the ability to specify and import passwords. I think that for IoT it would make sense to have some other type of authentication (e.g. a private key) as the notion of a password does not really fit there!

What type of IoT are you looking at?

@realStandal
Copy link

@aeneasr ambiguous to the kind at the moment. Id planned for the key-based once it was supported, nice to know the ability to do so is there, though :) Is y'all's documentation outdated and it is supported? No difference to me atm either way.

@aeneasr
Copy link
Member Author

aeneasr commented Feb 5, 2021

Importing is not yet implemented, scheduled for 0.7 which lands after MFA and flow refactoring. If you'd like to contribute however, let's chat :)

@BlueHotDog
Copy link

Hey, is there any workaround this? we're now exploring moving from our home-brew solution to something more robust but we already have users

@aeneasr
Copy link
Member Author

aeneasr commented Nov 1, 2021

That is great! So let's start with a design document to address this. I think we need these things in place:

  • Ability to import generic password hashes (e.g. BCrypt, PKBDF2, ...) using a standardized hash format (like the one we have for PKBDF2)
  • An REST API outline (with payloads and paths) for implementing generic password hashes
  • An REST API outline for importing dedicated imports for e.g. magento

Who would be open to start such a doc? I think that once we agree on the spec implementation is very straight forward and fast!

@snikch
Copy link

snikch commented Nov 1, 2021

I'm happy to start on this, I can find some time in the next two weeks. Do you have some examples of previous docs like this so I know approx how you'd like it formatted?

@aeneasr
Copy link
Member Author

aeneasr commented Nov 1, 2021

Unfortunately not, it’s a new process! The template has a layout and explanations which hopefully are helpful :)

@snikch
Copy link

snikch commented Nov 1, 2021

Sweet, I've chucked a card into our backlog and will find some time in the next two weeks.

@mcjimenez
Copy link

We have the same problem where we want to start using hydra and kratos to authenticate users, but we already have a user database and we want to make the move as seamless and transparent for users as possible. And that includes being able to use their existing passwords.

For us most of the features like password upgrade paths and so on discussed on this thread are nice to have, but we don't need it right now. Our problem is also easier since our existing user database already uses bcrypt.

With that in mind, we've implemented a change to the POST /identities API to be able to include a hashed (or cleartext) password, and a set of Verifiable Addresses (because our users already have their addresses verified). The (draft) PR with our current set of changes is on #1963

That's currently a draft PR because I don't know if you want to try to land that as a first approach and then add the rest of the functionality. If you think it's worth finishing this to land it, I can move it to a full PR and start the reviewing process.

@harnash
Copy link
Contributor

harnash commented Nov 18, 2021

@mcjimenez we have something similar in tests right now #1963 and perhaps we could have come up with a solution that would work for both use cases (ping @slayful).

@snikch
Copy link

snikch commented Dec 8, 2021

So upon further investigation, it turns out the majority of users in our dataset in Magento have Argon2 hashed passwords that we can import into Kratos. This means that the only blocker for us is the actual password import step which looks to be close to being solved via #1963. I'm happy to help there if anything is blocking @mcjimenez?

@aeneasr
Copy link
Member Author

aeneasr commented Dec 8, 2021

Nice! :) The biggest blocker for merging PR is the CLA. Not sure if @mcjimenez wants to sign it but if lot it would mean that we need a new PR with original changes :/

@g13013
Copy link

g13013 commented Dec 9, 2021

@aeneasr what do you think of my proposal ?

#605 (comment)

we are stuck by this issue and #970

as it takes a lot of time we unfortunately think about abandoning kratos as a solution for us mainly because it seems to be made for new projects and does not take into account existing projects.

@kszafran
Copy link
Contributor

kszafran commented Dec 9, 2021

@g13013 Have you considered migrating existing users by inserting directly into Kratos' database? I haven't worked out the details yet, but that's how I'm planning to move our current users to Kratos. (Or use POST /identities to create users and insert directly into the DB just the credentials.)

@g13013
Copy link

g13013 commented Dec 9, 2021

@kszafran yes, it doesn't work for us for 2 reasons:

  • Does not accept to create identities with their password, so the user has to pass by the self-service to provide the password
  • Password policies are too strict for our existing users DB.

@kszafran
Copy link
Contributor

kszafran commented Dec 9, 2021

I can't comment on password policies, but for the first bullet point, can't you insert user credentials directly into the DB? I imagine calling POST /identities is going to be done by some backend job/process, which could imaginably also have access to Kratos' database.

@harnash
Copy link
Contributor

harnash commented Dec 9, 2021

Inserting credentials directly to DB is a solution but it is quite tricky. We have our migration script that does that but it is quite fragile and it assumes that DB schema does not change. There is Wikia#29 but I need to check why the upstream PR was dropped. Importing via API will be a bit slower and it only updates one identity per request so the overhead with processing the request will be there.

@aeneasr
Copy link
Member Author

aeneasr commented Dec 13, 2021

There’s a PR open for credentials but it looks abandoned. From our team noone has time to work on this right now. But the good thing about open source is that everyone can step up and contribute :) So why not make a PR if this blocks you?

@mcjimenez
Copy link

I'm sorry for having gone dark on this. We're still working on this PR but we ran into some problem internally with signing the CLA, and we're trying to get that untangled

@zcmack
Copy link

zcmack commented Dec 29, 2021

@mcjimenez any updates on this? i'd be glad to create a new PR that is "inspired" by your work for the sake of resolving this issue. i wouldn't want to do this without your permission though, especially if its just corporate red tape that must be resolved.

@mcjimenez
Copy link

I'm sorry for the delay, I was on a Christmas break. It's mostly corporate red tape, which I hope will be fixed soon (as in no longer than this week). I'll get back to you if that's not the case

aeneasr added a commit that referenced this issue Feb 24, 2022
This patch introduces the ability to import passwords (cleartext, PKBDF2, Argon2, BCrypt) and Social Sign In connections when creating identities!

Closes #605
aeneasr added a commit that referenced this issue Feb 25, 2022
This patch introduces the ability to import passwords (cleartext, PKBDF2, Argon2, BCrypt) and Social Sign In connections when creating identities!

Closes #605
aeneasr added a commit that referenced this issue Feb 26, 2022
This patch introduces the ability to import passwords (cleartext, PKBDF2, Argon2, BCrypt) and Social Sign In connections when creating identities!

Closes #605
aeneasr added a commit that referenced this issue Feb 26, 2022
This patch introduces the ability to import passwords (cleartext, PKBDF2, Argon2, BCrypt) and Social Sign In connections when creating identities!

Closes #605
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this issue Jun 30, 2023
This patch introduces the ability to import passwords (cleartext, PKBDF2, Argon2, BCrypt) and Social Sign In connections when creating identities!

Closes ory#605
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request.
Projects
None yet
Development

Successfully merging a pull request may close this issue.