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.

Sign up for GitHub

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

Jump to bottom

feat: implement crypt(3) hashers #3303

Merged
merged 6 commits into from
Jun 9, 2023
Merged

feat: implement crypt(3) hashers #3303

merged 6 commits into from
Jun 9, 2023

Conversation

indietyp
Copy link
Contributor

@indietyp indietyp commented Jun 5, 2023 •
edited
Loading

Implement crypt(3) based hashers.

This PR implements md5crypt, sha256crypt, sha512crypt, which are considered legacy (like md5), but are used in legacy systems looking to convert to ory. They use the existing format of crypt(5) (which is compliant to PHC).

Related issue(s)

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 the approval (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.

Further Comments

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@indietyp indietyp mentioned this pull request Jun 5, 2023
Merged
6 tasks
@hperl hperl self-requested a review June 7, 2023 08:24
hperl
hperl requested changes Jun 7, 2023
View reviewed changes
Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution. This looks really good already, I just have two minor comments below.

hash/hash_comparator.go Outdated Show resolved Hide resolved
hash/hash_comparator.go Outdated Show resolved Hide resolved
@indietyp indietyp requested a review from hperl June 7, 2023 10:07
hperl
hperl previously approved these changes Jun 7, 2023
View reviewed changes
Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes, LGTM! 🎉

@aeneasr aeneasr merged commit afe06db into ory:master Jun 9, 2023
@indietyp indietyp deleted the bm/crypt branch June 9, 2023 18:18
@vinckr
Copy link
Member

vinckr commented Jun 13, 2023

Hello @indietyp @hperl

Would it be good to have docs for this? At least seems good IMO to mention it here.

@hperl
Copy link
Contributor

hperl commented Jun 13, 2023

Docs are already done :)
ory/docs#1426

@vinckr
Copy link
Member

vinckr commented Jun 13, 2023

Oh missed that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hperl hperl hperl left review comments

@aeneasr aeneasr Awaiting requested review from aeneasr aeneasr is a code owner

@zepatrik zepatrik Awaiting requested review from zepatrik zepatrik is a code owner

Assignees

@hperl hperl

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@indietyp @CLAassistant @vinckr @hperl @aeneasr