8000 [SecurityBundle] UserPasswordEncoderCommand: Improve & simplify the command usage by ogizanagi · Pull Request #14032 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[SecurityBundle] UserPasswordEncoderCommand: Improve & simplify the command usage #14032

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

[SecurityBundle] UserPasswordEncoderCommand: Improve & simplify the command usage #14032

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8ad5af5
[SecurityBundle] UserPasswordEncoderCommand: Improve & simplify the c…
ogizanagi Mar 24, 2015
df4640d
[SecurityBundle] UserPasswordEncoderCommand: ask user confirmation ab…
ogizanagi Mar 26, 2015
289de2f
[SecurityBundle] UserPasswordEncoderCommand: Let the bcrypt encoder g…
ogizanagi Mar 24, 2015
2a6abf7
[SecurityBundle] UserPasswordEncoderCommand: Add a note when bcrypt d…
ogizanagi Mar 26, 2015
b9374cc
[SecurityBundle] UserPasswordEncoderCommand: Tweak output, complying …
ogizanagi Mar 30, 2015
42b17dc
[SecurityBundle] updates symfony/console min version
ogizanagi Apr 6, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
218 changes: 81 additions & 137 deletions src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@
use Symfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Question\Question;
use Symfony\Component\Console\Helper\Table;
use Symfony\Component\Console\Style\SymfonyStyle;
use Symfony\Component\Security\Core\Encoder\BCryptPasswordEncoder;

/**
* Encode a user's password.
Expand All @@ -32,35 +34,45 @@ protected function configure()
{
$this
->setName('security:encode-password')
->setDescription('Encode a password.')
->addArgument('password', InputArgument::OPTIONAL, 'Enter a password')
->addArgument('user-class', InputArgument::OPTIONAL, 'Enter the user class configured to find the encoder you need.')
->addArgument('salt', InputArgument::OPTIONAL, 'Enter the salt you want to use to encode your password.')
->setDescription('Encodes a password.')
Copy link
Member

Choose a reason for hiding this comment

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

And this description could be: The User entity class path associated with the encoder used to encode the password

->addArgument('password', InputArgument::OPTIONAL, 'The plain password to encode.')
->addArgument('user-class', InputArgument::OPTIONAL, 'The User entity class path associated with the encoder used to encode the password.', 'Symfony\Component\Security\Core\User\User')
->addOption('empty-salt', null, InputOption::VALUE_NONE, 'Do not generate a salt or let the encoder generate one.')
Copy link
Member

Choose a reason for hiding this comment

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

This option oculd be renamed to generate-salt and be false by default. And the description could be: Generate a random salt and add it to the encoded password

Copy link
Member

Choose a reason for hiding this comment

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

My main concern of this command from a DX point of view is the management of the random salt. This is the behavior that I'd like to see:

  • Assume that users never want to provide the salt value explicitly.
  • Therefore, by default, the command manages the salt:
    • If it's Bcrypt, don't generate a salt.
    • If it's an encoder of any other type, generate a salt.
  • If the users want to use their own salts, allow to do that via an option (--salt ?)

So this would be the behavior of the command:

// encodes the password for the default Symfony User class and generates the salt randomly
// if we use bcrypt, the command doesn't generate a salt, but the user is not informed about that
$ php app/console security:encode-password PASSWORD

// encodes the password for our custom User class and generates the salt randomly
// if we use bcrypt, the command doesn't generate a salt, but the user is not informed about that
$ php app/console security:encode-password PASSWORD AppBundle\Entity\User

// encodes the password for the default Symfony User class and uses our custom salt
// if we use bcrypt, the command doesn't use this salt and it informs about it to the user
$ php app/console security:encode-password PASSWORD --salt="..."

// encodes the password for our custom User class and uses our custom salt
// if we use bcrypt, the command doesn't use this salt and it informs about it to the user
$ php app/console security:encode-password PASSWORD AppBundle\Entity\User --salt="..."

Copy link
Contributor

Choose a reason for hiding this comment

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

@javiereguiluz I agree very strongly with the first two bullets, but not with the last one. I really don't see the value of being able to specify a salt, so even giving the option makes the command more complicated and the behaviour more unexpected (why accept a salt option and then discard it if encoding with bcrypt? You can totally do it, but it's almost always a really crappy idea - and that's equally true for all salt-using encoders, see what I mean? The only difference is the bcrypt encoder is smart enough to make using a cryptographically strong generated salt the default behaviour).

I think good DX is about making the right thing easy, and making mistakes more difficult; specifying a salt is almost always a mistake. For those instances where it's not a mistake, I think that's a sufficiently power-user situation that this command has no obligation to cater to it 😛

So IMO --salt should be removed. Ideally --empty-salt should also be removed eventually, but is probably necessary for some custom encoders until a PR fixes the leaky abstraction in the password encoder design.

Copy link

Choose a reason for hiding this comment

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

quoting http://blog.ircmaxell.com/2015/03/thoughts-on-design-of-apis.html in regards to password_hash()

"For example, I think I made a big mistake with password_hash() allowing the user to specify a salt manually. In all (literally all) the cases I've seen users specifying a salt manually, it's either of a lower quality than would be generated automatically, or it's worse than that."

Copy link
Member

Choose a reason for hiding this comment

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

If we add the --salt option I don't think the command necessarily complicates:

  • If you know nothing about the salt, you won't use this option and the command won't ask you for it.
  • If you know what a salt is, but don't use this option, the command won't bother you and everything will work.
  • If you are an advanced user, read the command help, and need (for whatever reason) to use a custom salt, you just use this super easy --salt option. There is only one exception: if you are also using Bcrypt, the command will remind you that you cannot provide a salt in this case, will ignore it and everything will work as expected.

Copy link
Member

Choose a reason for hiding this comment

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

@jrobeson are you sure that the --salt option is anti DX?

Scenario: your database requires 10 bytes salt values.

Proposal 1:

Execute: php app/console security:encode-password raw_password --salt="ae037d6f72"

Proposal 2:

  1. Create a new Symfony console command
  2. Make this command extend the default SecurityEncodePassword command
  3. Override the generateSalt() command and think of a better implementation
  4. Execute php app/console security:encode-password raw_password

Copy link

Choose a reason for hiding this comment

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

and how often will that actually happen? It seems pretty clear that users make mistakes with salts more often than the situation you're describing.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know. But let's go back to the original discussion. What I propose is the following:

  • Don't require to provide a salt and never bother the user about the salt. The command does always the right thing automatically: generate it randomly or not generate it if using Bcrypt.
  • If the user, for whatever reason, needs to provide a custom salt, let him/her use the --salt option.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you @fabpot

@javiereguiluz "never assume anything on behalf of the developer" is a guideline. It's a great guideline. But it's not dogma. I don't think the meaning of the guideline is "we will let you shoot yourself in the foot if you want because theoretically you might need to shoot yourself in the foot", it's more like "if we assume too much we risk making your job harder not easier" - this is not one of those instances. The only legitimate use-case I can think of for providing a known salt to the encoder is to verify its output (i.e. to test that it works), but that's not the job of this command.

I don't think your use-cases are legitimate, sorry. Frankly I'm not even convinced that my use case is legitimate. They're theoretically plausible, but the solution to them isn't to make this command generate the salt (edit: I meant "to let users provide the salt"), it's to make a custom PasswordEncoderInterface implementation that generates conforming salts in the same way that the BCrypt one does.

Basically this whole argument has come about because the design of that component is flawed - it's a leaky abstraction. Encoders right now demand that we have special knowledge of the length and composition of the salt, which is wrong. We don't know and can't know if the encoder accepts raw binary salts or base64 or (god help us) only hexits; we don't know how long the salt needs to be. This is wrong. Until that's fixed, I think we should do the best we can, which is to generate a reasonably long base64 salt for everything except BCrypt. Accepting a salt override (or even an empty-salt) is a mistake - it shouldn't be any of our business (in the context of this command), and it's a burden and a problem for users, not a benefit.

Copy link
Member

Choose a reason for hiding this comment

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

I guess one of the reasons of this long discussion is that we're thinking about different use cases for this command. I proposed to update the description of this command as follows:

The <info>%command.name%</info> command encodes passwords according to your
security configuration. This command is mainly used to generate passwords for
the <comment>in_memory</comment> user provider type and for changing passwords
in the database while developing the application.

You don't use this command in the real application, you only use it for in_memory providers and for quick tests.

->setHelp(<<<EOF

The <info>%command.name%</info> command allows to encode a password using encoders
that are configured in the application configuration file, under the <comment>security.encoders</comment>.
The <info>%command.name%</info> command encodes passwords according to your
security configuration. This command is mainly used to generate passwords for
the <comment>in_memory</comment> user provider type and for changing passwords
in the database while developing the application.

Suppose that you have the following security configuration in your application:

For instance, if you have the following configuration for your application:
<comment>
security:
encoders:
Symfony\Component\Security\Core\User\User: plaintext
AppBundle\Model\User: bcrypt
# app/config/security.yml
security:
encoders:
Symfony\Component\Security\Core\User\User: plaintext
AppBundle\Entity\User: bcrypt
</comment>

According to the response you will give to the question "<question>Provide your configured user class</question>" your
password will be encoded the way it was configured.
- If you answer "<comment>Symfony\Component\Security\Core\User\User</comment>", the password provided will be encoded
with the <comment>plaintext</comment> encoder.
- If you answer <comment>AppBundle\Model\User</comment>, the password provided will be encoded
with the <comment>bcrypt</comment> encoder.
If you execute the command non-interactively, the default Symfony User class
is used and a random salt is generated to encode the password:

<info>php %command.full_name% --no-interaction [password]</info>

Pass the full user class path as the second argument to encode passwords for
your own entities:

<info>php %command.full_name% --no-interaction [password] AppBundle\Entity\User</info>

The command allows you to provide your own <comment>salt</comment>. If you don't provide any,
the command will take care about that for you.
Executing the command interactively allows you to generate a random salt for
encoding the password:

You can also use the non interactive way by typing the following command:
<info>php %command.full_name% [password] [user-class] [salt]</info>
<info>php %command.full_name% [password] AppBundle\Entity\User</info>

In case your encoder doesn't require a salt, add the <comment>empty-salt</comment> option:

<info>php %command.full_name% --empty-salt [password] AppBundle\Entity\User</info>

EOF
)
Expand All @@ -72,154 +84,86 @@ protected function configure()
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
$this->writeIntroduction($output);
$output = new SymfonyStyle($input, $output);

$input->isInteractive() ? $output->title('Symfony Password Encoder Utility') : $output->newLine();

$password = $input->getArgument('password');
$salt = $input->getArgument('salt');
$userClass = $input->getArgument('user-class');
$emptySalt = $input->getOption('empty-salt');

$helper = $this->getHelper('question');
$encoder = $this->getContainer()->get('security.encoder_factory')->getEncoder($userClass);
$bcryptWithoutEmptySalt = !$emptySalt && $encoder instanceof BCryptPasswordEncoder;

if ($bcryptWithoutEmptySalt) {
$emptySalt = true;
}

if (!$password) {
if (!$input->isInteractive()) {
$output->error('The password must not be empty.');

return 1;
}
$passwordQuestion = $this->createPasswordQuestion($input, $output);
$password = $helper->ask($input, $output, $passwordQuestion);
$password = $output->askQuestion($passwordQuestion);
}

if (!$salt) {
$saltQuestion = $this->createSaltQuestion($input, $output);
$salt = $helper->ask($input, $output, $saltQuestion);
}
$salt = null;

if ($input->isInteractive() && !$emptySalt) {
$emptySalt = true;

$output->writeln("\n <comment>Encoders are configured by user type in the security.yml file.</comment>");
$output->note('The command will take care of generating a salt for you. Be aware that some encoders advise to let them generate their own salt. If you\'re using one of those encoders, please answer \'no\' to the question below. '.PHP_EOL.'Provide the \'empty-salt\' option in order to let the encoder handle the generation itself.');

if (!$userClass) {
$userClassQuestion = $this->createUserClassQuestion($input, $output);
$userClass = $helper->ask($input, $output, $userClassQuestion);
if ($output->confirm('Confirm salt generation ?')) {
$salt = $this->generateSalt();
$emptySalt = false;
}
} elseif (!$emptySalt) {
$salt = $this->generateSalt();
}

$encoder = $this->getContainer()->get('security.encoder_factory')->getEncoder($userClass);
$encodedPassword = $encoder->encodePassword($password, $salt);

$this->writeResult($output);
$rows = array(
array('Encoder used', get_class($encoder)),
array('Encoded password', $encodedPassword),
);
if (!$emptySalt) {
$rows[] = array('Generated salt', $salt);
}
$output->table(array('Key', 'Value'), $rows);

$ F438 table = new Table($output);
$table
->setHeaders(array('Key', 'Value'))
->addRow(array('Encoder used', get_class($encoder)))
->addRow(array('Encoded password', $encodedPassword))
;
if (!$emptySalt) {
$output->note(sprintf('Make sure that your salt storage field fits the salt length: %s chars', strlen($salt)));
} elseif ($bcryptWithoutEmptySalt) {
$output->note('Bcrypt encoder used: the encoder generated its own built-in salt.');
}

$table->render();
$output->success('Password encoding succeeded');
}

/**
* Create the password question to ask the user for the password to be encoded.
*
* @param InputInterface $input
* @param OutputInterface $output
*
* @return Question
*/
private function createPasswordQuestion(InputInterface $input, OutputInterface $output)
private function createPasswordQuestion()
{
$passwordQuestion = new Question("\n > <question>Type in your password to be encoded:</question> ");
$passwordQuestion = new Question('Type in your password to be encoded');

$passwordQuestion->setValidator(function ($value) {
return $passwordQuestion->setValidator(function ($value) {
if ('' === trim($value)) {
throw new \Exception('The password must not be empty.');
}

return $value;
});
$passwordQuestion->setHidden(true);
$passwordQuestion->setMaxAttempts(20);

return $passwordQuestion;
}

/**
* Create the question that asks for the salt to perform the encoding.
* If there is no provided salt, a random one is automatically generated.
*
* @param InputInterface $input
* @param OutputInterface $output
*
* @return Question
*/
private function createSaltQuestion(InputInterface $input, OutputInterface $output)
{
$saltQuestion = new Question("\n > (Optional) <question>Provide a salt (press <enter> to generate one):</question> ");

$container = $this->getContainer();
$saltQuestion->setValidator(function ($value) use ($output, $container) {
if ('' === trim($value)) {
$value = base64_encode($container->get('security.secure_random')->nextBytes(30));

$output->writeln("\n<comment>The salt has been generated: </comment>".$value);
$output->writeln(sprintf("<comment>Make sure that your salt storage field fits this salt length: %s chars.</comment>\n", strlen($value)));
}

return $value;
});

return $saltQuestion;
}

/**
* Create the question that asks for the configured user class.
*
* @param InputInterface $input
* @param OutputInterface $output
*
* @return Question
*/
private function createUserClassQuestion(InputInterface $input, OutputInterface $output)
{
$userClassQuestion = new Question(" > <question>Provide your configured user class:</question> ");
$userClassQuestion->setAutocompleterValues(array('Symfony\Component\Security\Core\User\User'));

$userClassQuestion->setValidator(function ($value) use ($output) {
if ('' === trim($value)) {
$value = 'Symfony\Component\Security\Core\User\User';
$output->writeln("<info>You did not provide any user class.</info> <comment>The user class used is: Symfony\Component\Security\Core\User\User</comment> \n");
}

return $value;
});

return $userClassQuestion;
}

private function writeIntroduction(OutputInterface $output)
{
$output->writeln(array(
'',
$this->getHelperSet()->get('formatter')->formatBlock(
'Symfony Password Encoder Utility',
'bg=blue;fg=white',
true
),
'',
));

$output->writeln(array(
'',
'This command encodes any password you want according to the configuration you',
'made in your configuration file containing the <comment>security.encoders</comment> key.',
'',
));
})->setHidden(true)->setMaxAttempts(20);
}

private function writeResult(OutputInterface $output)
private function generateSalt()
{
$output->writeln(array(
'',
$this->getHelperSet()->get('formatter')->formatBlock(
'✔ Password encoding succeeded',
'bg=green;fg=white',
true
),
'',
));
return base64_encode($this->getContainer()->get('security.secure_random')->nextBytes(30));
}
}
Loading
0