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

Add Key Rotation to MessageEncryptor and MessageVerifier and simplify the Cookies middleware #29716

Merged
merged 2 commits into from Sep 24, 2017

Conversation

Projects
None yet
4 participants
@mikeycgto
Contributor

mikeycgto commented Jul 8, 2017

Summary

This PR introduces ActiveSupport::KeyRotator which wraps KeyGenerator as well as MessageEncryptor and MessageVerifier and provides an interface for easily rotating between different encryption ciphers or message digests, salts, and secrets.

This PR also simplifies the cookies middleware and removes several of the internal legacy middlewares. Upgrading both signed and encrypted legacy cookies is now handled by rotation capable middlewares which each use a KeyRotator instance. Is also introduces a new interface for configuring the middleware.

Other Information

This feature spawned from discussions had in the Rails’ basecamp.

I still need to update the configuring and security markdown guides with details of this change. I plan on adding a new section the security guide detailing how secrets and salts can be rotated. I also want to add more rotation tests to railties/test/application/middleware/cookies_test.rb.

Any and all feedback is welcomed!

/cc @matthewd @kaspth

@bdewater

This comment has been minimized.

Show comment
Hide comment
@bdewater

bdewater Jul 9, 2017

Contributor

Took a brief look at the code, looks nice 👏 One question: the rotator in it's current state seems to be for upgrading cookies and people using MessageEncryptor defaults, which is probably what most people need anyway. But passing in symbols and raising if it doesn't match a known scheme excludes people who use MessageEncryptor now with their own settings.

The shorthands for old and new defaults make sense, but it isn't is possible to pass in your own legacy scheme hash that is just passed on to create a new MessageEncryptor. Likewise "only :aes_gcm may be used for the primary scheme" seems very prescriptive and not in line with the 'provide sharp knives' philosophy. Should we make it a bit more general so it can rotate all configurations of MessageEncryptor?

Contributor

bdewater commented Jul 9, 2017

Took a brief look at the code, looks nice 👏 One question: the rotator in it's current state seems to be for upgrading cookies and people using MessageEncryptor defaults, which is probably what most people need anyway. But passing in symbols and raising if it doesn't match a known scheme excludes people who use MessageEncryptor now with their own settings.

The shorthands for old and new defaults make sense, but it isn't is possible to pass in your own legacy scheme hash that is just passed on to create a new MessageEncryptor. Likewise "only :aes_gcm may be used for the primary scheme" seems very prescriptive and not in line with the 'provide sharp knives' philosophy. Should we make it a bit more general so it can rotate all configurations of MessageEncryptor?

@kaspth

Made a focused review on how I'd like to bring the idea of key rotation to the fore for users (rather than focusing purely on our need to upgrade their cookies) 😊

Appreciate the work you've been doing so far! You seem really comfortable in the codebase 😄

Show outdated Hide outdated actionpack/lib/action_dispatch/railtie.rb Outdated
@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Jul 9, 2017

Contributor

Thanks @bdewater and @kaspth for the feedback!

As far as :cipher options go for the KeyRotator, I think I'm going to drop the symbol usage and just use the same strings MessageEncryptor (and thus OpenSSL) would expect. For instance, instead of using cipher: :aes_gcm we can just use cipher: 'aes-256-gcm'.

One question: the rotator in it's current state seems to be for upgrading cookies and people using MessageEncryptor defaults, which is probably what most people need anyway. But passing in symbols and raising if it doesn't match a known scheme excludes people who use MessageEncryptor now with their own settings.

I agree with this overall, @bdewater. Opening up KeyRotator to work with potentially any cipher option for MessageEncrytor would be a really nice feature. What would the expectation here for using the KeyRotator without a defined preset? The tricky part here is whether or not the supplied secret string should be passed through a KeyGenerator or not.

Maybe, in general, we can leave the :secret option as. It's mostly meant to mirror secret_key_base and should be used with a KeyGenerator and salt for deriving actual keys. Maybe we can then add a alternative :key option to the configuration which is not run through a KeyGenerator is passed directly to either MessageEncryptor or MessageVerifier. This sort of option would be meant for more advanced users who are generating sufficiently random and long keys on their own. The :key option would then not require a :salt option since no key derivation is occurring.

I'm going to rework this class some more such that all potential uses are easily supported in a clear and understandable API. It's far to versatile of a feature to leave for just preset options. Thanks very much for your feedback here!

Likewise "only :aes_gcm may be used for the primary scheme" seems very prescriptive and not in line with the 'provide sharp knives' philosophy.

This comment is actually out-of-date now. Prior to submitting this PR I had limits in place with what could be used as the primary_scheme. My original thinking here is that security APIs should be very easy to use correctly. Maybe we can explore this aspect a bit elsewhere with warnings or something...?

Contributor

mikeycgto commented Jul 9, 2017

Thanks @bdewater and @kaspth for the feedback!

As far as :cipher options go for the KeyRotator, I think I'm going to drop the symbol usage and just use the same strings MessageEncryptor (and thus OpenSSL) would expect. For instance, instead of using cipher: :aes_gcm we can just use cipher: 'aes-256-gcm'.

One question: the rotator in it's current state seems to be for upgrading cookies and people using MessageEncryptor defaults, which is probably what most people need anyway. But passing in symbols and raising if it doesn't match a known scheme excludes people who use MessageEncryptor now with their own settings.

I agree with this overall, @bdewater. Opening up KeyRotator to work with potentially any cipher option for MessageEncrytor would be a really nice feature. What would the expectation here for using the KeyRotator without a defined preset? The tricky part here is whether or not the supplied secret string should be passed through a KeyGenerator or not.

Maybe, in general, we can leave the :secret option as. It's mostly meant to mirror secret_key_base and should be used with a KeyGenerator and salt for deriving actual keys. Maybe we can then add a alternative :key option to the configuration which is not run through a KeyGenerator is passed directly to either MessageEncryptor or MessageVerifier. This sort of option would be meant for more advanced users who are generating sufficiently random and long keys on their own. The :key option would then not require a :salt option since no key derivation is occurring.

I'm going to rework this class some more such that all potential uses are easily supported in a clear and understandable API. It's far to versatile of a feature to leave for just preset options. Thanks very much for your feedback here!

Likewise "only :aes_gcm may be used for the primary scheme" seems very prescriptive and not in line with the 'provide sharp knives' philosophy.

This comment is actually out-of-date now. Prior to submitting this PR I had limits in place with what could be used as the primary_scheme. My original thinking here is that security APIs should be very easy to use correctly. Maybe we can explore this aspect a bit elsewhere with warnings or something...?

@bdewater

This comment has been minimized.

Show comment
Hide comment
@bdewater

bdewater Jul 10, 2017

Contributor

Maybe, in general, we can leave the :secret option as. It's mostly meant to mirror secret_key_base and should be used with a KeyGenerator and salt for deriving actual keys. Maybe we can then add a alternative :key option to the configuration which is not run through a KeyGenerator is passed directly to either MessageEncryptor or MessageVerifier. This sort of option would be meant for more advanced users who are generating sufficiently random and long keys on their own. The :key option would then not require a :salt option since no key derivation is occurring.

This sounds like a great approach! Better defaults while still allowing a configuration to fully preserve existing behaviour for interoperability 👍 My worry with combining MessageEncryptor with secret_key_base and no opt-out is that changing the key base could break more than just existing sessions, e.g. when you're using it to exchange encrypted messages across different (web/mobile/etc) apps that expect AES-256-CBC with HMAC-SHA1 and a long key.

Contributor

bdewater commented Jul 10, 2017

Maybe, in general, we can leave the :secret option as. It's mostly meant to mirror secret_key_base and should be used with a KeyGenerator and salt for deriving actual keys. Maybe we can then add a alternative :key option to the configuration which is not run through a KeyGenerator is passed directly to either MessageEncryptor or MessageVerifier. This sort of option would be meant for more advanced users who are generating sufficiently random and long keys on their own. The :key option would then not require a :salt option since no key derivation is occurring.

This sounds like a great approach! Better defaults while still allowing a configuration to fully preserve existing behaviour for interoperability 👍 My worry with combining MessageEncryptor with secret_key_base and no opt-out is that changing the key base could break more than just existing sessions, e.g. when you're using it to exchange encrypted messages across different (web/mobile/etc) apps that expect AES-256-CBC with HMAC-SHA1 and a long key.

@bdewater

This comment has been minimized.

Show comment
Hide comment
@bdewater

bdewater Jul 10, 2017

Contributor

Also a heads-up: you'll want to add the frozen string magic comment to new files, see #29728 :)

Contributor

bdewater commented Jul 10, 2017

Also a heads-up: you'll want to add the frozen string magic comment to new files, see #29728 :)

Show outdated Hide outdated railties/lib/rails/application.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/railtie.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/cookie_security.rb Outdated
Show outdated Hide outdated activesupport/lib/active_support/key_rotator.rb Outdated
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Jul 17, 2017

Member

I think using both key and secret wording seems confusing. How would users know which has passed through a key_generator and which has not?

I'm having trouble remembering right now for instance 😅

Perhaps it could be generated_key?

Member

kaspth commented Jul 17, 2017

I think using both key and secret wording seems confusing. How would users know which has passed through a key_generator and which has not?

I'm having trouble remembering right now for instance 😅

Perhaps it could be generated_key?

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Jul 17, 2017

Contributor

I think using both key and secret wording seems confusing. How would users know which has passed through a key_generator and which has not?

I'm having trouble remembering right now for instance 😅

Perhaps it could be generated_key?

Maybe raw_key or even raw_key_material is a better name for what it actually is?

This option will also be clearly documented in that it is used as an actual cipher key and should be sufficiently long, random, and properly encoded. That is, it should probably be a raw byte string from SecureRandom or something similar. It should not be encoded as a hex string or otherwise either, which can potentially lead to losses in entropy.

Contributor

mikeycgto commented Jul 17, 2017

I think using both key and secret wording seems confusing. How would users know which has passed through a key_generator and which has not?

I'm having trouble remembering right now for instance 😅

Perhaps it could be generated_key?

Maybe raw_key or even raw_key_material is a better name for what it actually is?

This option will also be clearly documented in that it is used as an actual cipher key and should be sufficiently long, random, and properly encoded. That is, it should probably be a raw byte string from SecureRandom or something similar. It should not be encoded as a hex string or otherwise either, which can potentially lead to losses in entropy.

@mikeycgto mikeycgto changed the title from Add ActiveSupport::KeyRotator and simplify the Cookies middleware to Add Key Rotation to MessageEncryptor and MessageVerifier and simplify the Cookies middleware Aug 1, 2017

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Aug 1, 2017

Contributor

With some great advice from @kaspth, I reworked this PR and added the rotation features directly to the MessageEncryptor and MessageVerifier classes with the help of some small mixins. Overall this approach feels a lot better and it addresses a lot of the feedback raised in this PR.

There are still some failing tests with regards to "upgrading" secret_token HMAC cookies to encrypted cookies. I should be able to easily handle this case in the cookies middleware directly with a legacy verifier in the EncryptedKeyRotatingCookieJar middleware class. I also need to add some more docs and update the configuration guide with notes on several new cookie middleware configuration options. These options should give a high degree of flexibility to developers and will enable them to use different digests for signed cookies (like SHA256) and different ciphers for encrypted cookies.

As always, any and all feedback is welcomed!

Contributor

mikeycgto commented Aug 1, 2017

With some great advice from @kaspth, I reworked this PR and added the rotation features directly to the MessageEncryptor and MessageVerifier classes with the help of some small mixins. Overall this approach feels a lot better and it addresses a lot of the feedback raised in this PR.

There are still some failing tests with regards to "upgrading" secret_token HMAC cookies to encrypted cookies. I should be able to easily handle this case in the cookies middleware directly with a legacy verifier in the EncryptedKeyRotatingCookieJar middleware class. I also need to add some more docs and update the configuration guide with notes on several new cookie middleware configuration options. These options should give a high degree of flexibility to developers and will enable them to use different digests for signed cookies (like SHA256) and different ciphers for encrypted cookies.

As always, any and all feedback is welcomed!

Show outdated Hide outdated actionpack/lib/action_dispatch.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
module ActiveSupport
module Messages
module Rotator # :nodoc:

This comment has been minimized.

@rafaelfranca

rafaelfranca Aug 14, 2017

Member

This will make all the constants inside it to be not documented.

@rafaelfranca

rafaelfranca Aug 14, 2017

Member

This will make all the constants inside it to be not documented.

@rafaelfranca rafaelfranca added this to the 5.2.0 milestone Aug 14, 2017

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Aug 19, 2017

Contributor

One last change I'm considering making to this PR is to provide the default salt values for both middleware's expand_outbound_rotation methods. This way, when defining rotation options, users do not have redefine the default salt values. Thoughts?

Note that, this is a little more involved than a simple default because of the different in salt values for different encryption schemes (CBC mode vs GCM cookies).

Contributor

mikeycgto commented Aug 19, 2017

One last change I'm considering making to this PR is to provide the default salt values for both middleware's expand_outbound_rotation methods. This way, when defining rotation options, users do not have redefine the default salt values. Thoughts?

Note that, this is a little more involved than a simple default because of the different in salt values for different encryption schemes (CBC mode vs GCM cookies).

@kaspth

All I had time to review today. I'll check back in later.

We need to find a different way to be notified of messages being verified/decrypted with legacy verifiers. Executing that block reads poorly 😊

Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 4, 2017

Contributor

Sounds good @kaspth!

Going to think about a different approach for updating cookies that verified/decrypted with a rotated instance. I agree the return within the block is hard to read and follow the execution flow.

Contributor

mikeycgto commented Sep 4, 2017

Sounds good @kaspth!

Going to think about a different approach for updating cookies that verified/decrypted with a rotated instance. I agree the return within the block is hard to read and follow the execution flow.

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 5, 2017

Contributor

Keeping the optional block for verify and decrypt_and_verify, we could do something like:

         def parse(name, signed_message)
+          deserialized = nil
+
           verified_message = @verifier.verify(signed_message) do |message|
-            return deserialize(name, message).tap { |deserialized| self[name] = { value: deserialized } }
+            deserialized = deserialize(name, message)
+            self[name] = { value: deserialized }
           end
 
-          deserialize name, verified_message
+          deserialized || deserialize(name, verified_message)

This should help improve the readability and avoid having a return for parse method within the "rotated instance used" block.

I still don't love this code though. An alternative would be to have separate method for explicitly trying to verify/decrypt with rotated instances. This leads to a less elegant API though.

Contributor

mikeycgto commented Sep 5, 2017

Keeping the optional block for verify and decrypt_and_verify, we could do something like:

         def parse(name, signed_message)
+          deserialized = nil
+
           verified_message = @verifier.verify(signed_message) do |message|
-            return deserialize(name, message).tap { |deserialized| self[name] = { value: deserialized } }
+            deserialized = deserialize(name, message)
+            self[name] = { value: deserialized }
           end
 
-          deserialize name, verified_message
+          deserialized || deserialize(name, verified_message)

This should help improve the readability and avoid having a return for parse method within the "rotated instance used" block.

I still don't love this code though. An alternative would be to have separate method for explicitly trying to verify/decrypt with rotated instances. This leads to a less elegant API though.

@kaspth

What I had time for in this pass, I'll be making another one. Let me know what you think :)

Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated railties/test/application/middleware/cookies_test.rb Outdated
Show outdated Hide outdated activesupport/lib/active_support/messages/rotator.rb Outdated
Show outdated Hide outdated activesupport/lib/active_support/messages/rotator.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 8, 2017

Contributor

Added more tests around the key option handling, which was just moved to the ActiveSupport::Messages::Rotator module. Definitely a lot better to integrate this code at this level since it will make MessageEncrytor/MessageVerifier more useful in general.

Refactored the cookies middleware to use these changes as well. Not sure I totally understand how you intended the on_legacy_rotation method to be used, so let me know what you think about changes to the two middleware's parse methods. I changed on_legacy_rotation to on_rotation_used since I think we should avoid using the word "legacy" as it has a lot of other potential meanings.

Contributor

mikeycgto commented Sep 8, 2017

Added more tests around the key option handling, which was just moved to the ActiveSupport::Messages::Rotator module. Definitely a lot better to integrate this code at this level since it will make MessageEncrytor/MessageVerifier more useful in general.

Refactored the cookies middleware to use these changes as well. Not sure I totally understand how you intended the on_legacy_rotation method to be used, so let me know what you think about changes to the two middleware's parse methods. I changed on_legacy_rotation to on_rotation_used since I think we should avoid using the word "legacy" as it has a lot of other potential meanings.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 10, 2017

Member

Yeah, that's what I had in mind for the on_rotation_used thing. Though I'm not too fond of the way it ended up looking.

I've worked on another alternative here by adding an on_rotation option. That'll then be called when the verifier/encryptor has resorted to rotating. (I realized it shouldn't take a message since it's just a notifier that rotation has taken place.)

It could prove dangerous in case somebody has subclassed verifier to override verified/verify, same for the encryptor, because there's now an extra keyword argument. But I don't know if people would be doing that.

I've also noticed that we're rotating on verify when we should hook in at the verified level instead. Then we'll support rotation for both.

To your worry about deserializing twice, I figured "hey, why not just let deserialize control when we've been rotating". It already handled upgrading for the serializer change, so it makes the best spot for this upgrading as well.
deserialize is a bit more bloated now, but the case statement really clears up the different code paths.

What do you think? 😊

diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb
index a752329497..2b8d8862dd 100644
--- a/actionpack/lib/action_dispatch/middleware/cookies.rb
+++ b/actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -535,12 +535,16 @@ def serialize(value)
           serializer.dump(value)
         end
 
-        def deserialize(name, value)
+        def deserialize(name)
+          rotate = false
+          value  = yield -> { rotate = true }
+
           if value
-            if needs_migration?(value)
-              Marshal.load(value).tap do |v|
-                self[name] = { value: v }
-              end
+            case
+            when needs_migration?(value)
+              self[name] = Marshal.load(value)
+            when rotate
+              self[name] = serializer.load(value)
             else
               serializer.load(value)
             end
@@ -584,16 +588,9 @@ def initialize(parent_jar)
 
       private
         def parse(name, signed_message)
-          write_upgraded_cookie = proc do |message|
-            return deserialize(name, message).tap { |value| self[name] = { value: value } }
-          end
-
-          @verifier.on_rotation_used(write_upgraded_cookie) do
-            deserialize(name, @verifier.verify(signed_message))
+          deserialize(name) do |rotate|
+            @verifier.verified(signed_message, on_rotation: rotate)
           end
-
-        rescue ActiveSupport::MessageVerifier::InvalidSignature
-          nil
         end
 
         def commit(options)
@@ -632,18 +629,11 @@ def initialize(parent_jar)
 
       private
         def parse(name, encrypted_message)
-          write_upgraded_cookie = proc do |message|
-            return deserialize(name, message).tap { |value| self[name] = { value: value } }
-          end
-
-          @encryptor.on_rotation_used(write_upgraded_cookie) do
-            deserialize(name, @encryptor.decrypt_and_verify(encrypted_message))
+          deserialize(name) do |rotate|
+            @encryptor.decrypt_and_verify(encrypted_message, on_rotation: rotate)
           end
-
         rescue ActiveSupport::MessageEncryptor::InvalidMessage, ActiveSupport::MessageVerifier::InvalidSignature
-          return try_legacy_verifier name, encrypted_message if defined? @legacy_verifier
-
-          nil
+          parse_legacy_signed_message(name, encrypted_message)
         end
 
         def commit(options)
@@ -652,13 +642,14 @@ def commit(options)
           raise CookieOverflow if options[:value].bytesize > MAX_COOKIE_SIZE
         end
 
-        def try_legacy_verifier(name, encrypted_message)
-          deserialize(name, @legacy_verifier.verify(encrypted_message)).tap do |deserialize|
-            self[name] = deserialize
-          end
+        def parse_legacy_signed_message(name, legacy_signed_message)
+          if defined?(@legacy_verifier)
+            deserialize(name) do |rotate|
+              rotate.call
 
-        rescue ActiveSupport::MessageVerifier::InvalidSignature
-          nil
+              @legacy_verifier.verified(legacy_signed_message)
+            end
+          end
         end
     end
 
diff --git a/activesupport/lib/active_support/message_encryptor.rb b/activesupport/lib/active_support/message_encryptor.rb
index aae3aab706..2018233f56 100644
--- a/activesupport/lib/active_support/message_encryptor.rb
+++ b/activesupport/lib/active_support/message_encryptor.rb
@@ -138,7 +138,7 @@ def encrypt_and_sign(value, expires_at: nil, expires_in: nil, purpose: nil)
 
     # Decrypt and verify a message. We need to verify the message in order to
     # avoid padding attacks. Reference: https://www.limited-entropy.com/padding-oracle-attacks/.
-    def decrypt_and_verify(data, purpose: nil)
+    def decrypt_and_verify(data, purpose: nil, **)
       _decrypt(verifier.verify(data), purpose)
     end
 
diff --git a/activesupport/lib/active_support/message_verifier.rb b/activesupport/lib/active_support/message_verifier.rb
index 36d85b9500..6aa4d9e2b9 100644
--- a/activesupport/lib/active_support/message_verifier.rb
+++ b/activesupport/lib/active_support/message_verifier.rb
@@ -133,7 +133,7 @@ def valid_message?(signed_message)
     #
     #   incompatible_message = "test--dad7b06c94abba8d46a15fafaef56c327665d5ff"
     #   verifier.verified(incompatible_message) # => TypeError: incompatible marshal file format
-    def verified(signed_message, purpose: nil)
+    def verified(signed_message, purpose: nil, **)
       if valid_message?(signed_message)
         begin
           data = signed_message.split("--".freeze)[0]
@@ -158,8 +158,8 @@ def verified(signed_message, purpose: nil)
     #
     #   other_verifier = ActiveSupport::MessageVerifier.new 'd1ff3r3nt-s3Krit'
     #   other_verifier.verify(signed_message) # => ActiveSupport::MessageVerifier::InvalidSignature
-    def verify(signed_message, purpose: nil)
-      verified(signed_message, purpose: purpose) || raise(InvalidSignature)
+    def verify(*args)
+      verified(*args) || raise(InvalidSignature)
     end
 
     # Generates a signed message for the provided value.
diff --git a/activesupport/lib/active_support/messages/rotator.rb b/activesupport/lib/active_support/messages/rotator.rb
index a1f7ab891f..14e9ed05ce 100644
--- a/activesupport/lib/active_support/messages/rotator.rb
+++ b/activesupport/lib/active_support/messages/rotator.rb
@@ -7,39 +7,24 @@ def initialize(*args)
         super
 
         @rotations = []
-        @on_rotation_used = nil
       end
 
       def rotate(*args)
         @rotations << create_rotation(*args)
       end
 
-      def on_rotation_used(on_rotation)
-        old_on_rotation_used, @on_rotation_used = @on_rotation_used, on_rotation
-
-        yield
-      ensure
-        @on_rotation_used = old_on_rotation_used
-      end
-
-      def has_rotations?
-        !@rotations.empty?
-      end
-
       module Encryptor
         include Rotator
 
         AES_CBC_EXPRESSION = /\Aaes-\d+-cbc\z/i.freeze
 
-        def decrypt_and_verify(*args)
+        def decrypt_and_verify(*args, on_rotation: nil)
           super
-
-        rescue MessageEncryptor::InvalidMessage, MessageVerifier::InvalidSignature => error
-          find_first_rotation { |encryptor| encryptor.decrypt_and_verify(*args) } || raise(error)
+        rescue MessageEncryptor::InvalidMessage, MessageVerifier::InvalidSignature
+          run_rotations(on_rotation) { |encryptor| encryptor.decrypt_and_verify(*args) } || raise
         end
 
         private
-
           def create_rotation(raw_key: nil, raw_signed_key: nil, **options)
             self.class.new \
               raw_key || extract_key(options),
@@ -65,15 +50,15 @@ def extract_signing_key(cipher:, signed_salt: nil, key_generator: nil, secret: n
       module Verifier
         include Rotator
 
-        def verify(*args)
-          super
-
-        rescue MessageVerifier::InvalidSignature => error
-          find_first_rotation { |verifier| verifier.verify(*args) } || raise(error)
+        def verified(*args, on_rotation: nil)
+          if verified = super
+            verified
+          else
+            run_rotations(on_rotation) { |verifier| verifier.verified(*args) }
+          end
         end
 
         private
-
           def create_rotation(raw_key: nil, digest: nil, serializer: nil, **options)
             self.class.new(raw_key || extract_key(options), digest: digest, serializer: serializer)
           end
@@ -85,18 +70,16 @@ def extract_key(key_generator: nil, secret: nil, salt:)
       end
 
       private
-
         def key_generator_for(secret)
           ActiveSupport::KeyGenerator.new(secret, iterations: 1000)
         end
 
-        def call_on_rotation_used(message)
-          @on_rotation_used.call(message) if @on_rotation_used
-        end
-
-        def find_first_rotation
+        def run_rotations(on_rotation)
           @rotations.find do |rotation|
-            return yield(rotation).tap { |message| call_on_rotation_used message } rescue next
+            if message = yield(rotation) rescue next
+              on_rotation.call if on_rotation
+              return message
+            end
           end
         end
     end
Member

kaspth commented Sep 10, 2017

Yeah, that's what I had in mind for the on_rotation_used thing. Though I'm not too fond of the way it ended up looking.

I've worked on another alternative here by adding an on_rotation option. That'll then be called when the verifier/encryptor has resorted to rotating. (I realized it shouldn't take a message since it's just a notifier that rotation has taken place.)

It could prove dangerous in case somebody has subclassed verifier to override verified/verify, same for the encryptor, because there's now an extra keyword argument. But I don't know if people would be doing that.

I've also noticed that we're rotating on verify when we should hook in at the verified level instead. Then we'll support rotation for both.

To your worry about deserializing twice, I figured "hey, why not just let deserialize control when we've been rotating". It already handled upgrading for the serializer change, so it makes the best spot for this upgrading as well.
deserialize is a bit more bloated now, but the case statement really clears up the different code paths.

What do you think? 😊

diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb
index a752329497..2b8d8862dd 100644
--- a/actionpack/lib/action_dispatch/middleware/cookies.rb
+++ b/actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -535,12 +535,16 @@ def serialize(value)
           serializer.dump(value)
         end
 
-        def deserialize(name, value)
+        def deserialize(name)
+          rotate = false
+          value  = yield -> { rotate = true }
+
           if value
-            if needs_migration?(value)
-              Marshal.load(value).tap do |v|
-                self[name] = { value: v }
-              end
+            case
+            when needs_migration?(value)
+              self[name] = Marshal.load(value)
+            when rotate
+              self[name] = serializer.load(value)
             else
               serializer.load(value)
             end
@@ -584,16 +588,9 @@ def initialize(parent_jar)
 
       private
         def parse(name, signed_message)
-          write_upgraded_cookie = proc do |message|
-            return deserialize(name, message).tap { |value| self[name] = { value: value } }
-          end
-
-          @verifier.on_rotation_used(write_upgraded_cookie) do
-            deserialize(name, @verifier.verify(signed_message))
+          deserialize(name) do |rotate|
+            @verifier.verified(signed_message, on_rotation: rotate)
           end
-
-        rescue ActiveSupport::MessageVerifier::InvalidSignature
-          nil
         end
 
         def commit(options)
@@ -632,18 +629,11 @@ def initialize(parent_jar)
 
       private
         def parse(name, encrypted_message)
-          write_upgraded_cookie = proc do |message|
-            return deserialize(name, message).tap { |value| self[name] = { value: value } }
-          end
-
-          @encryptor.on_rotation_used(write_upgraded_cookie) do
-            deserialize(name, @encryptor.decrypt_and_verify(encrypted_message))
+          deserialize(name) do |rotate|
+            @encryptor.decrypt_and_verify(encrypted_message, on_rotation: rotate)
           end
-
         rescue ActiveSupport::MessageEncryptor::InvalidMessage, ActiveSupport::MessageVerifier::InvalidSignature
-          return try_legacy_verifier name, encrypted_message if defined? @legacy_verifier
-
-          nil
+          parse_legacy_signed_message(name, encrypted_message)
         end
 
         def commit(options)
@@ -652,13 +642,14 @@ def commit(options)
           raise CookieOverflow if options[:value].bytesize > MAX_COOKIE_SIZE
         end
 
-        def try_legacy_verifier(name, encrypted_message)
-          deserialize(name, @legacy_verifier.verify(encrypted_message)).tap do |deserialize|
-            self[name] = deserialize
-          end
+        def parse_legacy_signed_message(name, legacy_signed_message)
+          if defined?(@legacy_verifier)
+            deserialize(name) do |rotate|
+              rotate.call
 
-        rescue ActiveSupport::MessageVerifier::InvalidSignature
-          nil
+              @legacy_verifier.verified(legacy_signed_message)
+            end
+          end
         end
     end
 
diff --git a/activesupport/lib/active_support/message_encryptor.rb b/activesupport/lib/active_support/message_encryptor.rb
index aae3aab706..2018233f56 100644
--- a/activesupport/lib/active_support/message_encryptor.rb
+++ b/activesupport/lib/active_support/message_encryptor.rb
@@ -138,7 +138,7 @@ def encrypt_and_sign(value, expires_at: nil, expires_in: nil, purpose: nil)
 
     # Decrypt and verify a message. We need to verify the message in order to
     # avoid padding attacks. Reference: https://www.limited-entropy.com/padding-oracle-attacks/.
-    def decrypt_and_verify(data, purpose: nil)
+    def decrypt_and_verify(data, purpose: nil, **)
       _decrypt(verifier.verify(data), purpose)
     end
 
diff --git a/activesupport/lib/active_support/message_verifier.rb b/activesupport/lib/active_support/message_verifier.rb
index 36d85b9500..6aa4d9e2b9 100644
--- a/activesupport/lib/active_support/message_verifier.rb
+++ b/activesupport/lib/active_support/message_verifier.rb
@@ -133,7 +133,7 @@ def valid_message?(signed_message)
     #
     #   incompatible_message = "test--dad7b06c94abba8d46a15fafaef56c327665d5ff"
     #   verifier.verified(incompatible_message) # => TypeError: incompatible marshal file format
-    def verified(signed_message, purpose: nil)
+    def verified(signed_message, purpose: nil, **)
       if valid_message?(signed_message)
         begin
           data = signed_message.split("--".freeze)[0]
@@ -158,8 +158,8 @@ def verified(signed_message, purpose: nil)
     #
     #   other_verifier = ActiveSupport::MessageVerifier.new 'd1ff3r3nt-s3Krit'
     #   other_verifier.verify(signed_message) # => ActiveSupport::MessageVerifier::InvalidSignature
-    def verify(signed_message, purpose: nil)
-      verified(signed_message, purpose: purpose) || raise(InvalidSignature)
+    def verify(*args)
+      verified(*args) || raise(InvalidSignature)
     end
 
     # Generates a signed message for the provided value.
diff --git a/activesupport/lib/active_support/messages/rotator.rb b/activesupport/lib/active_support/messages/rotator.rb
index a1f7ab891f..14e9ed05ce 100644
--- a/activesupport/lib/active_support/messages/rotator.rb
+++ b/activesupport/lib/active_support/messages/rotator.rb
@@ -7,39 +7,24 @@ def initialize(*args)
         super
 
         @rotations = []
-        @on_rotation_used = nil
       end
 
       def rotate(*args)
         @rotations << create_rotation(*args)
       end
 
-      def on_rotation_used(on_rotation)
-        old_on_rotation_used, @on_rotation_used = @on_rotation_used, on_rotation
-
-        yield
-      ensure
-        @on_rotation_used = old_on_rotation_used
-      end
-
-      def has_rotations?
-        !@rotations.empty?
-      end
-
       module Encryptor
         include Rotator
 
         AES_CBC_EXPRESSION = /\Aaes-\d+-cbc\z/i.freeze
 
-        def decrypt_and_verify(*args)
+        def decrypt_and_verify(*args, on_rotation: nil)
           super
-
-        rescue MessageEncryptor::InvalidMessage, MessageVerifier::InvalidSignature => error
-          find_first_rotation { |encryptor| encryptor.decrypt_and_verify(*args) } || raise(error)
+        rescue MessageEncryptor::InvalidMessage, MessageVerifier::InvalidSignature
+          run_rotations(on_rotation) { |encryptor| encryptor.decrypt_and_verify(*args) } || raise
         end
 
         private
-
           def create_rotation(raw_key: nil, raw_signed_key: nil, **options)
             self.class.new \
               raw_key || extract_key(options),
@@ -65,15 +50,15 @@ def extract_signing_key(cipher:, signed_salt: nil, key_generator: nil, secret: n
       module Verifier
         include Rotator
 
-        def verify(*args)
-          super
-
-        rescue MessageVerifier::InvalidSignature => error
-          find_first_rotation { |verifier| verifier.verify(*args) } || raise(error)
+        def verified(*args, on_rotation: nil)
+          if verified = super
+            verified
+          else
+            run_rotations(on_rotation) { |verifier| verifier.verified(*args) }
+          end
         end
 
         private
-
           def create_rotation(raw_key: nil, digest: nil, serializer: nil, **options)
             self.class.new(raw_key || extract_key(options), digest: digest, serializer: serializer)
           end
@@ -85,18 +70,16 @@ def extract_key(key_generator: nil, secret: nil, salt:)
       end
 
       private
-
         def key_generator_for(secret)
           ActiveSupport::KeyGenerator.new(secret, iterations: 1000)
         end
 
-        def call_on_rotation_used(message)
-          @on_rotation_used.call(message) if @on_rotation_used
-        end
-
-        def find_first_rotation
+        def run_rotations(on_rotation)
           @rotations.find do |rotation|
-            return yield(rotation).tap { |message| call_on_rotation_used message } rescue next
+            if message = yield(rotation) rescue next
+              on_rotation.call if on_rotation
+              return message
+            end
           end
         end
     end
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated guides/source/security.md Outdated
@kaspth

Think this is coming together really nicely!

Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/middleware/cookies.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/railtie.rb Outdated
Show outdated Hide outdated activesupport/lib/active_support/messages/rotator.rb Outdated
self.class.new(raw_key || extract_key(options), digest: digest, serializer: serializer)
end
def extract_key(key_generator: nil, secret: nil, salt:)

This comment has been minimized.

@kaspth

kaspth Sep 12, 2017

Member

Will this need an extra ** or have all the other options been removed by here?

@kaspth

kaspth Sep 12, 2017

Member

Will this need an extra ** or have all the other options been removed by here?

This comment has been minimized.

@mikeycgto

mikeycgto Sep 12, 2017

Contributor

See my patch and diff below. Keywords from the metadata need to pass through these methods.

@mikeycgto

mikeycgto Sep 12, 2017

Contributor

See my patch and diff below. Keywords from the metadata need to pass through these methods.

This comment has been minimized.

@kaspth

kaspth Sep 14, 2017

Member

If the keywords from the metadata (:purpose I assume?) need to pass into this method, why isn't there a capturing ** at the end?

If we don't need it, great! I just wanted to make sure it wasn't forgotten 😊

@kaspth

kaspth Sep 14, 2017

Member

If the keywords from the metadata (:purpose I assume?) need to pass into this method, why isn't there a capturing ** at the end?

If we don't need it, great! I just wanted to make sure it wasn't forgotten 😊

@@ -487,6 +487,20 @@ Defaults to `'signed cookie'`.
authenticated encrypted cookie salt. Defaults to `'authenticated encrypted
cookie'`.
* `config.action_dispatch.encrypted_cookie_cipher` sets the cipher to be
used for encrypted cookies. This defaults to `"aes-256-gcm"`.

This comment has been minimized.

@kaspth

kaspth Sep 12, 2017

Member

Does it make sense for us to let users change this?

@kaspth

kaspth Sep 12, 2017

Member

Does it make sense for us to let users change this?

This comment has been minimized.

@mikeycgto

mikeycgto Sep 12, 2017

Contributor

I think it's good to let this be configurable. I have this end goal of getting Rails to work with chacha20 support for those who want to get more hands on with OpenSSL deps.

@mikeycgto

mikeycgto Sep 12, 2017

Contributor

I think it's good to let this be configurable. I have this end goal of getting Rails to work with chacha20 support for those who want to get more hands on with OpenSSL deps.

This comment has been minimized.

@kaspth

kaspth Sep 14, 2017

Member

👍

@kaspth

kaspth Sep 14, 2017

Member

👍

Show outdated Hide outdated activesupport/lib/active_support/message_encryptor.rb Outdated
Show outdated Hide outdated guides/source/security.md Outdated
@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 12, 2017

Contributor

I like this patch a lot. The on_rotation seems like the best approach so far. I also think its easier to understand API-wise as well. Good call on moving the MessageVerifier changes to verified instead of verify. Definitely belongs on that method and not verify.

One small note, I had to add the following to both the verified and decrypt_and_verify:

diff --git a/activesupport/lib/active_support/messages/rotator.rb b/activesupport/lib/active_support/messages/rotator.rb
index 14e9ed0..1379d58 100644
--- a/activesupport/lib/active_support/messages/rotator.rb
+++ b/activesupport/lib/active_support/messages/rotator.rb
@@ -18,7 +18,7 @@ module Encryptor
 
         AES_CBC_EXPRESSION = /\Aaes-\d+-cbc\z/i.freeze
 
-        def decrypt_and_verify(*args, on_rotation: nil)
+        def decrypt_and_verify(*args, on_rotation: nil, **)
           super
         rescue MessageEncryptor::InvalidMessage, MessageVerifier::InvalidSignature
           run_rotations(on_rotation) { |encryptor| encryptor.decrypt_and_verify(*args) } || raise
@@ -50,7 +50,7 @@ def extract_signing_key(cipher:, signed_salt: nil, key_generator: nil, secret: n
       module Verifier
         include Rotator
 
-        def verified(*args, on_rotation: nil)
+        def verified(*args, on_rotation: nil, **)
           if verified = super
             verified
           else

Without the keyword splatting, lots of MessageEncryptor/MessageVerifier tests were raising ArgumentErrors with "unknown keyword: purpose"

Contributor

mikeycgto commented Sep 12, 2017

I like this patch a lot. The on_rotation seems like the best approach so far. I also think its easier to understand API-wise as well. Good call on moving the MessageVerifier changes to verified instead of verify. Definitely belongs on that method and not verify.

One small note, I had to add the following to both the verified and decrypt_and_verify:

diff --git a/activesupport/lib/active_support/messages/rotator.rb b/activesupport/lib/active_support/messages/rotator.rb
index 14e9ed0..1379d58 100644
--- a/activesupport/lib/active_support/messages/rotator.rb
+++ b/activesupport/lib/active_support/messages/rotator.rb
@@ -18,7 +18,7 @@ module Encryptor
 
         AES_CBC_EXPRESSION = /\Aaes-\d+-cbc\z/i.freeze
 
-        def decrypt_and_verify(*args, on_rotation: nil)
+        def decrypt_and_verify(*args, on_rotation: nil, **)
           super
         rescue MessageEncryptor::InvalidMessage, MessageVerifier::InvalidSignature
           run_rotations(on_rotation) { |encryptor| encryptor.decrypt_and_verify(*args) } || raise
@@ -50,7 +50,7 @@ def extract_signing_key(cipher:, signed_salt: nil, key_generator: nil, secret: n
       module Verifier
         include Rotator
 
-        def verified(*args, on_rotation: nil)
+        def verified(*args, on_rotation: nil, **)
           if verified = super
             verified
           else

Without the keyword splatting, lots of MessageEncryptor/MessageVerifier tests were raising ArgumentErrors with "unknown keyword: purpose"

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 13, 2017

Contributor

Once we're sure this is the configuration API we want, I can update the security and configuration guides as needed. Just want us to be sure this is the API before I do that 😃

Contributor

mikeycgto commented Sep 13, 2017

Once we're sure this is the configuration API we want, I can update the security and configuration guides as needed. Just want us to be sure this is the API before I do that 😃

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 13, 2017

Contributor

This latest merge reminded me of the following code we're now removing in the cookies middleware. Should this check for a missing secret_key_base exist elsewhere in the framework?

-       if ActiveSupport::LegacyKeyGenerator === key_generator
-          raise "You didn't set secret_key_base, which is required for this cookie jar. " \
-            "Read the upgrade documentation to learn more about this new config option."
Contributor

mikeycgto commented Sep 13, 2017

This latest merge reminded me of the following code we're now removing in the cookies middleware. Should this check for a missing secret_key_base exist elsewhere in the framework?

-       if ActiveSupport::LegacyKeyGenerator === key_generator
-          raise "You didn't set secret_key_base, which is required for this cookie jar. " \
-            "Read the upgrade documentation to learn more about this new config option."
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 14, 2017

Member

One small note, I had to add the following to both the verified and decrypt_and_verify

Huh, strange. I didn't have to add that when I tried the diff and ran some of the action_dispatch tests. But perhaps I was missing the Active Support tests.

Remember that it can't just be ** it has to be **options so we can pass them onto verifier.verified(*args, **options) etc.

Member

kaspth commented Sep 14, 2017

One small note, I had to add the following to both the verified and decrypt_and_verify

Huh, strange. I didn't have to add that when I tried the diff and ran some of the action_dispatch tests. But perhaps I was missing the Active Support tests.

Remember that it can't just be ** it has to be **options so we can pass them onto verifier.verified(*args, **options) etc.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 14, 2017

Member

Should this check for a missing secret_key_base exist elsewhere in the framework?

Yup, and it does! After the credentials thing, we're now validating Rails.application.secret_key_base when it's called with this:

def validate_secret_key_base(secret_key_base)
if secret_key_base.is_a?(String) && secret_key_base.present?
secret_key_base
elsif secret_key_base
raise ArgumentError, "`secret_key_base` for #{Rails.env} environment must be a type of String`"
elsif secrets.secret_token.blank?
raise ArgumentError, "Missing `secret_key_base` for '#{Rails.env}' environment, set this string with `rails credentials:edit`"
end
end

Also noticed that other places were checking for a missing secret_key_base and raising, so I'm happy to see that get a bit streamlined. (Think there was one within the KeyGenerator class too).

Member

kaspth commented Sep 14, 2017

Should this check for a missing secret_key_base exist elsewhere in the framework?

Yup, and it does! After the credentials thing, we're now validating Rails.application.secret_key_base when it's called with this:

def validate_secret_key_base(secret_key_base)
if secret_key_base.is_a?(String) && secret_key_base.present?
secret_key_base
elsif secret_key_base
raise ArgumentError, "`secret_key_base` for #{Rails.env} environment must be a type of String`"
elsif secrets.secret_token.blank?
raise ArgumentError, "Missing `secret_key_base` for '#{Rails.env}' environment, set this string with `rails credentials:edit`"
end
end

Also noticed that other places were checking for a missing secret_key_base and raising, so I'm happy to see that get a bit streamlined. (Think there was one within the KeyGenerator class too).

Show outdated Hide outdated actionpack/lib/action_dispatch/railtie.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/railtie.rb Outdated
Show outdated Hide outdated actionpack/lib/action_dispatch/railtie.rb Outdated
@@ -21,12 +21,17 @@ class Railtie < Rails::Railtie # :nodoc:
config.action_dispatch.use_authenticated_cookie_encryption = false

This comment has been minimized.

@kaspth

kaspth Sep 14, 2017

Member

There's a part of me that thinks we should turn:

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.use_authenticated_cookie_encryption = false

into:

config.action_dispatch.cookies = ActiveSupport::OrderedOptions.new
config.action_dispatch.cookies.signed_salt = "signed cookie"
config.action_dispatch.cookies.encrypted_salt = "encrypted cookie"
config.action_dispatch.cookies.encrypted_signed_salt = "signed encrypted cookie"
# These three are old 👆, but these three haven't shipped 👇.
config.action_dispatch.cookies.authenticated_encrypted_salt = "authenticated encrypted cookie"
config.action_dispatch.cookies.authenticated_encryption = false
config.action_dispatch.cookies.rotations = ActiveSupport::Messages::RotationConfiguration.new

We'd need to deprecate the old ones. Though I think it's highly unlikely that many apps have overriden those default salts, so the hurt isn't too huge. Either that or we just reassign for something so trivial.
It's probably out of scope for this, so let me look into that separately.

Though by the way, why do we need the authenticated_encrypted_cookie_salt? We could just as well reuse the encrypted_signed_cookie_salt?

@kaspth

kaspth Sep 14, 2017

Member

There's a part of me that thinks we should turn:

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.use_authenticated_cookie_encryption = false

into:

config.action_dispatch.cookies = ActiveSupport::OrderedOptions.new
config.action_dispatch.cookies.signed_salt = "signed cookie"
config.action_dispatch.cookies.encrypted_salt = "encrypted cookie"
config.action_dispatch.cookies.encrypted_signed_salt = "signed encrypted cookie"
# These three are old 👆, but these three haven't shipped 👇.
config.action_dispatch.cookies.authenticated_encrypted_salt = "authenticated encrypted cookie"
config.action_dispatch.cookies.authenticated_encryption = false
config.action_dispatch.cookies.rotations = ActiveSupport::Messages::RotationConfiguration.new

We'd need to deprecate the old ones. Though I think it's highly unlikely that many apps have overriden those default salts, so the hurt isn't too huge. Either that or we just reassign for something so trivial.
It's probably out of scope for this, so let me look into that separately.

Though by the way, why do we need the authenticated_encrypted_cookie_salt? We could just as well reuse the encrypted_signed_cookie_salt?

This comment has been minimized.

@mikeycgto

mikeycgto Sep 14, 2017

Contributor

When I added AEAD support I wanted to use a different salt value as this is a best practice when deriving keys for different uses.

@mikeycgto

mikeycgto Sep 14, 2017

Contributor

When I added AEAD support I wanted to use a different salt value as this is a best practice when deriving keys for different uses.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 14, 2017

Member

Yup, let's move forward with that configuration API :)

Member

kaspth commented Sep 14, 2017

Yup, let's move forward with that configuration API :)

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 14, 2017

Contributor

Remember that it can't just be ** it has to be **options so we can pass them onto verifier.verified(*args, **options) etc.

Good call on this. I can update those calls and even add a test case for it.

Contributor

mikeycgto commented Sep 14, 2017

Remember that it can't just be ** it has to be **options so we can pass them onto verifier.verified(*args, **options) etc.

Good call on this. I can update those calls and even add a test case for it.

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 14, 2017

Contributor

Yup, and it does! After the credentials thing, we're now validating Rails.application.secret_key_base when it's called with this:

Nice to "centralized" method for validating the configuration!

Contributor

mikeycgto commented Sep 14, 2017

Yup, and it does! After the credentials thing, we're now validating Rails.application.secret_key_base when it's called with this:

Nice to "centralized" method for validating the configuration!

Show outdated Hide outdated activesupport/lib/active_support/message_encryptor.rb Outdated
Show outdated Hide outdated activesupport/lib/active_support/messages.rb Outdated
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 17, 2017

Member

Looks like the documentation update is missing and then we need to do some commit squashing. Perhaps the Active Support changes and then the cookie changes are two separate commits? Unless you can spot a division that's more git history apt 😊

Member

kaspth commented Sep 17, 2017

Looks like the documentation update is missing and then we need to do some commit squashing. Perhaps the Active Support changes and then the cookie changes are two separate commits? Unless you can spot a division that's more git history apt 😊

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 17, 2017

Contributor

@kaspth yup, the docs is next up for me. Sounds good on the squash commit. That division makes the most sense to me.

Contributor

mikeycgto commented Sep 17, 2017

@kaspth yup, the docs is next up for me. Sounds good on the squash commit. That division makes the most sense to me.

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 18, 2017

Contributor

Thought some more about the RotationConfiguration interface and I think we should now allow :encrypted and :signed rotations to be defined at the same time. The options differ slightly and I think we might run into some problems as a result.

Additionally, allowing both encrypted and signed cookies to have the same configuration would lead to the same salt and thus the same key being used for different security features which is not a good idea security-wise. Thoughts @kaspth? Should be easy to remove this small part of the class.

Edit: It's probably OK to define both at the same time if just a :secret is provided and no salts are directly provided. I suppose this may actually be the most common use case (ie rotating the secret_key_base while using the default salts).

Contributor

mikeycgto commented Sep 18, 2017

Thought some more about the RotationConfiguration interface and I think we should now allow :encrypted and :signed rotations to be defined at the same time. The options differ slightly and I think we might run into some problems as a result.

Additionally, allowing both encrypted and signed cookies to have the same configuration would lead to the same salt and thus the same key being used for different security features which is not a good idea security-wise. Thoughts @kaspth? Should be easy to remove this small part of the class.

Edit: It's probably OK to define both at the same time if just a :secret is provided and no salts are directly provided. I suppose this may actually be the most common use case (ie rotating the secret_key_base while using the default salts).

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 20, 2017

Contributor

Updated the guides and the documentation some more.

I hope it's OK that I reworked the session cookie section of the Security guide a bit. Just felt odd at this point to have that section lead with legacy details. I'd rather explain security for the current version upfront then explain differences in prior versions after.

Want to just review it again with fresh eyes and then squash the commits.

Contributor

mikeycgto commented Sep 20, 2017

Updated the guides and the documentation some more.

I hope it's OK that I reworked the session cookie section of the Security guide a bit. Just felt odd at this point to have that section lead with legacy details. I'd rather explain security for the current version upfront then explain differences in prior versions after.

Want to just review it again with fresh eyes and then squash the commits.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 23, 2017

Member

Let's use the action_dispatch.use_authenticated_cookie_encryption option in the middleware then squash the commits.

I'm going to be out of the country the week after this. So this has to be wrapped up soon. If you can get to the squashing tomorrow then great. Otherwise I'll merge and do some follow up codewise then 😊

Member

kaspth commented Sep 23, 2017

Let's use the action_dispatch.use_authenticated_cookie_encryption option in the middleware then squash the commits.

I'm going to be out of the country the week after this. So this has to be wrapped up soon. If you can get to the squashing tomorrow then great. Otherwise I'll merge and do some follow up codewise then 😊

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 23, 2017

Contributor

Now I'm actually a bit confused on how action_dispatch.use_authenticated_cookie_encryption should work. Setting this value to false should lead to CBC cookies being used, correct?

For Rails 5.2 this value will default to true and CBC cookie upgrading will be added as a rotation (because the CBC salts are still set and upgrade_legacy_hmac_aes_cbc_cookies? is true).

I wrote a test case for use_authenticated_cookie_encryption to false and it's failing. I think somewhere between this PR and the AEAD cookie PR the EncryptedCookieJar become to simplified. Should be easy to fix this though but I want to make sure I'm clear on what the role of use_authenticated_cookie_encryption exactly is.

Contributor

mikeycgto commented Sep 23, 2017

Now I'm actually a bit confused on how action_dispatch.use_authenticated_cookie_encryption should work. Setting this value to false should lead to CBC cookies being used, correct?

For Rails 5.2 this value will default to true and CBC cookie upgrading will be added as a rotation (because the CBC salts are still set and upgrade_legacy_hmac_aes_cbc_cookies? is true).

I wrote a test case for use_authenticated_cookie_encryption to false and it's failing. I think somewhere between this PR and the AEAD cookie PR the EncryptedCookieJar become to simplified. Should be easy to fix this though but I want to make sure I'm clear on what the role of use_authenticated_cookie_encryption exactly is.

Add key rotation message Encryptor and Verifier
Both classes now have a rotate method where new instances are added for
each call. When decryption or verification fails the next rotation
instance is tried.
@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 23, 2017

Contributor

Squashed the commits. I think I broke something with the use_authenticated_cookie_encryption change. Will try to fix that ASAP.

Contributor

mikeycgto commented Sep 23, 2017

Squashed the commits. I think I broke something with the use_authenticated_cookie_encryption change. Will try to fix that ASAP.

Add key rotation cookies middleware
Using the action_dispatch.cookies_rotations interface, key rotation is
now possible with cookies. Thus the secret_key_base as well as salts,
ciphers, and digests, can be rotated without expiring sessions.

@kaspth kaspth merged commit 36888b9 into rails:master Sep 24, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 24, 2017

Member

Great! The build looked fine and green to me 😄👏

Member

kaspth commented Sep 24, 2017

Great! The build looked fine and green to me 😄👏

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 24, 2017

Contributor

One last thing, @kaspth: what about having the cookies middleware use CBC encryption when use_authenticated_cookie_encryption is set to false? I can add a test case and small patch to the cookies middleware to support this in a separate PR if need be.

Contributor

mikeycgto commented Sep 24, 2017

One last thing, @kaspth: what about having the cookies middleware use CBC encryption when use_authenticated_cookie_encryption is set to false? I can add a test case and small patch to the cookies middleware to support this in a separate PR if need be.

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 24, 2017

Member

What's it doing currently? I think false and nil should probably work the same for that config (depending on which Rails version the app was generated on).

Member

kaspth commented Sep 24, 2017

What's it doing currently? I think false and nil should probably work the same for that config (depending on which Rails version the app was generated on).

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 24, 2017

Contributor

Currently it's just responsible for enabling the upgrade behavior, which rotates CBC to GCM. I feel setting it to false should also lead to CBC being used. The following patch should suffice:

diff actionpack/lib/
diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb
index b383164..dbe46be 100644
--- a/actionpack/lib/action_dispatch/middleware/cookies.rb
+++ b/actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -599,9 +599,16 @@ class EncryptedKeyRotatingCookieJar < AbstractCookieJar # :nodoc:
       def initialize(parent_jar)
         super
 
-        key_len = ActiveSupport::MessageEncryptor.key_len(encrypted_cookie_cipher)
-        secret = request.key_generator.generate_key(request.authenticated_encrypted_cookie_salt, key_len)
-        @encryptor = ActiveSupport::MessageEncryptor.new(secret, cipher: encrypted_cookie_cipher, serializer: SERIALIZER)
+        if request.use_authenticated_cookie_encryption
+          key_len = ActiveSupport::MessageEncryptor.key_len(encrypted_cookie_cipher)
+          secret = request.key_generator.generate_key(request.authenticated_encrypted_cookie_salt, key_len)
+          @encryptor = ActiveSupport::MessageEncryptor.new(secret, cipher: encrypted_cookie_cipher, serializer: SERIALIZER)
+        else
+          key_len = ActiveSupport::MessageEncryptor.key_len("aes-256-cbc")
+          secret = request.key_generator.generate_key(request.encrypted_cookie_salt, key_len)
+          sign_secret = request.key_generator.generate_key(request.encrypted_signed_cookie_salt)
+          @encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, cipher: "aes-256-cbc", serializer: SERIALIZER)
+        end
 
         request.cookies_rotations.encrypted.each do |rotation_options|
           @encryptor.rotate serializer: SERIALIZER, **rotation_options

Edit: made a branch and can submit a PR for it: https://github.com/rails/rails/compare/master...mikeycgto:actiondispatch-use-aead-encrypted-cookies-patch?expand=1

Contributor

mikeycgto commented Sep 24, 2017

Currently it's just responsible for enabling the upgrade behavior, which rotates CBC to GCM. I feel setting it to false should also lead to CBC being used. The following patch should suffice:

diff actionpack/lib/
diff --git a/actionpack/lib/action_dispatch/middleware/cookies.rb b/actionpack/lib/action_dispatch/middleware/cookies.rb
index b383164..dbe46be 100644
--- a/actionpack/lib/action_dispatch/middleware/cookies.rb
+++ b/actionpack/lib/action_dispatch/middleware/cookies.rb
@@ -599,9 +599,16 @@ class EncryptedKeyRotatingCookieJar < AbstractCookieJar # :nodoc:
       def initialize(parent_jar)
         super
 
-        key_len = ActiveSupport::MessageEncryptor.key_len(encrypted_cookie_cipher)
-        secret = request.key_generator.generate_key(request.authenticated_encrypted_cookie_salt, key_len)
-        @encryptor = ActiveSupport::MessageEncryptor.new(secret, cipher: encrypted_cookie_cipher, serializer: SERIALIZER)
+        if request.use_authenticated_cookie_encryption
+          key_len = ActiveSupport::MessageEncryptor.key_len(encrypted_cookie_cipher)
+          secret = request.key_generator.generate_key(request.authenticated_encrypted_cookie_salt, key_len)
+          @encryptor = ActiveSupport::MessageEncryptor.new(secret, cipher: encrypted_cookie_cipher, serializer: SERIALIZER)
+        else
+          key_len = ActiveSupport::MessageEncryptor.key_len("aes-256-cbc")
+          secret = request.key_generator.generate_key(request.encrypted_cookie_salt, key_len)
+          sign_secret = request.key_generator.generate_key(request.encrypted_signed_cookie_salt)
+          @encryptor = ActiveSupport::MessageEncryptor.new(secret, sign_secret, cipher: "aes-256-cbc", serializer: SERIALIZER)
+        end
 
         request.cookies_rotations.encrypted.each do |rotation_options|
           @encryptor.rotate serializer: SERIALIZER, **rotation_options

Edit: made a branch and can submit a PR for it: https://github.com/rails/rails/compare/master...mikeycgto:actiondispatch-use-aead-encrypted-cookies-patch?expand=1

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Sep 24, 2017

Member

Had a bit of a change of heart on the rotate API as I realized that neither the verifier or encryptor ever mentions key generators or salts. They're only meant to work with generated secrets, so I didn't like to add the extra support there after all.

Think we'll have to go through Rails.application.key_generator somehow.

The cookies middleware then felt a little strange, so I chose to nix the :salt and :key_generator support for now. I wasn't happy with how it felt at the moment. Perhaps this is good enough for 5.2, then we can let it evolve a bit on its own once its out.

The updated code is in 36888b9..38308e6

Member

kaspth commented Sep 24, 2017

Had a bit of a change of heart on the rotate API as I realized that neither the verifier or encryptor ever mentions key generators or salts. They're only meant to work with generated secrets, so I didn't like to add the extra support there after all.

Think we'll have to go through Rails.application.key_generator somehow.

The cookies middleware then felt a little strange, so I chose to nix the :salt and :key_generator support for now. I wasn't happy with how it felt at the moment. Perhaps this is good enough for 5.2, then we can let it evolve a bit on its own once its out.

The updated code is in 36888b9..38308e6

@mikeycgto

This comment has been minimized.

Show comment
Hide comment
@mikeycgto

mikeycgto Sep 24, 2017

Contributor

Sounds good @kaspth. The changes in the documentation and Message::KeyRotator look good to me. For the best to keep this as simplified as possible 👍

Let me know if I should submit my use_authenticated_cookie_encryption PR. I feel like the expectation would be if this is false, CBC should be used for cookies.

Contributor

mikeycgto commented Sep 24, 2017

Sounds good @kaspth. The changes in the documentation and Message::KeyRotator look good to me. For the best to keep this as simplified as possible 👍

Let me know if I should submit my use_authenticated_cookie_encryption PR. I feel like the expectation would be if this is false, CBC should be used for cookies.

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