Add ability to import user password credentials #1963
Conversation
|
|
identity/handler.go
Outdated
| if !cr.Password.IsHashed { | ||
| var err error | ||
| var hasher hash.Hasher | ||
| if h.r.Config(ctx).HasherPasswordHashingAlgorithm() == Bcrypt { |
There was a problem hiding this comment.
Should PBKDF2 also be added as an option? Looks like support for comparing hashes was merged in #1659
There was a problem hiding this comment.
Yes! :) Can we maybe use the hash detection we already have from here: https://github.com/ory/kratos/blob/master/hash/hash_comparator.go#L21-L32 ? :)
There was a problem hiding this comment.
This code is not comparing hashes, is creating them only. The purpose of this part of the code is to receive a cleartext password and to store it hashed. I haven't found on the existing code any way to actually create a PBKDF2 hasher to hash a password. There seem to be code only to create a PBKDF2 hasher from an existing hashed password (on https://github.com/ory/kratos/blob/master/hash/hash_comparator.go#L67).
To be able to add support to PBKDF2 here I would need to define a way to configure the PBKDF2 hasher to actually be able to create it. And that's out of the scope for what I wanted to do here.
Something I do not like about my code, though, is that I basically copied the code from
https://github.com/ory/kratos/blob/master/driver/registry_default.go#L393-L402 because when trying to cast the handler to a RegistryDefault I got a circular dependency.
I've uploaded a new commit to the PR to deal with the circular dependency. To do so:
driver/config imports hash now. hash does not import driver/config. Instead, all the types that hash need are moved to config. After doing that, I found another new circular dependency (schema->config->hash->schema). To remove that one I moved what hash uses from schema (the errors) to its own package. Most of the changes on the commit are from refactoring schema/errors.go to schema/errors/errors.go.
This way the code to construct the hashers is only in one place (in the hash package) and adding new hashers should be simpler. Hopefully.
There was a problem hiding this comment.
Great stuff! I think this has some minor changes needed but is already almost good to go from a code perspecitve!
We definitely need some tests for this, so in particular testing the different hash types on import, as well as clear text hash import (is the PW being hashed correctly on import?).
Lastly, we need some docs, in particular:
identity/handler.go
Outdated
| if !cr.Password.IsHashed { | ||
| var err error | ||
| var hasher hash.Hasher | ||
| if h.r.Config(ctx).HasherPasswordHashingAlgorithm() == Bcrypt { |
There was a problem hiding this comment.
Yes! :) Can we maybe use the hash detection we already have from here: https://github.com/ory/kratos/blob/master/hash/hash_comparator.go#L21-L32 ? :)
| @@ -205,6 +210,30 @@ type adminCreateIdentity struct { | |||
| Body AdminCreateIdentityBody | |||
| } | |||
|
|
|||
| // CredentialsConfig is the struct that is being used as part of the identity credentials. | |||
| type CredentialsConfig struct { | |||
There was a problem hiding this comment.
CredentialsConfig is different per credential, it can also be e.g. credentials for WebAuthn, OIDC, etc.
If we move this to here, I would suggest PasswordCredentialsConfig. I assume you moved it here because of circular dependencies?
| // cleartext password and it will be hashed before storing. | ||
| // | ||
| // required: true | ||
| IsHashed bool `json:"is_hashed"` |
There was a problem hiding this comment.
This looks great! I have a short idea on the API design:
{
traits: { ... }
credentials: {
"password": {
"hash": "...", // OR
"cleartext": "..."
}
}
}
That would allow us to also add more import types in the future, such as importing OIDC credentials:
{
traits: { ... }
credentials: {
"oidc": {
"providers:" [ ... ]
}
}
}
WDYT?
| // VerifiableAddresses contains all the addresses that can be verified by the user. | ||
| // | ||
| // required: false | ||
| VerifiableAddresses []VerifiableAddress `json:"verifiable_addresses"` |
There was a problem hiding this comment.
The problem here will be that if the email from the traits must match the email set here, otherwise the entry will be deleted immediately. I think we need at least some type of validation here.
What should we do with the extra fields here? So Via, Status, ...?
|
Thank you, this looks great! Could you please sign the CLA (just two clicks), then we can merge it right away! |
|
great job |
|
hi @mcjimenez @aeneasr : |
|
This PR can only be merged once the CLA is signed. If I remember correctly, there's an issue at the place where @mcjimenez works which does not allow him to sign the CLA yet. Once this is clarified, the PR can be merged. |
|
This is a really great improvement, but can I suggest that the passwordCredential type includes an identifier for the hashing algorithm used? This is so that the import can be extended to support hashing algorithms that other identity providers use. I think it would be hugely beneficial to smooth the process of migrating from other systems to kratos. |
|
We already have a standard hashing format which includes the hash type and any (optional) additional configuration such as cost, salt, parallelism and more. This PR reuses this format. |
|
Due to some internal reasons I'm handing off the PR to a colleague. I'm closing this PR. The new PR is at #2151 |
This change implements a very simple way of adding a user to Kratos with a known password, either in clear text or in one of the supported hashes. The user can also be added in a Verified status. This is intended to support a simple way of importing an existing user database.
Related issue(s)
Issue #605
Checklist
introduces a new feature.
contributing code guidelines.
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.
works.