feat: add key:rotate command#10174
Conversation
memleakd
left a comment
memleakd
left a comment
There was a problem hiding this comment.
Thanks for working on this. I left a few inline questions about .env rewrite edge cases.
| unset($_ENV['encryption.previousKeys']); | ||
| service('superglobals')->unsetServer('encryption.previousKeys'); | ||
|
|
||
| $exitCode = $this->callKeyGenerateSilently($prefix, $options['length']); |
There was a problem hiding this comment.
Could --length be validated before writing previousKeys? For example, a negative or zero value reaches key:generate, fails in key generation, and leaves .env partially modified even though rotation did not complete.
| $value = implode(',', $previousKeys); | ||
| $replacement = "\nencryption.previousKeys = {$value}"; | ||
|
|
||
| if (! str_contains($contents, 'encryption.previousKeys')) { |
There was a problem hiding this comment.
Could this treat comments or unrelated text containing encryption.previousKeys as an existing setting? If .env has a comment mentioning this key, the branch below runs, but the replacement regex may not match anything. The command can then continue and rotate encryption.key without actually writing the old key to previousKeys.
| return file_put_contents($envFile, $injected !== $contents ? $injected : $contents . $replacement) !== false; | ||
| } | ||
|
|
||
| $contents = (string) preg_replace( |
There was a problem hiding this comment.
Should this also handle DotEnv's supported export encryption.previousKeys = ... syntax? As written, that line would make the earlier str_contains() check true, but this regex would not replace it, so the rotated-out key may not be persisted.
| private function callKeyGenerateSilently(string $prefix, string $length): int | ||
| { | ||
| $priorIo = CLI::getInputOutput(); | ||
|
|
||
| CLI::setInputOutput(new NullInputOutput()); | ||
|
|
||
| try { | ||
| return $this->call( | ||
| 'key:generate', | ||
| options: [ | ||
| 'force' => null, | ||
| 'prefix' => $prefix, | ||
| 'length' => $length, | ||
| ], | ||
| noInteractionOverride: true, | ||
| ); | ||
| } finally { | ||
| if ($priorIo instanceof InputOutput) { | ||
| CLI::setInputOutput($priorIo); | ||
| } else { | ||
| CLI::resetInputOutput(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we may need these types of calls again in the future. Maybe we could add a reusable helper, like callSilently(), so this IO-swap/restore logic doesn't need to be repeated?
Like:
protected function callSilently(
string $command,
array $arguments = [],
array $options = [],
?bool $noInteractionOverride = true,
): int {
// swap IO, call(), restore IO
}3 participants
Description
Adds a
key:rotatespark command that demotes the currentencryption.keytoencryption.previousKeysin .env and generates a fresh key, so existing ciphertext stays decryptable via theKeyRotationDecoratorfallback.The actual key write is delegated to
key:generateto avoid duplicating that logic;previousKeysis written first so a failure mid-rotation leaves a stale-but-decryptable.envrather than risking key loss. Options:--force/-f,--prefix,--length, and--keep=Nto cap the retained list.Two small support pieces in
system/CLI/:NullInputOutput— anInputOutputsink for silencing a sub-command invoked viaAbstractCommand::call().CLI::getInputOutput()—@internalgetter symmetric tosetInputOutput()/resetInputOutput(), so callers can save and restore the prior IO.Checklist: