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

AEAD encrypted cookies and sessions #28132

Merged
merged 1 commit into from May 28, 2017
Merged

Conversation

@mjc-gh
Copy link
Contributor

mjc-gh commented Feb 23, 2017

Summary

This PR is the start of migrating from HMAC AES-CBC encrypted cookies to AEAD encrypted cookies. Commit d4ea18a added AES-256-GCM for Authenticated Encryption support. I'm hoping this PR could be the start of migrating cookies and sessions to this form of encryption.

I think it's worth considering to what degree we should be supporting legacy signed and encrypted cookies as well. This PR includes a UpgradeLegacyHmacAesCbcCookieJar class which aims to seamlessly upgrade encrypted cookies. Should we be looking to deprecate older legacy schemes now?

Other Information

This PR comments out some tests that are now broken. Depending on how we move forward with encrypted cookies and to the degree to which legacy cookies are supported, I will update, fix, and add any tests as needed.

@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 23, 2017

Could you explain briefly too why migrating to AEAD is good?

@mjc-gh
Copy link
Contributor Author

mjc-gh commented Feb 23, 2017

Sure, by using AES with GCM, ciphertexts will be shorter and decryption and encryption will be quicker. With GCM, authentication and encryption are processed together instead of separately like we see with the HMAC and AES CBC authenticated encryption setup.

Additionally, the announcement today from Google about SHA1 collisions somewhat drove me to make this PR. While unrelated to security and encryption features​ in Rails, the announcement motivated me to look into what it would take to update Rails' cookies to use more modern encryption. If we do not go down the AEAD route for encrypted cookies, we should at least be exploring using SHA256 for the digest option for MessageEncryptor.

As an aside, AEAD in general aims to make cryptographic processes simpler and less error prone. I think it may be worthwhile making GCM the default for MessageEncryptor but that is beyond the scope of this PR.

@rafaelfranca
Copy link
Member

rafaelfranca commented Feb 23, 2017

Thanks for the explanation!

@eugeneius
Copy link
Member

eugeneius commented Feb 23, 2017

we should at least be exploring using SHA256 for the digest option for MessageEncryptor.

There's an open pull request to do just that (#25204), but it's blocked on the ability to transparently upgrade existing cookies to the new format (#18772) which I suspect we'll need for this change too.

@mjc-gh
Copy link
Contributor Author

mjc-gh commented Feb 23, 2017

@eugeneius there is a small cookie jar class in this PR to upgrade HMAC CBC encrypted cookies to GCM. This could be altered to handle the upgrade from SHA1 to SHA256 though.

I think in the long run introducing GCM cookies is better though. The smaller ciphertexts is a worthwhile improvement.

@eugeneius
Copy link
Member

eugeneius commented Feb 23, 2017

Oh, nice! You even mentioned it in the description 🤦‍♂️

@mjc-gh
Copy link
Contributor Author

mjc-gh commented Feb 23, 2017

Put together some small benchmarks comparing the two modes. The script also outputs the length of two ciphertexts, one for each mode.

The repo is here: https://github.com/mikeycgto/message_encryptor-benchmark

@bdewater
Copy link
Contributor

bdewater commented Feb 24, 2017

Additionally, the announcement today from Google about SHA1 collisions somewhat drove me to make this PR.

Important to note here this finding of a SHA1 collision has nothing to do with the security of HMAC-SHA1, which MessageVerifier uses. So people shouldn't panic if they cannot upgrade straight away.

That said, not only is an AEAD mode faster for encryption/decryption (AES-GCM especially with processor instructions) but it also produces smaller cookies (shown in #25874) so 👍👍👍 if we can transparently upgrade.

@mjc-gh
Copy link
Contributor Author

mjc-gh commented Feb 24, 2017

Agreed @bdewater.

HMACs in general are still secure even when the underlying hash function have known collisions issues. Google's announcement motivated me to look into what it would take to update Rails' cookies to use more modern encryption and security. This is actually something I've been thinking about for a while, today's news just lit the spark for me!

@mjc-gh
Copy link
Contributor Author

mjc-gh commented Feb 24, 2017

Notes on Legacy Support

Thought more about this and want to clarify somethings as far as legacy support goes. Currently there are two legacy cookie jar classes UpgradeLegacySignedCookieJar and UpgradeLegacyEncryptedCookieJar. Both of these classes include VerifyAndUpgradeLegacySignedMessage which upgrades cookies that are just HMAC'd using the old secret_token approach which was deprecated in Rails v4.

This PR adds a UpgradeLegacyHmacAesCbcCookieJar class for upgrading encrypted cookies to the new GCM encryption scheme. This jar is used when action_dispatch.encrypted_cookie_salt and action_dispatch.action_dispatch.encrypted_signed_cookie_salt are set along with the new action_dispatch.authenticated_encrypted_cookie_salt configuration value. These options should enable developers to finely control the migration path. Applications that need migration can continue to set the encrypted_cookie_salt and encrypted_signed_cookie_salt values. New applications can just have the authenticated_encrypted_cookie_salt value set and no migration will be attempted when decryption fails.

I think it's worth considering at this time if secret_token legacy support should be removed. Doing so will help simplify the cookie jar middleware code base significantly. The tests can also be cleaned up and far less cases will need to be covered.

If support is kept, I fully intend to test all legacy configurations. That is, I want to make sure "encrypted" cookies that are just signed with a secret_token are upgraded to GCM encryption as expected. I believe this already the case but this needs to be more thoroughly tested.

Thus, my main question is, should secret_token legacy support be removed now with these changes?

Possible Major API Changes

I'd also go as far as to suggest some major API changes to the cookie middleware. Specifically, we could deprecate the encrypted and signed methods for a single secured method. The two main reasons for this suggestion are:

  1. From a developer usage security prospective, it is best to limit options. Developers may be unsure if authenticity is sufficient or if confidential is needed for their use cases.
  2. Signed cookies are very large since the message is just base64 encoded. With the introduction of GCM cookies we now have smaller resulting ciphertexts. A single secured cookie method which just uses GCM encrypted cookies could be sufficient for when only authenticity is needed.

This suggestion is obviously a lot more significant than what the PR has initially set out to do. I figured it is worth suggesting and discussing though!

@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch 2 times, most recently Feb 27, 2017
@mjc-gh mjc-gh changed the title Start of AEAD encrypted cookies and sessions AEAD encrypted cookies and sessions Feb 27, 2017
@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch Feb 27, 2017
@bdewater
Copy link
Contributor

bdewater commented Feb 28, 2017

Thus, my main question is, should secret_token legacy support be removed now with these changes?

AFAIK Rails usually introduces deprecation warnings in minor versions before removing stuff. With 5.1 already in beta, removal seems more suited when master targets 5.2. Just my $0.02.

@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch 9 times, most recently Mar 1, 2017
...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt Outdated
@@ -0,0 +1,3 @@
# 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.use_authenticated_cookie_encryption = false

This comment has been minimized.

Copy link
@kaspth

kaspth May 15, 2017

Member

We need to copy over the intro from the _5_1.rb file and nestle the config to be under config.action_dispatch.

railties/lib/rails/application/configuration.rb Outdated
@x = Custom.new
@enable_dependency_loading = false
@read_encrypted_secrets = false
@use_authenticated_cookie_encryption = false

This comment has been minimized.

Copy link
@kaspth

kaspth May 15, 2017

Member

To nestle the config under action_dispatch we'll need to move the false assign to this file:

config.action_dispatch.encrypted_signed_cookie_salt = "signed encrypted cookie"

Your override in initializer further below in that file then needs to take the new namespace into account.

actionpack/CHANGELOG.md Outdated

To have existing cookies be seamlessly upgraded to this new scheme
please continue to set the `encrypted_cookie_salt` and
`encrypted_signed_cookie_salt` values.

This comment has been minimized.

Copy link
@kaspth

kaspth May 15, 2017

Member

Remember to rewrite this too 😉

actionpack/CHANGELOG.md Outdated
To have existing cookies be seamlessly upgraded to this new scheme
please continue to set the `encrypted_cookie_salt` and
`encrypted_signed_cookie_salt` values.

This comment has been minimized.

Copy link
@kaspth

kaspth May 15, 2017

Member

Don't forget to add your name too 😄

    *Michael Coyne*
@mjc-gh
Copy link
Contributor Author

mjc-gh commented May 15, 2017

@kaspth these changes sound good. Will get to them either later tonight or tomorrow morning sometime. Thanks for reviewing it again!

@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch May 16, 2017
@mjc-gh
Copy link
Contributor Author

mjc-gh commented May 16, 2017

Made some more changes and moved the use_authenticated_cookie_encryption configuration value to under the action_dispatch namespace. I added a small test to app_generator_test.rb as well to make sure the framework defaults file is generated.

One small question I have is if this config value is set to true in the new framework defaults initializer, will this initializer block here see the flag as true and thus set the salt as need? Maybe setting this salt should be done in an after_initialize block? Just wanted to ask about this and double check the configuration logic is sound.

Thanks again for all the reviews! Can't wait to see this feature merged in -- smaller 🍪 for all!!

@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch May 16, 2017
@mjc-gh
Copy link
Contributor Author

mjc-gh commented May 16, 2017

Thought a bit more about this and I realized that because the two old salt values are still set by default, upgrade_legacy_hmac_aes_cbc_cookies? will always be true (assuming users have opted in to AEAD cookies in the first place).

Perhaps we should add a second configuration flag, such as config.action_dispatch.upgrade_hmac_cbc_cookie_encryption that will, for now, default to true. We can then change upgrade_legacy_hmac_aes_cbc_cookies? to just examine this configuration value as well as the use_authenticated_cookie_encryption value.

Ultimately, I think having direct configuration flags for both AEAD cookies as well as supporting the upgrade behavior is easier for developers to deal with than indirectly controlling this behavior via the salt config values. This way, new applications or applications that no longer need the upgrade behavior can easily opt-out of it. Thoughts?

@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch May 19, 2017
@mjc-gh
Copy link
Contributor Author

mjc-gh commented May 19, 2017

Just rebased and my test_new_5_2_application_has_defaults in railties/test/generators/app_generator_test.rb is now failing. It seems this file isn't expected to be generated for a new 5.2 application (there is a test above it which indicate just that). However, since this feature is opt-in, we need this file to be generated so users can enable the new behavior if they'd like to. Any idea how we should solve this @kaspth?

@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch 3 times, most recently May 21, 2017
@mjc-gh
Copy link
Contributor Author

mjc-gh commented May 21, 2017

I decided to add the upgrade_legacy_hmac_aes_cbc_cookies configuration flag as mention above so users can explicitly now control both opting-in to AES GCM cookies and whether or not upgrade behavior should be used. This should be easier to manage than fiddling with salt configuration values.

Not sure how to handle the failing test with regards to new_framework_defaults_5_1.rb.tt file. This file should be generated for new 5.2 applications so users can opt-in to AES GCM cookies. Perhaps these two configuration values should be moved to independent initializer for new applications? Any thoughts on this?

@kaspth
Copy link
Member

kaspth commented May 21, 2017

However, since this feature is opt-in

It shouldn't be opt-in on a rails new 5.2 app. There it should be enabled by default, which then also solves the test failure. We need to add:

if respond_to?(:action_dispatch)
  action_dispatch.use_authenticated_cookie_encryption = true
end

right after here:

if respond_to?(:active_record)
active_record.cache_versioning = true
end

I don't understand the need for upgrade_hmac_cbc_cookie_encryption — that's what use_authenticated_cookie_encryption governs.

Copy link
Member

kaspth left a comment

Some last defaults wrangling, but we're getting pretty close!

guides/source/configuring.md Outdated
@@ -456,10 +456,14 @@ to `'http authentication'`.
Defaults to `'signed cookie'`.

* `config.action_dispatch.encrypted_cookie_salt` sets the encrypted cookies salt
value. Defaults to `'encrypted cookie'`.
value. Defaults to `'signed cookie'`.

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

This seems off?

This comment has been minimized.

Copy link
@mjc-gh

mjc-gh May 21, 2017

Author Contributor

Will double check this.

guides/source/configuring.md Outdated

* `config.action_dispatch.encrypted_signed_cookie_salt` sets the signed
encrypted cookies salt value. Defaults to `'signed encrypted cookie'`.
encrypted cookies salt value. Defaults to `'encrypted cookie'`.

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

Are we sure that's the default?

This comment has been minimized.

Copy link
@mjc-gh

mjc-gh May 21, 2017

Author Contributor

Will double check this.

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt Outdated
# No longer upgrade CBC HMAC legacy encrypted cookies. This should be disabled
# when this legacy form of encrption no longer needs to automatically upgraded
# to GCM encryption.
# Rails.application.config.action_dispatch.upgrade_legacy_hmac_aes_cbc_cookies = false

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

I think this gets solved by Rails removing support for upgrading the cookies from CBC, so I'd rather not make this a config.

This comment has been minimized.

Copy link
@mjc-gh

mjc-gh May 21, 2017

Author Contributor

See my latest comment for more discussion on this flag. I'm thinking to we just leave the upgrade behavior as is and have future version use "pure" GCM cookies...

...ib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_5_2.rb.tt Outdated

# 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 = false

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

I was recently schooled on the new_framework_defaults_* file by @matthewd and how I've been doing them wrong.

We should follow the now added example above: use the future default, but start it commented out.

This comment has been minimized.

Copy link
@mjc-gh

mjc-gh May 21, 2017

Author Contributor

Understood. This file was a bit of mystery to me as well. Did this discussion occur in the PR from dhh for the new fragment caching feature? I'm definitely interested in fully understanding its uses!

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

The intent is captured here: #28469 😊

railties/test/generators/app_generator_test.rb Outdated
run_generator

assert_file "config/initializers/new_framework_defaults_5_2.rb"
end

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

Newer Rails app load_defaults from the version they were created with:

# Initialize configuration defaults for originally generated Rails version.
config.load_defaults <%= Rails::VERSION::STRING.to_f %>

So a brand new 5.2 app shouldn't have this file, it's only for 5.1 upgrading apps.

This comment has been minimized.

Copy link
@mjc-gh

mjc-gh May 21, 2017

Author Contributor

Gotcha!

actionpack/CHANGELOG.md Outdated
Encrypted cookies may now use AES in GCM mode. To have existing cookies be
seamlessly upgraded to this new scheme set the
`action_dispatch.use_authenticated_cookie_encryption` configuration value to
`true`. This value can be set

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

Let's mention the AES GCM benefits here (faster, shorter — copy from the security guide if you like) and that new_framework_defaults_5.2.rb shows how to upgrade.

@mjc-gh
Copy link
Contributor Author

mjc-gh commented May 21, 2017

It shouldn't be opt-in on a rails new 5.2 app. There it should be enabled by default, which then also solves the test failure. We need to add:

if respond_to?(:action_dispatch)
  action_dispatch.use_authenticated_cookie_encryption = true
end

right after here:

if respond_to?(:active_record)
active_record.cache_versioning = true
end

I can make this change and remove the test I added.

I don't understand the need for upgrade_hmac_cbc_cookie_encryption — that's what use_authenticated_cookie_encryption governs.

Originally I had the upgrade_legacy_hmac_aes_cbc_cookies? method in the cookies middleware check if the salt values for CBC HMAC encryption were defined. If they were, the UpgradeLegacyHmacAesCbcCookieJar middleware is used and cookies are seamlessly upgraded. If they were not defined the EncryptedCookieJar middleware is used and no upgrading is attempted when decryption fails.

Due to these salt values still being defined the UpgradeLegacyHmacAesCbcCookieJar middleware will always be used even if upgrade behavior is not needed or desired. I thought adding an explicit flag would be easier to manage this behavior than clearing the salt values (thus having upgrade_legacy_hmac_aes_cbc_cookies? return false).

Perhaps this can be solved by removing the upgrade middleware altogether in future versions. I know cleaning up the cookies middleware in general is one of the overall goals for the GSoC project so perhaps we should just leave this be for now and have it get released as is in Rails 5.2?

@kaspth
Copy link
Member

kaspth commented May 21, 2017

Perhaps this can be solved by removing the upgrade middleware altogether in future versions. I know cleaning up the cookies middleware in general is one of the overall goals for the GSoC project so perhaps we should just leave this be for now and have it get released as is in Rails 5.2?

My thoughts exactly 👍 (though the GSoC project might not necessarily clean up old cookies versions, but we'll see).

I think it's fine to check for the presence of the 3 salt values because use_authenticated_cookie_encryption guards the new salt being assigned the upgrade won't happen.

actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
def upgrade_legacy_hmac_aes_cbc_cookies?
request.secret_key_base.present? &&
request.use_authenticated_cookie_encryption? &&
request.use_authenticated_cookie_encryption?

This comment has been minimized.

Copy link
@kaspth

kaspth May 21, 2017

Member

So this would be:

request.secret_key_base.present? &&
  request.authenticated_encrypted_cookie_salt.present? &&
  request.encrypted_signed_cookie_salt.present? &&
  request.encrypted_cookie_salt.present?

And we can kill the use_authenticated_cookie_encryption? and upgrade_legacy_hmac_aes_cbc_cookies? methods then.

authenticated_encrypted_cookie_salt was placed up further, since it's less likely to be assigned on upgrading apps and we'll avoid the presence check on the present in older releases salt values.

Related: I'm not sure why we check the presence of the secret_key_base here. Rails apps won't boot without it these days.

This comment has been minimized.

Copy link
@mjc-gh

mjc-gh May 21, 2017

Author Contributor

Checking secret_key_base seemed a bit unnecessary and I do think it's likely safe to drop at this point.

I'll also rework my commits soon and drop the upgrade_legacy_hmac_aes_cbc_cookies flag and revert the upgrade_legacy_hmac_aes_cbc_cookies? method to just check if the salt values are present?. I'll also update the above mention changes to the various guides. At that point, I think we'll be close to ready for this PR. Thanks again!!

This commit changes encrypted cookies from AES in CBC HMAC mode to
Authenticated Encryption using AES-GCM. It also provides a cookie jar
to transparently upgrade encrypted cookies to this new scheme. Some
other notable changes include:

- There is a new application configuration value:
  +use_authenticated_cookie_encryption+. When enabled, AEAD encrypted
  cookies will be used.

- +cookies.signed+ does not raise a +TypeError+ now if the name of an
  encrypted cookie is used. Encrypted cookies using the same key as
  signed cookies would be verified and serialization would then fail
  due the message still be encrypted.
@mjc-gh mjc-gh force-pushed the mjc-gh:aead-encrypted-cookies branch to 5a3ba63 May 22, 2017
@kaspth kaspth merged commit b88200f into rails:master May 28, 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 May 28, 2017

@mikeycgto great work! 👏

@mjc-gh
Copy link
Contributor Author

mjc-gh commented May 28, 2017

@kaspth thanks so much for all the help and the feedback! Excited to see this get merged and see Rails get more modern encryption!!

@kaspth
Copy link
Member

kaspth commented May 29, 2017

@mikeycgto no problem! Thanks for bringing Rails up to speed 💪

mjc-gh referenced this pull request in rack/rack Jun 4, 2017
This reverts commit 9a4700f.
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

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