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

Try to get admin_password from the ENV by default #261

Closed

Conversation

ton31337
Copy link

No description provided.

@ton31337 ton31337 requested a review from a team as a code owner May 31, 2024 07:02
@ton31337 ton31337 force-pushed the fix/use_env_for_grabbing_admin_password branch 4 times, most recently from 56dfb56 to cf3ba9c Compare May 31, 2024 08:02
In a situation where you run wp-cli with `--admin-password`, then plain-text
password is exposed in process list (e.g. `ps aufx`). We might be able to use
`--admin-password=$WP_CLI_CORE_INSTALL_ADMIN_PASSWORD`, but it's not possible also
when running in a nested mode (e.g. unshare() -> chroot() -> execvp() chain).

Thus, this relaxes a bit the installation and tries to get the default password
from the environment itself by default.

It doesn't break a current functionality.

```
% export WP_CLI_CORE_INSTALL_ADMIN_PASSWORD=password123 ; ./vendor/bin/wp core install --admin_user=admin --url=donatas.net --title=wp --admin_email=me@donatas.net
Success: WordPress installed successfully.

% ./vendor/bin/wp user check-password admin password123 && echo OK
OK
```

Signed-off-by: Donatas Abraitis <donatas@hostinger.com>
@ton31337 ton31337 force-pushed the fix/use_env_for_grabbing_admin_password branch from cf3ba9c to a8308bb Compare May 31, 2024 08:02
@swissspidy
Copy link
Member

Hi there and thanks for your contribution! Can you please share some additional context on this particular PR?

At first glance I don't think it makes sense to support this as-is.

Usually, if you want to use a custom default value for an argument like that, the way to do itis via a wp-cli.yml config file. See https://make.wordpress.org/cli/handbook/references/config/ for an example of how to set default values.

If that doesn't solve your use case, for example if you'd like the config file to read from an environment variable or something, I'd suggest first opening an issue outlining your problem, what you have tried so far, and a possible solution.

@ton31337
Copy link
Author

ton31337 commented May 31, 2024

Can you please share some additional context on this particular PR?

Hey, the context is actually inside the commit itself: a8308bb

P.S. created an issue #262.

@danielbachhuber
Copy link
Member

@ton31337 You can achieve a similar result with the --prompt= argument.

Including --prompt=admin_password will cause WP-CLI to prompt just for the --admin_password= argument:

$ wp core install --url=vanilla.test --title=vanilla --admin_user=daniel --admin_email=daniel@bachhuber.co --prompt=admin_password
2/4 [--admin_password=<password>]:

You can then pipe the value to the command:

$ echo 'foobar' | wp core install --url=vanilla.test --title=vanilla --admin_user=daniel --admin_email=daniel@bachhuber.co --prompt=admin_password
wp core install --title='vanilla' --admin_user='daniel' --admin_email='daniel@bachhuber.co' --admin_password='foobar'
Success: WordPress installed successfully.

Would this serve your needs?

@ton31337
Copy link
Author

ton31337 commented Jun 5, 2024

@danielbachhuber thanks for the response, but my case is a bit different. STDIN, neither file works for me, because with STDIN the password is visible in the process list (e.g. ps aufx). That's why I need this information to be parsed directly in PHP, and not by the arguments. I hope this gives you a better view of the issue :)

@danielbachhuber
Copy link
Member

@ton31337 I'd rather not scatter more environment variables throughout the codebase if possible. Are there any other implementations you can think of?

@ton31337
Copy link
Author

ton31337 commented Jun 9, 2024

Are there any other implementations you can think of?

Nope, this is the most proper way achieving what's desired. If you have any other idea, let me know.

@schlessera
Copy link
Member

schlessera commented Aug 7, 2024

@ton31337 This is just one of many places where directly passing sensitive information can be leaked into the process list or similar, so adding environment variables for these is not really scalable.

As @danielbachhuber mentioned, WP-CLI supports file-based approaches to solve this in a more generic way:

  1. You can put the sensitive information into a configuration file. The file would be a YAML file with the following structure:
    core install:
        admin_password: 123456
    You can then point your WP-CLI execution to a custom location for that file if you generate it dynamically:
    WP_CLI_CONFIG_PATH=/path/to/custom/wp-cli.yml wp core install
  2. You can let WP-CLI require an additional PHP file that gets executed from within the context of WP-CLI. This way, you have full control of the entire execution. You can for example hook into the random_password filter to control what the generated password will be:
    <?php
    // Hook into the WP-CLI process to act when the `wp core install` command is being triggered.
    WP_CLI::add_hook( 'before_invoke:core install', function() {
    
        // Compute the admin password in whatever way you want.
        // This can come from a file, be queried via an API, etc...
        $admin_password = '123456';
    
        // Hook into the `random_password` filter to return the computed password.
        WP_CLI::add_wp_hook( 'random_password', function() use ( $admin_password ) {
            return $admin_password;
        } );
    } );
    This can be used via the --require=<path-to-script.php> flag:
    wp core install --require=<path/to/script.php>
  3. You can combine approaches and have a custom config (from wherever it comes) to add the --require=<path-to-script.php> to the WP-CLI binary.
    The config file would then look like this:
    require:
        - path/to/script.php

More detailed documentation about:

@schlessera
Copy link
Member

I'm closing this PR, as this will not be merged as-is, and I will copy the comment above to the issue so we can continue the discussion there to see if this adequately solves your use case.

@schlessera schlessera closed this Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants