[FrameworkBundle] Add secrets management. #31101

jderusse · 2019-04-13T16:56:43Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#27351
License	MIT
Doc PR	symfony/symfony-docs/pull/11396

During the Foss Hackathon, we worked 2 wall days to talk and read RFC comment, other framework implementations (laravel, rails, ...)
We tried to cover every used case, building a simple and flexible solution that could be improved in the future or 3rd party bundles.
Here are the choice we tooks and why:

How it works:

Have a look to the draft of the documentation. it may help you to understand how it work

In short default usage:

# config/packages/secrets.yaml
framwork:
    secrets:
        encrypted_secrets_dir: '%kernel.project_dir%/config/secrets/%kernel.environment%/'
        encryption_key: '%kernel.project_dir%/config/secrets/encryption_%kernel.environment%.key'

APP_ENV=prod ./bin/console secrets:generate-key

APP_ENV=prod ./bin/console secrets:add DATABASE_URL my-secret

APP_ENV=prod ./bin/console secrets:list --reveal

 -------------- ------------------
  key            plaintext secret 
 -------------- ------------------
  DATABASE_URL   my-secret
 -------------- ------------------

# config/packages/doctrine.yaml
doctrine:
    dbal:
        password: '%env(default::secret:DATABASE_PASSWORD)%'
        # url: '%env(DATABASE_URL_WITHOUT_PASSWORD)'

Use %env()% syntax even if it's not an Environement Variable.

Even if it's not related to "environement variables"", secrets are related to the the environement. using %env()%` make perfect sense and is exactly what we need: executed at runtime + many EnvProcessors already defined

List of Storage implementation

The build-in version in symfony will only store secret in a file.

If users want something else: They could implements their own Storage with SecretStorageInterface.

Some example of storage that are out of scope for now:

ChainStorage: lookup for secret in multiple storages
Vault
AWS Secret management
FileStorage with another encryption algorithm

By default secrets and key are compartimentized per env

The build-in storage stores secrets and key are stored in a path that vary with the kernel.environement variable.
Thuse will - BY DESIGN - avoid users from being tempted to share the production key with other developpers/CI/...

Use lib sodium By default:

Because modern and easy to use (avoid mistake by using wrong combinaison of algorithme)

Symetric algorithm

In oposition with everything I thought before, @tobias conviced me to use a symetric algorithm. (Which mean that everybody who's able to generate a secret would also be able to read the secrets)

When using asymetric keys, the private key should be deployed on production and be accessible by few OPS
When using asymetric keys, the keys are not easily renewable (the private key is required to decrypt with the old key and recrypt with the new key)

Because even with asymetric algorithm, the key could be compromised (if an OPS is fired for instance) and not easily renewable,
We recommand to use symetric key (owned by few people) and renew that key (and secrets of course) everytime an owner leaves the project.

A big thank you to @Tobion, @mpdude and @nicolas-grekas .

fabpot

just CS comments for now

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

src/Symfony/Bundle/FrameworkBundle/Command/SecretsAddCommand.php

src/Symfony/Bundle/FrameworkBundle/Exception/SecretNotFoundException.php

src/Symfony/Bundle/FrameworkBundle/Secret/SodiumEncoder.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsAddCommand.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsGenerateKeyCommand.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsListCommand.php

src/Symfony/Bundle/FrameworkBundle/Secret/SodiumEncoder.php

sstok · 2019-04-14T07:33:10Z

Minor comment, it's environment not environement 👍

src/Symfony/Bundle/FrameworkBundle/Secret/FilesSecretStorage.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsListCommand.php

src/Symfony/Bundle/FrameworkBundle/Secret/MutableSecretStorageInterface.php

src/Symfony/Bundle/FrameworkBundle/Secret/SecretStorageInterface.php

src/Symfony/Bundle/FrameworkBundle/Secret/SodiumEncoder.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsAddCommand.php

src/Symfony/Bundle/FrameworkBundle/Secret/MutableSecretStorageInterface.php

chalasr

first round (wording only)

src/Symfony/Bundle/FrameworkBundle/Command/SecretsAddCommand.php

src/Symfony/Bundle/FrameworkBundle/Secret/FilesSecretStorage.php

src/Symfony/Bundle/FrameworkBundle/Secret/MutableSecretStorageInterface.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsGenerateKeyCommand.php

src/Symfony/Bundle/FrameworkBundle/Secret/SodiumEncoder.php

src/Symfony/Bundle/FrameworkBundle/Tests/Secret/FilesSecretStorageTest.php

noniagriconomie

nice feature :)

src/Symfony/Bundle/FrameworkBundle/Secret/EncoderInterface.php

src/Symfony/Bundle/FrameworkBundle/Secret/SodiumEncoder.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsListCommand.php

src/Symfony/Bundle/FrameworkBundle/Command/SecretsAddCommand.php

javiereguiluz · 2019-04-15T07:34:35Z

src/Symfony/Bundle/FrameworkBundle/Command/SecretsGenerateKeyCommand.php

+    private $secretsStorage;
+    private $encoder;
+
+    public function __construct(EncoderInterface $encoder, MutableSecretStorageInterface $secretsStorage)


If I'm right, we don't use prefixes like Mutable and Immutable in Symfony code.

I would drop the prefix as well

naming things are hard... :(

Here I wanted to distingquihed "readonly storage" from "writable storage" do you have any idea of naming?

note: we have a Symfony\Component\Security\Acl\Model\MutableAclInterface

SecretStorageInterface { get(): string; set(): void; list(): iterable; }

would be enough IMHO

👎 don't just name it get, set etc without "secret". It should be obvious from the method name that this is confidential data, not a generic data store.

given:

At first, users only need to fetch secrets from the storage. That's the purpose of the SecretStorageInterface.

Symfony provides a default storage (the FileSecretStorage) and a convenient way to add new secrets in it (the SecretsAddCommand). Maybe I should inject the FileSecretStorage and remove that interface?

We have 2 interface for reasons:

From our point of view, it didn't make sens to use symfony's command to manage 3rd party tools like vault. Users could use their favorite tool to add/delete secrets from them.

I think that a futur implementation of SecretStorageInterface could be a ChainSecretStorage that iterate over a list of SecretStorageInterface. Implementing setSecret in a chain wouldn't make sens.

Added the ChainSecretStorage which will improve the DX by allowing developper/3rd party to create new SecretStorages

src/Symfony/Bundle/FrameworkBundle/Command/SecretsListCommand.php

src/Symfony/Bundle/FrameworkBundle/Secret/EncoderInterface.php

src/Symfony/Bundle/FrameworkBundle/Secret/FilesSecretStorage.php

Tobion · 2019-07-19T23:00:14Z

I haven't see that one before. Thanks for the link. Why did he create the PR on his own fork instead of symfony?

jderusse · 2019-07-20T09:25:42Z

neither do I. There are a lot of common concept in both PR:

commands to add/update secrets
rotate keys
envProcessor

which let me think, we are on the right way.

But also many differences.

an edit command which open an editor to edit secrets
concret JwtHandler implementation compare to EncoderInterface + SecretStorageInterface
JWT Impl with async alg compare to raw sodium (in default impl)
all secrets in a single file compare to 1 file per secret (in default impl)

Using interface in this PR leave the possibility open to add a JwtEncoder and a SingleFileStorage.

PR rebased, and ready for review

ro0NL · 2019-07-21T14:00:58Z

would a JSON based secret storage be useful? to group multiple secrets in a single file ...

edit: oh you mentioned SingleFileStorage

jderusse · 2019-07-22T08:33:00Z

hello @ro0NL even if this PR leaves the possibility to implement a SingleFileStorage I don't see the usecase for such feature. Will it be better than 1 file per secret?

Note: storing all secrets in 1 file will have a performance drawback : reading/deconding the entier file for fetching a single secret

ro0NL · 2019-07-22T08:42:29Z

@jderusse im not sure yet :) i must say Docker makes it pretty easy to ship secret files into /run/secrets, but yes, im worried about the maintenance part in case of a "file per secret" long term.

I think security is a bigger issue then performance actually, as fetching one value reveals all (using the same key).

surelygroup · 2019-07-22T10:28:44Z

I I understand the one file concept right, when using one file only the json keys are revealed. Each secret (under each key) is encrypted independently.

Tobion · 2019-07-22T20:55:32Z

From performance and security perspective it doesn't matter if it's a single file or one file per secret.
The only difference is usuability. To me several files makes it easier because

it avoids conflicts: if several developers try to add a secret in a single file, the conflict is not resovable because the data is binary junk
nobody needs to worry about the format of several secrets in a file (one secret per line or json etc) and how to access individual secrets

javiereguiluz · 2019-09-19T15:57:22Z

Just a friendly reminder that Symfony 4.4/5.0 will enter its "feature freeze" period in 11 days, on October 1st. It'd be nice to have this feature in. I hope you can do a last-ditch effort to finish this. Thanks!

jderusse · 2019-09-19T16:27:47Z

PR rebased and RFR.

Unless I missed a point all comments are resolved. (or could be resolved in a Future dedicated PR, because the current interfaces in this PR are flexible enougth to allow such evolution (ie. SingleFileStorage or other encoders)

Please tell me if I missed a point.

fabpot · 2019-09-19T19:44:53Z

What about creating a Secret component instead of storing everything in the frameworkbundle, which should stay as an integration point for components?

jderusse · 2019-09-19T23:05:13Z

Depend of the point of view on the feature.

This PR is about providing a "secured" way to store configuration. The encryption thing is an implementation detail.

Moving to a component means focusing on loading/storing secret, probably providing/maintaining several backend (vault, KMS, ...).

At the time I opened the PR I wasn't sure it worth it. We talked about it with @nicolas-grekas which convince me that we should provide a single backend implementation in order to offer the simplest and best experience for the user.

fabpot · 2019-09-20T05:36:28Z

Having a component does not necessarily mean having to provide more than one backend. Dotenv only provides one (.env file) and the Profiler only provides a filesystem backend. We just need to be very clear in the documentation in the interfaces/classes.

fabpot · 2019-09-20T07:28:38Z

src/Symfony/Bundle/FrameworkBundle/Secret/Encoder/EncoderInterface.php

+interface EncoderInterface
+{
+    /**
+     * Generate the keys and material necessary for its operation.


fabpot · 2019-09-20T07:28:42Z

src/Symfony/Bundle/FrameworkBundle/Secret/Encoder/EncoderInterface.php

+    public function generateKeys(bool $override = false): array;
+
+    /**
+     * Encrypt a secret.


fabpot · 2019-09-20T07:28:48Z

src/Symfony/Bundle/FrameworkBundle/Secret/Encoder/EncoderInterface.php

+    public function encrypt(string $secret): string;
+
+    /**
+     * Decrypt a secret.


fabpot · 2019-09-20T07:30:41Z

src/Symfony/Bundle/FrameworkBundle/Secret/Encoder/SodiumEncoder.php

+        $this->encryptionKey = null;
+
+        $encryptionKey = sodium_crypto_stream_keygen();
+        (new Filesystem())->dumpFile($this->encryptionKeyPath, $encryptionKey);


Not sure we want to depend on Filesystem

jderusse · 2019-10-16T10:26:26Z

closing in favor of #33997

@jderusse

…ecret:...)%` processor to deal with secrets seamlessly (Tobion, jderusse, nicolas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [FrameworkBundle] Add `secrets:*` commands and `%env(secret:...)%` processor to deal with secrets seamlessly | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | Fix #27351 | License | MIT | Doc PR | symfony/symfony-docs/pull/11396 This PR continues #31101, please see there for previous discussions. The attached patch has been fine-tuned on nicolas-grekas#33 with @jderusse. This PR is more opinionated and thus a lot simpler than #31101: only Sodium is supported to encrypt/decrypt (polyfill possible), and only local filesystem is available as a storage, with little to no extension point. That's on purpose: the goal here is to provide an experience, not software building blocks. In 5.1, this might be extended and might lead to a new component, but we'd first need reports from real-world needs. Having this straight-to-the-point in 4.4 will allow gathering these needs (if they exist) and will immediately provide a nice workflow for the need we do want to solve now: forwarding secrets from dev to prod using git in a secure way. The workflow this will allow is the following: - public/private key pairs are generated in the `config/secrets/%kernel.environment%/` folder using `bin/console secrets:generate-keys` - for the prod env, the corresponding private key should be deployed to the server using whatever means the hosting provider allows - this key MUST NOT be committed - the public key is used to encrypt secrets and thus *may* be committed in the git repository to allow anyone *that can commit* to add secrets - this is done using `bin/console secrets:set` DI configuration can reference secrets using `%env(secret:...)%` in e.g `services.yaml`. There is also `bin/console secrets:remove` and `bin/console debug:secrets` to complete the toolbox. In terms of design, vs #31101, this groups the dual "encoder" + "storage" concepts in a single "vault" one. That's part of what makes this PR simpler. That's all folks :) Commits ------- c4653e1 Restrict secrets management to sodium+filesystem 02b5d74 Add secrets management 8c8f623 Proof of concept for encrypted secrets

carsonbot added Status: Needs Review FrameworkBundle Feature labels Apr 13, 2019

jderusse force-pushed the encrypted-secrets-jd branch from f365e0b to d5ad8cd Compare April 13, 2019 17:31

fabpot reviewed Apr 13, 2019

View reviewed changes

kevin-verschaeve reviewed Apr 13, 2019

View reviewed changes

sstok suggested changes Apr 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Secret/FilesSecretStorage.php Outdated Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/Secret/FilesSecretStorage.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 14, 2019

onEXHovia reviewed Apr 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/SecretsListCommand.php Outdated Show resolved Hide resolved

jderusse force-pushed the encrypted-secrets-jd branch from f164b9b to a17c165 Compare April 14, 2019 09:23

Tobion reviewed Apr 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Secret/MutableSecretStorageInterface.php Outdated Show resolved Hide resolved

nicolas-grekas added this to the next milestone Apr 14, 2019

Tobion reviewed Apr 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Secret/SecretStorageInterface.php Outdated Show resolved Hide resolved

Tobion reviewed Apr 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Secret/SodiumEncoder.php Outdated Show resolved Hide resolved

Tobion reviewed Apr 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/SecretsAddCommand.php Outdated Show resolved Hide resolved

Tobion reviewed Apr 14, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Secret/MutableSecretStorageInterface.php Outdated Show resolved Hide resolved

jderusse force-pushed the encrypted-secrets-jd branch 3 times, most recently from 826fb3c to bef764e Compare April 14, 2019 20:10

chalasr reviewed Apr 14, 2019

View reviewed changes

noniagriconomie reviewed Apr 15, 2019

View reviewed changes

javiereguiluz reviewed Apr 15, 2019

View reviewed changes

jderusse force-pushed the encrypted-secrets-jd branch 4 times, most recently from 68d5fb9 to 29351bb Compare April 15, 2019 14:50

Tobion reviewed Apr 15, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Secret/FilesSecretStorage.php Outdated Show resolved Hide resolved

Tobion reviewed Apr 15, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Secret/FilesSecretStorage.php Outdated Show resolved Hide resolved

jderusse force-pushed the encrypted-secrets-jd branch from 09709e6 to 072b311 Compare July 20, 2019 09:12

jderusse changed the base branch from master to 4.4 July 20, 2019 09:12

jderusse force-pushed the encrypted-secrets-jd branch from 072b311 to 7ecd091 Compare July 20, 2019 09:26

jderusse force-pushed the encrypted-secrets-jd branch from 7ecd091 to 2083c18 Compare July 22, 2019 06:26

Proof of concept for encrypted secrets

7b1e2fc

jderusse force-pushed the encrypted-secrets-jd branch from 2083c18 to c2d8a52 Compare September 19, 2019 16:23

Add secrets management

afd2f0f

jderusse force-pushed the encrypted-secrets-jd branch from c2d8a52 to afd2f0f Compare September 19, 2019 23:57

fabpot requested changes Sep 20, 2019

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Reviewed labels Sep 20, 2019

nicolas-grekas mentioned this pull request Oct 16, 2019

[FrameworkBundle] Add secrets:* commands and %env(secret:...)% processor to deal with secrets seamlessly #33997

Merged

jderusse closed this Oct 16, 2019

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

jderusse deleted the encrypted-secrets-jd branch March 5, 2020 20:03

Uh oh!

[FrameworkBundle] Add secrets management. #31101

[FrameworkBundle] Add secrets management. #31101

Uh oh!

Conversation

jderusse commented Apr 13, 2019

Uh oh!

fabpot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sstok commented Apr 14, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

noniagriconomie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tobion commented Jul 19, 2019

Uh oh!

jderusse commented Jul 20, 2019

Uh oh!

ro0NL commented Jul 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jderusse commented Jul 22, 2019

Uh oh!

ro0NL commented Jul 21, 2019 •

edited

Loading