[Security] Add configuration for Argon2i encryption #26175
Conversation
| /** | ||
| * Argon2iPasswordEncoder constructor. | ||
| * | ||
| * @param int $memoryCost |
There was a problem hiding this comment.
since this is for PHP 7.1, you can move the types to the constructor's signature, and remove the docblock altogether.
| { | ||
| if (\defined('PASSWORD_ARGON2I')) { | ||
| $this->config = array( | ||
| 'memory_cost' => $memoryCost ?: PASSWORD_ARGON2_DEFAULT_MEMORY_COST, |
There was a problem hiding this comment.
?? instead of ?: (same below)
|
@nicolas-grekas Ready for next review. |
|
SecurityBundle also needs an update, so that you can configure these settings using yaml. |
| $this->config = array( | ||
| 'memory_cost' => $memoryCost ?? PASSWORD_ARGON2_DEFAULT_MEMORY_COST, | ||
| 'time_cost' => $timeCost ?? PASSWORD_ARGON2_DEFAULT_TIME_COST, | ||
| 'threads' => $threads ?? PASSWORD_ARGON2_DEFAULT_THREADS, |
There was a problem hiding this comment.
we could fully-qualify these constants though (as we do in encodePasswordNative)
|
is it possible to support this config when using ext-sodium on older versions too ? |
|
@stof There are I can't say how these 2 parameters are related to |
| @@ -385,6 +385,9 @@ private function addEncodersSection(ArrayNodeDefinition $rootNode) | |||
| ->max(31) | |||
| ->defaultValue(13) | |||
| ->end() | |||
| ->integerNode('memory_cost')->setDefaultValue(1024)->end() | |||
There was a problem hiding this comment.
You are right. Fixed it.
|
|
|
||
| public function __construct(int $memoryCost = null, int $timeCost = null, int $threads = null) | ||
| { | ||
| if (\defined('PASSWORD_ARGON2I')) { |
There was a problem hiding this comment.
these options are used in encodePasswordNative only which is restricted to php 7.2+, we should probably have the same check here
| */ | ||
| class Argon2iPasswordEncoder extends BasePasswordEncoder implements SelfSaltingEncoderInterface | ||
| { | ||
| private $config; |
There was a problem hiding this comment.
the default value should be array(), otherwise password_hash($raw, \PASSWORD_ARGON2I, $this->config); will throw a warning
this should cover @chalasr's comment below
|
almost there, tests should be updated (see failures) |
|
@nicolas-grekas The tests should now run successfully. But the runners fail on installing the libsodium extension. Do you know why? |
|
I suppose you'll need to split the new functional tests in dedicated test methods, and add them the |
|
Thank you @CoalaJoe. |
…oalaJoe) This PR was merged into the 4.1-dev branch. Discussion ---------- [Security] Add configuration for Argon2i encryption | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26174 | License | MIT | Doc PR | [#9300](symfony/symfony-docs#9300) Feedback? Current situation: Configuration only applies if argon2i is natively supported. Commits ------- 1300fec [Security] Add configuration for Argon2i encryption
This PR was merged into the master branch. Discussion ---------- Update configuration for argon2i encoder From: symfony/symfony#26175 Commits ------- a3e9bf2 Update configuration for argon2i encoder
Feedback?
Current situation: Configuration only applies if argon2i is natively supported.