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 NativePasswordEncoder #31140

Merged
merged 1 commit into from Apr 18, 2019

Conversation

@nicolas-grekas
Copy link
Member

commented Apr 17, 2019

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.

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Todo:

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

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

is there any reason to keep BCryptPasswordEncoder?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

is there any reason to keep BCryptPasswordEncoder?

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

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-encoder-native branch 3 times, most recently from adcef7e to 40f1daf Apr 17, 2019

@nicolas-grekas
Copy link
Member Author

left a comment

PR ready

@@ -416,11 +416,14 @@ private function addEncodersSection(ArrayNodeDefinition $rootNode)
->integerNode('cost')
->min(4)
->max(31)
->defaultValue(13)
->defaultNull()

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Author Member

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

@@ -532,6 +533,10 @@ private function createEncoder($config, ContainerBuilder $container)
return new Reference($config['id']);
}
if ('auto' === $config['algorithm']) {

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Author Member

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

{
$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);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Author Member

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.

This comment has been minimized.

Copy link
@crayner

crayner Apr 22, 2019

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?

throw new \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;

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Author Member

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

This comment has been minimized.

Copy link
@rosier

rosier Apr 18, 2019

Contributor

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 18, 2019

Author Member

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

This comment has been minimized.

Copy link
@rosier

rosier Apr 18, 2019

Contributor

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

Show resolved Hide resolved src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php Outdated
Show resolved Hide resolved src/Symfony/Component/Security/Core/Encoder/NativePasswordEncoder.php Outdated
@@ -532,6 +533,10 @@ private function createEncoder($config, ContainerBuilder $container)
return new Reference($config['id']);
}
if ('auto' === $config['algorithm']) {
$config['algorithm'] = SodiumPasswordEncoder::isSupported() ? 'sodium' : 'native';

This comment has been minimized.

Copy link
@weaverryan

weaverryan Apr 17, 2019

Member

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

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 17, 2019

Author Member

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.

This comment has been minimized.

Copy link
@stof

stof Apr 18, 2019

Member

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 comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Apr 18, 2019

Author Member

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 that attackers 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 nicolas-grekas force-pushed the nicolas-grekas:sec-encoder-native branch 2 times, most recently from 4fcca31 to 6aa50d1 Apr 17, 2019

@ro0NL

ro0NL approved these changes Apr 17, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-encoder-native branch 2 times, most recently from 1c79f90 to 406660f Apr 17, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-encoder-native branch from 406660f to 04de8fe Apr 17, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-encoder-native branch from 04de8fe to 9c1b4ce Apr 18, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:sec-encoder-native branch from 9c1b4ce to 28f7961 Apr 18, 2019

@chalasr

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Thank you @nicolas-grekas.

@chalasr chalasr merged commit 28f7961 into symfony:master Apr 18, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

chalasr added a commit that referenced this pull request Apr 18, 2019

feature #31140 [Security] Add NativePasswordEncoder (nicolas-grekas)
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 nicolas-grekas deleted the nicolas-grekas:sec-encoder-native branch Apr 18, 2019

chalasr added a commit that referenced this pull request Apr 19, 2019

feature #31170 [Security] deprecate BCryptPasswordEncoder in favor of…
… 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 #31140

Commits
-------

e197398 [Security] deprecate BCryptPasswordEncoder in favor of NativePasswordEncoder

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

@fabpot fabpot referenced this pull request May 9, 2019

Merged

Release v4.3.0-BETA1 #31435

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.