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

Encrypted cookies #8112

Merged
merged 10 commits into from Nov 15, 2012
6 changes: 3 additions & 3 deletions actionpack/lib/action_controller/metal/http_authentication.rb
Expand Up @@ -249,9 +249,9 @@ def authentication_request(controller, realm, message = nil)
end

def secret_token(request)
secret = request.env["action_dispatch.secret_token"]
raise "You must set config.secret_token in your app's config" if secret.blank?
secret
key_generator = request.env["action_dispatch.key_generator"]
http_auth_salt = request.env["action_dispatch.http_auth_salt"]
key_generator.generate_key(http_auth_salt)
end

# Uses an MD5 digest based on time to generate a value to be used only once.
Expand Down
Expand Up @@ -121,11 +121,11 @@ def exists?

class NullCookieJar < ActionDispatch::Cookies::CookieJar #:nodoc:
def self.build(request)
secret = request.env[ActionDispatch::Cookies::TOKEN_KEY]
host = request.host
secure = request.ssl?
key_generator = request.env[ActionDispatch::Cookies::GENERATOR_KEY]
host = request.host
secure = request.ssl?

new(secret, host, secure)
new(key_generator, host, secure)
end

def write(*)
Expand Down
9 changes: 5 additions & 4 deletions actionpack/lib/action_dispatch.rb
Expand Up @@ -81,10 +81,11 @@ module Http
end

module Session
autoload :AbstractStore, 'action_dispatch/middleware/session/abstract_store'
autoload :CookieStore, 'action_dispatch/middleware/session/cookie_store'
autoload :MemCacheStore, 'action_dispatch/middleware/session/mem_cache_store'
autoload :CacheStore, 'action_dispatch/middleware/session/cache_store'
autoload :AbstractStore, 'action_dispatch/middleware/session/abstract_store'
autoload :CookieStore, 'action_dispatch/middleware/session/cookie_store'
autoload :EncryptedCookieStore, 'action_dispatch/middleware/session/cookie_store'
autoload :MemCacheStore, 'action_dispatch/middleware/session/mem_cache_store'
autoload :CacheStore, 'action_dispatch/middleware/session/cache_store'
end

mattr_accessor :test_app
Expand Down
106 changes: 75 additions & 31 deletions actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -1,5 +1,6 @@
require 'active_support/core_ext/hash/keys'
require 'active_support/core_ext/module/attribute_accessors'
require 'active_support/message_verifier'

module ActionDispatch
class Request < Rack::Request
Expand Down Expand Up @@ -27,7 +28,7 @@ def cookie_jar
# cookies[:login] = { value: "XJ-122", expires: 1.hour.from_now }
#
# # Sets a signed cookie, which prevents users from tampering with its value.
# # The cookie is signed by your app's <tt>config.secret_token</tt> value.
# # The cookie is signed by your app's <tt>config.secret_key_base</tt> value.
# # It can be read using the signed method <tt>cookies.signed[:key]</tt>
# cookies.signed[:user_id] = current_user.id
#
Expand Down Expand Up @@ -79,8 +80,12 @@ def cookie_jar
# * <tt>:httponly</tt> - Whether this cookie is accessible via scripting or
# only HTTP. Defaults to +false+.
class Cookies
HTTP_HEADER = "Set-Cookie".freeze
TOKEN_KEY = "action_dispatch.secret_token".freeze
HTTP_HEADER = "Set-Cookie".freeze
GENERATOR_KEY = "action_dispatch.key_generator".freeze
SIGNED_COOKIE_SALT = "action_dispatch.signed_cookie_salt".freeze
ENCRYPTED_COOKIE_SALT = "action_dispatch.encrypted_cookie_salt".freeze
ENCRYPTED_SIGNED_COOKIE_SALT = "action_dispatch.encrypted_signed_cookie_salt".freeze


# Raised when storing more than 4K of session data.
CookieOverflow = Class.new StandardError
Expand All @@ -103,21 +108,27 @@ class CookieJar #:nodoc:
DOMAIN_REGEXP = /[^.]*\.([^.]*|..\...|...\...)$/

def self.build(request)
secret = request.env[TOKEN_KEY]
env = request.env
key_generator = env[GENERATOR_KEY]
options = { signed_cookie_salt: env[SIGNED_COOKIE_SALT],
encrypted_cookie_salt: env[ENCRYPTED_COOKIE_SALT],
encrypted_signed_cookie_salt: env[ENCRYPTED_SIGNED_COOKIE_SALT] }

host = request.host
secure = request.ssl?

new(secret, host, secure).tap do |hash|
new(key_generator, host, secure, options).tap do |hash|
hash.update(request.cookies)
end
end

def initialize(secret = nil, host = nil, secure = false)
@secret = secret
def initialize(key_generator, host = nil, secure = false, options = {})
@key_generator = key_generator
@set_cookies = {}
@delete_cookies = {}
@host = host
@secure = secure
@options = options
@cookies = {}
end

Expand Down Expand Up @@ -220,15 +231,15 @@ def clear(options = {})
# cookies.permanent.signed[:remember_me] = current_user.id
# # => Set-Cookie: remember_me=BAhU--848956038e692d7046deab32b7131856ab20e14e; path=/; expires=Sun, 16-Dec-2029 03:24:16 GMT
def permanent
@permanent ||= PermanentCookieJar.new(self, @secret)
@permanent ||= PermanentCookieJar.new(self, @key_generator, @options)
end

# Returns a jar that'll automatically generate a signed representation of cookie value and verify it when reading from
# the cookie again. This is useful for creating cookies with values that the user is not supposed to change. If a signed
# 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_token+.
# This jar requires that you set a suitable secret for the verification on your app's +config.secret_key_base+.
#
# Example:
#
Expand All @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I follow what do you mean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

#
# cookies.encrypted[:discount] = 45
# # => Set-Cookie: discount=ZS9ZZ1R4cG1pcUJ1bm80anhQang3dz09LS1mbDZDSU5scGdOT3ltQ2dTdlhSdWpRPT0%3D--ab54663c9f4e3bc340c790d6d2b71e92f5b60315; path=/
#
# cookies.encrypted[:discount] # => 45
def encrypted
@encrypted ||= EncryptedCookieJar.new(self, @key_generator, @options)
end

def write(headers)
Expand All @@ -261,8 +288,10 @@ def write_cookie?(cookie)
end

class PermanentCookieJar < CookieJar #:nodoc:
def initialize(parent_jar, secret)
@parent_jar, @secret = parent_jar, secret
def initialize(parent_jar, key_generator, options = {})
@parent_jar = parent_jar
@key_generator = key_generator
@options = options
end

def []=(key, options)
Expand All @@ -283,11 +312,11 @@ def method_missing(method, *arguments, &block)

class SignedCookieJar < CookieJar #:nodoc:
MAX_COOKIE_SIZE = 4096 # Cookies can typically store 4096 bytes.
SECRET_MIN_LENGTH = 30 # Characters

def initialize(parent_jar, secret)
ensure_secret_secure(secret)
def initialize(parent_jar, key_generator, options = {})
@parent_jar = parent_jar
@options = options
secret = key_generator.generate_key(@options[:signed_cookie_salt])
@verifier = ActiveSupport::MessageVerifier.new(secret)
end

Expand All @@ -314,26 +343,41 @@ def []=(key, options)
def method_missing(method, *arguments, &block)
@parent_jar.send(method, *arguments, &block)
end
end

protected

# To prevent users from using something insecure like "Password" we make sure that the
# secret they've provided is at least 30 characters in length.
def ensure_secret_secure(secret)
if secret.blank?
raise ArgumentError, "A secret is required to generate an " +
"integrity hash for cookie session data. Use " +
"config.secret_token = \"some secret phrase of at " +
"least #{SECRET_MIN_LENGTH} characters\"" +
"in config/initializers/secret_token.rb"
class EncryptedCookieJar < SignedCookieJar #:nodoc:
def initialize(parent_jar, key_generator, options = {})
if ActiveSupport::DummyKeyGenerator === key_generator
raise "Encrypted Cookies must be used in conjunction with config.secret_key_base." +
"Set config.secret_key_base in config/initializers/secret_token.rb"
end

if secret.length < SECRET_MIN_LENGTH
raise ArgumentError, "Secret should be something secure, " +
"like \"#{SecureRandom.hex(16)}\". The value you " +
"provided, \"#{secret}\", is shorter than the minimum length " +
"of #{SECRET_MIN_LENGTH} characters"
@parent_jar = parent_jar
@options = options
secret = key_generator.generate_key(@options[:encrypted_cookie_salt])
sign_secret = key_generator.generate_key(@options[:encrypted_signed_cookie_salt])
@encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret)
end

def [](name)
if encrypted_message = @parent_jar[name]
@encryptor.decrypt_and_verify(encrypted_message)
end
rescue ActiveSupport::MessageVerifier::InvalidSignature,
ActiveSupport::MessageVerifier::InvalidMessage
nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding: Why swallow the errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emboss Thanks to you for reviewing :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spastorino De rien, my pleasure!

end

def []=(key, options)
if options.is_a?(Hash)
options.symbolize_keys!
else
options = { :value => options }
end
options[:value] = @encryptor.encrypt_and_sign(options[:value])

raise CookieOverflow if options[:value].size > MAX_COOKIE_SIZE
@parent_jar[key] = options
end
end

Expand Down
23 changes: 20 additions & 3 deletions actionpack/lib/action_dispatch/middleware/session/cookie_store.rb
Expand Up @@ -57,8 +57,7 @@ def destroy_session(env, session_id, options)
def unpacked_cookie_data(env)
env["action_dispatch.request.unsigned_session_cookie"] ||= begin
stale_session_check! do
request = ActionDispatch::Request.new(env)
if data = request.cookie_jar.signed[@key]
if data = cookie_jar(env)[@key]
data.stringify_keys!
end
data || {}
Expand All @@ -72,8 +71,26 @@ def set_session(env, sid, session_data, options)
end

def set_cookie(env, session_id, cookie)
cookie_jar(env)[@key] = cookie
end

def get_cookie
cookie_jar(env)[@key]
end

def cookie_jar(env)
request = ActionDispatch::Request.new(env)
request.cookie_jar.signed
end
end

class EncryptedCookieStore < CookieStore

private

def cookie_jar(env)
request = ActionDispatch::Request.new(env)
request.cookie_jar.signed[@key] = cookie
request.cookie_jar.encrypted
end
end
end
Expand Down
4 changes: 4 additions & 0 deletions actionpack/lib/action_dispatch/railtie.rb
Expand Up @@ -13,6 +13,10 @@ class Railtie < Rails::Railtie
config.action_dispatch.rescue_responses = { }
config.action_dispatch.default_charset = nil
config.action_dispatch.rack_cache = false
config.action_dispatch.http_auth_salt = 'http authentication'
config.action_dispatch.signed_cookie_salt = 'signed cookie'
config.action_dispatch.encrypted_cookie_salt = 'encrypted cookie'
config.action_dispatch.encrypted_signed_cookie_salt = 'signed encrypted cookie'

config.action_dispatch.default_headers = {
'X-Frame-Options' => 'SAMEORIGIN',
Expand Down
6 changes: 4 additions & 2 deletions actionpack/test/controller/flash_test.rb
@@ -1,4 +1,6 @@
require 'abstract_unit'
# FIXME remove DummyKeyGenerator and this require in 4.1
require 'active_support/key_generator'

class FlashTest < ActionController::TestCase
class TestController < ActionController::Base
Expand Down Expand Up @@ -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')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two spaces after = 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 44f12bb


class TestController < ActionController::Base
add_flash_types :bar
Expand Down Expand Up @@ -291,7 +293,7 @@ def test_added_flash_types_method

# Overwrite get to send SessionSecret in env hash
def get(path, parameters = nil, env = {})
env["action_dispatch.secret_token"] ||= SessionSecret
env["action_dispatch.key_generator"] ||= Generator
super
end

Expand Down
6 changes: 4 additions & 2 deletions actionpack/test/controller/http_digest_authentication_test.rb
@@ -1,4 +1,6 @@
require 'abstract_unit'
# FIXME remove DummyKeyGenerator and this require in 4.1
require 'active_support/key_generator'

class HttpDigestAuthenticationTest < ActionController::TestCase
class DummyDigestController < ActionController::Base
Expand Down Expand Up @@ -40,8 +42,8 @@ def authenticate_with_request

setup do
# Used as secret in generating nonce to prevent tampering of timestamp
@secret = "session_options_secret"
@request.env["action_dispatch.secret_token"] = @secret
@secret = "4fb45da9e4ab4ddeb7580d6a35503d99"
@request.env["action_dispatch.key_generator"] = ActiveSupport::DummyKeyGenerator.new(@secret)
end

teardown do
Expand Down