Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Security] Extract password hashing from security-core - with proper wording#39802

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
wouterj merged 1 commit intosymfony:5.xfromchalasr:password-hasher
Feb 12, 2021

Conversation

chalasr
Copy link
Member

@chalasrchalasr commentedJan 12, 2021•
edited
Loading

QA
Branch?5.x
Bug fix?no
New feature?yes
Deprecations?no
TicketsFixes#39698
LicenseMIT
Doc PRtodo

This PR renames password "encoders" to passwordhashers (naming widely used, see e.g. django or laravel).
This also takes the opportunity to extract the logic related to password hashing from security-core, moving it to a new password-hasher component.
Nowadays, many modern web apps and APIs don't deal with passwords at all, that's why splitting makes sense as a step towards making security-core not tied to the password concept.

For upgrading, applications will have to usepasswords_hashers instead ofencoders in their security configuration, and type-hint againstPasswordHasherInterface (and related) instead ofPasswordEncoderInterface.

The proposed API is not much different from the encoder one regarding behavior and signatures, and it is slightly more close to the PHP built-in password hashing API:

namespaceSymfony\Component\PasswordHasher;interface PasswordHasherInterface{publicfunctionhash(string$plainPassword):string;publicfunctionverify(string$hashedPassword,string$plainPassword):bool;publicfunctionneedsRehash(string$hashedPassword):bool;}

TavoNiievez, wouterj, OskarStark, Ioni14, ismail1432, welcoMattic, TomasVotruba, yceruto, sstok, apfelbox, and javiereguiluz reacted with heart emojiOskarStark, ismail1432, welcoMattic, mtarld, yceruto, sstok, and javiereguiluz reacted with rocket emoji
stof
stof previously requested changesJan 12, 2021
@chalasrchalasrforce-pushed thepassword-hasher branch 2 times, most recently fromed07ff3 to1dd61ccCompareJanuary 12, 2021 18:15
derrabus
derrabus previously requested changesJan 12, 2021
@derrabus
Copy link
Member

I like the idea of moving password hasing out of security-core. Would it make sense to create a contracts package for the new hasher interfaces?

@stof
Copy link
Member

I don't think these interfaces need to be in contracts. HasherInterface is not something you would typehint anyway most of the time (you need the factory to get the right hasher for that user class).
And HasherFactoryInterface is not really a contract that make sense to reimplement outsidesymfony/security-* as it depends on other concepts there. So to me, there is no reason to use the interfaces without also using the component itself.

derrabus reacted with thumbs up emoji

@chalasrchalasrforce-pushed thepassword-hasher branch 4 times, most recently from7203a1e tob40a7e0CompareJanuary 12, 2021 21:18
@goetas
Copy link
Contributor

A note on the interface implementation, it happened multiple times that the neeed for a re hashing of a password was something depended on the user/token holding it, not just the password itself. Would it be possible to talke that into consideration somehow?

@chalasrchalasrforce-pushed thepassword-hasher branch 4 times, most recently fromb39ced8 tob6ed024CompareJanuary 15, 2021 11:37
chalasr added a commit that referenced this pull requestJan 16, 2021
…TypeDeclarationsPass (chalasr)This PR was merged into the 4.4 branch.Discussion----------[DependencyInjection] Skip deprecated definitions in CheckTypeDeclarationsPass| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -When a definition uses a deprecated class , `CheckTypeDeclarationsPass` (with `$autoload = true`) will autoload the class, which triggers a deprecation notice. That breaks the CI in#39802 because the compiler pass is registered inside the SecurityBundle test suite.I propose to stop checking deprecated definitions. Makes sense?Commits-------531c81a [DI] Skip deprecated definitions in CheckTypeDeclarationsPass
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Thanks! Good to me,once UPGRADE files are updated :)

@wouterj
Copy link
Member

Thanks a lot@chalasr! I can't wait to see all the deprecated stuff be removed in 6.0 :)

chalasr and OskarStark reacted with hooray emojiOskarStark reacted with heart emoji

@wouterjwouterj merged commitc757845 intosymfony:5.xFeb 12, 2021
@chalasrchalasr deleted the password-hasher branchFebruary 12, 2021 16:02
@localheinzlocalheinz mentioned this pull requestFeb 14, 2021
1 task
chalasr added a commit that referenced this pull requestFeb 14, 2021
…for "native" and "auto" (chalasr)This PR was merged into the 5.3-dev branch.Discussion----------[PasswordHasher] Use bcrypt as default hash algorithm for "native" and "auto"| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -As suggested in#39802 (comment),  based onhttps://twitter.com/TerahashCorp/status/1155129705034653698Commits-------332817a Use bcrypt as default password hash algorithm for "native" and "auto"
symfony-splitter pushed a commit to symfony/password-hasher that referenced this pull requestFeb 14, 2021
…for "native" and "auto" (chalasr)This PR was merged into the 5.3-dev branch.Discussion----------[PasswordHasher] Use bcrypt as default hash algorithm for "native" and "auto"| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | yes| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -As suggested insymfony/symfony#39802 (comment),  based onhttps://twitter.com/TerahashCorp/status/1155129705034653698Commits-------332817ac29 Use bcrypt as default password hash algorithm for "native" and "auto"
chalasr added a commit that referenced this pull requestMar 5, 2021
…ions (chalasr)This PR was merged into the 5.3-dev branch.Discussion----------[Security] Re-add accidentally removed property declarations| Q             | A| ------------- | ---| Branch?       | 5.x| Bug fix?      | no| New feature?  | no| Deprecations? | no| Tickets       | -| License       | MIT| Doc PR        | -spotted while playing with psalm locally,  mistake made in#39802Commits-------bccf736 [Security] Readd accidentally removed property declarations
@fabpotfabpot mentioned this pull requestApr 18, 2021
nicolas-grekas added a commit that referenced this pull requestJun 17, 2021
…dEncoder (derrabus)This PR was merged into the 5.3 branch.Discussion----------[Security] Restore extension point in MessageDigestPasswordEncoder| Q             | A| ------------- | ---| Branch?       | 5.3| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Tickets       |#41696 (comment)| License       | MIT| Doc PR        | N/AUntil Symfony 5.2, it was possible to extend `MessageDigestPasswordEncoder` and override the way password and salt are merged. This broke with#39802. I've restored the old logic and added a test case to cover that scenario.Commits-------4568876 [Security] Restore extension point in MessageDigestPasswordEncoder
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

@javiereguiluzjaviereguiluzjaviereguiluz approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@wouterjwouterjwouterj approved these changes

@derrabusderrabusderrabus left review comments

Assignees
No one assigned
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

[Security] Change "Encode" to "Hash" to avoid any confusion
10 participants
@chalasr@derrabus@stof@goetas@apfelbox@wouterj@nicolas-grekas@javiereguiluz@OskarStark@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp