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

[assain]

@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.

[kaspth]

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

[assain]

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

[kaspth]

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

[kaspth]

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

[kaspth]

Why is this change needed?

[assain]

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]

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.

[kaspth]

Shouldn't say use_authenticated_cookie_encryption but just use_authenticated_encryption 😊

[assain]

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

[kaspth]

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

[kaspth]

Keep the period.

[kaspth]

self.class.default_cipher

[kaspth]

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

[kaspth]

as default for -> when

[kaspth]

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]

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.

[assain]

@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: ""

[kaspth]

Yep, see the other comment about why 😊

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

[kaspth]

So close now!

[kaspth]

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.

[kaspth]

Yep, see the other comment about why 😊

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

[kaspth]

Needs a # :nodoc same for the use_authenticated_message_encryption accessor.

[kaspth]

This shouldn't be changed.

[assain]

Oops! My bad!

[kaspth]

This neither.

[kaspth]

Use double quoted strings.