Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

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

Jump to bottom

Change default KeyGenerator digest to SHA1 to fix cookies in rolling upgrades #26023

Merged
merged 1 commit into from Jul 21, 2023
Merged

Change default KeyGenerator digest to SHA1 to fix cookies in rolling upgrades #26023

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 7 additions & 1 deletion config/initializers/cookie_rotator.rb
View file
Open in desktop
@@ -1,14 +1,20 @@
# frozen_string_literal: true

# TODO: Remove after 4.2.0
Rails.application.configure do
config.active_support.key_generator_hash_digest_class = OpenSSL::Digest::SHA1
end

Rails.application.config.after_initialize do
Rails.application.config.action_dispatch.cookies_rotations.tap do |cookies|
authenticated_encrypted_cookie_salt = Rails.application.config.action_dispatch.authenticated_encrypted_cookie_salt
signed_cookie_salt = Rails.application.config.action_dispatch.signed_cookie_salt

secret_key_base = Rails.application.secret_key_base

# TODO: Switch to SHA1 after 4.2.0
key_generator = ActiveSupport::KeyGenerator.new(
secret_key_base, iterations: 1000, hash_digest_class: OpenSSL::Digest::SHA1
secret_key_base, iterations: 1000, hash_digest_class: OpenSSL::Digest::SHA256
Copy link
Contributor

Choose a reason for hiding this comment

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

The way I understood this hash_digest_class argument is that it was essentially specifiying which algorithm the incoming cookies had been created with, which in our case for existing non-updated-to-rails-7 instances is SHA1 -- and it seems like this change would tell the rotator that the incoming cookies to be rotated were created with SHA256? Or do I misunderstand this setting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are not misunderstanding, it tells Rails how to read existing cookies that use SHA256.

This is so the version with this PR can coexist with an upcoming that switches to outputting SHA256.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so is the rotator behavior then to just silently do nothing when it gets cookies created with any other digest than what is specified? I guess it would have to be, because once you rotate you don't want to keep rotating.

So in that case, just to think out loud a bit...

  • The combination of leaving the default as SHA1 and putting in a rotator that only rotates SHA256, does seem like it would solve the "how can do rolling deploys to get rails-6 instances running rails-7 with minimal session disruption?". During the deploy, all prior cookies will be SHA1, they'll be read fine. Newly generated cookies will be SHA1 due to explicitly setting to the Digest::SHA1 class and not accepting rails 7 defaults; and the cookie rotator will just do nothing for now since there won't be any inbound SHA256-created cookies coming in.

Then on a subsequent release, if we flip off the digest_class setting (and thus the Rails 7 default of SHA256 takes effect), and also flip the rotator to look to rotate SHA1-created cookies, then during the rolling deploy:

  • The first app servers flipped over will see some SHA1 cookies and use the rotator to flip them to their new setting of SHA256
  • The last app servers flipped over MIGHT see some traffic from sessions that have already been flipped to SHA256, in which case they will flip them back to SHA1 (due to this PR).
  • In theory a session could be flipped back and forth many times like this during the rolling deploy, depending how long it is, but it wouldn't matter because the cookies would keep getting read.
  • Once the deploy is done and all app servers are on the R7 code and have the SHA256 default behavior and the cookie rotator, the flip flopping will stop and all cookies will slowly be rotated to SHA256 (over some number of weeks/months/etc -- whatever is decided as reasonable period)
  • After that X duration, we can remove the rotator entirely, even at the cost of lossing session for anyone who hasn't visited for longer than the duration.

Is that roughly accurate and more in line with the intentions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes!

Ok, cool - I think that works.

Not sure how to test this ... ideally you could create like a 2-server mini cluster just running locally and have an http server flip inbound requests back and forth between them, and then just make a bunch of requests and watch that the format gets adjusted back and forth all while preserving session.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested by reverting the Rails 7 update and it seems to work!

)
key_len = ActiveSupport::MessageEncryptor.key_len

Expand Down