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.
There was a problem hiding this comment.
Added the upfront validation.
| $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.
There was a problem hiding this comment.
Added support for export.
| 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. All inputs (--prefix,--length,--keep) are validated up-front, before any .env mutation, so an invalid value cannot leave the file half-rotated. Once validation passes,previousKeysis written first; ifkey:generatewere ever to fail afterwards, the worst case is a stale-but-decryptable .env rather than a key-loss event. The replace/insert path uses anchored regexes that recognize DotEnv's optionalexportprefix and ignore comments mentioning the setting name.Options:
--force/-f,--prefix,--length,--keep=N.The IO-swap pattern used to silence the
key:generatesub-command was extracted to a reusableAbstractCommand::callSilently()helper in #10177 (per @michalsn's review); this branch will be rebased onto that once it lands.Checklist: