[Security] Saltless Encoder Interface #21620
Conversation
| namespace Symfony\Component\Security\Core\Encoder; | ||
|
|
||
| /** | ||
| * SaltlessEncoderInterface is the interface for encoders that do not require a |
There was a problem hiding this comment.
I would say ```is a marker interface for encoders...``
| @@ -93,9 +93,9 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| $emptySalt = $input->getOption('empty-salt'); | |||
|
|
|||
| $encoder = $this->getContainer()->get('security.encoder_factory')->getEncoder($userClass); | |||
| $bcryptWithoutEmptySalt = !$emptySalt && $encoder instanceof BCryptPasswordEncoder; | |||
| $saltless = !$emptySalt && $encoder instanceof SaltlessEncoderInterface; | |||
There was a problem hiding this comment.
should be $saltlessWithoutEmptySalt to describe what it really detects
|
Awesome, thanks for the feedback so quickly, @stof! I wasn't exactly sure how to implement it but wanted to shove it on GitHub before I forgot about it 😄 |
|
I'm not just not sure about the naming of the interface. These encoders are still using a salt, but they generate it internally and don't require storing it separately for the verification step. @symfony/deciders do you have a better idea for the naming ? |
|
I like this idea a lot ... but I don't like the proposed name
|
|
I really like the idea of moving away from the |
|
@javiereguiluz this interface is just a marker one. so I would not give it a name which is more generic than the real implementation. If I was designing this system from scratch, I would change 2 things:
However, we cannot easily switch to such system, as it implies a BC break in the storage format for people not using bcrypt today, and so writing a migration layer would be very hard. But starting to talk about hashing now in a marker interface for the existing API would be a mistake IMO (more confusing than anything else) |
|
I think for now "Encoder" is good enough. However, I would like to move on to a name change (soon tm) to "Hasher" because it's already confusing and complex without this naming issue. |
|
I've renamed } elseif ($saltlessWithoutEmptySalt) {
$io->note('Saltless encoder used: the encoder generated its own built-in salt.'); |
|
@zanderbaldwin |
|
Sure. From the comments above I'm not sure what to name the interface - was anything concretely decided on? |
https://en.wikipedia.org/wiki/Key_derivation_function A Key derivation function is something completely else, a Key derivation is used a generate a "passphrase", to be used an encryption key. As using your password directly is considered highly insecure because of the minimum required length of key, which is not achieved with a 10 character password (you need at least 128bits). PBKDF (Password-Based Key Derivation Function) is actually designed for this and for NOT for passwords, because it's slow and not fully protected against memory attacks. (I was not aware of that at the time 😅 ). |
|
I think that whatever is decided it's going to be a compromise - so I'll leave it up to the members of @symfony/deciders to inform me what the interface should be called 🙂 |
|
@stof any help to move this one forward? |
|
|
|
I like Ryan's proposal. |
|
I've implemented Ryan's proposal 👍 |
|
I like it as well |
| } elseif ($bcryptWithoutEmptySalt) { | ||
| $errorIo->note('Bcrypt encoder used: the encoder generated its own built-in salt.'); | ||
| } elseif ($saltlessWithoutEmptySalt) { | ||
| $errorIo->note('Saltless encoder used: the encoder generated its own built-in salt.'); |
There was a problem hiding this comment.
should be changed from Saltless to Self-salting based on the renaming
|
I think the failure is a false positive related to this security-bundle requiring symfony/security, but the CI building syfmony/security-http. That's a bug in the CI script. But not for this PR. |
|
Does the target branch need to be changed from |
|
Yes please |
|
tests are legitimately failing, both on Windows and Linux |
| @@ -115,11 +115,10 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| $userClass = $this->getUserClass($input, $io); | |||
| $emptySalt = $input->getOption('empty-salt'); | |||
|
|
|||
| $encoderFactory = $this->encoderFactory ?: $this->getContainer()->get('security.encoder_factory'); | |||
There was a problem hiding this comment.
This is a BC layer, it must be kept
There was a problem hiding this comment.
Can you revert? This is what breaks tests
There was a problem hiding this comment.
Sorry about that. Changed it back.
A new interface for all encoders that do not require a user-generated salt.
|
Can anyone clarify if the test is now failing for Because I get the following: $ cd "<repoRoot>"
# On pull request branch ("zanbaldwin:saltless-encoder-interface").
$ composer install
# Package "symfony/phpunit-bridge" installed.
$ cd "<repoRoot>/src/Symfony/Bundle/SecurityBundle"
$ composer install
# Package "symfony/security" installed at version "3.4.x-dev 2910bac".
$ ../../../../vendor/bin/simple-phpunit
# Tests fail :(
$ rm -rf vendor/symfony/security
$ ln -s ../../../../Component/Security vendor/symfony/security
$ ../../../../vendor/bin/simple-phpunit
# Tests pass! :) |
|
@zanbaldwin : Yes this is expected. Tests will pass once the PR code is merged up to master. AppVeyor failure is not related. |
Bad change has been reverted, as requested
|
Thank you @zanbaldwin. |
This PR was merged into the 3.4 branch. Discussion ---------- [Security] Saltless Encoder Interface | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | A new interface for encoders that do not require a user-generated salt (generate their own built-in) as suggested by @stof ([comment](https://github.com/symfony/symfony/pull/21604/files#r101225470)), this will become useful as more password encoders are added in the future (such as #21604). Commits ------- 7c4aa0b Saltless Encoder Interface
A new interface for encoders that do not require a user-generated salt (generate their own built-in) as suggested by @stof (comment), this will become useful as more password encoders are added in the future (such as #21604).