Encrypted cookies #8112

Merged
merged 10 commits into from Nov 15, 2012

Projects

None yet
@spastorino
Member
  1. Sign cookies using key deriver
    Rails will derive keys if config.secret_key_base is set instead of the old config.secret_key. If secret_key_base is not set to keep backwards compatibility Rails will not derive and just use the raw secret_key as in previous versions.
    Users can change the default salt for signing cookies through config.action_dispatch.signed_cookie_salt setting which is by default 'signed cookie'.

  2. cookie.encrypted method => EncryptedCookieJar
    This adds a encrypted method to CookieJar that returns a jar that'll automatically encrypt cookie values before sending them to the client and will decrypt them for read.
    It also prevents from tampering by the user (or a 3rd party) throwing an ActiveSupport::MessageVerifier::InvalidSignature exception.
    You must set config.secret_token_base to be able to use this feature.
    Users are allowed to change the default salt used for encryption by changing config.action_dispatch.encrypted_cookie_salt which defaults to 'encrypted cookie' and can also change the salt using for signing by changing config.action_dispatch.encrypted_signed_cookie_salt which defaults to 'signed encrypted cookie'

  3. encrypted cookie store
    This adds encrypted cookie store. You now can set config.session_store :encrypted_cookie_store, key: '_myapp_session' in order to use encrypted cookies to store your session. This also makes new apps to use encrypted cookie store but default.

  4. Use derive keys for http authentication
    Just that it changes http authentication to use derived keys.
    Users are allowed to change the default salt by changing config.action_dispatch.http_auth_salt which defaults to 'http authentication'

In general, please help me reviewing all this. Help me improving the docs for all this. And help me suggesting better names for the options, mostly …

config.secret_key_base
config.action_dispatch.signed_cookie_salt
config.action_dispatch.encrypted_cookie_salt
config.action_dispatch.encrypted_signed_cookie_salt
config.action_dispatch.http_auth_salt

Note to committers: Leave this for me to merge :).

/cc @NZKoz @meder @thaidn @emboss @nahi please help me reviewing giving crypto review :).

@homakov
homakov commented Nov 3, 2012

ah you did it! dreamed about this feature

@scottwb
scottwb commented Nov 3, 2012

+1 I love it. I've been wanting this in Rails for a long time, but there seems to have been quite the resistance early on. I've been using encryped_cookie_store all along:

Rails 2.3: https://github.com/FooBarWidget/encrypted_cookie_store
Rails 3.0: https://github.com/scottwb/encrypted_cookie_store
Rails 3.2: https://github.com/validas/encrypted_cookie_store

I never bought into the argument that this would lull me into storing sensitive data in my cookies.

Thanks for making this happen!

/cc @mattsnyder

@thaidn
thaidn commented Nov 5, 2012

Thanks for working on this. I will take a look sometime this week.

@spastorino
Member

@thaidn ok thanks, will wait :)

@NZKoz
Member
NZKoz commented Nov 5, 2012

The Short version of the changes is:

  • Rename secret_key to secret_key_base, if you set secret_key then we verify cookies based on the raw secret (and fire a deprecation warning)
  • Use the key_generator code with static salts when deriving keys everywhere we derive keys
  • Provide config options for people to change those salts if they want to.
  • We don't advertise the salt configuration options as they don't seem necessary at all as the entropy comes from the base secret. However the option's there for tinfoil hats
  • Introduce an EncryptedCookieJar which is encrypted (and signed) with keys derived from the base secret.
  • Default to using the encrypted cookie jar for the session data

What we don't have yet, is a configuration option to transparently take an old session, and upgrade it to a new session. Broadly speaking I'm scared of automatic upgrades in crypto systems as I'm too dumb to know if there's an issue here. It might be nice to have this as an option to encourage people to upgrade, but we'd need some guidance on the sanity, or otherwise, of that.

@carlosantoniodasilva carlosantoniodasilva commented on the diff Nov 5, 2012
actionpack/test/controller/flash_test.rb
@@ -217,7 +219,7 @@ def test_redirect_to_with_adding_flash_types
class FlashIntegrationTest < ActionDispatch::IntegrationTest
SessionKey = '_myapp_session'
- SessionSecret = 'b3c631c314c0bbca50c1b2843150fe33'
+ Generator = ActiveSupport::DummyKeyGenerator.new('b3c631c314c0bbca50c1b2843150fe33')
@carlosantoniodasilva
carlosantoniodasilva Nov 5, 2012 Ruby on Rails member

Two spaces after = 😄

@spastorino
spastorino Nov 5, 2012 Ruby on Rails member

will fix it

@spastorino
spastorino Nov 16, 2012 Ruby on Rails member

Done 44f12bb

@carlosantoniodasilva

@spastorino looks awesome bro 👍

@frodsan frodsan commented on the diff Nov 5, 2012
actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -237,7 +248,23 @@ def permanent
#
# cookies.signed[:discount] # => 45
def signed
- @signed ||= SignedCookieJar.new(self, @secret)
+ @signed ||= SignedCookieJar.new(self, @key_generator, @options)
+ end
+
+ # Returns a jar that'll automatically encrypt cookie values before sending them to the client and will decrypt them for read.
+ # If the cookie was tampered with by the user (or a 3rd party), an ActiveSupport::MessageVerifier::InvalidSignature exception
+ # will be raised.
+ #
+ # This jar requires that you set a suitable secret for the verification on your app's +config.secret_key_base+.
+ #
+ # Example:
@spastorino
spastorino Nov 6, 2012 Ruby on Rails member

Not sure I follow what do you mean

@rafaelfranca
rafaelfranca Nov 14, 2012 Ruby on Rails member

I think we are not using the Example: label in the documentation anymore. ✂️ == "Remove this line"

@spastorino
spastorino Nov 16, 2012 Ruby on Rails member

About this I started to document copying what he already have here https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L244
@frodsan can you take a look at all that?

@jeremy
Member
jeremy commented Nov 5, 2012
What we don't have yet, is a configuration option to transparently take an old session, and upgrade it to a new session. 

CookieStore does all its work using get_cookie and set_cookie methods that a UpgradeSignatureToEncryptionCookieStore subclass can override to try the signed cookie jar first, then the encrypted jar. It'd need both key generators though.

@rafaelfranca rafaelfranca commented on the diff Nov 14, 2012
activesupport/lib/active_support/key_generator.rb
@@ -20,4 +21,51 @@ def generate_key(salt, key_size=64)
OpenSSL::PKCS5.pbkdf2_hmac_sha1(@secret, salt, @iterations, key_size)
end
end
+
+ class CachingKeyGenerator
+ def initialize(key_generator)
+ @key_generator = key_generator
+ @cache_keys = {}.extend(Mutex_m)
+ end
+
+ def generate_key(salt, key_size=64)
+ @cache_keys.synchronize do
+ @cache_keys["#{salt}#{key_size}"] ||= @key_generator.generate_key(salt, key_size)
+ end
+ end
+ end
+
+ class DummyKeyGenerator
@rafaelfranca
rafaelfranca Nov 14, 2012 Ruby on Rails member

I think we should put # :nodoc: here since this class is not to be using in the applications

@spastorino
spastorino Nov 16, 2012 Ruby on Rails member

Done d348c43

@rafaelfranca rafaelfranca commented on the diff Nov 14, 2012
activesupport/lib/active_support/key_generator.rb
@@ -20,4 +21,51 @@ def generate_key(salt, key_size=64)
OpenSSL::PKCS5.pbkdf2_hmac_sha1(@secret, salt, @iterations, key_size)
end
end
+
+ class CachingKeyGenerator
@rafaelfranca
rafaelfranca Nov 14, 2012 Ruby on Rails member

Documentation to this class would be great

@spastorino
spastorino Nov 16, 2012 Ruby on Rails member

Done e6e3317 review please /cc @frodsan

@spastorino spastorino merged commit ef8b845 into master Nov 15, 2012

1 check was pending

Details default The Travis build is in progress
@spastorino
Member

I will check @jeremy and @rafaelfranca points later. If there is some other concern let me know

@emboss emboss commented on the diff Nov 16, 2012
actionpack/lib/action_dispatch/middleware/cookies.rb
end
+ rescue ActiveSupport::MessageVerifier::InvalidSignature,
+ ActiveSupport::MessageVerifier::InvalidMessage
+ nil
@emboss
emboss Nov 16, 2012

Just for my understanding: Why swallow the errors?

@spastorino
spastorino Nov 16, 2012 Ruby on Rails member

I just wanted to keep the API similar to what we already have https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/cookies.rb#L333-334
Anyway, I'd think about removing this rescue nil thing. @jeremy @josevalim thoughts? do you guys know why this was that way in the first place?

@spastorino
spastorino Nov 16, 2012 Ruby on Rails member

Another discussion related to this thing is if you would return 400 to clients when something goes wrong or if you would just ignore "broken" cookies. This may happen if you change the secret for instance.

@josevalim
josevalim Nov 17, 2012 Ruby on Rails member

Yes, in case someone tempers the cookie or, more important, you changed the secret because you updated a Rails version or the previous one was "stolen", you should be able to change the secret and we will simply discard the invalid cookies.

@emboss
emboss Nov 18, 2012

Ah OK, makes sense now. Thanks, @spastorino & @josevalim !

@spastorino
spastorino Nov 19, 2012 Ruby on Rails member

@emboss Thanks to you for reviewing :)

@emboss
emboss Nov 19, 2012

@spastorino De rien, my pleasure!

@emboss
emboss commented Nov 16, 2012

a) How much of a pain would it be if we switched the key generator from PBKDF2-HMAC-SHA-1 to HMAC-SHA-256?

The reason I'm asking is because using HMAC-SHA-1 in the key generator limits the overall security to the 160 bits output of SHA-1 there. Since we generally use longer keys (for example 32 byte keys for the default AES-256-CBC in the MessageEncryptor) for encryption, it would be nice if we could use something that gives us a security margin of 32 bytes = 256 bits as well, like SHA-256 does. The same argument can be made for MessageVerifier.

It would be nice if the security parameters add up - but I know that especially PBKDF2-HMAC-SHA-256 (using PKCS5::pbkdf2_hmac) can be a bit of a pain, a lot of people would probably have to upgrade their (native) OpenSSL library to make it work?

b) How do you feel about warning or raising an error if user-provided salts are less than eight characters long (the PBKDF#2 recommendation for minimum salt length)? Kudos for adding the possibility to provide your own salts from a professing tinfoil hat ;)

@spastorino
Member

@jeremy I've added the cookie store to upgrade here 8d06b62...8eefdb6

@spastorino
Member

Also added this docs d56cfad

@NZKoz
Member
NZKoz commented Nov 17, 2012

@emboss I tried that the first time around but I couldn't find a ruby which I deploy on which supported HMAC-SHA-256, so if we did we'd need to fall back to sha1.

And if we did that we'd break people's stuff if they upgraded their openssl suddenly their app's cookies don't comply.

We could add a config option for it perhaps, but not change the defaults.

@emboss
emboss commented Nov 18, 2012

@NZKoz OK, I imagined changing the default would probably break things, but a config option sounds great!

@spastorino
Member

@emboss can you provide a Pull Request for this?

@emboss
emboss commented Nov 20, 2012

@spastorino Sure, I'll give it a shot! Any deadlines I have to keep an eye on?

@guilleiguaran
Member

@emboss no pressure but...

wut

:trollface:

@guilleiguaran
Member

@emboss nvm, the option is a nice-to-have but isn't a blocker for Rails 4 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment