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

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

Merged
merged 5 commits into from Aug 4, 2017

Conversation

2 participants
@fjarrett
Contributor

fjarrett 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 :-)

@danielbachhuber

👍
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 this to the 1.1.4 milestone Aug 4, 2017

@danielbachhuber

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 4, 2017

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

Member

danielbachhuber commented Aug 4, 2017

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

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 4, 2017

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?

Member

danielbachhuber commented Aug 4, 2017

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

@fjarrett

This comment has been minimized.

Show comment
Hide comment
@fjarrett

fjarrett Aug 4, 2017

Contributor

@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
Contributor

fjarrett commented Aug 4, 2017

@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 ) {

This comment has been minimized.

@danielbachhuber

danielbachhuber Aug 4, 2017

Member

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?

@danielbachhuber

danielbachhuber Aug 4, 2017

Member

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?

This comment has been minimized.

@fjarrett

fjarrett Aug 4, 2017

Contributor

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

@fjarrett

fjarrett Aug 4, 2017

Contributor

@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

This comment has been minimized.

Show comment
Hide comment
@danielbachhuber

danielbachhuber Aug 4, 2017

Member

👍 Thanks for your work on this @fjarrett

Member

danielbachhuber commented Aug 4, 2017

👍 Thanks for your work on this @fjarrett

@danielbachhuber danielbachhuber merged commit ca83ade into wp-cli:master Aug 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber changed the title from Generate keys/salts, use API as fallback to Generate keys/salts locally and use WordPress.org API as fallback Aug 4, 2017

@fjarrett fjarrett deleted the fjarrett:gen-key-salts branch Aug 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment