diff --git a/CLAUDE.md b/CLAUDE.md index e4c9e7890..dfa9cdd4d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -236,6 +236,16 @@ A few harness details bite when writing an integration test against a real branc rather than a bug. `anAccountStampedNow()` compares everything else exactly and takes only `passDate` from the actual, after checking it is a timestamp from the last few seconds. Injecting `sleep(1)` before the write is how to reproduce it, and how to show a fix works. +- **An interrupted CLI install test poisons every later run, with an error that looks like a code + defect.** `DatabaseUtil::createUser()` grants with `GRANT … IDENTIFIED BY`, and MariaDB 11.8 + refuses that for an account that **already exists** — *"Can't find any matching row in the user + table"* (SQLSTATE 28000, 1133). It creates a new one happily, so the helper works exactly once + per user: a run that dies before its teardown leaves `syspass@`, `syspass@` and + `sp_*` users behind, and `InstallCommandTest` then fails for everyone afterwards. Confirm it is + not yours by running the same test on a stashed tree, then + `SELECT user, host FROM mysql.user` and drop the leftovers — everything but `syspass@%`, + `root@%`, `root@localhost`, `healthcheck@*` and `mariadb.sys@localhost` is a leftover. + - **Faker's `randomNumber($n)` includes zero**, and forms read a zero id as "not given". A fixture drawing a group or profile id that way fails about one run in a hundred, on CI, in whichever pull request happened to be open. Use `numberBetween(1, …)`. diff --git a/src/Infrastructure/Adapter/In/Cli/Commands/CommandBase.php b/src/Infrastructure/Adapter/In/Cli/Commands/CommandBase.php index 4a79c04a6..d42d3b99b 100644 --- a/src/Infrastructure/Adapter/In/Cli/Commands/CommandBase.php +++ b/src/Infrastructure/Adapter/In/Cli/Commands/CommandBase.php @@ -30,7 +30,9 @@ use SP\Application\Config\Services\ConfigFile; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Style\StyleInterface; +use function SP\__; use function SP\getFromEnv; /** @@ -40,6 +42,8 @@ */ abstract class CommandBase extends Command { + private const REDACTED = '***'; + /** * @var string[] */ @@ -70,6 +74,104 @@ protected static function getEnvVarOrOption( ?: $input->getOption($option); } + /** + * Tell the operator that a secret they put on the command line was visible, and take it out of + * what `ps` shows from here on. + * + * Called once at the start of a command rather than beside each password, so the value is gone + * before the work starts — a master-password rotation re-encrypts every account, and the whole + * of that is time another local user could be reading `/proc//cmdline`. + * + * Nothing is said when the value came from the environment or from the prompt: those are the + * two ways to give a command a secret that other users on the host cannot read, and they are + * what the warning points at. + */ + protected static function warnAboutSecretsOnTheCommandLine( + InputInterface $input, + StyleInterface $style, + string ...$options + ): void { + $given = array_values( + array_filter($options, static fn(string $option): bool => !empty($input->getOption($option))) + ); + + if ($given === []) { + return; + } + + self::hideSecretsFromTheProcessTitle(...$options); + + $style->warning( + sprintf( + __('A password given as %s is visible to every local user while the command runs. Use the environment variable or let the command ask for it.'), + implode(', ', array_map(static fn(string $option): string => '--' . $option, $given)) + ) + ); + } + + /** + * Take the named options' values out of what `ps` shows for this process. + * + * A value passed as `--masterPassword=…` is in `argv`, and `argv` is `/proc//cmdline`, + * which every local user on the host can read for as long as the command runs — minutes, for a + * rotation that re-encrypts every account. That is a different threat from the one the CLI is + * otherwise exempt from: `sp:backup` needs no demo guard because whoever can run it already has + * `config/config.xml`, but this is about the *other* users on a shared host, who have neither + * that file nor any way to read the environment of a process they do not own. + * + * `cli_set_process_title()` rewrites that memory, so the value is gone from `ps` for the rest + * of the run. The title is rebuilt from the real command line with only the secrets replaced, + * rather than replaced wholesale, so the process still says what it is. + * + * This shrinks the window to the moment before the command starts; it cannot close it, and it + * does nothing about shell history or a script with the password written into it. The warning + * beside each call is the other half. + */ + protected static function hideSecretsFromTheProcessTitle(string ...$options): void + { + if (!function_exists('cli_set_process_title')) { + return; + } + + /** @var string[] $argv */ + $argv = $_SERVER['argv'] ?? []; + + if ($argv === []) { + return; + } + + $masked = []; + $maskNext = false; + + foreach ($argv as $argument) { + if ($maskNext) { + $masked[] = self::REDACTED; + $maskNext = false; + + continue; + } + + foreach ($options as $option) { + // Both spellings Symfony accepts: --option=value and --option value. + if (str_starts_with($argument, sprintf('--%s=', $option))) { + $argument = sprintf('--%s=%s', $option, self::REDACTED); + + break; + } + + if ($argument === sprintf('--%s', $option)) { + $maskNext = true; + + break; + } + } + + $masked[] = $argument; + } + + @cli_set_process_title(implode(' ', $masked)); + } + /** * @return string|false */ diff --git a/src/Infrastructure/Adapter/In/Cli/Commands/Crypt/UpdateMasterPasswordCommand.php b/src/Infrastructure/Adapter/In/Cli/Commands/Crypt/UpdateMasterPasswordCommand.php index e5fc79038..fcf6a5db1 100644 --- a/src/Infrastructure/Adapter/In/Cli/Commands/Crypt/UpdateMasterPasswordCommand.php +++ b/src/Infrastructure/Adapter/In/Cli/Commands/Crypt/UpdateMasterPasswordCommand.php @@ -117,6 +117,8 @@ protected function execute( ): int { $style = new SymfonyStyle($input, $output); + self::warnAboutSecretsOnTheCommandLine($input, $style, 'masterPassword', 'currentMasterPassword'); + if (!$this->lock()) { $style->warning(__('The command is already running in another process')); diff --git a/src/Infrastructure/Adapter/In/Cli/Commands/InstallCommand.php b/src/Infrastructure/Adapter/In/Cli/Commands/InstallCommand.php index 25579772e..a2654dd17 100644 --- a/src/Infrastructure/Adapter/In/Cli/Commands/InstallCommand.php +++ b/src/Infrastructure/Adapter/In/Cli/Commands/InstallCommand.php @@ -162,6 +162,8 @@ protected function execute(InputInterface $input, OutputInterface $output): int { $style = new SymfonyStyle($input, $output); + self::warnAboutSecretsOnTheCommandLine($input, $style, 'adminPassword', 'masterPassword', 'databasePassword'); + try { // Throws when sysPass is already installed and --forceInstall was not given; // a fresh system installs without the flag diff --git a/tests/Unit/Infrastructure/Adapter/In/Cli/Commands/SecretsAreNotLeftOnTheCommandLineTest.php b/tests/Unit/Infrastructure/Adapter/In/Cli/Commands/SecretsAreNotLeftOnTheCommandLineTest.php new file mode 100644 index 000000000..8b968008d --- /dev/null +++ b/tests/Unit/Infrastructure/Adapter/In/Cli/Commands/SecretsAreNotLeftOnTheCommandLineTest.php @@ -0,0 +1,142 @@ +. + */ + +namespace SP\Tests\Unit\Infrastructure\Adapter\In\Cli\Commands; + +use PHPUnit\Framework\Attributes\Group; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use ReflectionMethod; +use SP\Infrastructure\Adapter\In\Cli\Commands\CommandBase; + +/** + * A password given to a command as an option is in `argv`, and `argv` is `/proc//cmdline`, + * which every other local user on the host can read for as long as the command runs. + * + * That is a different threat from the one the CLI is otherwise exempt from. `sp:backup` needs no + * demo guard because whoever can run it already has `config/config.xml` — but the users this is + * about have neither that file nor any way to read the environment of a process they do not own, + * and `sp:updateMasterPassword` re-encrypts every account, so the window is minutes rather than an + * instant. + * + * `cli_set_process_title()` rewrites that memory. This asserts it against `/proc` directly, because + * the whole point is what another process can see, and because the platform could stop honouring it + * without anything else noticing. + */ +#[Group('unitary')] +class SecretsAreNotLeftOnTheCommandLineTest extends TestCase +{ + private const SECRET = 'correct-horse-battery-staple'; + private const OTHER_SECRET = 'a-second-secret-value'; + + /** @var string[] */ + private array $argv = []; + + protected function setUp(): void + { + parent::setUp(); + + if (!function_exists('cli_set_process_title') || !is_readable('/proc/self/cmdline')) { + self::markTestSkipped('needs a Linux CLI SAPI with a readable /proc'); + } + + $this->argv = $_SERVER['argv'] ?? []; + } + + protected function tearDown(): void + { + $_SERVER['argv'] = $this->argv; + + // Put the runner's own title back, so a later `ps` still finds it by name. + @cli_set_process_title(implode(' ', $this->argv)); + + parent::tearDown(); + } + + /** + * Both spellings the console accepts — `--option=value` and `--option value` — are taken out, + * and the command still says what it is. + */ + #[Test] + public function aPasswordGivenAsAnOptionIsTakenOutOfTheProcessTitle(): void + { + $_SERVER['argv'] = [ + 'bin/cli.php', + 'sp:updateMasterPassword', + '--masterPassword=' . self::SECRET, + '--currentMasterPassword', + self::OTHER_SECRET, + '--update', + ]; + + self::hide('masterPassword', 'currentMasterPassword'); + + $cmdline = self::cmdline(); + + self::assertStringNotContainsString(self::SECRET, $cmdline); + self::assertStringNotContainsString(self::OTHER_SECRET, $cmdline); + self::assertStringContainsString('sp:updateMasterPassword', $cmdline); + self::assertStringContainsString('--update', $cmdline); + } + + /** + * An option that carries no secret is left alone — the title is rebuilt from the real command + * line rather than replaced, so a process being looked at is still identifiable. + */ + #[Test] + public function everythingElseIsLeftAsItWas(): void + { + $_SERVER['argv'] = ['bin/cli.php', 'sp:backup', '--path=/var/backups/nightly']; + + self::hide('masterPassword'); + + self::assertStringContainsString('--path=/var/backups/nightly', self::cmdline()); + } + + /** + * And the check that shows the assertion above is worth something: without the call, the secret + * is exactly where it was. + */ + #[Test] + public function withoutTheCallTheSecretIsVisible(): void + { + @cli_set_process_title('bin/cli.php sp:updateMasterPassword --masterPassword=' . self::SECRET); + + self::assertStringContainsString(self::SECRET, self::cmdline()); + } + + private static function hide(string ...$options): void + { + (new ReflectionMethod(CommandBase::class, 'hideSecretsFromTheProcessTitle'))->invoke(null, ...$options); + } + + private static function cmdline(): string + { + clearstatcache(); + + return str_replace("\0", ' ', (string)file_get_contents('/proc/self/cmdline')); + } +}