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

!!! TASK: Clean up password hashing strategies #2920

Merged
merged 4 commits into from Feb 13, 2023
Merged

!!! TASK: Clean up password hashing strategies #2920

merged 4 commits into from Feb 13, 2023

Conversation

kdambekalns
Copy link
Member

@kdambekalns kdambekalns commented Oct 10, 2022 •
edited by kitsunet

Cleans up the password hashing strategies shipped with Flow.

Upgrade instructions

If you implemented PasswordHashingStrategyInterface, take note that type declarations will be added for the next major version (9.0) so adjust your implementation to use the added type declarations like in the core implementations.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

@kdambekalns kdambekalns self-assigned this Oct 10, 2022
@kdambekalns
Copy link
Member Author

password_hash() returns strings with $2y$ instead of "our" $2a$ inside. But password_verify() decodes "our" hashed BCrypt passwords fine, see https://3v4l.org/L0QnW#v8.1.11

@kdambekalns kdambekalns mentioned this pull request Oct 10, 2022
Closed
1 task
@kdambekalns
TASK: Clean up password hashing strategies
3c657ba 
This make use of tyoing and password_hash() / password_verify() in the
BCrypt strategy.
@kdambekalns
!!! TASK: Add typing to PasswordHashingStrategyInterface
6549214
@kdambekalns kdambekalns force-pushed the task/cleanup-password-hashing-strategies branch from bcde95e to 6549214 Compare October 10, 2022 08:32
@kdambekalns
Copy link
Member Author

Note: We can of course drop the interface change (or move it to 9.0), but I'd say the risk is low. Who would implement their own hashing strategy? 😇

@kdambekalns
Copy link
Member Author

Ping…

@kdambekalns
Copy link
Member Author

@markusguenther (and others), any objections to the interface change?

@fcool
Copy link
Contributor

fcool commented Dec 14, 2022 •
edited

Ah... hmmm...
Strictly speaking this would be a breaking change, which might be not appropriate for 8.2 - at least without REALLY good reasons...

What about NOT changing the interface in 8.2 - but do so in 9.0? (Rest of the code can stay as it is, if I see it correctly, as "additional hints" are allowed)

On the other hand... I cannot believe, that this interface has been implemented too often outside the Flow packages themself. And it would be quite easy to be forward compatible. (So I would not say, we absolute cannot merge that into 8.2)

@bwaidelich
Copy link
Member

Strictly speaking this would be a breaking change, which might be not appropriate for 8.2 - at least without REALLY good reasons...

I agree

kitsunet
kitsunet requested changes Feb 13, 2023
View reviewed changes
Copy link
Member

@kitsunet kitsunet left a comment

Choose a reason for hiding this comment

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

Agreed with merging ASAP but without the Interface change.

Neos.Flow/Classes/Security/Cryptography/PasswordHashingStrategyInterface.php Outdated Show resolved Hide resolved
Neos.Flow/Classes/Security/Cryptography/PasswordHashingStrategyInterface.php Outdated Show resolved Hide resolved
@kitsunet kitsunet changed the base branch from 8.2 to 8.3 February 13, 2023 06:49
@kdambekalns @kitsunet
Revert change to PasswordHashingStrategyInterface
1bc9ae1 
The addition of type hints is breaking and thus needs to be
done in the next major.

Co-authored-by: Christian Müller <christian@flownative.com>
jonnitto
jonnitto approved these changes Feb 13, 2023
View reviewed changes
Copy link
Member

@jonnitto jonnitto left a comment

Choose a reason for hiding this comment

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

Fine by reading

@kdambekalns
Copy link
Member Author

(Rest of the code can stay as it is, if I see it correctly, as "additional hints" are allowed)

No, as the test results show…

kdambekalns
kdambekalns commented Feb 13, 2023
View reviewed changes
Copy link
Member Author

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

Revert remaining changes to methods from interface

Neos.Flow/Classes/Security/Cryptography/BCryptHashingStrategy.php Outdated Show resolved Hide resolved
Neos.Flow/Classes/Security/Cryptography/BCryptHashingStrategy.php Outdated Show resolved Hide resolved
Neos.Flow/Classes/Security/Cryptography/Pbkdf2HashingStrategy.php Outdated Show resolved Hide resolved
Neos.Flow/Classes/Security/Cryptography/Pbkdf2HashingStrategy.php Outdated Show resolved Hide resolved
Neos.Flow/Tests/Unit/Security/Cryptography/Fixture/TestHashingStrategy.php Outdated Show resolved Hide resolved
Neos.Flow/Tests/Unit/Security/Cryptography/Fixture/TestHashingStrategy.php Outdated Show resolved Hide resolved
@kitsunet
Copy link
Member

Unfortunate :/ But then lets merge this as is now.

@kitsunet kitsunet merged commit 5d31e72 into neos:8.3 Feb 13, 2023
@kdambekalns kdambekalns deleted the task/cleanup-password-hashing-strategies branch February 16, 2023 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fcool fcool fcool approved these changes

@kitsunet kitsunet kitsunet approved these changes

@albe albe albe approved these changes

@jonnitto jonnitto jonnitto approved these changes

@bwaidelich bwaidelich Awaiting requested review from bwaidelich

@robertlemke robertlemke Awaiting requested review from robertlemke

@skurfuerst skurfuerst Awaiting requested review from skurfuerst

@markusguenther markusguenther Awaiting requested review from markusguenther

Assignees

@kdambekalns kdambekalns

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@kdambekalns @fcool @bwaidelich @kitsunet @albe @jonnitto