Skip to content

Add MessageVerifiers and MessageEncryptors classes #44179

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

Conversation

jonathanhefner
Copy link
Member

@jonathanhefner jonathanhefner commented Jan 15, 2022

Currently, Rails.application.message_verifier(name) returns a MessageVerifier instance using a secret derived from the given name. Instances created this way always generate messages using the default options for MessageVerifier.new. Also, if there are older options to rotate for message verification, each instance created this way must be configured individually. In cases where Rails itself uses Rails.application.message_verifier (e.g. in Active Storage), discovering the name of the instance may require digging through Rails' source code.

To alleviate these issues, this commit adds a MessageVerifiers factory class, which acts as a central point for configuring and creating MessageVerifier instances. Options passed to MessageVerifiers#rotate will be applied to MessageVerifier instances that MessageVerifiers#[] creates. So the following:

foo_verifier = Rails.application.message_verifier(:foo)
foo_verifier.rotate(old_options)
bar_verifier = Rails.application.message_verifier(:bar)
bar_verifier.rotate(old_options)

Can be rewritten as:

Rails.application.message_verifiers.rotate(old_options)
foo_verifier = Rails.application.message_verifiers[:foo]
bar_verifier = Rails.application.message_verifiers[:bar]

Additionally, Rails.application.message_verifiers supports a :secret_key_base option, so old secret_key_base values can be rotated:

Rails.application.message_verifiers.rotate(secret_key_base: "old secret_key_base")

MessageVerifiers memoizes MessageVerifier instances. This also allows MessageVerifier instances to be overridden entirely:

Rails.application.message_verifiers[:foo] = ActiveSupport::MessageVerifier.new(other_secret)

For parity, this commit also adds a MessageEncryptors factory class, which fulfills the same role for MessageEncryptor instances.

@jonathanhefner jonathanhefner force-pushed the add-message_verifiers-message_encryptors branch from b18963e to 4c5fe1a Compare January 15, 2022 20:55
# :call-seq: rotate(**options)
#
# Adds +options+ to the list of viable options. Messages will be signed
# using the first added options. When verifying, however, each viable
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# using the first added options. When verifying, however, each viable
# using the first added option. When verifying, however, each viable

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe "options" is correct here. Each options Hash is added to a list of options Hashes. The first options Hash in the list will be used to sign messages. But when verifying messages, each options Hash in the list is tried.

When I wrote this method doc, I did struggle a bit in trying to be concise and consistent across different methods. I didn't want the reader to have to juggle too many terms.

Does the following read any better?

Adds options as an entry in the list of viable options. Messages will be signed using the first entry in the list. When verifying, however, each entry will be tried, in order, until one succeeds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry @jonathanhefner I misunderstood what was happening here.

Your suggestion works, or perhaps simply: "Messages will be signed using the first of the added options"

Copy link
Member Author

Choose a reason for hiding this comment

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

@lewispb I took another pass at it. Please let me know if it still needs improvement!

@jonathanhefner jonathanhefner force-pushed the add-message_verifiers-message_encryptors branch 2 times, most recently from 854681b to 2bc54f0 Compare February 5, 2022 16:58
@rails-bot
Copy link

rails-bot bot commented May 6, 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.

Comment on lines +43 to +61
def changing_configuration!
if @codecs.any?
raise <<~MESSAGE
Cannot change #{self.class} configuration after it has already been applied.

The configuration has been applied with the following salts:
#{@codecs.keys.map { |salt| "- #{salt.inspect}" }.join("\n")}
MESSAGE
end
end
Copy link
Member Author

@jonathanhefner jonathanhefner Sep 19, 2022

Choose a reason for hiding this comment

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

I also wanted to point out another design decision that I made, in case there are any objections: rotations cannot be added nor cleared after a MessageVerifier (or MessageEncryptor) has been built or set (via []=).

The workflow I envisioned was that apps add rotations to Rails.application.message_verifiers in e.g. before_initialize, and libraries get verifiers from Rails.application.message_verifiers at a later stage.

The only way we could "clear rotations" after instances have been created would be to drop those instances. But that does not help if libraries already hold references to those instances.

And disallowing rotate is consistent with disallowing clear_rotations. Additionally, if we did allow rotate, we would need to decide whether new rotations apply to instances set via []=. It feels like they should not apply, because []= is an explicit override mechanism, but the inconsistency might be confusing.

The current implementation is conservative, and doesn't prevent us from expanding the behavior in the future.

@jonathanhefner
Copy link
Member Author

jonathanhefner commented Sep 21, 2022

After #39623 (comment), I've given more thought to the API for rotating old secret_key_base values. I think a nice API might look something like:

Rails.application.message_verifiers.rotate(secret_key_base: old_secret_key_base)

The problem is the RotationCoordinator / MessageVerifiers classes should not be concerned with the concept of a secret_key_base.

However, a bespoke secret generator proc can be. So, based on this idea, I've pushed a commit that forwards rotation options to matching secret generator kwargs.

Thus, the Rails.application.message_verifiers secret generator now accepts a secret_key_base: kwarg (which defaults to Rails.application.secret_key_base), and when a secret_key_base: option is given to rotate, it will be forwarded to the secret generator, such as in this test.

@rafaelfranca What do you think of this API? I still need to update the API docs, but I wanted to get your reaction first.

@jonathanhefner jonathanhefner force-pushed the add-message_verifiers-message_encryptors branch 2 times, most recently from 16ca379 to 9244b06 Compare September 22, 2022 16:18
@rafaelfranca
Copy link
Member

I like it! It will simplify a lot how to rotate those message verifiers. Let's finish the concept.

@jonathanhefner jonathanhefner force-pushed the add-message_verifiers-message_encryptors branch from 9244b06 to 09a227a Compare September 22, 2022 21:13
Currently, `Rails.application.message_verifier(name)` returns a
`MessageVerifier` instance using a secret derived from the given `name`.
Instances created this way always generate messages using the default
options for `MessageVerifier.new`.  Also, if there are older options to
rotate for message verification, each instance created this way must be
configured individually.  In cases where Rails itself uses
`Rails.application.message_verifier` (e.g. in Active Storage),
discovering the name of the instance may require digging through
Rails' source code.

To alleviate these issues, this commit adds a `MessageVerifiers` factory
class, which acts as a central point for configuring and creating
`MessageVerifier` instances.  Options passed to `MessageVerifiers#rotate`
will be applied to `MessageVerifier` instances that `MessageVerifiers#[]`
creates.  So the following:

  ```ruby
  foo_verifier = Rails.application.message_verifier(:foo)
  foo_verifier.rotate(old_options)
  bar_verifier = Rails.application.message_verifier(:bar)
  bar_verifier.rotate(old_options)
  ```

Can be rewritten as:

  ```ruby
  Rails.application.message_verifiers.rotate(old_options)
  foo_verifier = Rails.application.message_verifiers[:foo]
  bar_verifier = Rails.application.message_verifiers[:bar]
  ```

Additionally, `Rails.application.message_verifiers` supports a
`:secret_key_base` option, so old `secret_key_base` values can be
rotated:

  ```ruby
  Rails.application.message_verifiers.rotate(secret_key_base: "old secret_key_base")
  ```

`MessageVerifiers` memoizes `MessageVerifier` instances.  This also
allows `MessageVerifier` instances to be overridden entirely:

  ```ruby
  Rails.application.message_verifiers[:foo] = ActiveSupport::MessageVerifier.new(other_secret)
  ```

For parity, this commit also adds a `MessageEncryptors` factory class,
which fulfills the same role for `MessageEncryptor` instances.
@jonathanhefner jonathanhefner force-pushed the add-message_verifiers-message_encryptors branch from 09a227a to baade55 Compare September 22, 2022 21:21
@jonathanhefner jonathanhefner merged commit f05b6dd into rails:main Sep 26, 2022
jonathanhefner added a commit that referenced this pull request Sep 29, 2022
The CHANGELOG entry for #44179 originally focused on
`ActiveSupport::MessageVerifiers` and `ActiveSupport::MessageEncryptors`,
and made sense in `activesupport/CHANGELOG.md`.  However, prior to
merge, the focus changed to `Rails.application.message_verifiers`.
Therefore, the CHANGELOG entry should be in `railties/CHANGELOG.md`.
byroot added a commit to byroot/rails that referenced this pull request Jan 27, 2025
Ref: rails#44179
Fix: rails#54357

`MessageVerifier` and `MessageEncryptor` were intended to accept
an `on_rotation` callback from the `#rotate` method or from the
`#on_rotation` method.

This is particularly useful for codecs that aren't created by the
user, but by the framework, like `ActiveRecord::Base.signed_id_verifier`
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