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

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

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 8, 2019

Q A
Branch? 4.4
Bug fix? no
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

This PR allows encrypting any env vars - not only those using the %env(secret:<...>)% processor (and the processor is removed actually).

It does so by introducing a new EnvVarLoaderInterface (and a corresponding container.env_var_loader tag), which are objects that should return a list of key/value pairs that will be accessible via the regular %env(FOO)% syntax.

The PR fixes a few issues found meanwhile. One is especially important: files in the vault should end with .php to protect against inadvertant exposures of the document root.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:di-env-loader branch from 7a811ad to ba2148f Nov 8, 2019
@lyrixx
lyrixx approved these changes Nov 8, 2019
@fabpot
fabpot approved these changes Nov 8, 2019
nicolas-grekas added a commit that referenced this pull request Nov 8, 2019
…ve SecretEnvVarProcessor (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[DI][FrameworkBundle] add EnvVarLoaderInterface - remove SecretEnvVarProcessor

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

This PR allows encrypting any env vars - not only those using the `%env(secret:<...>)%` processor (and the processor is removed actually).

It does so by introducing a new `EnvVarLoaderInterface` (and a corresponding `container.env_var_loader` tag), which are objects that should return a list of key/value pairs that will be accessible via the regular `%env(FOO)%` syntax.

The PR fixes a few issues found meanwhile. One is especially important: files in the vault should end with `.php` to protect against inadvertant exposures of the document root.

Commits
-------

ba2148f [DI][FrameworkBundle] add EnvVarLoaderInterface - remove SecretEnvVarProcessor
@nicolas-grekas nicolas-grekas merged commit ba2148f into symfony:4.4 Nov 8, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
fabbot.io Your code looks good.
Details
@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-env-loader branch Nov 8, 2019
$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));

This comment has been minimized.

Copy link
@stof

stof Nov 8, 2019

Member

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

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Nov 8, 2019

I wanted to try something, and ... 😍

consul-to-dic


How to:

Start a consul server:

./consul  agent -ui -dev -bootstrap

Install a Symfony 4.4 and apply this diff:

diff --git a/config/routes.yaml b/config/routes.yaml
index c3283aa..e913084 100644
--- a/config/routes.yaml
+++ b/config/routes.yaml
@@ -1,3 +1,3 @@
-#index:
-#    path: /
-#    controller: App\Controller\DefaultController::index
+index:
+   path: /
+   controller: App\Controller\HomeController::home
diff --git a/config/services.yaml b/config/services.yaml
index 5c4b417..14dcf17 100644
--- a/config/services.yaml
+++ b/config/services.yaml
@@ -10,6 +10,8 @@ services:
     _defaults:
         autowire: true      # Automatically injects dependencies in your services.
         autoconfigure: true # Automatically registers your services as commands, event subscribers, etc.
+        bind:
+            string $name: '%env(name)%'
 
     # makes classes in src/ available to be used as services
     # this creates a service per class whose id is the fully-qualified class name
diff --git a/src/Controller/HomeController.php b/src/Controller/HomeController.php
new file mode 100644
index 0000000..c23728b
--- /dev/null
+++ b/src/Controller/HomeController.php
@@ -0,0 +1,20 @@
+<?php
+
+namespace App\Controller;
+
+use Symfony\Component\HttpFoundation\Response;
+
+class HomeController
+{
+    private $name;
+
+    public function __construct(string $name)
+    {
+        $this->name = $name;
+    }
+
+    public function home(): Response
+    {
+        return new Response("Web site name: {$this->name}");
+    }
+}
diff --git a/src/Env/ConsulEnvVarLoader.php b/src/Env/ConsulEnvVarLoader.php
new file mode 100644
index 0000000..f699702
--- /dev/null
+++ b/src/Env/ConsulEnvVarLoader.php
@@ -0,0 +1,23 @@
+<?php
+
+namespace App\Env;
+
+use Symfony\Component\DependencyInjection\EnvVarLoaderInterface;
+
+class ConsulEnvVarLoader implements EnvVarLoaderInterface
+{
+    public function loadEnvVars(): array
+    {
+        $response = file_get_contents('http://127.0.0.1:8500/v1/kv/website-config');
+
+        $consulValue = json_decode($response, true)[0]['Value'];
+        $decoded = json_decode(base64_decode($consulValue), true);
+
+        // Shape:
+        // array:1 [▼
+        //     "name" => "my super website"
+        // ]
+
+        return $decoded;
+    }
+}
\ No newline at end of file

Then, you can update the consul KV:

./consul  kv put website-config '{"name": "Symfony read this var from consul"}'

SO SIMPLE thanks @nicolas-grekas

@stof

This comment has been minimized.

Copy link
Member

stof commented Nov 8, 2019

Wouldn't this decrypt all secrets eagerly all the time now ? what is the perf impact of that in case your requests does not instantiate all services needing some of the secrets (and you have lots of them) ?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Nov 8, 2019

Wouldn't this decrypt all secrets eagerly all the time now ? what is the perf impact of that in case your requests does not instantiate all services needing some of the secrets (and you have lots of them) ?

Correct. Locally I decrypt 24 strings of 128bytes in 1ms. We might decide to group secrets in one file, but then that wouldn't play well with git merges. An alternative would be to encourage using secrets:decrypt-to-local when deploying (then unsetting the decryption key ideally). That would reclaim all the perf.

@jderusse

This comment has been minimized.

Copy link
Contributor

jderusse commented Nov 8, 2019

What's about removing the local flag?
Developers now have the ability to declare their own secret with their own .env file?

if (null === $env = $this->container->getParameter("env($name)")) {
return null;
try {
while ((false === $env || null === $env) && $this->loaders->valid()) {

This comment has been minimized.

Copy link
@jderusse

jderusse Nov 8, 2019

Contributor

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.

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Nov 8, 2019

Author Member

fixed in #34301

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

nicolas-grekas commented Nov 8, 2019

What's about removing the local flag?

we discussed this with @weaverryan and my opinion is that should keep it:
decrypt-to-local, encrypt-from-local and secrets:list need the local vault anyway.
Removing the possibility to use it from the secrets:set and secrets:remove command wouldn't help anything to me, on the implementation side. Keeping them might actually help the workflow of ppl, by using common commands for local & encrypt secrets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.