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

Add encrypted secrets #28038

Merged
merged 41 commits into from Feb 23, 2017
Merged

Add encrypted secrets #28038

merged 41 commits into from Feb 23, 2017

Conversation

@kaspth
Copy link
Member

kaspth commented Feb 16, 2017

Fixes #25095

Adds config/secrets.yml.enc using Sekrets to allow storing production app secrets right in source control. When running bin/rails secrets:edit the file is decrypted and opened in a tmp file that then gets destroyed after the file is encrypted again and written back to its original location.

Differs sligthly in the proposed first-call experience in that we generate a key instead of asking for it. Also supports storing the key in config/secrets.yml.key instead of always going the RAILS_MASTER_KEY.

Pending:

  • Tests.
  • Restricting rails s boot when no decryption key is present.
  • Merging encrypted into the standard secrets after decryption.
@kaspth kaspth added the railties label Feb 16, 2017
@kaspth kaspth added this to the 5.1.0 milestone Feb 16, 2017
@kaspth kaspth self-assigned this Feb 16, 2017
@kaspth kaspth requested a review from dhh Feb 16, 2017
railties/lib/rails/commands/secrets/edit_command.rb Outdated
require "active_support/core_ext/string/strip"
require "rails/generators/rails/app/app_generator"

require "rails/engine"

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 16, 2017

Author Member

Sekrets depends on Rails::Engine out of the box.

@kaspth
Copy link
Member Author

kaspth commented Feb 16, 2017

Later I'd like to inline just the parts of Sekrets we need, since it seems to a lot more than we require as well as pull in a number of dependencies (nothing wrong with that, but it's best Rails keeps slim).

@ahoward how would you feel about that? We'd inline the parts we're using into it's own private namespace and give you credit with: # Adapted from Ara T. Howard's Sekrets gem..

Also: as a heads up, it seems Sekrets isn't distributed with a license :)

@kaspth
Copy link
Member Author

kaspth commented Feb 16, 2017

Dang, I've got trouble getting https://github.com/kaspth/rails/blob/226db6a15bc5ce5aca1854d7d2a00ab221795f86/railties/lib/rails/application.rb#L388 to recognize config/secrets.yml.enc. config.paths don't seem to be too willing to cooperate, I imagine I need to add a glob to it somehow.

Copy link
Member

maclover7 left a comment

Overall makes sense to me, nice work @kaspth. Added a few grammar comments.

Assuming we should add some documentation about this new feature?

Re config.paths, I think we just need to change this line.

railties/lib/rails/commands/secrets/USAGE Outdated
`RAILS_MASTER_KEY` environment variable, with the former taking precedence.

By default Rails adds `config/secrets.yml.key` and `config/secrets.yml.enc` to
`.gitignore`. If you don't use Git add these to the respective version control

This comment has been minimized.

Copy link
@maclover7

maclover7 Feb 16, 2017

Member

use Git, add

railties/lib/rails/commands/secrets/USAGE Outdated
Rails looks for the decryption key in config/secrets.yml.key as well as the
`RAILS_MASTER_KEY` environment variable, with the former taking precedence.

By default Rails adds `config/secrets.yml.key` and `config/secrets.yml.enc` to

This comment has been minimized.

Copy link
@maclover7

maclover7 Feb 16, 2017

Member

default, Rails

railties/lib/rails/commands/secrets/USAGE Outdated
to edit production secrets.

When the temporary file is next saved the contents are encrypted and written to
config/secrets.yml.enc while the file itself is destroyed to protect secrets

This comment has been minimized.

Copy link
@maclover7

maclover7 Feb 16, 2017

Member

secrets from leaking.

@dhh
Copy link
Member

dhh commented Feb 17, 2017

I don't understand why config/secrets.yml.enc would be gitignored? Isn't the whole point to check the encrypted secrets into the repo, but keeping the key out of the repo? So think we should remove that file from gitignore, leaving only the key file there.

I'm also hesitant to generating this key upfront. It's pretty easy for someone then starting a new project to use this feature but forget to do something about the key file, like commit it to 1Password. If they forget that, then change computers, the encrypted data is inaccessible. IMO, better that there's a kick-off for the first time you use encrypted secrets that explains that we're generating the key, putting it there, and you should store it securely somewhere outside of the repo.

Otherwise I'm liking the simplicity of this a lot!

@dhh
Copy link
Member

dhh commented Feb 17, 2017

@jeremy Could you review as well?

@dhh
Copy link
Member

dhh commented Feb 17, 2017

So I'd add bin/rails secrets:new or something like that for the initialization. Could also just combine the two steps and have bin/rails secrets, which would initialize if secrets.yml.enc is missing, and otherwise go to the edit session.

@dhh dhh requested a review from jeremy Feb 17, 2017
@kaspth
Copy link
Member Author

kaspth commented Feb 18, 2017

I don't understand why config/secrets.yml.enc would be gitignored?

It shouldn't! That was me being a dummy, thanks 👍

Gemfile.lock Outdated
fattr (>= 2.2.1)
highline (>= 1.6.15)
main (>= 5.1.1)
map (>= 6.3.0)

This comment has been minimized.

Copy link
@matthewd

matthewd Feb 18, 2017

Member

That's a lot of deps 😐

This comment has been minimized.

Copy link
@simi

simi Feb 18, 2017

Contributor

I was thinking the same.

I'm not sure if this is really worth it to integrate since you can do almost all of this as described in https://github.com/ahoward/sekrets/blob/master/README when you need this feature. I think the only difference is key generation (where Sekrets will ask you for key). That can be solved in Sekrets and I think it will be enough to update guides to recommend this approach for storing secret data within project.

But maybe I'm missing something.

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 18, 2017

Author Member

Agreed: #28038 (comment) 😊

@@ -80,7 +80,7 @@ def paths
@paths ||= begin
paths = super
paths.add "config/database", with: "config/database.yml"
paths.add "config/secrets", with: "config/secrets.yml"
paths.add "config/secrets", with: "config", glob: "secrets.yml{,.enc}"

This comment has been minimized.

Copy link
@kaspth

kaspth Feb 18, 2017

Author Member

@maclover7 thanks for the tip!

@kaspth
Copy link
Member Author

kaspth commented Feb 18, 2017

We could potentially try to remove the need for the config/secrets.yml.enc file entirely and just enable encryption on string keys in config/secrets.yml.

So when reading config/secrets.yml we traverse it and any string values starting with a signature ala this we decrypt. Then on write we just encrypt every string value (and prefix the signature).

@kaspth kaspth force-pushed the kaspth:encrypted-secrets branch Feb 18, 2017
kaspth added 13 commits Feb 15, 2017
Adds support for atom and other editors with open commands that
return immediately.
Couldn't be called without moving to the generator. Prefer duplication.
Adds a more pleasant first boot experience where the key and file
plus ignoring them is set up if they're not present.
* Add support for immediately exiting editors such as atom
  (though done crudely with Time.now), while keeping vim etc. happy.
* Prefill content if the file has none (in time, I'd like to inline sekrets
  with proper attribution and making it easier to do this prefill before the editing).
* Add puts statement to guide clue the user in to what's happening.
Helps parsing yml files and wraps Sekrets to make it easier to remove later.
The whole point of this exercise is to add it to version control! 😇
Removes sekrets as a dependency but leaves an attribution in its place.
@dhh dhh merged commit 1166094 into rails:master Feb 23, 2017
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
codeclimate no new or fixed issues
Details
@kaspth kaspth deleted the kaspth:encrypted-secrets branch Feb 23, 2017
@simi
Copy link
Contributor

simi commented Feb 23, 2017

Will be nice to add this to CHANGELOG also.

@drnic
Copy link
Contributor

drnic commented Feb 24, 2017

Is production.secret_key_base value relevant if new encrypted secrets is enabled? Is it the same or diff to RAILS_MASTER_KEY?

If we run rake secrets:setup can we replace/delete config/secrets.yml to avoid ppl using it?

@drnic
Copy link
Contributor

drnic commented Feb 24, 2017


def generate_key
cipher = new_cipher
SecureRandom.hex(cipher.key_len)[0, cipher.key_len]

This comment has been minimized.

Copy link
@emboss

emboss Feb 24, 2017

This is limiting the key's entropy. The output of

SecureRandom.hex(n).size # => 2n

is twice as long because n is telling SecureRandom "give me an output that is equivalent to n random bytes". Hex encoding encodes every byte as two characters, so the resulting hex string would be twice as long.

For the default AES-256 n = 32, and by cutting the 64 hex bytes down to 32 again, the entropy in that value is now only equivalent to 16 random bytes anymore. This makes brute forcing the key much easier: Instead of 2^256 now there are only 2^128 possible keys.

I would suggest simply using the full output of SecureRandom.hex(cipher.key_len) and decoding the resulting 64 hex bytes when the key is read from the file, yielding the 32-byte binary key.

This comment has been minimized.

Copy link
@nickmalcolm

nickmalcolm May 3, 2017

Contributor

Looks like this has been addressed: #28139

end

def new_cipher
OpenSSL::Cipher.new("aes-256-cbc")

This comment has been minimized.

Copy link
@emboss

emboss Feb 24, 2017

It would be preferable to use an AEAD cipher mode like AES-GCM that also guarantees the integrity of the resulting ciphertext. As a fallback when the underlying OpenSSL version does not support GCM yet, "Encrypt-then-Mac" could be used. ActiveSupport::MessageEncryptor already supports this with its #encrypt_and_sign method - would it be possible to reuse it in this context?

Let me know what you think, I'd be happy to help.

This comment has been minimized.

Copy link
@morgoth

morgoth Feb 24, 2017

Member

@emboss you might be interested in #28139

This comment has been minimized.

Copy link
@emboss

emboss Feb 24, 2017

@morgoth OK, it's already being discussed, that's great, thank you for the hint!

@coderanger
Copy link

coderanger commented Feb 26, 2017

Sorry to sealion in here but someone referred me to this PR from Twitter and it seems like you've set things up to use only a single key. This will likely make key rotations in production environments either very tricky or impossible. I would ask if you have the time before the feature goes live to make it possible to keep multiple copies of encrypted settings such that new code and new keys don't have to be updated in perfect lockstep.

Also I would recommend warning people about the dangers of storing the key in an environment variable. It does make life easier for PaaS users but caveat emptor. Tools like Airbrake and Sentry will need to be updated ASAP to not automatically log the environment variable (and configuration for each should be used in the interim). Also storing a key in an environment variable means it is passed to all subprocesses of Rails, which can be an issue for things like ImageMagick for thumbnail generation.

@schneems
Copy link
Member

schneems commented Feb 26, 2017

Key rotation would be amazing. We don't have that for secret_key_base now do we?

@schneems
Copy link
Member

schneems commented Feb 26, 2017

Also rollbar and sentry AFAIK use env var whitelisting so they won't need to be updated. If airbrake does something different then it has bigger problems than this PR.

@coderanger
Copy link

coderanger commented Feb 26, 2017

Probably the best that can be done for rotation for the moment is to rework the filename structure so rather than a single secrets.yml.enc make room for multiple files. Once authenticated encryption of one type or another gets added, the system can then be improved to try all of them in order until one works. But having the folder structure in place now will make the transition easier even if the rotation system is added after initial release.

@ahoward
Copy link
Contributor

ahoward commented Mar 1, 2017

@coderanger - one of the main uses we've found for app specific secrets is keeping things like credit cards, private certs, etc, stored in a repo. aka, not just *.yml based config. this led to the development of an 'Sekrets.load(path)' api. from there it is trivial to support Sekrets.load("#{ Rails.env }.yml") etc. so totally agree.

end

def path
@root.join("config", "secrets.yml.enc").to_s

This comment has been minimized.

Copy link
@morgoth

morgoth Mar 21, 2017

Member

@kaspth Do you think this could be somehow configurable? I can imagine having few encrypted files based on environment

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 21, 2017

Author Member

I'm open to hearing that argument. But let's revisit that with a written up use case once 5.1 has been out for some time proper 😊

This comment has been minimized.

Copy link
@januszm

januszm May 23, 2017

👍 for making it configurable. Actually I would not use this feature at all until it's possible to set different encrypted secretes for production, development or staging (or any other) environment.

This comment has been minimized.

Copy link
@ce07c3

ce07c3 Jun 7, 2017

I cannot use this feature until there is a configureable option for the paths.

This comment has been minimized.

Copy link
@kaspth

kaspth Jun 7, 2017

Author Member

I'm open to hearing that argument. But let's revisit that with a written up use case

@ce07c3 ^ all yours. Volunteer the write up if you're in a hurry 😊

This comment has been minimized.

Copy link
@ce07c3

ce07c3 Jun 7, 2017

@kaspth I have messy but working code now. I named my files secrets.yml.enc.staging|production|..., which clashed with if path.end_with?(".enc") (took a while to figure that out), but now I am wondering: are there identical by value but different sets of paths used? I did an override on path and key_path inside Rails::Secrets class << self, but those aren't called until later. Any idea how I am supposed to wire custom paths up? Also, how is @read_encrypted_secrets set?

This comment has been minimized.

Copy link
@ce07c3

ce07c3 Jun 7, 2017

@kaspth Also, how is @read_encrypted_secrets set? figured that out :) I was a bit tired. The rest I am still wondering about, perhaps I just need to dig a bit deeper.

This comment has been minimized.

Copy link
@morgoth

morgoth Jun 21, 2017

Member

@ce07c3 Are you still working on making it configurable? Can I help you somehow?

This comment has been minimized.

Copy link
@RyanSnodgrass

RyanSnodgrass Sep 22, 2017

I don't know if anyone will hear this, but I would also like to get a different master key per environment.

# Attempt to read encrypted secrets from `config/secrets.yml.enc`.
# Requires an encryption key in `ENV["RAILS_MASTER_KEY"]` or
# `config/secrets.yml.key`.
config.read_encrypted_secrets = true

This comment has been minimized.

Copy link
@claudiob

claudiob Mar 22, 2017

Member

Hello @kaspth 😄

The USAGE file above says:

A shared: top level key is also supported such that any keys there is merged into the other environments.

This leads me to believe that any "shared" secret key will automatically be available in development.

However this is not true: encrypted secrets are currently only read in production.

How do you feel about changing this and reading encrypted secrets in every environment?

If you think about it, config/secrets.yml.enc is already organized by environment (just like config/secrets.yml), so even if you read it in development, you will not be importing the keys under production:, only those under shared: and development:.

This comment has been minimized.

Copy link
@kaspth

kaspth Mar 25, 2017

Author Member

We skirted away from silently reading them in development just because users have a key and it's not our recommendation to use encrypted secrets as dev secrets storage.

This comment has been minimized.

Copy link
@joemsak

joemsak Apr 14, 2017

@kaspth Could you help me understand better what you recommend for development environment? I feel like my "development" keys are still just as worthy of keeping secret and using in the same way as production, no?

I tried finding something in edge guides or blog posts about upcoming rails 5.1 updates, and the PRs/issues around encrypted keys, but I don't find any advice on how to handle secret keys in my test / development environments, as it pertains to this new feature. In the past, I have been using Dotenv.

Thanks

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 14, 2017

Author Member

At the moment we don't recommend using encrypted secrets in dev/test.

This comment has been minimized.

Copy link
@joemsak

joemsak Apr 14, 2017

Right, I got that. So, what is recommended? I need clarity on that.

Keep using Dotenv, or?

This comment has been minimized.

Copy link
@kaspth

kaspth Apr 14, 2017

Author Member

Whatever works for you 😊

This comment has been minimized.

Copy link
@januszm

januszm May 23, 2017

I'd say that the recommended way is to use the same method for storing/accessing secrets in either production or dev environment.

@claudiob
Copy link
Member

claudiob commented Mar 22, 2017

@kaspth @dhh

Once I have enabled encrypted secrets, I always need to open the editor to add a new key:

$ rails secrets:edit
[… add a new key in the editor, save and exit …]
# New secrets encrypted and saved.

How do you feel about a new feature to programmatically add encrypted secrets without needing an editor?

$ rails secrets:add API_KEY=123
# New secret API_KEY encrypted and saved.

The concept is very similar to the way in which Travis encrypts environment variables.

@dhh
Copy link
Member

dhh commented Mar 22, 2017

@ce07c3
Copy link

ce07c3 commented Jun 21, 2017

@unikitty37
Copy link

unikitty37 commented Jul 18, 2017

Just a small point, but could somebody add a few lines to the USAGE file (and, preferably, the Rails 5.1 release notes as that's what comes up when you search the web) detailing how you actually get the secrets OUT again?

The release notes say

Secrets will be decrypted in production, using a key stored either in the RAILS_MASTER_KEY environment variable, or in a key file.

but they forget to mention where they're decrypted TO, and I suspect most people wanting to use this feature won't be too keen on having to spelunk through the 17 changed files on this PR…

@ce07c3
Copy link

ce07c3 commented Jul 18, 2017

@kaspth
Copy link
Member Author

kaspth commented Jul 18, 2017

Please do investigate, then!

@ce07c3
Copy link

ce07c3 commented Jul 18, 2017

@kaspth
Copy link
Member Author

kaspth commented Jul 18, 2017

It's an encrypted yaml file. Hence it's also yaml when it's decrypted, not just for "editing purposes".

@vizcay
Copy link
Contributor

vizcay commented Aug 3, 2017

I've just updated to Rails 5.1, moved all my keys to config/secrets.key.enc, updated .profile exporting RAILS_MASTER_KEY and then my deployment process started to fail because capistrano didn't have access to this variable.. I fixed it by adding set :default_env, { RAILS_MASTER_KEY: IO.read('config/secrets.yml.key') } to my config/deploy.rb file.. maybe someone finds this useful.

@ce07c3
Copy link

ce07c3 commented Aug 3, 2017

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.