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

Default Message Encryptor Cipher to AES-256-GCM From AES-256-CBC #29263

Merged
merged 1 commit into from Jun 11, 2017

Conversation

Projects
None yet
4 participants
@assain
Copy link
Contributor

assain commented May 29, 2017

@kaspth
Currently ActiveSupport::MessageEncryptor defaults to CBC encryption, but it could stand to benefit from defaulting to aes-256-gcm encryption for the same reasons as cookies.

Gemfile.lock Outdated
@@ -433,4 +433,4 @@ DEPENDENCIES
websocket-client-simple!

BUNDLED WITH
1.14.6
1.15.0

This comment has been minimized.

@kaspth

kaspth May 29, 2017

Member

Please remove these Gemfile.lock changes. See Eileen's contributing to Rails workshop around amending and rebasing for some help on how: https://youtu.be/7zoD6NZy6vY?t=1h25s

This comment has been minimized.

@assain

assain May 29, 2017

Author Contributor

Thank you for the feedback @kaspth, I'll fix it and add another commit 👍

This comment has been minimized.

@kaspth

kaspth May 29, 2017

Member

Don't add another commit. Just rebase/amend this one :)

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented May 29, 2017

For some reason Travis didn't run your pull request. Try the rebase/amend then force push.

activesupport/test/message_encryptor_test.rb Outdated
@@ -119,7 +119,7 @@ def assert_not_decrypted(value)
end

def assert_not_verified(value)
assert_raise(ActiveSupport::MessageVerifier::InvalidSignature) do
assert_raise(ActiveSupport::MessageEncryptor::InvalidMessage) do

This comment has been minimized.

@kaspth

kaspth Jun 3, 2017

Member

Why is this change needed?

This comment has been minimized.

@assain

assain Jun 3, 2017

Author Contributor

Previously the setup method used aes-256-cbc cipher while writing this test and had made use of Message Verifier. However, after playing around with the tests, I realized this test was failing because ActiveSupport::MessageVerifier::InvalidSignature was raised by message verifier, and the new default cipher - aes-256-gcm used NullVerifier.

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jun 3, 2017

Okay, now we'll need a similar setting to what the AEAD cookies has here: https://github.com/rails/rails/pull/28132/files#diff-7f4141d796a8aad409136b2b6dc774b0R15

E.g. that there's a new 5.2 config to default the encryptor to gcm:

# Other configs in new_framework_defaults_5_2.rb…

# Default encrypted messages to use AES-256-GCM encryption.
# Rails.application.active_support.message_encryptor.use_authenticated_encryption = true

Then auto default that to true in load_defaults under "5.2".

If we don't have that config, we break backwardscompatibility — that's what the test failures show.

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt Outdated
@@ -13,3 +13,6 @@
# Use AES 256 GCM authenticated encryption for encrypted cookies.
# Existing cookies will be converted on read then written with the new scheme.
# Rails.application.config.action_dispatch.use_authenticated_cookie_encryption = true

# Use AES 256 GCM authenticated encryption as default for encrypting messages.
# Rails.application.config.active_support.message_encryptor.use_authenticated_cookie_encryption = true

This comment has been minimized.

@kaspth

kaspth Jun 6, 2017

Member

Shouldn't say use_authenticated_cookie_encryption but just use_authenticated_encryption 😊

This comment has been minimized.

@assain

assain Jun 6, 2017

Author Contributor

I had corrected that mistake, don't know how it crept in again. Apologies! :)

@@ -13,3 +13,6 @@
# Use AES 256 GCM authenticated encryption for encrypted cookies.
# Existing cookies will be converted on read then written with the new scheme.
# Rails.application.config.action_dispatch.use_authenticated_cookie_encryption = true

This comment has been minimized.

@kaspth

kaspth Jun 6, 2017

Member

Ideally, we should be able to remove this config and use the below one to infer it. But that's for another time.

cc @mikeycgto

activesupport/lib/active_support/message_encryptor.rb Outdated
# bits. If you are using a user-entered secret, you can generate a suitable
# key by using <tt>ActiveSupport::KeyGenerator</tt> or a similar key
# derivation function.
# derivation function

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

Keep the period.

activesupport/lib/active_support/message_encryptor.rb Outdated
@@ -66,7 +77,7 @@ def initialize(secret, *signature_key_or_options)
sign_secret = signature_key_or_options.first
@secret = secret
@sign_secret = sign_secret
@cipher = options[:cipher] || DEFAULT_CIPHER
@cipher = options[:cipher] || MessageEncryptor.default_cipher

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

self.class.default_cipher

activesupport/lib/active_support/message_encryptor.rb Outdated
@@ -19,7 +19,18 @@ module ActiveSupport
# encrypted_data = crypt.encrypt_and_sign('my secret data') # => "NlFBTTMwOUV5UlA1QlNEN2xkY2d6eThYWWh..."
# crypt.decrypt_and_verify(encrypted_data) # => "my secret data"
class MessageEncryptor
DEFAULT_CIPHER = "aes-256-cbc"
class << self
#use_authenticated_message_encryption is used to activate 'aes-256-gcm' as default cipher

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

This comment doesn't really add anything to me. Let's just document it elsewhere.

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt Outdated
@@ -13,3 +13,6 @@
# Use AES 256 GCM authenticated encryption for encrypted cookies.
# Existing cookies will be converted on read then written with the new scheme.
# Rails.application.config.action_dispatch.use_authenticated_cookie_encryption = true

# Use AES 256 GCM authenticated encryption as default for encrypting messages.

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

as default for -> when

activesupport/lib/active_support/railtie.rb Outdated
ActiveSupport::MessageEncryptor.use_authenticated_message_encryption = true
else
ActiveSupport::MessageEncryptor.use_authenticated_message_encryption = false
end

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

This all needs to be wrapped in an initializer (with a descriptive name) so it's run when the app boots and not when this file is parsed.

This Railtie subclass is marked as # :nodoc meaning it won't show up in documentation, so just skip the comments here.

I'd rewrite to this within the initializer:

if config.active_support.respond_to?(:use_authenticated_message_encryption)
  ActiveSupport::MessageEncryptor.use_authenticated_message_encryption =
    config.active_support.use_authenticated_message_encryption
end

We have to use respond_to? because config.active_support, an instance of ActiveSupport::OrderedOptions, raises if the key is unassigned.

@kaspth

This comment has been minimized.

Copy link
Member

kaspth commented Jun 11, 2017

Woah, hey you got the tests to pass! 👏 — make sure you look at Code Climate too, there's one thing missing there. 😊

Now we just need to test that the default change works. Let's just go with setting use_authenticated_message_encryption in the Message Encryptor test and inspecting default_cipher for now.

Also: default_cipher should be marked as def default_cipher # :nodoc:, don't think that should be public API for people.

activesupport/lib/active_support/railtie.rb Outdated
@@ -7,6 +7,13 @@ class Railtie < Rails::Railtie # :nodoc:

config.eager_load_namespaces << ActiveSupport

initializer "active_support.set_authenticated_message_encryption_as_default" do |app|

This comment has been minimized.

@assain

assain Jun 11, 2017

Author Contributor

@kaspth
Whenever, I make this change, Travis CI fails, because of sessions_test.rb which outputs:

  1. Failure:
    ApplicationTests::MiddlewareSessionTest#test_session_upgrading_from_AES-CBC-HMAC_encryption_to_AES-GCM_encryption [test/application/middleware/session_test.rb:351]:
    Expected: "1"
    Actual: ""

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

Yep, see the other comment about why 😊

Let's just clip "_as_default" from the initializer title while we're here.

@kaspth
Copy link
Member

kaspth left a comment

So close now!

railties/test/application/middleware/session_test.rb Outdated
@@ -339,6 +339,9 @@ def read_raw_cookie

# Enable AEAD cookies
config.action_dispatch.use_authenticated_cookie_encryption = true

# Enable Authenticated Message Encryption.
config.active_support.use_authenticated_message_encryption = true

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

This will be true by default, so there's no need to set this.

We should assign the cbc cipher: to the legacy encryptor in cookies middleware. Then the test passes.

activesupport/lib/active_support/railtie.rb Outdated
@@ -7,6 +7,13 @@ class Railtie < Rails::Railtie # :nodoc:

config.eager_load_namespaces << ActiveSupport

initializer "active_support.set_authenticated_message_encryption_as_default" do |app|

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

Yep, see the other comment about why 😊

Let's just clip "_as_default" from the initializer title while we're here.

activesupport/lib/active_support/message_encryptor.rb Outdated
class << self
attr_accessor :use_authenticated_message_encryption

def default_cipher

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

Needs a # :nodoc same for the use_authenticated_message_encryption accessor.

railties/test/application/middleware/session_test.rb Outdated
@@ -290,7 +290,7 @@ def read_raw_cookie
get "/foo/read_encrypted_cookie"
assert_equal "2", last_response.body

cipher = "aes-256-gcm"

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

This shouldn't be changed.

This comment has been minimized.

@assain

assain Jun 11, 2017

Author Contributor

Oops! My bad!

railties/test/application/middleware/session_test.rb Outdated
@@ -339,7 +339,8 @@ def read_raw_cookie

# Enable AEAD cookies
config.action_dispatch.use_authenticated_cookie_encryption = true
RUBY

RUBY

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

This neither.

actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
@@ -630,7 +630,7 @@ def initialize(parent_jar)
secret = key_generator.generate_key(request.encrypted_cookie_salt || "")[0, ActiveSupport::MessageEncryptor.key_len]
sign_secret = key_generator.generate_key(request.encrypted_signed_cookie_salt || "")

@legacy_encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, digest: digest, serializer: ActiveSupport::MessageEncryptor::NullSerializer)
@legacy_encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, cipher: 'aes-256-cbc', digest: digest, serializer: ActiveSupport::MessageEncryptor::NullSerializer)

This comment has been minimized.

@kaspth

kaspth Jun 11, 2017

Member

Use double quoted strings.

set message_encryptor default cipher to aes-256-gcm
- Introduce a method to select default cipher, and maintain backward compatibility
@kaspth

kaspth approved these changes Jun 11, 2017

@kaspth kaspth merged commit 6d402c6 into rails:master Jun 11, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
codeclimate no new or fixed issues
Details

y-yagi added a commit to y-yagi/rails that referenced this pull request Jun 11, 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.