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

Update secrets to use modern crypto #28139

Merged
merged 1 commit into from Mar 2, 2017

Conversation

@stouset
Copy link
Contributor

stouset commented Feb 23, 2017

Fixes #28135 by replacing the default mode with AES-128-GCM, allowing the mode to be configured manually, correctly generating keys, and using a random initialization vector on every encryption.

railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt Outdated
@@ -17,6 +17,7 @@ Rails.application.configure do
# 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.secrets_cipher = "aes-128-gcm"

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 23, 2017 Member

I'm all to add a config for this but I don't think we should promote it by default.

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

I agree. I only erred on including it because existing users of the beta will need to know how to either switch back (bad), or migrate to GCM (better).

There's also the unfortunate case of machines with ancient OpenSSL < 1.0.1, which doesn't support GCM.

railties/lib/rails/application/configuration.rb Outdated
@@ -53,6 +53,7 @@ def initialize(*)
@x = Custom.new
@enable_dependency_loading = false
@read_encrypted_secrets = false
@secrets_cipher = "aes-128-gcm"

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 23, 2017 Member

I think we can remove this default here since we set it at bootstrap.

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

Is it better to leave it here and not set it in the bootstrap (to keep defaults all in the same place)?

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

(fixed, assuming your answer to the above question is a yes)

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

railties/test/secrets_test.rb Outdated
end_of_secrets

Rails.application.config.secrets_cipher = "aes-128-cbc"
Rails.application.instance_variable_set(:@secrets, nil) # Dance around caching 💃🕺

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 23, 2017 Member

lol for the emoji

railties/lib/rails/application/configuration.rb Outdated
@@ -14,7 +14,7 @@ class Configuration < ::Rails::Engine::Configuration
:ssl_options, :public_file_server,
:session_options, :time_zone, :reload_classes_only_on_change,
:beginning_of_week, :filter_redirect, :x, :enable_dependency_loading,
:read_encrypted_secrets
:read_encrypted_secrets, :secrets_cipher

This comment has been minimized.

@rafaelfranca

rafaelfranca Feb 23, 2017 Member

We need to add an explanation for this config at configuration.md in guides.

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

I'll add that in a moment.

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

Done. 798280f

@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 23, 2017

Thank you so much for the pull request.

railties/lib/rails/secrets.rb Outdated
@@ -30,20 +32,24 @@ def parse(paths, env:)
end

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

This comment has been minimized.

@matthewd

matthewd Feb 23, 2017 Member

This is random bytes, right?

If so, I think we should probably pack/unpack to hex, to make it safer for people to move around.

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

Encoding to a human-readable format should be the responsibility of an outer layer; internally, crypto libraries should only ever deal with bytes. It's too easy to get things wrong otherwise (see: this exact method getting key generation wrong due to mixing and matching hex-encoding and binary).

This comment has been minimized.

@matthewd

matthewd Feb 23, 2017 Member

ISTM this is the outer layer. This class is responsible for receiving the key from the user (3 lines down), so it's going to have to know how to convert it back to raw bytes.

This method is currently a simple pass-through to the cipher doing the real work, so it feels reasonable for this to hexify -- and for the key method below to de-hexify. That keeps said work well away from the "real stuff" down in encrypt/decrypt.

This comment has been minimized.

@stouset

stouset Feb 23, 2017 Author Contributor

Done.

29057f1

@stouset stouset force-pushed the stouset:update-secrets-to-use-modern-crypto branch 4 times, most recently Feb 23, 2017
railties/lib/rails/secrets.rb Outdated
cipher(:decrypt, data)
cipher = new_cipher.decrypt
cipher.key = key
cipher.iv = data[0, cipher.iv_len]

This comment has been minimized.

@ptoomey3

ptoomey3 Feb 23, 2017 Contributor

Should you be checking cipher.authenticated? here as well just in case anyone swaps out config.secrets_cipher?

EDIT: I meant to post this one line below.

This comment has been minimized.

@stouset stouset force-pushed the stouset:update-secrets-to-use-modern-crypto branch 2 times, most recently Feb 23, 2017
railties/lib/rails/secrets.rb Outdated

cipher.key = key
cipher.iv = data[0, cipher.iv_len]
cipher.auth_tag = data[cipher.iv_len, auth_tag_len]

This comment has been minimized.

@ptoomey3

ptoomey3 Feb 24, 2017 Contributor

I don't think you can even call the setter:

cipher = OpenSSL::Cipher.new("aes-128-cbc")
cipher.auth_tag = ""
OpenSSL::Cipher::CipherError: authentication tag not supported by this cipher

This comment has been minimized.

@stouset

stouset Feb 24, 2017 Author Contributor

sigh, you're again right. Sorry, haven't yet figured a way to test the file in isolation, and I'm muxing this with the work I actually get paid to do. :)

1fef784dd63598df121de2c9f70cca9556b88439

This comment has been minimized.

@matthewd

matthewd Feb 24, 2017 Member

haven't yet figured a way to test the file in isolation

Either of cd railties; bundle exec ruby -Itest test/secrets_test.rb or cd railties; bin/test test/secrets_test.rb should do it, if that helps.

@stouset stouset force-pushed the stouset:update-secrets-to-use-modern-crypto branch 2 times, most recently Feb 24, 2017
railties/lib/rails/secrets.rb Outdated
cipher.iv = data[0, cipher.iv_len]

cipher.auth_tag = data[cipher.iv_len, auth_tag_len] if
cipher.authenticated?

This comment has been minimized.

@bdewater

bdewater Feb 24, 2017 Contributor

This seems to miss a check for auth_tag byte length, see ruby/openssl#63

I've implemented this for MessageEncryptor here:

# Currently the OpenSSL bindings do not raise an error if auth_tag is
# truncated, which would allow an attacker to easily forge it. See
# https://github.com/ruby/openssl/issues/63
raise InvalidMessage if aead_mode? && auth_tag.bytes.length != 16

This comment has been minimized.

@stouset

stouset Feb 24, 2017 Author Contributor

The only possible way for this to produce fewer bytes of the auth_tag is if the remainder of the message is empty (and an empty message validates against whatever fragment of an auth_tag was provided).

Either way I think we can agree that the OpenSSL API for this is absolutely garbage. I'll put in a check for it as an extra precaution.

This comment has been minimized.

@stouset

stouset Feb 24, 2017 Author Contributor

Apparently Ruby 2.4 has a new auth_tag_len= method on Cipher, which potentially forces this check to happen internally to OpenSSL. What's the minimum Ruby version supported by Rails 5.1?

This comment has been minimized.

@bdewater

bdewater Feb 24, 2017 Contributor

It's Ruby 2.2.2, as of 2.4 the OpenSSL gem has been pulled out of the standard library and gemified as version 2.0. Unfortunately that requires at least Ruby 2.3.

While we're on the subject of OpenSSL API, according to the gem C source for ossl_cipher_set_auth_tag_len: "Note that not all AEAD ciphers support this method." 😒

railties/lib/rails/secrets.rb Outdated
@@ -12,10 +13,15 @@ def initialize
end
end

# OpenSSL::Cipher uses a hardcoded tag length of 16 bytes
AUTH_TAG_LEN = 16

This comment has been minimized.

@vipulnsward

vipulnsward Feb 24, 2017 Member

This constant is not available from openssl lib? If it is we should use that.

This comment has been minimized.

@stouset

stouset Feb 24, 2017 Author Contributor

It isn't. It's just hardcoded as 16 in the Ruby source.

@bdewater
Copy link
Contributor

bdewater commented Feb 24, 2017

Come to think of it, ActiveSupport is a dependency of Railties (unless I'm mistaken). Why wasn't MessageEncryptor used to implement this feature in the first place, or aren't we switching to it now? It already does everything (including AEAD since #25874) we're trying to fix here.

@stouset
Copy link
Contributor Author

stouset commented Feb 24, 2017

@bdewater Now you tell me. :(

I can rewrite this to use MessageEncryptor if you'd prefer.

@kaspth
Copy link
Member

kaspth commented Feb 24, 2017

@bdewater that was the plan, but with all the back and forth on the user experience changes I forgot to make the change.

guides/source/configuring.md Outdated
@@ -140,6 +140,8 @@ defaults to `:debug` for all environments. The available log levels are: `:debug

* `secrets.secret_key_base` is used for specifying a key which allows sessions for the application to be verified against a known secure key to prevent tampering. Applications get `secrets.secret_key_base` initialized to a random key present in `config/secrets.yml`.

* `config.secrets_cipher` allows you to change the cipher used to protect the `config/secrets.yml` file. This defaults to `"aes-128-gcm"`. Versions of OpenSSL earlier than 1.0.01, unfortunately, don't support GCM mode, so degrading this to `"aes-128-cbc"` may be necessary. If you find yourself in this situation, strongly consider updating OpenSSL to a modern version if at all possible rather than weakening this setting.

This comment has been minimized.

@kaspth

kaspth Feb 24, 2017 Member

Not correct. config/secrets.yml.enc isn't an encrypted version of config/secrets.yml. They're separate.

I'm okay with requiring a specific >= OpenSSL version since encrypted secrets is an optional feature.

This comment has been minimized.

@stouset

stouset Feb 24, 2017 Author Contributor

👍

secrets.yml.enc being a totally different thing than secrets.yml is very counterintuitive. Shouldn't things like the secret_key_base in there be something that goes into the encrypted version too?

Apologies if I'm a bit rusty — haven't used Rails extensively in about a year at this point.

This comment has been minimized.

@wlipa

wlipa Feb 25, 2017

Keeping both secrets.yml and secrets.yml.enc seems like a booby trap. It'd be easy to put a secret in the clear version and check it in.

This comment has been minimized.

@kaspth

kaspth Mar 1, 2017 Member

Shouldn't things like the secret_key_base in there be something that goes into the encrypted version too?

By default secrets.yml reads a SECRET_KEY_BASE from the ENV, but yes when opting in to encrypted secrets you'd move it stuff it in config/secrets.yml.enc.

This comment has been minimized.

@kaspth

kaspth Mar 1, 2017 Member

@wlipa perhaps, but I'm not sure how likely people would be to do that. E.g. the command to edit encrypted secrets is bin/rails secrets:edit so if you haven't run that, then it's not the right file 😊

This comment has been minimized.

@wlipa

wlipa Mar 1, 2017

That's reasonable. The naming is still confusing, because now secrets.yml is for things that are not actually secret.

This comment has been minimized.

@stouset

stouset Mar 1, 2017 Author Contributor

@matthewd If you have a file named secrets.yml, people are going to put production secrets into it. Documentation won't matter. Comments in the YAML file warning against it won't matter. <blink><marquee><font color="red"> won't matter.

Having that file is begging people to do the wrong thing. If it's supposed to be for development-only, have it be set in config/environments/development.rb.

This comment has been minimized.

@kaspth

kaspth Mar 1, 2017 Member

@stouset those who would just jam secrets into a file that sounds secret-y are not who we design Rails for. We trust that people will try to do the right thing and that they can handle conventions as well as adjusting to new conventions. Such as when we introduced secrets.yml in 4.1 and when we're now adding encrypted secrets alongside that.

This comment has been minimized.

@stouset

stouset Mar 1, 2017 Author Contributor

Speaking as an infosec person, I don't think that's a reasonable position to take.

API design matters. It doesn't matter how smart and capable your users are if your APIs guide users to make the wrong choice. Naming a file secrets.yml and putting things that look like secrets into them is sending people the wrong message, and users — smart, capable users — will misunderstand its purpose and make mistakes. In this case, security-impacting mistakes.

On the flip side, what's the actual cost of changing the design of this feature to guide people away from making mistakes? My guess is: not much.

Your argument reminds me of this image:

ityr0voz1qrpqdhkxraul0oh4juqljmnzq-kfjmwr8o

Bottles on the left are cleaning chemicals. Bottles on the right are juice. You're taking a gamble that the developers Rails caters to are never going to mistake the bottles on the left for the bottles on the right. I've seen from repeated experience that they will.

This comment has been minimized.

@jeremy

jeremy Mar 1, 2017 Member

I'd totally expect config/secrets.yml to be the decrypted config/secrets.enc, too… even knowing what's actually happening and why. 😊

The mixed heritage behind this feature is muddling things up. We have a mix of approaches like

  • config/secrets.yml is a gitignored file that's copied in as needed in dev and on production deploy
  • config/secrets.yml is a checked-in file that doesn't contain actual secrets, just ERB preprocessing that references environment variables with actual secrets
  • config/secrets.yml is a checked-in file that contains actual secrets, but only for dev/test.

And they're competing for mindshare and, literally, file naming.

In any case, this is way out of scope for this PR. Good topic for further investigation.

railties/lib/rails/application/bootstrap.rb Outdated
@@ -80,6 +80,7 @@ module Bootstrap
end

initializer :set_secrets_root, group: :all do
Rails::Secrets.cipher = config.secrets_cipher

This comment has been minimized.

@kaspth

kaspth Feb 24, 2017 Member

Let's dispense with the config and decide on the best encryption default.

@kaspth
Copy link
Member

kaspth commented Feb 24, 2017

Thanks so much for the pull request! Reporting security issues very early in our release phase means a lot ❤️

Given your suggestions I think most of this could be boiled down to:

gem "openssl", ">= 1.0.01" # Enforce OpenSSL that supports GCM authenticated ciphers.
require "openssl"

require "active_support/message_encryptor"

# Later…

def generate_key
  encryptor.random_key.unpack("H*").first # Requires adding `random_key` to MessageEncryptor.
end

def encrypt(text)
  encryptor.encrypt_and_sign(text)
end

def decrypt(data)
  encryptor.decrypt_and_verify(data)
end

private
  def encryptor
    @encryptor ||= ActiveSuport::MessageEncryptor.new(key, cipher: "aes-128-gcm") # If we pick an `authenticated?` cipher the encryptor handles `auth_tag` checks, iv seed etc.
  end
@bdewater
Copy link
Contributor

bdewater commented Feb 24, 2017

OpenSSL (not the gem, the system library) 1.0.1 was released in March 2012 and support for it ended at the end of last year. Besides AES-GCM it also brought support for TLSv1.1 and 1.2. I think it's a fair assumption that somebody's server already has this or a newer version installed for modern internet crypto.

@morgoth morgoth mentioned this pull request Feb 24, 2017
3 of 3 tasks complete
@stouset
Copy link
Contributor Author

stouset commented Feb 24, 2017

@bdewater You'd be surprised.

$ /usr/sbin/system_profiler SPSoftwareDataType | grep "System Version"
      System Version: macOS 10.12.3 (16D32)
$ /usr/bin/openssl version                                  
OpenSSL 0.9.8zh 14 Jan 2016
$ /usr/bin/openssl ciphers | grep -I GCM

Obviously users can install newer OpenSSL versions with homebrew. Just pointing out that older OpenSSL can be endemic, and at the very least this does require users to install a new third-party dependency on their local machines in order to manipulate the config/secrets.yml.enc. Happy to remove the configuration though.

@bdewater
Copy link
Contributor

bdewater commented Feb 24, 2017

Surprised indeed! I was expecting something like RHEL 6 lagging behind, but that's on 1.0.1 too.

@jeremy
Copy link
Member

jeremy commented Feb 24, 2017

(Note that OpenSSL has been deprecated on macOS since 10.7, though. Ruby builders/installers all provide their own or link to Homebrew.)

@kaspth
Copy link
Member

kaspth commented Mar 1, 2017

@stouset interested in trying out #28139 (comment)?

If you'd rather pass this on, I'd happy to take it over from here. You'd get full credit in every commit I make 😊

@stouset
Copy link
Contributor Author

stouset commented Mar 1, 2017

Yep, apologies. Happy to take this over the finish line.

@stouset stouset force-pushed the stouset:update-secrets-to-use-modern-crypto branch 2 times, most recently Mar 1, 2017
@stouset
Copy link
Contributor Author

stouset commented Mar 1, 2017

Ok, this is done and tests pass.

That said, default for MessageEncryptor is still AES-256-CBC. I'm not sure the level of pain involved in migrating, but AES-128-GCM should be the default going forward. 256-bit AES is overkill and incurs a ~40% performance penalty, plus GCM itself is significantly faster (and less prone to a mistake) than a custom two-pass cipher+hmac.

Interestingly, SHA-1 as a digest for non-authenticated ciphers in MessageEncryptor isn't actually of concern, since it's used in an HMAC which only requires the underlying compression function to be weakly collision resistant. But it's probably totally possible to rip out the non-AEAD code entirely if we require Ruby 2.2+, which depends on OpenSSL >= 1.0.1 (and thus has access to GCM).

railties/lib/rails/secrets.rb Outdated
cipher = new_cipher
SecureRandom.hex(cipher.key_len)[0, cipher.key_len]
cipher = OpenSSL::Cipher.new(CIPHER)
key = SecureRandom.random_bytes(cipher.key_len)

This comment has been minimized.

@bdewater

bdewater Mar 1, 2017 Contributor

No need to unpack with SecureRandom.hex(cipher.key_len)

This comment has been minimized.

@stouset

stouset Mar 1, 2017 Author Contributor

Good catch

@stouset
Copy link
Contributor Author

stouset commented Mar 1, 2017

@rafaelfranca Any further comments? This is good to go otherwise.

Copy link
Member

rafaelfranca left a comment

LGTM. @kaspth @matthewd anything else?

railties/lib/rails/secrets.rb Outdated
cipher = OpenSSL::Cipher.new(CIPHER)
key = SecureRandom.random_bytes(cipher.key_len)

key.unpack('H*').first

This comment has been minimized.

@rafaelfranca

rafaelfranca Mar 2, 2017 Member

Just a small code style thing, this string should be double-quoted, but don't bother changing only this, I can do it.

This comment has been minimized.

@stouset

stouset Mar 2, 2017 Author Contributor

Was already done. :)

Edit: whoops, apparently the change hadn't been pushed. It went a way as a result of using SecureRandom#hex.

@stouset stouset force-pushed the stouset:update-secrets-to-use-modern-crypto branch to 6aa6f9a Mar 2, 2017
@jeremy
jeremy approved these changes Mar 2, 2017
@simi
simi approved these changes Mar 2, 2017
@kaspth kaspth merged commit 0203c37 into rails:master Mar 2, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth
Copy link
Member

kaspth commented Mar 2, 2017

@stouset thanks so much for the help on this! It wasn't my intention to have encrypted secrets use sub par encryption, but I goofed on that. So it's very nice of you to jump in and help fix that mistake! ❤️

@kaspth
Copy link
Member

kaspth commented Mar 2, 2017

As to the usability concerns brought up in #28139 (comment), you're right! I take what I said there back and we should do something about it. Currently we're debating fixing this through better names for the files.

That was the meat of your argument, right? That knowing secrets.yml isn't the same as secrets.yml.enc is tough for users?

@wlipa
Copy link

wlipa commented Mar 2, 2017

Consider having something like a secrets folder with a top level secrets file and then a folder for each environment. This will address the issue of different security needs for different envs.

At each level, you can either have secrets.yml (plaintext), or secrets.yml.enc (encrypted), but it would be an error to have both. That way, once an approach for the given env is chosen, it will be natural to keep the same method, which will reduce accidental check-in errors.

secrets/
  secrets.yml [or]
  secrets.yml.enc
  development/
    secrets.yml [or]
    secrets.yml.enc

The Rails.secrets would be the union of the top level secrets and the env specific secrets.

@stouset stouset deleted the stouset:update-secrets-to-use-modern-crypto branch Mar 2, 2017
@stouset
Copy link
Contributor Author

stouset commented Mar 2, 2017

@kaspth No worries. Even I got it wrong a few times, and (in theory) I should know what I'm doing.

Yes, that's the meat of my argument. I don't necessarily have any overwhelmingly-good ideas — designing something is harder than tearing it down ;) and I've been out of the Rails ecosystem for a bit so I don't have a good intuition for what would be idiomatic here any more.

That can definitely be another ticket, though. Having a file named secrets that has secret-looking things in it is going to result in people putting real secrets there (speaking from experience here, I've seen this happen at previous companies).

kaspth added a commit that referenced this pull request Mar 2, 2017
Includes a script to ease an app's upgrade.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants
You can’t perform that action at this time.