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

chalasr · 2021-01-12T16:56:09Z

Q	A
Branch?	5.x
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fixes #39698
License	MIT
Doc PR	todo

This PR renames password "encoders" to password hashers (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 use passwords_hashers instead of encoders in their security configuration, and type-hint against PasswordHasherInterface (and related) instead of PasswordEncoderInterface.

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:

namespace Symfony\Component\PasswordHasher;

interface PasswordHasherInterface
{
    public function hash(string $plainPassword): string;

    public function verify(string $hashedPassword, string $plainPassword): bool;

    public function needsRehash(string $hashedPassword): bool;
}

src/Symfony/Component/Security/Password/SaltingPasswordHasherInterface.php

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php

src/Symfony/Component/Security/Core/Encoder/UserPasswordEncoder.php

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

src/Symfony/Component/Security/Core/Encoder/EncoderAwareInterface.php

src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php

src/Symfony/Component/Security/Password/Hasher/HasherFactoryInterface.php

derrabus · 2021-01-12T18:40:12Z

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 · 2021-01-12T18:44:42Z

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 outside symfony/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.

goetas · 2021-01-13T19:57:41Z

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?

…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

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

nicolas-grekas

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

…ght naming

wouterj · 2021-02-12T15:55:01Z

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

…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 on https://twitter.com/TerahashCorp/status/1155129705034653698 Commits ------- 332817a Use bcrypt as default password hash algorithm for "native" and "auto"

…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 symfony/symfony#39802 (comment), based on https://twitter.com/TerahashCorp/status/1155129705034653698 Commits ------- 332817ac29 Use bcrypt as default password hash algorithm for "native" and "auto"

…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 #39802 Commits ------- bccf736 [Security] Readd accidentally removed property declarations

…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/A Until 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

chalasr added the Security label Jan 12, 2021

chalasr added this to the 5.x milestone Jan 12, 2021

chalasr requested a review from wouterj as a code owner January 12, 2021 16:56

carsonbot added Status: Needs Review Feature labels Jan 12, 2021

chalasr force-pushed the password-hasher branch from 472bb3a to b6c56a2 Compare January 12, 2021 16:56

chalasr mentioned this pull request Jan 12, 2021

[Security] Change "Encode" to "Hash" to avoid any confusion #39698

Closed

nicolas-grekas reviewed Jan 12, 2021

View reviewed changes

src/Symfony/Component/Security/Password/SaltingPasswordHasherInterface.php Outdated Show resolved Hide resolved

stof previously requested changes Jan 12, 2021

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 12, 2021

chalasr force-pushed the password-hasher branch 2 times, most recently from ed07ff3 to 1dd61cc Compare January 12, 2021 18:15

derrabus previously requested changes Jan 12, 2021

View reviewed changes

src/Symfony/Component/Security/Core/Encoder/EncoderAwareInterface.php Outdated Show resolved Hide resolved

src/Symfony/Component/Security/Guard/Provider/GuardAuthenticationProvider.php Outdated Show resolved Hide resolved

stof reviewed Jan 12, 2021

View reviewed changes

src/Symfony/Component/Security/Password/Hasher/HasherFactoryInterface.php Outdated Show resolved Hide resolved

chalasr force-pushed the password-hasher branch 4 times, most recently from 7203a1e to b40a7e0 Compare January 12, 2021 21:18

aschempp mentioned this pull request Jan 14, 2021

Rename password encoders to password hashers contao/contao#2634

Closed

chalasr force-pushed the password-hasher branch 4 times, most recently from b39ced8 to b6ed024 Compare January 15, 2021 11:37

chalasr mentioned this pull request Jan 16, 2021

[DependencyInjection] Skip deprecated definitions in CheckTypeDeclarationsPass #39861

Merged

chalasr force-pushed the password-hasher branch from b6ed024 to f161b1f Compare January 17, 2021 11:20

chalasr added Status: Needs Review and removed Status: Needs Work labels Feb 12, 2021

nicolas-grekas reviewed Feb 12, 2021

View reviewed changes

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md Show resolved Hide resolved

nicolas-grekas reviewed Feb 12, 2021

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php Show resolved Hide resolved

nicolas-grekas approved these changes Feb 12, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Feb 12, 2021

chalasr force-pushed the password-hasher branch from 41cbc66 to 6b09800 Compare February 12, 2021 15:32

javiereguiluz approved these changes Feb 12, 2021

View reviewed changes

[Security] Extract password hashing from security-core - using the ri…

c5c981c

…ght naming

chalasr force-pushed the password-hasher branch from 6b09800 to c5c981c Compare February 12, 2021 15:42

nicolas-grekas added the Ready label Feb 12, 2021

wouterj approved these changes Feb 12, 2021

View reviewed changes

wouterj merged commit c757845 into symfony:5.x Feb 12, 2021

wouterj mentioned this pull request Feb 12, 2021

[Security] Extract password hashing from security-core - with proper wording symfony/symfony-docs#14966

Closed

chalasr deleted the password-hasher branch February 12, 2021 16:02

chalasr mentioned this pull request Feb 13, 2021

[PasswordHasher] Use bcrypt as default hash algorithm for "native" and "auto" #40176

Merged

localheinz mentioned this pull request Feb 14, 2021

[PasswordHasher] Fix: Typo #40181

Merged

1 task

This was referenced Mar 3, 2021

Configure "password_hashers" instead of "encoders" as of Symfony 5.3 symfony/maker-bundle#831

Closed

[Security] Re-add accidentally removed property declarations #40367

Merged

wouterj mentioned this pull request Mar 7, 2021

[RFC] Symfony Security rework tracking issue #30914

Closed

35 tasks

fabpot mentioned this pull request Apr 18, 2021

Release v5.3.0-BETA1 #40853

Merged

matason mentioned this pull request Jun 6, 2021

[PasswordHasher] Create documentation for the Password Hasher component added in 5.3 symfony/symfony-docs#15413

Closed

derrabus mentioned this pull request Jun 14, 2021

[Security] Restore extension point in MessageDigestPasswordEncoder #41703

Merged

dvesh3 mentioned this pull request Jun 28, 2021

[Security] Password encoder factory backward compatibility #41877

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chalasr commented Jan 12, 2021 •

edited

Loading

derrabus commented Jan 12, 2021

stof commented Jan 12, 2021

goetas commented Jan 13, 2021

nicolas-grekas left a comment •

edited

Loading

wouterj commented Feb 12, 2021

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

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

Conversation

chalasr commented Jan 12, 2021 • edited Loading

derrabus commented Jan 12, 2021

stof commented Jan 12, 2021

goetas commented Jan 13, 2021

nicolas-grekas left a comment • edited Loading

Choose a reason for hiding this comment

wouterj commented Feb 12, 2021

chalasr commented Jan 12, 2021 •

edited

Loading

nicolas-grekas left a comment •

edited

Loading