[DI] Add support for secrets #23621
[DI] Add support for secrets #23621
Conversation
|
Thanks for working on such an important feature. However, creating a PR directly instead of an issue, and not providing a description makes this very difficult to review. Honestly, I don't care about the technical implementation of this feature. I only care about being easy to learn and simple to use. Please, show us how do you propose to use this feature:
I want to carefully think about all this before implementing the feature. Otherwise, we can make the same mistakes as RubyOnRails, which added secret support in 5.1, but forgot to add a command to display the secret values and they are adding it in their upcoming 5.2 version. Thanks! |
|
@javiereguiluz I'm writing a blog post, but it will not be ready before I go in vacation. As stated in the description, some people have requested access to my POC, it's why I've opened this PR. If you think it hurt, feel free to close it for now. This PR doesn't deal with encryption, it only load files at runtime. It's better to use a lower level layer (such as Docker or Kubernetes secrets) for encryption. The purpose of this PR is just to give access to secrets exposed as files. Tests should be self-explanatory. |
|
For the record, and because @javiereguiluz talked about RubyOnRails, I don't see which problem it solves. Ot put another way, it's far from being a complete solution. I do understand that it allows passwords to be safely stored in a Git repository, but the master password still needs to be stored in a file or an env var on machines. So, it does not solve the most important issue to me: how to avoid passwords to be stored permanently on servers. Solutions like Hashicorp Vault (and many others) lets you "fix" that, but then why not use those solutions to store all passwords. So, regarding this pull request, I think that a good question could be: how do Symfony integrates with solutions like Vault? |
|
To clarify, this PR is not related at all with the secret feature of RoR. I've updated the PR description with more information. I don't know how |
|
To summarize:
It's not in the scope of this PR, use Docker or Kubernetes secrets (or anything else) for that.
Yes. One
It's not in the scope of this PR, use Docker or Kubernetes secrets (or anything else) for that.
It's not in the scope of this PR, use Docker or Kubernetes secrets (or anything else) for that.
It's the main goal of this PR, it basically works the same way that
Well, it's the main goad of this PR.
See the updated description. I'll publish a blog post and a demo app in some weeks. |
How about these syntax? Second parameter is for whatever resolver. |
|
The new "secret()" prefix allows fetching the content of a file at runtime. Is there anything specific to "secret" management? I mean, the name could be "file_content()" or some similar alternative that convey what this does actually, isn't it? Not suggesting yet this is what we should do, just wondering :) This PR duplicates a lot of logic from env() vars management. I think there is another way to reuse the existing code infrastructure in place. I'd suggest to keep using the "env()" syntax, but add special behavior for what's inside the brackets. "env()" would refer to a generic context, defaulting to env vars, but that we could point to another source for contextual data, like local files. I tried something similar in #20276. Not sure the syntax nor the feature then are exactly what we want to have now, but I think it could be a good starting point. |
This is actually done by the underlying system, Kubernetes allows you to inject "secrets" inside your running container via a pseudo-filesystem. It's basically a way to do the same thing env vars do, but in a much more secure way as env variables leak out of your app. |
|
@dunglas props for pushing this! \o/ As @marfillaster mentioned, it seems important not having to hardcode path to secret but inject the path via env. This allows ops more flexibility with their setup. |
|
I think the management of "env()" inside "secret()" should be removed because it adds complexity, yet looks unneeded if you allow configuring the relative prefix of paths (ie make that "kernel.project_dir" default configurable). For more advanced use cases, they should be rare (all vaults put secrets in the same root, isn't it?), then admins could use symlinks. |
|
I like the simplicity of parameters:
secret: '%secret(/path/to/file.ext)%
Looks like it :) |
This is reasonable, if it's per Symfony environment, you could run development with Compose and production with Kube, having different prefix on each.
They actually can't ("shouldn't need to", to be precise) because the symlink would need to be inside the container, complicating your whole runtime. You don't want the app image to need any tweaks before it gets run, it should be ready-set-go as much as possible. For example, if you downloaded an Android APK and needed to switch a few bits around with a hex editor before running it, it wouldn't make a great UX. |
This PR was merged into the 3.4 branch. Discussion ---------- [DI] Allow processing env vars | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | see description | License | MIT | Doc PR | - This PR is an updated version of #20276 ~~(it embeds #23899 for now.)~~ It superscedes/closes: - [DI] Add support for secrets #23621 ping @dunglas - Runtime container parameter not found event filter #23669 ping @marfillaster - [DependencyInjection] [DX] Support for JSON string environment variables #23823 ping @Pierstoval - add support for composite environment variables #17689 ping @greg0ire - [DI] ENV parameters at runtime with PHP 7 strict types not working properly #20434 ping @sandrokeil - Issue when using a SQLite database and the DATABASE_URL env var #23527 ping @javiereguiluz #22151 is another story, so not fixed here. The way it works is via `%env(foo:BAR)%` prefixes, where "foo" can be bound to any services you'd like. By default, the following prefixes are supported: - `bool`, `int`, `float`, `string`, `base64` - `const` (for referencing PHP constants) - `json` (supporting only json **arrays** for type predictability) - `file` (eg for access to secrets stored in files.) - `resolve` (for processing parameters inside env vars.) New prefixes can be added by implementing the new `EnvProviderInterface`, and tagging with `container.env_provider` (see `Rot13EnvProvider` in tests.) Prefixes can be combined to chain processing, eg. `%env(json:base64:file:FOO)%` will be roughly equivalent to `json_decode(base64_decode(file_get_content(getenv('FOO'))))`. Commits ------- 1f92e45 [DI] Allow processing env vars
The goal of this PR is to allow to use a low level secret management system such as Docker Secrets, Kubernetes Secrets with Symfony.
What this PR basically does is allowing to inject dynamically the content of a file (because Docker and Kubernetes secrets are mounted as files in containers) in the container. It can be used like the following:
The path can also be defined as an environment variable (useful to create applications working with different secret management systems):
It can also resolve relative paths (it will look for a file relative to
kernel.project_dirif this parameter exist, or to the container file if it doesn't):This PR doesn't deal at all with secret encryption and storage (it should be delegated to a lower level system).
It doesn't allow, for instance, to store passwords in a Git repository.
However, it handles dynamic secrets (for instance Kubernetes secrets can be changed at runtime) the same way dynamic environment variables are already handled.
This PR is not ready yet, I've opened it for early reviews.