Make it possible to rotate the secret key gracefully for ActiveStorage

[santib]

Summary

Currently, when for some reason we need to change the app secret key, ActiveStorage's previously generated urls will stop working. By supporting rotation in the ActiveStorage verifier, we can rotate the app secret key while still supporting old generated urls either for a grace period or forever.

Other Information

I based the solution on config.action_dispatch.cookies_rotations.

I wanted to validate the idea and solution before adding anything else (like a section in the rails guides).

I don't like how this looks app.config.active_storage.rotations.active_storage but couldn't think of a better name.

[santib]

Hey @georgeclaghorn did you have a chance to take a look at it? I'd like to know if this PR makes sense or if I'm missing something, thanks 😄

[rails-bot]

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]

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.

[kaspth]

Good idea! We should fix this. We also gotta find a place to document this in the Active Storage guide.

[santib]

Good idea! We should fix this. We also gotta find a place to document this in the Active Storage guide.

@kaspth From your comments I understand that you:

• Don't want to have a rotation configuration object, instead expose the verifier directly.
• Want to expose the verifier object ASAP so it can be configured both in config/environments/* and in config/initializers/*

I tried using the before_configuration hook but then I realized it won't be possible since the secret key base can be configured afterwards. So in short I can't find any way to do this without the rotation configuration object.

That said, I agree that changing activesupport/lib/active_support/messages/rotation_configuration.rb didn't feel good. So I'm all ears how to proceed without the rotation configuration object if you have any ideas, otherwise my suggestion would be to create a rotation configuration class under the active_storage engine which would be similar to activesupport/lib/active_support/messages/rotation_configuration.rb but specific for our use case. Let me know your thoughts, thanks 😄

[rails-bot]

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.

[brunoprietog]

What happened to this? It would be great to see it merged

[rafaelfranca]

@santib do you want to finish this? I think we should add a new rotation object only for Active Storage with a simpler API. We don't need a kind argumentin therotate` method for example. Just an object that stores an array and have a rotate, so we can do:

app.config.active_storage.key_rotation.rotate "old secret"

[santib]

Hey @rafaelfranca sure I can finish this but I'll need a couple of days to find some time if that's ok, thanks for the heads up

[rafaelfranca]

@jonathanhefner I think you PR to add the message_verifies invalidate this one right?

[jonathanhefner]

Yes, with #44179, users could call Rails.application.message_verifiers.rotate(...) sometime before the "active_storage.verifier" initializer runs.

Note that the MessageVerifier returned by app.message_verifier("ActiveStorage") uses Rails.application.key_generator for its secret. In other words, the old secret_key_base must be processed by ActiveSupport::KeyGenerator#generate_key before it can be rotated.

So, if I understand this PR correctly, the configuration would look something like:

key_generator = ActiveSupport::KeyGenerator.new(old_secret_key_base, iterations: 1000)

config.active_storage.key_rotations.tap do |rotations|
  rotations.rotate(key_generator.generate_key("ActiveStorage"))
end

With #44179, the configuration would look something like:

key_generator = ActiveSupport::KeyGenerator.new(old_secret_key_base, iterations: 1000)

Rails.application.message_verifiers.rotate(secret_generator: key_generator.method(:generate_key))

We could possibly simplify these configurations by modifying Rails.application.key_generator to accept an optional base arg. If the arg is provided, key_generator would return a new ActiveSupport::KeyGenerator; otherwise, it would return @caching_key_generator.

Or we could add a Rails.application.secret_generator method that accepts a base arg and returns a proc. So the configuration would look something like:

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

[shamas-]

One year ping on this in case it can still be completed

[collimarco]

I just posted a related question on Reddit. It would be useful to have a plan or documentation in case you need to change the secret key.