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] Add configuration for Argon2i encryption#26175

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
fabpot merged 1 commit intosymfony:masterfromCoalaJoe:feature/#26174-argon2i-configuration
Feb 20, 2018
Merged

[Security] Add configuration for Argon2i encryption#26175

fabpot merged 1 commit intosymfony:masterfromCoalaJoe:feature/#26174-argon2i-configuration
Feb 20, 2018

Conversation

@CoalaJoe
Copy link
Contributor

@CoalaJoeCoalaJoe commentedFeb 14, 2018•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#26174
LicenseMIT
Doc PR#9300

Feedback?

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

Copy link
Member

@nicolas-grekasnicolas-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

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,

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
ContributorAuthor

@nicolas-grekas Ready for next review.

@nicolas-grekasnicolas-grekas added this to the4.1 milestoneFeb 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

@stofstofFeb 14, 2018•
edited
Loading

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 inencodePasswordNative)

@stof
Copy link
Member

stof commentedFeb 14, 2018•
edited
Loading

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

@CoalaJoe
Copy link
ContributorAuthor

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

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

->max(31)
->defaultValue(13)
->end()
->integerNode('memory_cost')->setDefaultValue(1024)->end()

Choose a reason for hiding this comment

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

defaultNull instead?

Copy link
ContributorAuthor

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
ContributorAuthor

  • 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

@chalasrchalasr 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?


publicfunction__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 Argon2iPasswordEncoderextends BasePasswordEncoderimplements SelfSaltingEncoderInterface
{
private$config;

Choose a reason for hiding this comment

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

the default value should bearray(), otherwisepassword_hash($raw, \PASSWORD_ARGON2I, $this->config); will throw a warning
this should cover@chalasr's comment below

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Thanks. Done.

@nicolas-grekasnicolas-grekas changed the titleFeature/#26174 argon2i configuration[Security] Add configuration for Argon2i encryptionFeb 19, 2018
@nicolas-grekas
Copy link
Member

almost there, tests should be updated (see failures)

@CoalaJoe
Copy link
ContributorAuthor

@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-grekasnicolas-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-grekasnicolas-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 commentedFeb 20, 2018•
edited
Loading

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

Thank you@CoalaJoe.

CoalaJoe, Isarien, and PReimers reacted with thumbs up emoji

@fabpotfabpot merged commit1300fec intosymfony:masterFeb 20, 2018
fabpot added a commit that referenced this pull requestFeb 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 requestMar 1, 2018
This PR was merged into the master branch.Discussion----------Update configuration for argon2i encoderFrom:symfony/symfony#26175Commits-------a3e9bf2 Update configuration for argon2i encoder
@fabpotfabpot mentioned this pull requestMay 7, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@chalasrchalasrchalasr left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

6 participants

@CoalaJoe@nicolas-grekas@stof@fabpot@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp