Skip to content

Generate keys/salts locally and use WordPress.org API as fallback#25

Merged
danielbachhuber merged 5 commits intowp-cli:masterfrom
frankiejarrett:gen-key-salts
Aug 4, 2017
Merged

Generate keys/salts locally and use WordPress.org API as fallback#25
danielbachhuber merged 5 commits intowp-cli:masterfrom
frankiejarrett:gen-key-salts

Conversation

@frankiejarrett
Copy link
Contributor

@frankiejarrett frankiejarrett commented Aug 4, 2017

Rather than using the WordPress.org for key/salt generation by default, it should be used as a fallback only when random_int() (introduced in PHP 7) isn't available.

Relying on an external service here just creates a network bottleneck in provisioning workflow times, and I believe the default behavior should avoid requiring a network connection at all.

In addition, using --skip-salts with --extra-php is a little annoying because that placeholder in the mustache template is not able to insert keys/salts under the big commented area where they belong.

Here were the averages I came up with after running each 5 times:

The HTTP request way

0.14s user 0.05s system 28% cpu 0.732 total

The Generate way

0.11s user 0.04s system 81% cpu 0.183 total

Obviously CPU goes up since we're crunching cryptographically-secure random numbers, but the total time difference is quite drastic at 500ms or more. Precious provisioning time :-)

Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

👍
Can you add some basic tests around ensuring that salts are correctly added to wp-config.php? You can use `wp config get --constant=`` to verify the existence of a constant after generation.

@danielbachhuber danielbachhuber added command:config Related to 'config' command command:config-create Related to 'config create' command labels Aug 4, 2017
@danielbachhuber danielbachhuber added this to the 1.1.4 milestone Aug 4, 2017
@danielbachhuber
Copy link
Member

Thanks for your work on this @fjarrett

In addition, using --skip-salts with --extra-php is a little annoying because that placeholder in the mustache template is not able to insert keys/salts under the big commented area where they belong.

This will be fun to do with wp-cli/ideas#4

@danielbachhuber
Copy link
Member

@fjarrett Looks like the original tests aren't quite passing: https://travis-ci.org/wp-cli/config-command/builds/260988991

Can you try running behat features/config-create.feature locally to verify the entire feature passes?

@frankiejarrett
Copy link
Contributor Author

@danielbachhuber OK I guess @require-php-7.0 doesn't quite work the way I thought it would...

Fatal error: Call to undefined function random_int() in src/Config_Command.php on line 376

$assoc_args['secure-auth-salt'] = self::unique_key();
$assoc_args['logged-in-salt'] = self::unique_key();
$assoc_args['nonce-salt'] = self::unique_key();
} catch ( Exception $e ) {
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 follow this try ... catch logic. If random_int() doesn't exist, which it wouldn't on < PHP 7.0, my assumption is that you'd see a fatal error like we're seeing in the test.

Can you clarify why you're using try ... catch? Or, do we need to use function_exists() instead?

Copy link
Contributor Author

@frankiejarrett frankiejarrett Aug 4, 2017

Choose a reason for hiding this comment

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

@danielbachhuber Oh duh - my bad. I (wrongly) assumed the try/catch block used in core was to catch whether the random_int() was available, but you're right, obviously that would throw a uncatchable fatal!

I see now it's to catch something completely different:

If an appropriate source of randomness cannot be found, an Exception will be thrown.

So the try/catch is still needed, but you're right that a function_exists( 'random_int' ) should be here too.

@danielbachhuber
Copy link
Member

👍 Thanks for your work on this @fjarrett

@danielbachhuber danielbachhuber merged commit ca83ade into wp-cli:master Aug 4, 2017
@danielbachhuber danielbachhuber changed the title Generate keys/salts, use API as fallback Generate keys/salts locally and use WordPress.org API as fallback Aug 4, 2017
@frankiejarrett frankiejarrett deleted the gen-key-salts branch August 8, 2017 16:36
schlessera pushed a commit that referenced this pull request Jan 6, 2022
Generate keys/salts, use API as fallback
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

command:config Related to 'config' command command:config-create Related to 'config create' command

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants