This PR adds a newNativePasswordEncoder that defaults to the best available hashing algo topassword_hash(). Best is determined by "us" or "php", the goal being that this will change in the future as new algos are published.

This provides a native encoder that we should recommend using by default.

nicolas-grekas added this to thenext milestone

Apr 17, 2019

carsonbot added Status: Needs Review Security Feature labels

Apr 17, 2019

Copy link

MemberAuthor

nicolas-grekas commentedApr 17, 2019•
edited
Loading

Todo:

fix tests
update SecurityExtension
find a default cost for Argon2 that is at least as good as13 for Bcrypt's (any idea?)

chalasr reviewed

Apr 17, 2019

View reviewed changes

src/Symfony/Component/Security/Core/Encoder/NativePasswordEncoder.php OutdatedShow resolvedHide resolved

Copy link

Contributor

ro0NL commentedApr 17, 2019

is there any reason to keepBCryptPasswordEncoder?

Copy link

MemberAuthor

nicolas-grekas commentedApr 17, 2019

is there any reason to keep BCryptPasswordEncoder?

I don't think so, we should deprecate it in 4.4

nicolas-grekas force-pushed thesec-encoder-native branch 3 times, most recently fromadcef7e to40f1dafCompare

April 17, 2019 14:12

nicolas-grekas commented

Apr 17, 2019

View reviewed changes

Copy link

MemberAuthor

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.

PR ready

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

		->min(4)
		->max(31)
		->defaultValue(13)
		->defaultNull()

Copy link

MemberAuthor

nicolas-grekasApr 17, 2019

Choose a reason for hiding this comment

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

The defaults belong to the implementation now, not to the configuration.

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

		->scalarNode('memory_cost')->defaultNull()->end()
		->scalarNode('time_cost')->defaultNull()->end()
		->scalarNode('threads')->defaultNull()->end()
		->scalarNode('threads')

Copy link

MemberAuthor

nicolas-grekasApr 17, 2019

Choose a reason for hiding this comment

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

libsodium hardcodes threads to 1, and this makes sense in PHP too.

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

		returnnewReference($config['id']);
		}

		if ('auto' ===$config['algorithm']) {

Copy link

MemberAuthor

nicolas-grekasApr 17, 2019

Choose a reason for hiding this comment

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

"auto" should be the recommended default - I'm going to submit it as a recipe

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

		'class' => NativePasswordEncoder::class,
		'arguments' => [
		$config['time_cost'],
		(($config['memory_cost'] ??0) <<10) ?:null,

Copy link

MemberAuthor

nicolas-grekasApr 17, 2019•
edited
Loading

Choose a reason for hiding this comment

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

memory_cost (from password_hash) is inkib but SodiumPasswordEncoder and NativePasswordEncoder accept bytes - this makes the conversion

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

		{
		$cost =$cost ??13;
		$opsLimit =$opsLimit ??max(6,\defined('SODIUM_CRYPTO_PWHASH_OPSLIMIT_MODERATE') ? \SODIUM_CRYPTO_PWHASH_OPSLIMIT_MODERATE :6);
		$memLimit =$memLimit ??max(64 1024 1024,\defined('SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE') ? \SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE :64 1024 1024);

Copy link

MemberAuthor

nicolas-grekasApr 17, 2019

Choose a reason for hiding this comment

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

Argon2 with opslimit = 6 & memlimit=64MiB roughly operate in the same amount of time as BCrypt's cost=13
Also, in PHP7.4 SODIUM_CRYPTO_PWHASH_OPSLIMIT_MODERATE=6 and SODIUM_CRYPTO_PWHASH_MEMLIMIT_INTERACTIVE=32MiB
So our defaults are a bit more strict than PHP 7.4, which matches the fact that a cost=13 for BCrypt is stricter than PHP's default.

Copy link

craynerApr 22, 2019

Choose a reason for hiding this comment

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

Just wondering about using SODIUM Constants in the Native Encoder, as what happens if PHP >7.2 but Sodium is NOT installed. Native should work?

Copy link

Member

chalasrMay 31, 2019•
edited
Loading

Choose a reason for hiding this comment

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

The code above checks thatSODUM_* constants are defined before using them, and fallback to sensible defaults (equivalent hardcoded values) otherwise

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

		thrownew \InvalidArgumentException('$cost must be in the range of 4-31.');
		}

		$this->algo =\defined('PASSWORD_ARGON2I') ?max(PASSWORD_DEFAULT,\defined('PASSWORD_ARGON2ID') ?PASSWORD_ARGON2ID :PASSWORD_ARGON2I) :PASSWORD_DEFAULT;

Copy link

MemberAuthor

nicolas-grekasApr 17, 2019

Choose a reason for hiding this comment

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

PASSWORD_ARGON2ID > PASSWORD_ARGON2I > PASSWORD_BCRYPT, unless PASSWORD_DEFAULT is set to an even higher algo in the future

Copy link

Contributor

rosierApr 18, 2019

Choose a reason for hiding this comment

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

So if the argon algo is available the native encoder and the sodium encoder basically do the same thing?

I am wondering if it would be better for DX to use only PASSWORD_DEFAULT in the native encoder.

Then the config would become:
auto => use best supported / recommended encoder
native => use php default algo
sodium => use best supported / available sodium algo

Copy link

MemberAuthor

nicolas-grekasApr 18, 2019

Choose a reason for hiding this comment

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

I think always using the best available algo is required for best security. I don't understand how that relates to DX.

Copy link

Contributor

rosierApr 18, 2019

Choose a reason for hiding this comment

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

I think always using the best available algo is required for best security.

Agreed.

I don't understand how that relates to DX.

I just think it might be confusing that there are 2 encoders that can do the same thing (encode with the argon algo).

weaverryan reviewed

Apr 17, 2019

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php OutdatedShow resolvedHide resolved

src/Symfony/Component/Security/Core/Encoder/NativePasswordEncoder.php OutdatedShow resolvedHide resolved

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

		}

		if ('auto' ===$config['algorithm']) {
		$config['algorithm'] = SodiumPasswordEncoder::isSupported() ?'sodium' :'native';

Copy link

Member

weaverryanApr 17, 2019

Choose a reason for hiding this comment

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

is the sodium encoder superior to our native encoder? Is that why it's preferred?

Copy link

MemberAuthor

nicolas-grekasApr 17, 2019

Choose a reason for hiding this comment

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

yes, in two aspects: it is faster for the same cost parameters, and it can be more up to date than the native one, as an extension can move faster to new algos.

Copy link

Member

stofApr 18, 2019

Choose a reason for hiding this comment

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

being faster for the same cost may not actually be superior. The whole point of a password hashing algorithm is that it should not be too fast.

Copy link

MemberAuthor

nicolas-grekasApr 18, 2019

Choose a reason for hiding this comment

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

being faster for the same cost may not actually be superior. The whole point of a password hashing algorithm is that it should not be too fast.

this answer is wrong ;)
what matters is thatattackers must spend significant resources to compute password hashes when they leaked. Being faster for the same cost is just improving UX and reducing greenhouse emissions. So yes, it matters a lot.

nicolas-grekas force-pushed thesec-encoder-native branch 2 times, most recently from4fcca31 to6aa50d1Compare

April 17, 2019 19:19

ro0NL approved these changes

Apr 17, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Apr 17, 2019

nicolas-grekas force-pushed thesec-encoder-native branch 2 times, most recently from1c79f90 to406660fCompare

April 17, 2019 21:04

nicolas-grekas mentioned this pull request

Apr 17, 2019

[symfony/security-bundle] set encoder to "auto" for all UserInterface by defaultsymfony/recipes#576

Closed

nicolas-grekas force-pushed thesec-encoder-native branch from406660f to04de8feCompare

April 17, 2019 21:15

chalasr reviewed

Apr 18, 2019

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php OutdatedShow resolvedHide resolved

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/CompleteConfigurationTest.phpShow resolvedHide resolved

src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.phpShow resolvedHide resolved

nicolas-grekas force-pushed thesec-encoder-native branch from04de8fe to9c1b4ceCompare

April 18, 2019 06:52

stof reviewed

Apr 18, 2019

View reviewed changes

src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php OutdatedShow resolvedHide resolved

stof reviewed

Apr 18, 2019

View reviewed changes

src/Symfony/Component/Security/Core/Encoder/NativePasswordEncoder.php OutdatedShow resolvedHide resolved

weaverryan mentioned this pull request

Apr 18, 2019

[4.4] NativePasswordEncoder & password re-hashingsymfony/maker-bundle#382

Closed

[Security] Add NativePasswordEncoder

28f7961

nicolas-grekas force-pushed thesec-encoder-native branch from9c1b4ce to28f7961Compare

April 18, 2019 11:39

chalasr approved these changes

Apr 18, 2019

View reviewed changes

weaverryan approved these changes

Apr 18, 2019

View reviewed changes

chalasr mentioned this pull request

Apr 18, 2019

Document the new "native" and "auto" encoder algorithmssymfony/symfony-docs#11452

Closed

Copy link

Member

chalasr commentedApr 18, 2019

Thank you@nicolas-grekas.

chalasr merged commit28f7961 intosymfony:master

Apr 18, 2019

chalasr pushed a commit that referenced this pull request

Apr 18, 2019

feature#31140[Security] Add NativePasswordEncoder (nicolas-grekas)

89ec311

This PR was merged into the 4.3-dev branch.Discussion----------[Security] Add NativePasswordEncoder| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -This PR adds a new `NativePasswordEncoder` that defaults to the best available hashing algo to `password_hash()`. Best is determined by "us" or "php", the goal being that this will change in the future as new algos are published.This provides a native encoder that we should recommend using by default.Commits-------28f7961 [Security] Add NativePasswordEncoder

nicolas-grekas deleted the sec-encoder-native branch

April 18, 2019 14:01

nicolas-grekas mentioned this pull request

Apr 18, 2019

[Security] deprecate BCryptPasswordEncoder in favor of NativePasswordEncoder#31170

Merged

chalasr pushed a commit that referenced this pull request

Apr 19, 2019

feature#31170[Security] deprecate BCryptPasswordEncoder in favor of…

823d375

… NativePasswordEncoder (nicolas-grekas)This PR was merged into the 4.3-dev branch.Discussion----------[Security] deprecate BCryptPasswordEncoder in favor of NativePasswordEncoder| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Follow up of#31140Commits-------e197398 [Security] deprecate BCryptPasswordEncoder in favor of NativePasswordEncoder

nicolas-grekas modified the milestones:next,4.3

Apr 30, 2019

nicolas-grekas mentioned this pull request

May 7, 2019

[4.3] make:user should suggest "auto" instead of Argon2i/BCryptsymfony/maker-bundle#386

Closed