8000 [DI][FrameworkBundle] add EnvVarLoaderInterface - remove SecretEnvVarProcessor by nicolas-grekas · Pull Request #34295 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[DI][FrameworkBundle] add EnvVarLoaderInterface - remove SecretEnvVarProcessor #34295

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 8, 2019
Merged

[DI][FrameworkBundle] add EnvVarLoaderInterface - remove SecretEnvVarProcessor #34295

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Definition;
use Symfony\Component\DependencyInjection\EnvVarLoaderInterface;
use Symfony\Component\DependencyInjection\EnvVarProcessorInterface;
use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
use Symfony\Component\DependencyInjection\Exception\LogicException;
Expand Down Expand Up @@ -376,6 +377,8 @@ public function load(array $configs, ContainerBuilder $container)
->addTag('console.command');
$container->registerForAutoconfiguration(ResourceCheckerInterface::class)
->addTag('config_cache.resource_checker');
$container->registerForAutoconfiguration(EnvVarLoaderInterface::class)
->addTag('container.env_var_loader');
$container->registerForAutoconfiguration(EnvVarProcessorInterface::class)
->addTag('container.env_var_processor');
$container->registerForAutoconfiguration(ServiceLocator::class)
Expand Down
7 changes: 1 addition & 6 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/secrets.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

<services>
<service id="secrets.vault" class="Symfony\Bundle\FrameworkBundle\Secrets\SodiumVault">
<tag name="container.env_var_loader" />
<argument>%kernel.project_dir%/config/secrets/%kernel.environment%</argument>
<argument type="service" id="secrets.decryption_key" on-invalid="ignore" />
</service>
Expand All @@ -31,11 +32,5 @@
<service id="secrets.local_vault" class="Symfony\Bundle\FrameworkBundle\Secrets\DotenvVault">
<argument>%kernel.project_dir%/.env.local</argument>
</service>

<service id="secrets.env_var_processor" class="Symfony\Bundle\FrameworkBundle\Secrets\SecretEnvVarProcessor">
<argument type="service" id="secrets.vault" />
<argument type="service" id="secrets.local_vault" on-invalid="ignore" />
<tag name="container.env_var_processor" />
</service>
</services>
</container>
6 changes: 6 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/config/services.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,5 +121,11 @@
<argument type="service" id="request_stack" />
<tag name="kernel.event_subscriber" />
</service>

<service id="container.env_var_processor" class="Symfony\Component\DependencyInjection\EnvVarProcessor">
<tag name="container.env_var_processor" />
<argument type="service" id="service_container" />
<argument type="tagged_iterator" tag="container.env_var_loader" />
</service>
</services>
</container>
2 changes: 1 addition & 1 deletion src/Symfony/Bundle/FrameworkBundle/Secrets/DotenvVault.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public function reveal(string $name): ?string
{
$this->lastMessage = null;
$this->validateName($name);
$v = \is_string($_SERVER[$name] ?? null) ? $_SERVER[$name] : ($_ENV[$name] ?? null);
$v = \is_string($_SERVER[$name] ?? null) && 0 !== strpos($name, 'HTTP_') ? $_SERVER[$name] : ($_ENV[$name] ?? null);

if (null === $v) {
$this->lastMessage = sprintf('Secret "%s" not found in "%s".', $name, $this->getPrettyPath($this->dotenvFile));
Expand Down
59 changes: 0 additions & 59 deletions src/Symfony/Bundle/FrameworkBundle/Secrets/SecretEnvVarProcessor.php
View file
Open in desktop

This file was deleted.

41 changes: 24 additions & 17 deletions src/Symfony/Bundle/FrameworkBundle/Secrets/SodiumVault.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@

namespace Symfony\Bundle\FrameworkBundle\Secrets;

use Symfony\Component\DependencyInjection\EnvVarLoaderInterface;

/**
* @author Tobias Schultze <http://tobion.de>
* @author Jérémy Derussé <jeremy@derusse.com>
* @author Nicolas Grekas <p@tchwork.com>
*
* @internal
*/
class SodiumVault extends AbstractVault
class SodiumVault extends AbstractVault implements EnvVarLoaderInterface
{
private $encryptionKey;
private $decryptionKey;
Expand Down Expand Up @@ -56,8 +58,8 @@ public function generateKeys(bool $override = false): bool
// ignore failures to load keys
}

if ('' !== $this->decryptionKey && !file_exists($this->pathPrefix.'sodium.encrypt.public')) {
$this->export('sodium.encrypt.public', $this->encryptionKey);
if ('' !== $this->decryptionKey && !file_exists($this->pathPrefix.'encrypt.public.php')) {
$this->export('encrypt.public', $this->encryptionKey);
}

if (!$override && null !== $this->encryptionKey) {
Expand All @@ -69,10 +71,10 @@ public function generateKeys(bool $override = false): bool
$this->decryptionKey = sodium_crypto_box_keypair();
$this->encryptionKey = sodium_crypto_box_publickey($this->decryptionKey);

$this->export('sodium.encrypt.public', $this->encryptionKey);
$this->export('sodium.decrypt.private', $this->decryptionKey);
$this->export('encrypt.public', $this->encryptionKey);
$this->export('decrypt.private', $this->decryptionKey);

$this->lastMessage = sprintf('Sodium keys have been generated at "%s*.{public,private}".', $this->getPrettyPath($this->pathPrefix));
$this->lastMessage = sprintf('Sodium keys have been generated at "%s*.public/private.php".', $this->getPrettyPath($this->pathPrefix));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this change looks weird to me, as / is the path separator and so this would create some confusion here.


return true;
}
Expand All @@ -82,12 +84,12 @@ public function seal(string $name, string $value): void
$this->lastMessage = null;
$this->validateName($name);
$this->loadKeys();
$this->export($name.'.'.substr_replace(md5($name), '.sodium', -26), sodium_crypto_box_seal($value, $this->encryptionKey ?? sodium_crypto_box_publickey($this->decryptionKey)));
$this->export($name.'.'.substr(md5($name), 0, 6), sodium_crypto_box_seal($value, $this->encryptionKey ?? sodium_crypto_box_publickey($this->decryptionKey)));

$list = $this->list();
$list[$name] = null;
uksort($list, 'strnatcmp');
file_put_contents($this->pathPrefix.'sodium.list', sprintf("<?php\n\nreturn %s;\n", var_export($list, true), LOCK_EX));
file_put_contents($this->pathPrefix.'list.php', sprintf("<?php\n\nreturn %s;\n", var_export($list, true), LOCK_EX));

$this->lastMessage = sprintf('Secret "%s" encrypted in "%s"; you can commit it.', $name, $this->getPrettyPath(\dirname($this->pathPrefix).\DIRECTORY_SEPARATOR));
}
Expand All @@ -97,7 +99,7 @@ public function reveal(string $name): ?string
$this->lastMessage = null;
$this->validateName($name);

if (!file_exists($file = $this->pathPrefix.$name.'.'.substr_replace(md5($name), '.sodium', -26))) {
if (!file_exists($file = $this->pathPrefix.$name.'.'.substr_replace(md5($name), '.php', -26))) {
$this->lastMessage = sprintf('Secret "%s" not found in "%s".', $name, $this->getPrettyPath(\dirname($this->pathPrefix).\DIRECTORY_SEPARATOR));

return null;
Expand Down Expand Up @@ -131,15 +133,15 @@ public function remove(string $name): bool
$this->lastMessage = null;
$this->validateName($name);

if (!file_exists($file = $this->pathPrefix.$name.'.'.substr_replace(md5($name), '.sodium', -26))) {
if (!file_exists($file = $this->pathPrefix.$name.'.'.substr_replace(md5($name), '.php', -26))) {
$this->lastMessage = sprintf('Secret "%s" not found in "%s".', $name, $this->getPrettyPath(\dirname($this->pathPrefix).\DIRECTORY_SEPARATOR));

return false;
}

$list = $this->list();
unset($list[$name]);
file_put_contents($this->pathPrefix.'sodium.list', sprintf("<?php\n\nreturn %s;\n", var_export($list, true), LOCK_EX));
file_put_contents($this->pathPrefix.'list.php', sprintf("<?php\n\nreturn %s;\n", var_export($list, true), LOCK_EX));

$this->lastMessage = sprintf('Secret "%s" removed from "%s".', $name, $this->getPrettyPath(\dirname($this->pathPrefix).\DIRECTORY_SEPARATOR));

Expand All @@ -150,7 +152,7 @@ public function list(bool $reveal = false): array
{
$this->lastMessage = null;

if (!file_exists($file = $this->pathPrefix.'sodium.list')) {
if (!file_exists($file = $this->pathPrefix.'list.php')) {
return [];
}

Expand All @@ -167,6 +169,11 @@ public function list(bool $reveal = false): array
return $secrets;
}

public function loadEnvVars(): array
{
return $this->list(true);
}

private function loadKeys(): void
{
if (!\function_exists('sodium_crypto_box_seal')) {
Expand All @@ -177,12 +184,12 @@ private function loadKeys(): void
return;
}

if (file_exists($this->pathPrefix.'sodium.decrypt.private')) {
$this->decryptionKey = (string) include $this->pathPrefix.'sodium.decrypt.private';
if (file_exists($this->pathPrefix.'decrypt.private.php')) {
$this->decryptionKey = (string) include $this->pathPrefix.'decrypt.private.php';
}

if (file_exists($this->pathPrefix.'sodium.encrypt.public')) {
$this->encryptionKey = (string) include $this->pathPrefix.'sodium.encrypt.public';
if (file_exists($this->pathPrefix.'encrypt.public.php')) {
$this->encryptionKey = (string) include $this->pathPrefix.'encrypt.public.php';
} elseif ('' !== $this->decryptionKey) {
$this->encryptionKey = sodium_crypto_box_publickey($this->decryptionKey);
} else {
Expand All @@ -196,7 +203,7 @@ private function export(string $file, string $data): void
$data = str_replace('%', '\x', rawurlencode($data));
$data = sprintf("<?php // %s on %s\n\nreturn \"%s\";\n", $name, date('r'), $data);

if (false === file_put_contents($this->pathPrefix.$file, $data, LOCK_EX)) {
if (false === file_put_contents($this->pathPrefix.$file.'.php', $data, LOCK_EX)) {
$e = error_get_last();
throw new \ErrorException($e['message'] ?? 'Failed to write secrets data.', 0, $e['type'] ?? E_USER_WARNING);
}
Expand Down
16 changes: 8 additions & 8 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Secrets/SodiumVaultTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,19 @@ public function testGenerateKeys()
$vault = new SodiumVault($this->secretsDir);

$this->assertTrue($vault->generateKeys());
$this->assertFileExists($this->secretsDir.'/test.sodium.encrypt.public');
$this->assertFileExists($this->secretsDir.'/test.sodium.decrypt.private');
$this->assertFileExists($this->secretsDir.'/test.encrypt.public.php');
$this->assertFileExists($this->secretsDir.'/test.decrypt.private.php');

$encKey = file_get_contents($this->secretsDir.'/test.sodium.encrypt.public');
$decKey = file_get_contents($this->secretsDir.'/test.sodium.decrypt.private');
$encKey = file_get_contents($this->secretsDir.'/test.encrypt.public.php');
$decKey = file_get_contents($this->secretsDir.'/test.decrypt.private.php');

$this->assertFalse($vault->generateKeys());
$this->assertStringEqualsFile($this->secretsDir.'/test.sodium.encrypt.public', $encKey);
$this->assertStringEqualsFile($this->secretsDir.'/test.sodium.decrypt.private', $decKey);
$this->assertStringEqualsFile($this->secretsDir.'/test.encrypt.public.php', $encKey);
$this->assertStringEqualsFile($this->secretsDir.'/test.decrypt.private.php', $decKey);

$this->assertTrue($vault->generateKeys(true));
$this->assertStringNotEqualsFile($this->secretsDir.'/test.sodium.encrypt.public', $encKey);
$this->assertStringNotEqualsFile($this->secretsDir.'/test.sodium.decrypt.private', $decKey);
$this->assertStringNotEqualsFile($this->secretsDir.'/test.encrypt.public.php', $encKey);
$this->assertStringNotEqualsFile($this->secretsDir.'/test.decrypt.private.php', $decKey);
}

public function testEncryptAndDecrypt()
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/DependencyInjection/Compiler/CheckTypeDeclarationsPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ private function checkType(Definition $checkedDefinition, $value, \ReflectionPar
$checkFunction = sprintf('is_%s', $parameter->getType()->getName());

if (!$parameter->getType()->isBuiltin() || !$checkFunction($value)) {
throw new InvalidParameterTypeException($this->currentId, \gettype($value), $parameter);
throw new InvalidParameterTypeException($this->currentId, \is_object($value) ? \get_class($value) : \gettype($value), $parameter);
}
}
}
25 changes: 25 additions & 0 deletions src/Symfony/Component/DependencyInjection/EnvVarLoaderInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\DependencyInjection;

/**
* EnvVarLoaderInterface objects return key/value pairs that are added to the list of available env vars.
*
* @author Nicolas Grekas <p@tchwork.com>
*/
interface EnvVarLoaderInterface
{
/**
* @return string[] Key/value pairs that can be accessed using the regular "%env()%" syntax
*/
public function loadEnvVars(): array;
}
37 changes: 32 additions & 5 deletions src/Symfony/Component/DependencyInjection/EnvVarProcessor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
namespace Symfony\Component\DependencyInjection;

use Symfony\Component\DependencyInjection\Exception\EnvNotFoundException;
use Symfony\Component\DependencyInjection\Exception\ParameterCircularReferenceException;
use Symfony\Component\DependencyInjection\Exception\RuntimeException;

/**
Expand All @@ -20,10 +21,17 @@
class EnvVarProcessor implements EnvVarProcessorInterface
{
private $container;
private $loaders;
private $loadedVars = [];

public function __construct(ContainerInterface $container)
/**
* @param EnvVarLoaderInterface[] $loaders
*/
public function __construct(ContainerInterface $container, \Traversable $loaders = null)
{
$this->container = $container;
$this->loaders = new \IteratorIterator($loaders ?? new \ArrayIterator());
$this->loaders = $this->loaders->getInnerIterator();
}

/**
Expand Down Expand Up @@ -127,12 +135,31 @@ public function getEnv($prefix, $name, \Closure $getEnv)
} elseif (isset($_SERVER[$name]) && 0 !== strpos($name, 'HTTP_')) {
$env = $_SERVER[$name];
} elseif (false === ($env = getenv($name)) || null === $env) { // null is a possible value because of thread safety issues
if (!$this->container->hasParameter("env($name)")) {
throw new EnvNotFoundException(sprintf('Environment variable not found: "%s".', $name));
foreach ($this->loadedVars as $vars) {
if (false !== $env = ($vars[$name] ?? false)) {
break;
}
}

if (null === $env = $this->container->getParameter("env($name)")) {
return null;
try {
while ((false === $env || null === $env) && $this->loaders->valid()) {
Copy link
Member
@jderusse jderusse Nov 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, it does trigger an exception Cannot resume an already running generator
reproduct case:

  • a service need a secret %env(FOO)%
  • this line iterate over registered env_var_loader (with DIC service locator)
  • by default, the SodiumVault requires an env variable defined by config option decryption_env_var
  • if the env variable does not exists, this is line is reached a second time and trigger the exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in #34301

$loader = $this->loaders->current();
$this->loaders->next();
$this->loadedVars[] = $vars = $loader->loadEnvVars();
$env = $vars[$name] ?? false;
}
} catch (ParameterCircularReferenceException $e) {
// skip loaders that need an env var that is not defined
}

if (false === $env || null === $env) {
if (!$this->container->hasParameter("env($name)")) {
throw new EnvNotFoundException(sprintf('Environment variable not found: "%s".', $name));
}

if (null === $env = $this->container->getParameter("env($name)")) {
return null;
}
}
}

Expand Down
Loading
0