Skip to content

Switch ActiveSupport::MessageVerifier's default serialization to JSON #42843

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

Merged

Conversation

sabulikia
Copy link
Contributor

@sabulikia sabulikia commented Jul 22, 2021

Co-authored-by: David Buckley david.buckley@shopify.com
Co-authored-by: Saba Kiaei saba.kiaei@shopify.com

Summary

This PR introduces JSON as the default serializer for ActiveSupport::MessageVerifier replacing Marshal.

Given that a serializer of choice can be provided to ActiveSupport::MessageVerifier, the goal with this PR is to ensure a safe default serialization method.

This will help ensure that future signing secret leaks do not end up becoming a vector for deserialization attacks.

@buckley-w-david
Copy link

Looks like there's some additional places that we weren't aware of that make use of MessageVerifer, we'll have to investigate and address those as well.

@sabulikia sabulikia marked this pull request as draft July 22, 2021 20:02
@sabulikia sabulikia force-pushed the message-verifier-default-serializer branch from c9557a9 to 0973b90 Compare July 22, 2021 20:43
@sabulikia sabulikia marked this pull request as ready for review July 22, 2021 20:56
@sabulikia sabulikia force-pushed the message-verifier-default-serializer branch 5 times, most recently from 1079a43 to 16c9a89 Compare October 15, 2021 19:03
@sabulikia sabulikia force-pushed the message-verifier-default-serializer branch 5 times, most recently from eee4686 to 684fa1b Compare November 5, 2021 14:28
@rails-bot
Copy link

rails-bot bot commented Feb 3, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Feb 3, 2022
@sabulikia sabulikia force-pushed the message-verifier-default-serializer branch from 684fa1b to 4c66b02 Compare February 4, 2022 22:44
@rails-bot rails-bot bot removed the stale label Feb 4, 2022
@sabulikia sabulikia force-pushed the message-verifier-default-serializer branch 2 times, most recently from 7b5cb20 to 4c0df8a Compare February 4, 2022 23:20
@sabulikia sabulikia force-pushed the message-verifier-default-serializer branch 4 times, most recently from 345f789 to c7dc723 Compare February 22, 2022 15:31
@sabulikia sabulikia force-pushed the message-verifier-default-serializer branch from c7dc723 to 5256c90 Compare March 1, 2022 18:02
@sabulikia
Copy link
Contributor Author

I've changed this PR to target Rails 7.1, rebased, fixed the merge conflicts, have a passing CI, and updated the docs.

@tenderlove
Copy link
Member

Great, thank you. This looks good to me so I'll merge it!

@tenderlove tenderlove merged commit a40d781 into rails:main Mar 1, 2022
tenderlove added a commit that referenced this pull request Mar 1, 2022
…er-default-serializer"

This reverts commit a40d781, reversing
changes made to ad2529b.
tenderlove added a commit that referenced this pull request Mar 1, 2022
…e-verifier-default-serializer""

This reverts commit fd4e63c.
sampatbadhe added a commit to sampatbadhe/rails that referenced this pull request Oct 27, 2022
- Rails.application.config.active_support.default_message_encryptor_serializer introduced in rails#42846
- Rails.application.config.active_support.default_message_verifier_serializer introduced in rails#42843
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Apr 17, 2023
In rails#42843 and rails#42846, several config settings were added to control the
default serializer for `MessageEncryptor` and `MessageVerifier`, and to
provide a migration path from a default `Marshal` serializer to a
default `JSON` serializer:

* `config.active_support.default_message_encryptor_serializer`
  * Supports `:marshal`, `:hybrid`, or `:json`.
* `config.active_support.default_message_verifier_serializer`
  * Supports `:marshal`, `:hybrid`, or `:json`.
* `config.active_support.fallback_to_marshal_deserialization`
  * Affects `:hybrid` for both `MessageEncryptor` and `MessageVerifier`.
* `config.active_support.use_marshal_serialization`
  * Affects `:hybrid` for both `MessageEncryptor` and `MessageVerifier`.

This commit unifies those config settings into a single setting,
`config.active_support.message_serializer`, which supports `:marshal`,
`:json_allow_marshal`, and `:json` values.  So, for example,

  ```ruby
  config.active_support.default_message_encryptor_serializer = :hybrid
  config.active_support.default_message_verifier_serializer = :hybrid
  config.active_support.fallback_to_marshal_deserialization = true
  config.active_support.use_marshal_serialization = false
  ```

becomes

  ```ruby
  config.active_support.message_serializer = :json_allow_marshal
  ```

and

  ```ruby
  config.active_support.default_message_encryptor_serializer = :hybrid
  config.active_support.default_message_verifier_serializer = :hybrid
  config.active_support.fallback_to_marshal_deserialization = false
  config.active_support.use_marshal_serialization = false
  ```

becomes

  ```ruby
  config.active_support.message_serializer = :json
  ```

This commit also replaces `ActiveSupport::JsonWithMarshalFallback` with
`ActiveSupport::Messages::SerializerWithFallback`, which implements a
generic mechanism for serializer fallback.  The `:marshal` serializer
uses this mechanism too, so

  ```ruby
  config.active_support.default_message_encryptor_serializer = :hybrid
  config.active_support.default_message_verifier_serializer = :hybrid
  config.active_support.fallback_to_marshal_deserialization = false
  config.active_support.use_marshal_serialization = true
  ```

becomes

  ```ruby
  config.active_support.message_serializer = :marshal
  ```

Additionally, the logging behavior of `JsonWithMarshalFallback` has been
replaced with notifications which include the names of the intended and
actual serializers, as well as the serialized and deserialized message
data.  This provides a more targeted means of tracking serializer
fallback events.  It also allows the user to "silence" such events, if
desired, without an additional config setting.

All of these changes make it easier to add migration paths for new
serializers such as `ActiveSupport::MessagePack`.
jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request May 8, 2023
In rails#42843 and rails#42846, several config settings were added to control the
default serializer for `MessageEncryptor` and `MessageVerifier`, and to
provide a migration path from a default `Marshal` serializer to a
default `JSON` serializer:

* `config.active_support.default_message_encryptor_serializer`
  * Supports `:marshal`, `:hybrid`, or `:json`.
* `config.active_support.default_message_verifier_serializer`
  * Supports `:marshal`, `:hybrid`, or `:json`.
* `config.active_support.fallback_to_marshal_deserialization`
  * Affects `:hybrid` for both `MessageEncryptor` and `MessageVerifier`.
* `config.active_support.use_marshal_serialization`
  * Affects `:hybrid` for both `MessageEncryptor` and `MessageVerifier`.

This commit unifies those config settings into a single setting,
`config.active_support.message_serializer`, which supports `:marshal`,
`:json_allow_marshal`, and `:json` values.  So, for example,

  ```ruby
  config.active_support.default_message_encryptor_serializer = :hybrid
  config.active_support.default_message_verifier_serializer = :hybrid
  config.active_support.fallback_to_marshal_deserialization = true
  config.active_support.use_marshal_serialization = false
  ```

becomes

  ```ruby
  config.active_support.message_serializer = :json_allow_marshal
  ```

and

  ```ruby
  config.active_support.default_message_encryptor_serializer = :hybrid
  config.active_support.default_message_verifier_serializer = :hybrid
  config.active_support.fallback_to_marshal_deserialization = false
  config.active_support.use_marshal_serialization = false
  ```

becomes

  ```ruby
  config.active_support.message_serializer = :json
  ```

This commit also replaces `ActiveSupport::JsonWithMarshalFallback` with
`ActiveSupport::Messages::SerializerWithFallback`, which implements a
generic mechanism for serializer fallback.  The `:marshal` serializer
uses this mechanism too, so

  ```ruby
  config.active_support.default_message_encryptor_serializer = :hybrid
  config.active_support.default_message_verifier_serializer = :hybrid
  config.active_support.fallback_to_marshal_deserialization = false
  config.active_support.use_marshal_serialization = true
  ```

becomes

  ```ruby
  config.active_support.message_serializer = :marshal
  ```

Additionally, the logging behavior of `JsonWithMarshalFallback` has been
replaced with notifications which include the names of the intended and
actual serializers, as well as the serialized and deserialized message
data.  This provides a more targeted means of tracking serializer
fallback events.  It also allows the user to "silence" such events, if
desired, without an additional config setting.

All of these changes make it easier to add migration paths for new
serializers such as `ActiveSupport::MessagePack`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants