feat: allow importing salted sha hashing algorithms #2741

zambien · 2022-09-16T18:23:22Z

Allows importing of salted SHA (SSHA, SSHA256, and SSHA512) passwords into Ory Kratos

Related issue(s)

#2422

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
security@ory.sh) 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.

CLAassistant · 2022-09-16T18:23:27Z

All committers have signed the CLA.

zambien · 2022-09-16T18:23:45Z

I'll get started on updating the docs soon!

codecov · 2022-09-16T18:53:39Z

Codecov Report

Merging #2741 (d914324) into master (acf9261) will decrease coverage by 0.38%.
The diff coverage is 89.62%.

❗ Current head d914324 differs from pull request most recent head de64823. Consider uploading reports for the commit de64823 to get more accurate results

@@            Coverage Diff             @@
##           master    #2741      +/-   ##
==========================================
- Coverage   77.78%   77.40%   -0.38%     
==========================================
  Files         316      312       -4     
  Lines       19881    19484     -397     
==========================================
- Hits        15464    15082     -382     
+ Misses       3258     3244      -14     
+ Partials     1159     1158       -1

Impacted Files	Coverage Δ
identity/handler_import.go	`63.04% <0.00%> (ø)`
hash/hash_comparator.go	`85.61% <90.38%> (+1.40%)`	⬆️
identity/credentials_migrate.go	`74.19% <100.00%> (ø)`
driver/config/config.go	`82.77% <0.00%> (-5.24%)`	⬇️
courier/courier.go	`70.58% <0.00%> (-3.88%)`	⬇️
courier/smtp.go	`69.56% <0.00%> (-3.82%)`	⬇️
identity/handler.go	`87.95% <0.00%> (-1.40%)`	⬇️
selfservice/strategy/webauthn/nodes.go	`100.00% <0.00%> (ø)`
identity/credentials_webauthn.go
x/keys.go
... and 4 more

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

hash/hash_comparator.go

hperl

Thank you very much for your PR. It looks very good already. I have just left some comments for you.

hperl

Thanks for the update! It looks very good now 🎉, but I think we still need to iterate over the salting. 🧂

hash/hash_comparator.go

hugotiburtino · 2022-09-30T10:22:12Z

So in short, I think we should inline the functionality and make the salting more flexible. WDYT?

Definitely having the possibility of prepending the salt would save many lifes (i.e. many password resets)

zambien · 2022-10-14T11:23:46Z

So in short, I think we should inline the functionality and make the salting more flexible. WDYT?

Definitely having the possibility of prepending the salt would save many lifes (i.e. many password resets)

@hugotiburtino you have a use case where you are using SSHA, SSHA256, or SSHA512 where the salt is prepended instead of being appended? There is another thread where the latest comment @aeneasr thought to use the initial solution for SSHA (not using PHC). I want to be sure to consider everyone's needs. Even though using the {SSHA}... format is easier I agree that isn't really a standard, but is a norm/more. Although every implementation I've seen for SSHA uses the appended implementation.

Edit: punctuation

elbotho · 2022-10-14T13:12:41Z

@hugotiburtino you have a use case where you are using SSHA, SSHA256, or SSHA512 where the salt is prepended instead of being appended? There is another thread where the latest comment @aeneasr thought to use the initial solution for SSHA (not using PHC). I want to be sure to consider everyone's needs. Even though using the {SSHA}... format is easier I agree that isn't really a standard, but is a norm/more. Although every implementation I've seen for SSHA uses the appended implementation.

Edit: punctuation

Our use case (same org. as @hugotiburtino) is actually not SSHA but a custom flavour of SHA so we would be well served with the PHC syntax. Thanks for your consideration.

If we can help you with this PR in any way, just get in touch.

merge first into ory/kratos's master.

to maintain the convention.

since the former's hash comparator was added after the latter's one.

Refactor SSHA and SHA comparators

Revert "Refactor SSHA and SHA comparators"

It is like #3 but without trying to solve the conflicts of current ory/kratos master branch (head at f5c4cca), which caused many unwanted changes.

Abstract SSHA comparator and minor changes without merging from ory/kratos master

hperl

Thank you for the changes, LGTM!

There was a bug in the error handling for short SSHA passwords that I fixed in fea0bbd. PTAL.

aeneasr · 2022-11-21T16:56:32Z

We probably also need to update the documentation to now include regular SHA?

elbotho · 2022-11-23T16:37:05Z

We probably also need to update the documentation to now include regular SHA?

@aeneasr that's right. I'm awaiting the review of ory/docs#1090 so we can explain the formatting in the same way and can add the changes then.

aeneasr · 2022-12-06T11:34:44Z

That's merged now! There's a few conflicts left that need resolving.

@hperl are the changes good to go now?

hperl

This looks very good now 🎉!

I just merged master, if CI passes we should be ready to merge 💪

aeneasr · 2023-01-28T08:23:28Z

epic!

zambien added 4 commits September 16, 2022 13:33

add ssha, ssha256, and ssha512 hashing algorithms

ea26fed

working on adding ssha tests to identity

e6153a3

tests passing

ab49015

remove accidental duplicate lowercase TestHandler files for SSHA

6bac8cf

zambien requested review from aeneasr and zepatrik as code owners September 16, 2022 18:23

Merge branch 'master' into ssha_algs

c908d20

zambien mentioned this pull request Sep 16, 2022

docs: add salted sha documentation ory/docs#1001

Merged

6 tasks

zambien added 2 commits September 19, 2022 10:16

return the same error message when passwords do not match

2f4047c

Merge branch 'ssha_algs' of github.com:zambien/kratos into ssha_algs

2538556

hperl requested changes Sep 21, 2022

View reviewed changes

hash/hash_comparator.go Outdated Show resolved Hide resolved

hash/hash_comparator.go Outdated Show resolved Hide resolved

hperl requested changes Sep 21, 2022

View reviewed changes

zambien marked this pull request as draft September 21, 2022 18:00

zambien marked this pull request as ready for review September 22, 2022 14:06

pull latest pwscheme master

f87bbec

hperl requested changes Sep 23, 2022

View reviewed changes

hash/hash_comparator.go Outdated Show resolved Hide resolved

hugotiburtino and others added 8 commits October 27, 2022 10:47

demo(serlo-hash): hacky code for dealing with sha1 salt + pass

9639dff

feat: add more general sha1 implementation

7dfb1a4

add tests, revert to hex hack

d8140b9

without hex lib

c519b84

add more tests

16a76fb

generalise sha, replace pwsheme lib

2d72ae0

add sha256 and sha512

7cef479

sha: update tests

fbf077a

hugotiburtino and others added 10 commits October 30, 2022 22:03

style(hash): put decodeFirebaseScryptHash before sha since it was

f1103d3

merge first into ory/kratos's master.

refactor(hash): abstract SSHA comparators

4e86499

refactor(hash): reduce SSHA regex and handling to just one

4b0db1e

refactor(hash): avoid unecessary double converting and minor renaming

71b4d60

to maintain the convention.

style(hasher): put tests for firescrypt after the ones for scrypt

787fbbe

since the former's hash comparator was added after the latter's one.

Merge pull request #3 from serlo/ssha_algs

07dce17

Refactor SSHA and SHA comparators

Revert "Refactor SSHA and SHA comparators"

f0ef517

Merge pull request #4 from zambien/revert-3-ssha_algs

33332d8

Revert "Refactor SSHA and SHA comparators"

refactor(hash): abstract SSHA comparator and minor improvements

7f0e826

It is like #3 but without trying to solve the conflicts of current ory/kratos master branch (head at f5c4cca), which caused many unwanted changes.

Merge pull request #5 from serlo/ssha_algs

c6b3a6e

Abstract SSHA comparator and minor changes without merging from ory/kratos master

aeneasr requested a review from hperl November 21, 2022 15:10

fix: bug in parser error handling

fea0bbd

hperl previously approved these changes Nov 21, 2022

View reviewed changes

elbotho mentioned this pull request Dec 6, 2022

docs: add importing sha passwords ory/docs#1144

Closed

ci42 mentioned this pull request Dec 12, 2022

Allow import peppered password hashes #2946

Closed

6 tasks

jonas-jonas assigned hperl Jan 27, 2023

hperl self-requested a review January 27, 2023 08:56

hperl added 2 commits January 27, 2023 11:25

chore: guard against unknown SHA hashes

3a18248

Merge remote-tracking branch 'origin/master' into ssha_algs

95aa572

hperl dismissed their stale review via 95aa572 January 27, 2023 11:44

hperl previously approved these changes Jan 27, 2023

View reviewed changes

chore: fix linter warnings

de64823

hperl dismissed their stale review via de64823 January 27, 2023 12:06

hperl self-requested a review January 27, 2023 12:17

hperl approved these changes Jan 27, 2023

View reviewed changes

aeneasr merged commit 132255e into ory:master Jan 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow importing salted sha hashing algorithms #2741

feat: allow importing salted sha hashing algorithms #2741

zambien commented Sep 16, 2022 •

edited

Loading

CLAassistant commented Sep 16, 2022 •

edited

Loading

zambien commented Sep 16, 2022

codecov bot commented Sep 16, 2022 •

edited

Loading

hperl left a comment

hperl left a comment

hugotiburtino commented Sep 30, 2022

zambien commented Oct 14, 2022 •

edited

Loading

elbotho commented Oct 14, 2022

hperl left a comment

aeneasr commented Nov 21, 2022

elbotho commented Nov 23, 2022

aeneasr commented Dec 6, 2022

hperl left a comment

aeneasr commented Jan 28, 2023

feat: allow importing salted sha hashing algorithms #2741

feat: allow importing salted sha hashing algorithms #2741

Conversation

zambien commented Sep 16, 2022 • edited Loading

Related issue(s)

Checklist

CLAassistant commented Sep 16, 2022 • edited Loading

zambien commented Sep 16, 2022

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

hperl left a comment

Choose a reason for hiding this comment

hperl left a comment

Choose a reason for hiding this comment

hugotiburtino commented Sep 30, 2022

zambien commented Oct 14, 2022 • edited Loading

elbotho commented Oct 14, 2022

hperl left a comment

Choose a reason for hiding this comment

aeneasr commented Nov 21, 2022

elbotho commented Nov 23, 2022

aeneasr commented Dec 6, 2022

hperl left a comment

Choose a reason for hiding this comment

aeneasr commented Jan 28, 2023

zambien commented Sep 16, 2022 •

edited

Loading

CLAassistant commented Sep 16, 2022 •

edited

Loading

codecov bot commented Sep 16, 2022 •

edited

Loading

zambien commented Oct 14, 2022 •

edited

Loading