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

Cookie-base session store auto-upgrade #9978

Merged
merged 1 commit into from Apr 1, 2013
Merged

Conversation

@trevorturk
Copy link
Contributor

trevorturk commented Mar 28, 2013

Changes:

  • Automatically configure cookie-based sessions to be encrypted if secret_key_base is set, signed if only secret_token is set.
  • This eliminates the need to introduce the new EncryptedCookieStore and UpgradeSignatureToEncryptionCookieStore cookie-based session stores, so we leave only the existing config.session_store :cookie_store option and remove the two new options introduced in 4.0.0.beta1: encrypted_cookie_store and upgrade_signature_to_encryption_cookie_store.
  • Automatically upgrade existing signed cookie-based sessions from Rails 3.x to be encrypted if both secret_key_base and secret_token are set. Simply upgrade to the new key generator if only secret_token is set. (Pull request #9909 took care of transparently upgrading signed cookies to use the new key generator.)

Reasoning:

The signed cookie jar has changed to use the new Rails-internal key generator, so you are discouraged from having a known/shared secret for signing cookies / sharing sessions.

If signed cookies and signed session cookies are considered internal to Rails, then we can upgrade from signed to encrypted automatically if they opt-in by setting secret_key_base.

If that all makes sense, then we can just have the one session store for Rails: cookie_store. This reduces the number of config options and allows us to state the caveats around upgrading plainly in one place.

/cc @jeremy @spastorino @neerajdotname

@fxn (or someone else interested in documentation) I'm wondering if we should include a link to the upgrade guide, which is where the caveats/warnings/etc about setting secret_key_base are currently.

Should we use: http://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html or something? Should we just leave this as-is?

Further possible work:

I'd like to continue with a bit more work here, if these ideas make sense:

  1. Standardize on name instead of key when referring to cookies. We use both now and @jeremy prefers name which is fine by me. Also standardize private spacing as per JK's comments.
  2. I think we also need to fix PermanentCookieJar's [] method, which may have been broken in 2b773e1. It's confusing key and name. I'm not sure why there's no failing test.
  3. Rename DummyKeyGenerator to LegacyKeyGenerator -- "dummy" isn't a very descriptive name. I've settled on "legacy" to refer to signed stuff generated by Rails 3.x. I'm open to other names, but I'd like to move away from "dummy".
  4. Don't remove Dummy/LegacyKeyGenerator in 4.1. I think we should keep this around a bit longer to ensure that people can upgrade from 3.2.x to 4.1.x etc. There's "fixme" comments in place that I'd like to remove.
  5. Use Dummy/LegacyKeyGenerator where it makes sense instead of MessageVerifier directly. Anything referring to signed cookies should use Dummy/LegacyKeyGenerator, I believe.
@steveklabnik
Copy link
Member

steveklabnik commented Mar 28, 2013

@fxn (or someone else interested in documentation) I'm wondering if we should include a link to the upgrade guide, which is where the caveats/warnings/etc about setting secret_key_base are currently.

Seems like something that should be in the API docs too, especially if it's that important.

@trevorturk
Copy link
Contributor Author

trevorturk commented Mar 28, 2013

@steveklabnik can you point me to where you think the right spot might be? I'll happily update this PR with more docs.

@steveklabnik
Copy link
Member

steveklabnik commented Mar 28, 2013

Hmm. Yeah, I'm not sure where the best place to put them would be. Maybe @fxn has a better idea. It just seems odd that the upgrade guide would have the docs, and the docs would point to the guide, you know? :)

@trevorturk
Copy link
Contributor Author

trevorturk commented Mar 28, 2013

Totally, @steveklabnik -- this is an important setting, so let's make sure it's easy to read more about it :)

@jeremy
jeremy reviewed Mar 28, 2013
View changes
actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
else
EncryptedCookieJar.new(self, @key_generator, @options)
end
end

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 28, 2013

Member

Can omit the begin...end wrapper

@jeremy
jeremy reviewed Mar 28, 2013
View changes
actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
else
signed
end
end

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 28, 2013

Member

Can omit the begin...end wrapper

@@ -371,6 +403,14 @@ def []=(key, options)
raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
@parent_jar[key] = options
end

private

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 28, 2013

Member

✂️

@@ -440,6 +460,28 @@ def []=(key, options)
raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
@parent_jar[key] = options
end

private

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 28, 2013

Member

✂️

# are both set. It reads legacy cookies signed with the old dummy key generator and
# encrypts and re-saves them using the new key generator to provide a smooth upgrade path.
class UpgradeLegacyEncryptedCookieJar < EncryptedCookieJar #:nodoc:
include VerifyAndUpgradeLegacySignedMessage

This comment has been minimized.

Copy link
@jeremy

jeremy Mar 28, 2013

Member

nice technique with this module!

@jeremy
Copy link
Member

jeremy commented Mar 28, 2013

Love this. Thanks @trevorturk. +1 on the future work, too. Each are good bite-sized PRs.

…kies; Automatically configure cookie-based sessions to use the best cookie jar given the app's config
@trevorturk
Copy link
Contributor Author

trevorturk commented Mar 28, 2013

@jeremy I removed the unnecessary begin/end, but left the private spacing because the style matches the rest of the file. I'll change this for the entire file in a refactoring commit as part of my upcoming further work pull requests.

I rebased, and I think this is ready to merge. We can improve the docs separately. Thanks for the feedback!

@ghost ghost assigned spastorino Mar 29, 2013
@spastorino
Copy link
Member

spastorino commented Mar 31, 2013

@trevorturk PR looks great. So after applying this we would have a way to use encrypted, signed and the upgrading path. The only drawback I see is you're not able to use signed cookies only without warnings.

@trevorturk
Copy link
Contributor Author

trevorturk commented Apr 1, 2013

Thanks for reviewing, @spastorino!

After applying this, the cookie-based session store would be automatically configured to use the best possible cookie store given the app's configuration . The logic would work like this:

  • If only secret_token is set: upgrade from the old signed session cookies to the new signed session cookies.
  • If only secret_key_base is set: use the new encrypted session cookies.
  • If both secret_token and secret_key_base are set: upgrade from old signed cookies to encrypted cookies.

You would still be able to use cookies[:signed] just as before without warnings, though. The only deprecation warning is the one from here: https://github.com/rails/rails/blob/master/railties/lib/rails/application.rb#L139-L141 that's triggered by not setting secret_key_base.

@spastorino
Copy link
Member

spastorino commented Apr 1, 2013

@trevorturk exactly, the warn raises if you don't set secret_key_base. So users are not able to use signed cookie store for sessions without warnings.

@spastorino
Copy link
Member

spastorino commented Apr 1, 2013

So the question to answer is, do we want users to use SignedCookieStore or not? /cc @jeremy @NZKoz
I'd allow for users that do not store things that browsers shouldn't read.

spastorino added a commit that referenced this pull request Apr 1, 2013
Cookie-base session store auto-upgrade
@spastorino spastorino merged commit f9d23b3 into rails:master Apr 1, 2013
@trevorturk
Copy link
Contributor Author

trevorturk commented Apr 1, 2013

Thanks @spastorino!

We had a chat and did a benchmark showing no significant performance impact for using encrypted session cookies versus signed. So, the recommendation is to reduce the number of configuration options for users and to choose the best cookie store we can, given their app's config.

Here's the benchmark:

require 'benchmark'

require 'active_support/key_generator'
require 'active_support/message_verifier'
require 'active_support/message_encryptor'

Benchmark.bm do |r|
  N = 100000

  r.report("signed") do
    key_generator = ActiveSupport::DummyKeyGenerator.new('b3c631c314c0bbca50c1b2843150fe33')
    secret = key_generator.generate_key('signed cookie salt')
    verifier = ActiveSupport::MessageVerifier.new(secret)
    signed_message = verifier.generate('test')

    N.times { verifier.verify(signed_message) }
  end

  r.report("encrypted") do
    key_generator = ActiveSupport::KeyGenerator.new('b3c631c314c0bbca50c1b2843150fe33')
    secret = key_generator.generate_key('encrypted cookie salt')
    sign_secret = key_generator.generate_key('encrypted cookie salt')
    encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret)
    encrypted_message = encryptor.encrypt_and_sign('test')

    N.times { encryptor.decrypt_and_verify(encrypted_message) }
  end
end

#             user       system     total    real
# signed      3.240000   0.010000   3.250000 (  3.263722)
# encrypted   5.450000   0.010000   5.460000 (  5.472302)
@jeremy
Copy link
Member

jeremy commented Apr 1, 2013

:shipit:

@NZKoz
Copy link
Member

NZKoz commented Apr 1, 2013

Great work @trevorturk! If you wanted to convert from using Marshal to JSON for session serialization that'd be cool too ;)

@vovimayhem

This comment has been minimized.

Copy link

vovimayhem commented on 274a3aa Apr 3, 2013

Hey what happened to the EncryptedCookieStore?

This comment has been minimized.

Copy link
Contributor Author

trevorturk replied Apr 3, 2013

@vovimayhem, please see #9978 for details.

@vovimayhem
Copy link

vovimayhem commented Apr 3, 2013

Nevermind... didn't read this PR.

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.