8000 [FrameworkBundle] Add `secrets:*` commands and `%env(secret:...)%` processor to deal with secrets seamlessly by nicolas-grekas · Pull Request #33997 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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

Jump to bottom
Merged
merged 3 commits into from
Oct 20, 2019
Merged

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

merged 3 commits into from
Oct 20, 2019

Conversation

nicolas-grekas
Copy link
Member
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 :)

@nicolas-grekas nicolas-grekas added this to the next milestone Oct 16, 2019
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 16, 2019
Closed
@jderusse jderusse mentioned this pull request Oct 16, 2019
Closed
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 16, 2019
Merged
stof
stof reviewed Oct 16, 2019
View reviewed changes
@stof
Copy link
Member
stof commented Oct 16, 2019

How does the management of the secret key would work on Saas hosting like Heroku or Symfony Cloud ? I don't think they let us inject some files on the filesystem.

stof
stof reviewed Oct 16, 2019
View reviewed changes
@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 16, 2019 •
edited
Loading

How does the management of the secret key would work on Saas hosting like Heroku or Symfony Cloud ? I don't think they let us inject some files on the filesystem.

All PaaS providers have a build and/or deploy step so this file could be dealt with there. For SFCloud at least there are many possible solutions. I think it's the same for all hosting providers.

Since the key file is a PHP file, it's also possible to dump e.g a reference to an env var if that's what one wants to do:
echo "<?php return base64_decode(\$_SERVER['SODIUM_DECRYPT_KEY']);" > prod.sodium.decrypt.key

@stof
Copy link
Member
stof commented Oct 16, 2019

Since the key file is a PHP file

hmm, this is what I missed. I thought it was the key file itself.

jderusse
jderusse reviewed Oct 16, 2019
View reviewed changes
Taluu
Taluu approved these changes Oct 16, 2019
View reviewed changes
jderusse
jderusse reviewed Oct 16, 2019
View reviewed changes
@javiereguiluz
Copy link
Member

I've been playing with this feature inside the Symfony Demo application. First, thanks Nicolas for working on this. I don't know if this could disappoint some who expected a more complex feature (not only filesystem-based, etc.) ... but to me it was perfect and it didn't fall short at anything I needed. I love it ❤️

Now, let's comment some details.

  1. The headers of this table could be "Name" + "Value" instead of "Name" + "Secret" because I think that "secret" means everything (name + value):
$ php bin/console debug:secrets --reveal
 ------------- ----------------------------------
  name          secret
 ------------- ----------------------------------
  ...           ...
 ------------- ----------------------------------
  1. Secret names are used as part of the PHP file that stores their encrypted values. That prevents using some useful characters for the secret name. For example: db/password, aws/config/root, etc. (names like these are used for example extensively in the Hashicorp Vault docs: https://learn.hashicorp.com/vault/getting-started/first-secret)

I guess it would be complicated to change this ... but just a comment in case we could do some effort.

  1. I tried to create app/secret secret, but I couldn't because of 2), so I created app-secret (with a dash, not an underscore). It worked great ... but when I used it in framework.yaml like this: '%env(secret:app-secret)%' I saw this error:
Invalid env(secret:app-secret) name: only "word" characters are allowed.
  1. I don't understand why can't we store null as the value of some secret. I think it's a super common scenario, moreover in non-prod environments. Right now it's not possible:
In SodiumVault.php line 61:

  Argument 2 passed to Symfony\Bundle\FrameworkBundle\Secrets\SodiumVault::seal() must be of the ty
  pe string, null given, called in /Users/javier/work/symfony-demo/vendor/symfony/framework-bundle/
  Command/SecretsSetCommand.php on line 66
  1. The text of the question in the secrets:set command is Please type the secret value 🤫: The emoji looks great in GitHub ... but in my console at least it looks like a big green circle. Moreover, at least on macOS, the OS already displays a special 🔑 key icon to tell the user that the input will be hidden:

secret-set

  1. After running secrets:generate-keys I only see this message:

image

Maybe we can improve the output to add more information about the successful message? E.g.: which files have been generated and in which file. Also, a basic info explaining what to do: commit the public key but not the private key, etc.

  1. When you run secrets:generate-keys a second time, the error message is:

image

We could improve it explaining that even keys already exist, you can rotate them with the --rotate command option.

  1. I guess it's not worth it but ... the name of the keys looks a bit long and not very "friendly". Do we need to add the env name in the key? And the word "sodium" too?

Before:

config/
  secrets/
    dev/
      dev.sodium.decrypt.key
      dev.sodium.encrypt.key

After:

config/
  secrets/
    dev/
      private.key
      public.key

Or:

config/
  secrets/
    dev/
      keys/
        private.key
        public.key

Same for the names of the encrypted values: do we need to include the env name? config/secrets/prod/prod.foo.sodium instead of config/secrets/prod/foo.sodium?

  1. How can I store a "complex" secret value that includes "\n"? For example, imagine that I want to set an aws-policy secret with this value:
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "Stmt1426528957000",
      "Effect": "Allow",
      "Action": ["ec2:*"],
      "Resource": ["*"]
    }
  ]
}

In Hashicorp Vault I can do this:

$ vault write aws/roles/my-role \
        credential_type=iam_user \
        policy_document=-<<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "Stmt1426528957000",
      "Effect": "Allow",
      "Action": [
        "ec2:*"
      ],
      "Resource": [
        "*"
      ]
    }
  ]
}
EOF

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 16, 2019 •
edited
Loading

Thanks for the feedback @javiereguiluz

The headers of this table could be "Name" + "Value"

updated

Secret names are used as part of the PHP file that stores their encrypted values

using more fancy chars is anyway incompatible with %env(secret:...)% references, which allow even fewer chars as you spotted, so I think we should't try to accept more chars here (but we could on env(), that's #33806).

I don't understand why can't we store null

You can store only strings. Store the empty one if you want to (using STDIN as input as the interactive mode doesn't allow empty strings. I don't think that's useful enough to fix.)
The type error is now fixed and replaced with an "aborting" message.

The text of the question in the secrets:set command is Please type the secret value 🤫:

emoji removed

After running secrets:generate-keys I only see this message:

you'll now also see:
image

Maybe we can improve the output to add more information about the successful message? E.g.: which files have been generated and in which file.

I'm not sure we can, because commands don't know anything about where keys are put (the vault is a black box to them.)

Also, a basic info explaining what to do: commit the public key but not the private key, etc.

maybe the above message is enough a hint? if you have a suggestion, I'm listening.

We could improve it explaining that even keys already exist, you can rotate them with the --rotate command option.

that's a dangerous hint as ppl may erase the existing keys by mistake. I'd better have them check the help (or read the doc) to discover how to get past this.

the name of the keys looks a bit long and not very "friendly". Do we need to add the env name in the key? And the word "sodium" too?

yes, we do: if for any reason a file is moved out of the directory where it is, then you'd suddenly lose all context to know what this is about. With these, you're a bit safer. Same for s 8000 ecrets' files. The word sodium provides some possible future extensibility.

How can I store a "complex" secret value that includes "\n"?

Addressed by adding an additional command-line argument which is a file where to read the secret from ("-" for STDIN).

@nicolas-grekas nicolas-grekas force-pushed the encrypted-secrets-jd branch 4 times, most recently from 73bcdf8 to 1c0fbb7 Compare October 16, 2019 15:46
@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Oct 16, 2019 •
edited
Loading

BTW: public/private are generic terms, encrypt/decrypt are more accurate because we do have more context about the use case.

@weaverryan
Copy link
Member

Hiya!

I agree with Javier - this is fun to use! After kicking the tires, here's my feedback FWIW

A) For local dev workflow, fallback to env vars

There's a workflow problem currently for local development. If the app requires 10 "secrets", does the user need to create their own dev secrets directory and manually add all 10? Or should the config/secrets/dev directory be committed so they have them all? If so, how does the dev override some value locally without having uncommitted changes?

I think the workflow should look like this:

  1. If using secrets, the entire config/secrets/dev directory SHOULD be committed (ok, they could choose to commit or not commit the actual "private key" depending on how sensitive the default/dev values are in the vault).

  2. Now, what if the dev needs to override the database_url secret locally? I think (well, it was Nicolas idea) that we could add a secrets:set database_url --local flag. This would create a dev.database_url.sodium.local file... and the system would know to look for and load these .local files. Then, a simple gitignore for config/secrets/dev/*.local could be used to ignore those overrides.

** B) Smaller Stuff**

    1. I was surprised when secrets:set made the keys for me. I'm not sure that's needed. Creating a key is a pretty important thing, so maybe we should throw an error and tell them what command to run.
    1. I know why... but i don't like that secrets:debug isn't there (it's debug:secrets). It means that when you look at all the secrets command when you run bin/console, you don't discover this command. If we decided this was a command that "listed" secrets instead of "debugging" them, we would have decided to call it secrets:list or secrets:show.
    1. [OK] New encryption keys have been generated. - can we tell them where they were generated exactly?
    1. I don't like the encrypy/decrypt in the names. I saw your message about why you did this, but it could be dangerous: it's not clear which is the secret file that should NOT be committed.
    1. // dev.foo.sodium on Wed, 16 Oct 2019 18:44:11 +0000 in the output - I like the date here, but what are we trying to communicate? Maybe that this contains the foo key and that it was dumped via the secrets:set command?
    1. // Secrets could not be revealed as not decryption key has been found - could we say where it looked for the decryption key?
    1. I see that dev and prod secrets will have two different directories. This makes sense. Maybe we could add some output to make this difference even more clear - e.g. "listing all secrets for the dev environment" or "setting value into the dev secrets".
    1. once I have keys, I can just regenerate new ones with the command... which replace the old ones and kill any existing keys. Could we add a "are you sure" confirmation and also educate them about the --rotate option?
    1. Minor: if you try to remove a secret before even your keys exist, you get:

Encryption key not found in "/Users/weaverryan/Sites/os/symfony-demo/config/secrets/dev/dev."

That path - dev/dev. looked weird to me.

    1. The filenames look like dev.foo.sodium. What's the purpose of the dev. prefix? And is the sodium suffix needed? I assume this is how we are determining the algo to use (for the future, when multiple might be used), but I wish it could be stored elsewhere - foo would be a nice filename :)

@jderusse
Copy link
Member

IMHO you should never commit the private key. If you don't care about the secret and want an easy way to share the key, then those are not secrets and you don't have to use the "secrets feature".
if each user have their own secret (--local flag) then you don't have to share the secrets and don't need the "secrets feature" too. User should use the standard env configuration for that

Symfony is flexible enough to use different ways to configure 2 environments.
Below are 3 ways to use secrets only in prod.

  1. using distinguished conf
# /config/packages/database.yaml
doctrine:
  dbal:
    url: '%env(DATABASE_URL)%'
    password: 'root'  # yes, you can provide both DSN (without password) and password attribute

# /config/packages/prod/database.yaml
doctrine:
  dbal:
    password: '%env(secret:DATABASE_PASSWORD)%'

# .env
DATABASE_URL=mysql://db_user@127.0.0.1:3306/db_name
  1. using parameter fallback
# /config/packages/database.yaml
doctrine:
  dbal:
    url: '%env(DATABASE_URL)%'
    password: '%env(default:empty_database_password:secret:DATABASE_PASSWORD)%'
parameters:
  empty_database_password: '%env(DATABASE_PASSWORD)%'
  1. using parameters : when the secret is inside a complexe string (like DSN) and you don't want to store the entiere DSN in the secret
# /config/packages/messenger.yaml
framework:
  messenger:
    transports:
      async: '%env(resolve:MESSENGER_TRANSPORT_DSN)%'
parameters:
  amqp_password: '%env(secret:AMQP_PASSWORD)'

# .env
MESSENGER_TRANSPORT_DSN=amqp://guest:guest@localhost:5672/%2f/messages
# .env.prod
MESSENGER_TRANSPORT_DSN=amqp://guest:%amqp_password%@localhost:5672/%2f/messages

note: I personnaly would loved to use resolve without intermediate parameters, but this has been rejected in #31000

# /config/packages/messenger.yaml
framework:
  messenger:
    transports:
      async: '%env(resolve:MESSENGER_TRANSPORT_DSN)%'
# .env
MESSENGER_TRANSPORT_DSN=amqp://guest:guest@localhost:5672/%2f/messages
# .env.prod
MESSENGER_TRANSPORT_DSN=amqp://guest:%env(secret:AMQP_PASSWORD)%@localhost:5672/%2f/messages

@nicolas-grekas nicolas-grekas force-pushed the encrypted-secrets-jd branch 2 times, most recently from b6f3784 to be8b267 Compare October 18, 2019 15:03
jderusse
jderusse reviewed Oct 18, 2019
8000
View reviewed changes
@nicolas-grekas nicolas-grekas force-pushed the encrypted-secrets-jd branch 2 times, most recently from 3846ce4 to 78af50f Compare October 18, 2019 15:22
weaverryan
weaverryan approved these changes Oct 18, 2019
View reviewed changes
Copy link
Member
@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

+1 from me! Excellent work

@nicolas-grekas
Copy link
Member Author

Now with two new commands:

  • secrets:decrypt-to-local
  • secrets:encrypt-from-local

and secrets:list sorting the secrets by name.

@fabpot
Copy link
Member
fabpot commented Oct 20, 2019

Thank you @nicolas-grekas.

fabpot added a commit that referenced this pull request Oct 20, 2019
@fabpot
feature #33997 [FrameworkBundle] Add secrets:* commands and `%env(s…
16d5285 
…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
@fabpot fabpot merged commit c4653e1 into symfony:4.4 Oct 20, 2019
@nicolas-grekas nicolas-grekas deleted the encrypted-secrets-jd branch October 21, 2019 07:22
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
This was referenced Nov 12, 2019
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@Taluu Taluu Taluu approved these changes

@stof stof stof left review comments

@jderusse jderusse jderusse approved these changes

@yceruto yceruto yceruto left review comments

@kevin-verschaeve kevin-verschaeve kevin-verschaeve left review comments

@chalasr chalasr chalasr left review comments

@fabpot fabpot fabpot approved these changes

@weaverryan weaverryan weaverryan approved these changes

Assignees
No one assigned
Labels
Feature FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.

RFC - envProcessor to decrypt secrets for simple applications
0