feat(hash): Add PBKDF2 as password hashing algorithm #1774

sawadashota · 2021-09-22T09:25:19Z

Related issue(s)

Checklist

I have read the contributing guidelines.
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.

Further Comments

zepatrik

This looks pretty good already. I am not sure though if this is the intended way discussed in the issue. This does not allow to update imported hashes but just adds PBKDF2 as an additional hasher. The password upgrade process might actually be implemented on top, so we might actually need it as is.
We could have something along the lines of

hashers:
  bcrypt:
    cost: 10
  pbkdf2:
    upgrade_to: bcrypt

in the config, which would mean that all pbkdf2 hashes will be upgraded once checked.

driver/config/.schema/config.schema.json

sawadashota · 2021-09-23T07:23:16Z

@zepatrik Thank you for your super fast review!
I have fixed it to use PBKDF2 as just compare hash for existing credential and upgrade hashing algorithm after authenticated.

And I remove configuration for PBKDF2 because we can upgrade verified and not primary hashing algorithm automatically.

codecov · 2021-09-23T07:55:13Z

Codecov Report

Merging #1774 (30b1b5c) into master (dc36fda) will decrease coverage by 0.69%.
The diff coverage is 76.59%.

❗ Current head 30b1b5c differs from pull request most recent head 0baeccb. Consider uploading reports for the commit 0baeccb to get more accurate results

@@            Coverage Diff             @@
##           master    #1774      +/-   ##
==========================================
- Coverage   74.85%   74.16%   -0.70%     
==========================================
  Files         291      267      -24     
  Lines       14963    13242    -1721     
==========================================
- Hits        11201     9821    -1380     
+ Misses       2960     2757     -203     
+ Partials      802      664     -138

Impacted Files	Coverage Δ
driver/config/config.go	`81.16% <ø> (-0.54%)`	⬇️
hash/hasher_argon2.go	`72.72% <0.00%> (-7.28%)`	⬇️
selfservice/strategy/password/login.go	`72.72% <47.36%> (-11.59%)`	⬇️
hash/hasher_pbkdf2.go	`81.81% <81.81%> (ø)`
hash/hash_comparator.go	`83.72% <89.47%> (+7.25%)`	⬆️
hash/hasher_bcrypt.go	`88.88% <100.00%> (+1.38%)`	⬆️
text/message_system.go	`0.00% <0.00%> (-100.00%)`	⬇️
text/message_recovery.go	`63.04% <0.00%> (-36.96%)`	⬇️
text/message_validation.go	`64.70% <0.00%> (-35.30%)`	⬇️
ui/node/attributes.go	`27.58% <0.00%> (-33.29%)`	⬇️
... and 95 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc36fda...0baeccb. Read the comment docs.

sawadashota · 2021-09-23T09:36:55Z

Added tests.

zepatrik

Looking pretty good now 👍
One big thing missing now is documentation. I am also not 💯 sure how that should look like, maybe you can start and we discuss it then?

zepatrik · 2021-09-23T09:40:39Z

selfservice/strategy/password/login.go

@@ -68,6 +71,12 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow)
 		return nil, s.handleLoginError(w, r, f, &p, errors.WithStack(schema.NewInvalidCredentialsError()))
 	}

+	if !s.d.Hasher().IsSameAlgorithm([]byte(o.HashedPassword)) {


~~This check does not make sense here. The clear-text password does not contain the algorithm.~~ - you fixed that already 😉

In the long term, we have to make it configurable which hash alg should be upgraded to which one. And some, like PBKDF2, will just be upgraded by default, as they are only intended for import.
For this PR it would be sufficient to just auto-upgrade PBKDF2 without a config option. So adding a check here that returns whether a hash is auto-upgrade (currently only PBKDF2), should be the way to go IMO.

Made it configurable!

One big thing missing now is documentation

Hmm... currently, Kratos doesn't have ability to import user credentials now.
So we have 2 choices to document currently.

Document "Wait to resolve Add ability to import user credentials #605 " 323fdc3

Implement for Add ability to import user credentials #605 in thi PR

Which do you think better?

Both are nice :) It would be awesome to be able to import users - this has been something the community has asked for for a long time. However, I think it's fine with documenting that this is coming in the future for now in this PR

hash/hash_comparator.go

seremenko-wish · 2021-09-29T21:35:37Z

hashers:
  bcrypt:
    cost: 10
  pbkdf2:
    upgrade_to: bcrypt

What's the point of the upgrade_to option under a specific algorithm? By default, hashing system works in compatibility mode, which means that all known password hashing types are supported during the hash validation process. So if you are going to migrate identities with pbkdf2 hashes, you could use those hashes and never migrate them. But if you want to gradually migrate to a different algorithm during login and setting flows, it makes sense to have one boolean setting hashers.migrate that would force migration to hashers.algorithm

sawadashota · 2021-09-30T06:02:57Z

Sounds good!
Then configuration goes to like...

hashers:
  algorithm: bcrypt
  migrate: true
  bcrypt:
    cost: 10

@aeneasr @zepatrik Do you have any concerns?

zepatrik · 2021-09-30T09:12:37Z

Sounds reasonable to me, only thing would be that you might want to specify a per-alg migrate flag.
So e.g.

hashers:
  algorithm: bcrypt
  bcrypt:
    cost: 10
  pbkdf2:
    migrate: true
  argon2:
    migrate: false

which would keep argon2 hashes, but migrate pbkdf2. Not sure if this is actually a use case, but I can imagine you could safe some resources with that strategy.

But a general migrate flag is probably good for now.

aeneasr · 2021-09-30T13:27:23Z

I would enforce migration. We don't want to stick with old PKBDF2 hashes in the DB forever but slowly migrate everyone over :)

seremenko-wish · 2021-09-30T17:39:38Z

I also think we have to force passwords migration, maybe even do not provide an option not to migrate.

As I separate topic, I wanted to discuss a format of hash. Because we already use the PHC standard for password hashes, we have to stick with it https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md. So hash for this algorithm should have a format:

$pbkdf2-<digest>$i=<iterations>$<salt>$<hash>

Example here https://www.npmjs.com/package/@phc/pbkdf2#phc-string-format

aeneasr · 2021-10-01T09:16:31Z

Ok, that makes a ton of sense @seremenko-wish - thank you for the input

sawadashota · 2021-10-01T10:41:45Z

For now, I want to clarify what is configurable to enforce migration.
Is it from only pbkdf2 or all non-primary algorithms?
I think the later is simple if it is enough.

hashers:
  algorithm: bcrypt
  bcrypt:
    cost: 10
  # Cannot configure for pbkdf2. Auto-upgrade to primary algorithm.
  #pbkdf2:
  #  migrate: true
  argon2:
    migrate: false # Is this configurable like this, leave it or auto-upgrade to primary algorithm?

aeneasr · 2021-10-01T11:08:41Z

I would remove the migrate option. We always migrate to the globally configured hasher (hashers.algorithm). So:

hashers:
  algorithm: bcrypt
  bcrypt:
    cost: 10
  pbkdf2:
    # not sure if this needs any configuration?
  argon2:
    memory: ...
    # ....

sawadashota · 2021-10-05T05:43:42Z

I fixed to always migrate to the globally configured hasher.

sawadashota · 2021-10-12T01:10:50Z

Resolved conflict.

aeneasr · 2021-10-12T06:10:43Z

Awesome, I will take a look at this now!

aeneasr · 2021-10-12T06:14:13Z

@sawadashota what do you think about this comment from @seremenko-wish ?

I also think we have to force passwords migration, maybe even do not provide an option not to migrate.

As I separate topic, I wanted to discuss a format of hash. Because we already use the PHC standard for password hashes, we have to stick with it https://github.com/P-H-C/phc-string-format/blob/master/phc-sf-spec.md. So hash for this algorithm should have a format:

$pbkdf2-<digest>$i=<iterations>$<salt>$<hash>

Example here https://www.npmjs.com/package/@phc/pbkdf2#phc-string-format

I think it would make sense to follow the PHC format here.

sawadashota · 2021-10-12T09:44:41Z

I think it makes sense but I have a small question about this format $pbkdf2-<digest>$i=<iterations>$<salt>$<hash>.
The param name looks defined by simonepri/phc-pbkdf2 repository because PHC doesn't define for PBKDF2.
https://github.com/simonepri/phc-pbkdf2#phc-string-format

Referring to RFC2898, I expect iteration parameter is described as c.
Then format will be $pbkdf2-<digest>$c=<iterations>$<salt>$<hash>.

aeneasr · 2021-10-12T09:50:11Z

@seremenko-wish maybe you can chime in here?

seremenko-wish · 2021-10-12T16:20:45Z

@sawadashota I see that number of iterations is defined as c in RFC, but I haven't found a module that uses c for it yet. But I found a bunch of packages that use i instead:

https://passlib.readthedocs.io/en/stable/lib/passlib.hash.pbkdf2_digest.html#passlib.hash.pbkdf2_sha1 does not use named parameters at all :/

As there is no defined standard for this specific algorithm, it would be up to you to decide what named parameter to use for iterations.

aeneasr · 2021-10-12T17:06:17Z

@sawadashota I see that number of iterations is defined as c in RFC

As far as I understand, the RFC itself does not touch on the PHC format. Might make sense though to use c for consistency as the RFC will probably rarely change?

I also did some research and found that Auth0 uses the format from https://github.com/simonepri/phc-pbkdf2#phc-string-format . I assume Auth0 probably uses that library internally.

I also found a few more docs but unfortunately no concrete guidance:

https://libpasta.github.io/technical-details/supported/ (does not say what params pkdbf2 has)
https://meta.discourse.org/t/export-password-hashes-in-the-phc-format/133805 (they use i for iterations and have an additional l parameter - i think it could refer to dkLen from RFC 2898?)
https://community.auth0.com/t/cant-log-in-using-imported-pbkdf2-hashed-passwords/61815/9 (hash import actually not working in auth0)

So yeah basically the question is:

Do we want to stick with the nomenclautur from the RFC - so c and dkLen or
stick to the nomenclautur established in the one NPM library + one or two other resources around this with i and l?

https://docs.djangoproject.com/en/3.2/topics/auth/passwords/

Signed-off-by: sawadashota <[email protected]>

sawadashota · 2021-10-15T11:29:51Z

@aeneasr Could you review PR?

aeneasr · 2021-10-19T12:11:21Z

It's part of 0.8.0 now so due to be released before the ory summit

aeneasr

This looks ready to be merged from a code level! Just one thing :)

Could you please add docs to e.g. https://www.ory.sh/kratos/docs/next/concepts/credentials/username-email-password and explain how we expect PKBDF2 hashes to look like and what the parameters mean?

aeneasr · 2021-10-19T14:52:13Z

selfservice/strategy/password/login.go

@@ -68,6 +71,12 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow)
 		return nil, s.handleLoginError(w, r, f, &p, errors.WithStack(schema.NewInvalidCredentialsError()))
 	}

+	if !s.d.Hasher().IsSameAlgorithm([]byte(o.HashedPassword)) {


Both are nice :) It would be awesome to be able to import users - this has been something the community has asked for for a long time. However, I think it's fine with documenting that this is coming in the future for now in this PR

aeneasr · 2021-10-19T21:30:40Z

Failing e2e tests are probably my fault

Signed-off-by: sawadashota <[email protected]>

sawadashota · 2021-10-21T06:56:49Z

Added Hashed Password Format section to https://www.ory.sh/kratos/docs/next/concepts/credentials/username-email-password !
Documents for BCrypt and Argon2 are also added but if they are unnecessary, I will remove them.

Signed-off-by: sawadashota <[email protected]>

aeneasr

Great job! Thank you! :)

sawadashota requested review from aeneasr and zepatrik as code owners September 22, 2021 09:25

zepatrik reviewed Sep 22, 2021

View reviewed changes

driver/config/.schema/config.schema.json Outdated Show resolved Hide resolved

driver/config/.schema/config.schema.json Outdated Show resolved Hide resolved

sawadashota marked this pull request as draft September 23, 2021 09:03

sawadashota force-pushed the feat-1659 branch from cd3518a to c7beb57 Compare September 23, 2021 09:36

sawadashota marked this pull request as ready for review September 23, 2021 09:36

zepatrik reviewed Sep 23, 2021

View reviewed changes

aeneasr reviewed Sep 23, 2021

View reviewed changes

hash/hash_comparator.go Show resolved Hide resolved

sawadashota force-pushed the feat-1659 branch 2 times, most recently from bc887f9 to 713b1c3 Compare October 4, 2021 10:17

sawadashota force-pushed the feat-1659 branch from b2555e3 to 2b97863 Compare October 12, 2021 01:09

sawadashota added 12 commits October 13, 2021 22:40

fix: PBKDF2 encoded body respects Django style

61ff7e7

https://docs.djangoproject.com/en/3.2/topics/auth/passwords/

fix: Parse uint32 string

ea81e4c

Update docs

c5fa8df

fix: PBKDF2 test

2c6b1f8

fix: login flow test

07a897b

Signed-off-by: sawadashota <[email protected]>

Document abount PBKDF2 format

2ace692

Signed-off-by: sawadashota <[email protected]>

fix: Auto-upgrade password hash algorithm to hashers.algorithm

9a5bfe6

Signed-off-by: sawadashota <[email protected]>

fix: migrate password hash method name

1afc1f5

Signed-off-by: sawadashota <[email protected]>

Document about auto-migration

97777e0

Signed-off-by: sawadashota <[email protected]>

Add tests

7bd1c82

Signed-off-by: sawadashota <[email protected]>

fix: Encoding format for PBKDDF2

9699a1f

Add test using upper case i

30b1b5c

Signed-off-by: sawadashota <[email protected]>

sawadashota force-pushed the feat-1659 branch from 18891e3 to 30b1b5c Compare October 13, 2021 13:40

aeneasr added this to the v0.8.0-alpha.1 milestone Oct 19, 2021

Merge branch 'master' into feat-1659

b572366

aeneasr reviewed Oct 19, 2021

View reviewed changes

chore: code review

6e7ed6f

sawadashota added 3 commits October 21, 2021 14:23

chore: Add doc about import credentials

2815d1a

Signed-off-by: sawadashota <[email protected]>

chore(docs): Stored hash password format

338afb9

Signed-off-by: sawadashota <[email protected]>

chore: Format docs

486660a

Signed-off-by: sawadashota <[email protected]>

fix: Use raw string for gosimple lint

0baeccb

Signed-off-by: sawadashota <[email protected]>

aeneasr requested changes Oct 25, 2021

View reviewed changes

aeneasr approved these changes Oct 25, 2021

View reviewed changes

Merge branch 'master' into feat-1659

9c09126

aeneasr merged commit 33cc7e0 into ory:master Oct 25, 2021

sawadashota deleted the feat-1659 branch October 26, 2021 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hash): Add PBKDF2 as password hashing algorithm #1774

feat(hash): Add PBKDF2 as password hashing algorithm #1774

sawadashota commented Sep 22, 2021

zepatrik left a comment

sawadashota commented Sep 23, 2021

codecov bot commented Sep 23, 2021 •

edited

Loading

sawadashota commented Sep 23, 2021

zepatrik left a comment

zepatrik Sep 23, 2021

sawadashota Sep 27, 2021

aeneasr Oct 19, 2021

seremenko-wish commented Sep 29, 2021

sawadashota commented Sep 30, 2021

zepatrik commented Sep 30, 2021 •

edited

Loading

aeneasr commented Sep 30, 2021

seremenko-wish commented Sep 30, 2021

aeneasr commented Oct 1, 2021

sawadashota commented Oct 1, 2021

aeneasr commented Oct 1, 2021

sawadashota commented Oct 5, 2021

sawadashota commented Oct 12, 2021

aeneasr commented Oct 12, 2021

aeneasr commented Oct 12, 2021

sawadashota commented Oct 12, 2021

aeneasr commented Oct 12, 2021

seremenko-wish commented Oct 12, 2021

aeneasr commented Oct 12, 2021

sawadashota commented Oct 15, 2021

aeneasr commented Oct 19, 2021

aeneasr left a comment

aeneasr Oct 19, 2021

aeneasr commented Oct 19, 2021

sawadashota commented Oct 21, 2021

aeneasr left a comment

feat(hash): Add PBKDF2 as password hashing algorithm #1774

feat(hash): Add PBKDF2 as password hashing algorithm #1774

Conversation

sawadashota commented Sep 22, 2021

Related issue(s)

Checklist

Further Comments

zepatrik left a comment

Choose a reason for hiding this comment

sawadashota commented Sep 23, 2021

codecov bot commented Sep 23, 2021 • edited Loading

Codecov Report

sawadashota commented Sep 23, 2021

zepatrik left a comment

Choose a reason for hiding this comment

zepatrik Sep 23, 2021

Choose a reason for hiding this comment

sawadashota Sep 27, 2021

Choose a reason for hiding this comment

aeneasr Oct 19, 2021

Choose a reason for hiding this comment

seremenko-wish commented Sep 29, 2021

sawadashota commented Sep 30, 2021

zepatrik commented Sep 30, 2021 • edited Loading

aeneasr commented Sep 30, 2021

seremenko-wish commented Sep 30, 2021

aeneasr commented Oct 1, 2021

sawadashota commented Oct 1, 2021

aeneasr commented Oct 1, 2021

sawadashota commented Oct 5, 2021

sawadashota commented Oct 12, 2021

aeneasr commented Oct 12, 2021

aeneasr commented Oct 12, 2021

sawadashota commented Oct 12, 2021

aeneasr commented Oct 12, 2021

seremenko-wish commented Oct 12, 2021

aeneasr commented Oct 12, 2021

sawadashota commented Oct 15, 2021

aeneasr commented Oct 19, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Oct 19, 2021

Choose a reason for hiding this comment

aeneasr commented Oct 19, 2021

sawadashota commented Oct 21, 2021

aeneasr left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 23, 2021 •

edited

Loading

zepatrik commented Sep 30, 2021 •

edited

Loading