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.

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

[Security] Add configuration for Argon2i encryption #26175

Merged
merged 1 commit into from Feb 20, 2018
Merged

[Security] Add configuration for Argon2i encryption #26175

merged 1 commit into from Feb 20, 2018

Conversation

CoalaJoe
Copy link
Contributor

@CoalaJoe CoalaJoe commented Feb 14, 2018

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

Feedback?

Current situation: Configuration only applies if argon2i is natively supported.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

thanks for working on this
would you mind adding some tests please?

/**
* Argon2iPasswordEncoder constructor.
*
* @param int $memoryCost
Copy link
Member

Choose a reason for hiding this comment

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

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,
Copy link
Member

Choose a reason for hiding this comment

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

?? instead of ?: (same below)

@CoalaJoe
Copy link
Contributor Author

@nicolas-grekas Ready for next review.

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Feb 14, 2018
@nicolas-grekas
Copy link
Member

SecurityBundle also needs an update, so that you can configure these settings using yaml.
See src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
and src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

$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,
Copy link
Member

@stof stof Feb 14, 2018

Choose a reason for hiding this comment

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

we could fully-qualify these constants though (as we do in encodePasswordNative)

@stof
Copy link
Member

stof commented Feb 14, 2018

is it possible to support this config when using ext-sodium on older versions too ?

@CoalaJoe
Copy link
Contributor Author

@stof There are $opslimit and $memlimit in crypto_pwhash_str(). But there seems to be no way of using multiple threads.

I can't say how these 2 parameters are related to time_cost and memory_cost because they have not been documented on php.net yet. (https://secure.php.net/manual/en/function.sodium-crypto-pwhash-str.php)

@@ -385,6 +385,9 @@ private function addEncodersSection(ArrayNodeDefinition $rootNode)
->max(31)
->defaultValue(13)
->end()
->integerNode('memory_cost')->setDefaultValue(1024)->end()
Copy link
Member

Choose a reason for hiding this comment

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

defaultNull instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. Fixed it.

@CoalaJoe
Copy link
Contributor Author

  • Add this note to the documentation later?

As Argon2 doesn't have any “bad” values, however consuming more resources is considered better than consuming less. Users are encouraged to adjust the cost factors for the platform they're developing for.
From the RFC

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Should it throw when configuring these options while they aren't supported?


public function __construct(int $memoryCost = null, int $timeCost = null, int $threads = null)
{
if (\defined('PASSWORD_ARGON2I')) {
Copy link
Member

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Done.

@nicolas-grekas nicolas-grekas changed the title Feature/#26174 argon2i configuration [Security] Add configuration for Argon2i encryption Feb 19, 2018
@nicolas-grekas
Copy link
Member

almost there, tests should be updated (see failures)

@CoalaJoe
Copy link
Contributor Author

@nicolas-grekas The tests should now run successfully. But the runners fail on installing the libsodium extension. Do you know why?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

failures related to pecl.php.net being down today

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

oups sorry I voted too fast: the test suite should still pass when the libsodium is not installed, by skipping the cases that require the extension

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Feb 20, 2018

I suppose you'll need to split the new functional tests in dedicated test methods, and add them the @requires extension libsodium annotation.

@fabpot
Copy link
Member

fabpot commented Feb 20, 2018

Thank you @CoalaJoe.

@fabpot fabpot merged commit 1300fec into symfony:master Feb 20, 2018
fabpot added a commit that referenced this pull request Feb 20, 2018
…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
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 1, 2018
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
@fabpot fabpot mentioned this pull request May 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants