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.

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

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

Merged
merged 1 commit into from Jul 21, 2023

Conversation

ClearlyClaire
Copy link
Contributor

@ClearlyClaire ClearlyClaire commented Jul 17, 2023

Follow-up to #25668

Rails 7 changed the default KeyGenerator digest algorithm from SHA1 to SHA256, thus breaking encrypted cookies.

The official Rails documentation suggests configuring encrypted cookie rotation from cookies using the SHA1 digest algorithm, which we did, and it covers a clean Rails 6 to Rails 7 update path.

However, larger Mastodon instances typically have multiple puma servers running, and performing a rolling update will almost surely make Rails 6 and Rails 7 code coexist for a few seconds or minutes, in which case a client might get served with Rails 7, upgrading the cookie from SHA1 to SHA256, then back to Rails 6, which won't be able to read the cookie.

This PR changes Rails' default digest back to SHA1 so generated encrypted cookies remain compatible with 4.1.4, allowing rolling upgrades where multiple versions of the code coexist for a short time, and sets cookie rotation to handle reading from cookies using the SHA256 digest, so that rolling upgrades to a later version switching to SHA256 are possible.

The idea is to change the digest back to Rails 7's default (SHA256) in 4.2.1 (and switch the rotation to handle SHA1 cookies).

cc @mjankowski

@ClearlyClaire ClearlyClaire mentioned this pull request Jul 17, 2023
@ClearlyClaire ClearlyClaire requested a review from a team July 17, 2023 12:54
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!

@renchap renchap added this to the 4.2.0 milestone Jul 21, 2023
@Gargron Gargron merged commit 934c7b3 into mastodon:main Jul 21, 2023
25 checks passed
ClearlyClaire added a commit to glitch-soc/mastodon that referenced this pull request Jul 30, 2023
* Cleanup unused portions of statuses/status partial (mastodon#26045)

* Wrong count in response when removing favourite/reblog (mastodon#24365)

Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* Paperclip: add support for Azure blob storage (mastodon#23607)

* Fix a missing redirection on getting-started in multi column mode (mastodon#26070)

* Fix haml-lint Rubocop `Style/NumericPredicate` cop (mastodon#26040)

* Change casing for 'Server Settings' string (mastodon#26011)

* Move localized subject mailer shared example to separate file (mastodon#25889)

* Fix haml-lint Rubocop `Lint/UnusedBlockArguments` cop (mastodon#26039)

* Fix `Lint/Void` cop (mastodon#25922)

* Add stricter protocol fields validation for accounts (mastodon#25937)

* Improve the bug report templates (mastodon#25621)

* Fix the crossorigin attribute (mastodon#26096)

* Fix replica being used even if not explicitly defined (mastodon#26074)

* Clean up unused application records (mastodon#24871)

* Change thread view to scroll to the selected post rather than the post being replied to (mastodon#24685)

* Change default KeyGenerator digest to SHA1 to fix cookies in rolling upgrades (mastodon#26023)

* change focus ui for keyboard only input (mastodon#25935)

* Use username as display name for suspended users or users with blank display names (mastodon#25276)

* Fix CSP headers being unintendedly wide (mastodon#26105)

* Fix linting issue (mastodon#26106)

* Replace 'favourite' by 'favorite' for American English (mastodon#26009)

* Override default Action Mailer `preview_path` (mastodon#26110)

* Favourits -> Favorites (mastodon#26109)

* Bump version to v4.1.5 (mastodon#26108)

* Fix incorrect connect timeout in outgoing requests (mastodon#26116)

* Fix missing translation strings for importing lists (mastodon#26120)

* Use valid email address for first account (mastodon#26114)

* Update haml-lint 0.49.1 (mastodon#26118)

* Fix focus and hover styles in web UI (mastodon#26125)

* Remove back button from bookmarks, favourites and lists screens in web UI (mastodon#26126)

* Remove 16:9 cropping from web UI (mastodon#26132)

* Change design of link previews in web UI (mastodon#26136)

* change poll form element colors to fit with the rest of the ui (mastodon#26139)

* Add `lang` attribute to trending links (mastodon#26111)

* Update dependency rdf-normalize to v0.6.1 (mastodon#26130)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update dependency brakeman to v6.0.1 (mastodon#26141)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Update dependency postcss to v8.4.27 (mastodon#26144)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix unexpected redirection to /explore after sign-in (mastodon#26143)

* Update dependency aws-sdk-s3 to v1.131.0 (mastodon#26145)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Add report.updated webhook (mastodon#24211)

* Fix LinkCrawlWorker crashing on `null` `created_at` (mastodon#26151)

* Fix UI Overlap with the loupe icon in the Explore Tab (mastodon#26113)

* Fix missing border on error screen in light theme in web UI (mastodon#26152)

* Fix missing action label on sensitive videos and embeds in web UI (mastodon#26135)

* Fix `lang` for UI texts in link preview (mastodon#26149)

* Add published date and author to news on the explore screen in web UI (mastodon#26155)

* Coverage for `Auth::OmniauthCallbacks` controller (mastodon#26147)

* fix poll input active style (mastodon#26162)

* Add `published_at` attribute to preview cards (mastodon#26153)

* Update dependency sass to v1.64.1 (mastodon#26146)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Revert poll colors to green outside of compose form (mastodon#26164)

* Preserve translation on status re-import (mastodon#26168)

* Fix missing GIF badge in account gallery (mastodon#26166)

* Reformat large text arg in `FetchLinkCardService` spec (mastodon#26183)

* Ignore long line in regex initializer (mastodon#26182)

* Reformat large key values in service specs (mastodon#26181)

* Reformat large hash in `ContextHelper` module (mastodon#26180)

* Use heredoc SQL blocks in `AddFromAccountIdToNotifications` migration (mastodon#26178)

* Extract private methods in `StatusCacheHydrator` (mastodon#26177)

* New Crowdin Translations (automated) (mastodon#26072)

Co-authored-by: GitHub Actions <noreply@github.com>
Co-authored-by: Claire <claire.github-309c@sitedethib.com>

* Remove the `sr` locale override .rb files (mastodon#25927)

* Use correct naming on controller concern specs (mastodon#26197)

* Migrate to request specs in `/api/v2/filters` (mastodon#25721)

* Fix wrong filters sometimes applying in streaming (mastodon#26159)

* Refactor streaming's filtering logic & improve documentation (mastodon#26213)

* Add role badges to the WebUI (mastodon#25649)

* Change interaction modal in web UI (mastodon#26075)

Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>

* Fix crash when processing Flag activity with no status (mastodon#26189)

* Storage: add :azure to remaining callers (mastodon#26080)

* Remove queued_at value from pubsub payloads (mastodon#26173)

* Fix emoji picker button scrolling with textarea content in single-column view (mastodon#25304)

* Change the wording of the dismissable explore prompt (mastodon#25917)

* Update dependency haml_lint to v0.49.2 (mastodon#26222)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

* Fix: Streaming server memory leak in HTTP EventSource cleanup (mastodon#26228)

* Swap debug statements in streaming server (mastodon#26231)

* Fix missing return values in streaming (mastodon#26233)

* [Glitch] Wrong count in response when removing favourite/reblog

Port 4c18928 to glitch-soc

Co-authored-by: Claire <claire.github-309c@sitedethib.com>
Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Fix a missing redirection on getting-started in multi column mode

Port 586b1c9 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Change thread view to scroll to the selected post rather than the post being replied to

Port e4ea80d to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Replace 'favourite' by 'favorite' for American English

Port 217ef7f to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] change poll form element colors to fit with the rest of the ui

Port 80809ef to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Add `lang` attribute to trending links

Port 76fce34 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Fix UI Overlap with the loupe icon in the Explore Tab

Port 9a567ec to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Fix missing border on error screen in light theme in web UI

Port d1a9f60 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Fix missing action label on sensitive videos and embeds in web UI

Port 714a206 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] fix poll input active style

Port 49d2e89 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Revert poll colors to green outside of compose form

Port ce1f35d to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Add published date and author to news on the explore screen in web UI

Port f826a95 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Preserve translation on status re-import

Port 6781dc6 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Fix missing GIF badge in account gallery

Port a4b69be to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* Fix interaction modal being broken because of glitch-soc's theming system

* [Glitch] Change interaction modal in web UI

Port b4e739f to glitch-soc

Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>
Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* [Glitch] Change the wording of the dismissable explore prompt

Port a4ec187 to glitch-soc

Signed-off-by: Claire <claire.github-309c@sitedethib.com>

* Fix CSP tests in glitch-soc

---------

Signed-off-by: Claire <claire.github-309c@sitedethib.com>
Co-authored-by: Matt Jankowski <matt@jankowski.online>
Co-authored-by: Christian Schmidt <github@chsc.dk>
Co-authored-by: Misty De Méo <mistydemeo@gmail.com>
Co-authored-by: Stanislas Signoud <signez@stanisoft.net>
Co-authored-by: gunchleoc <fios@foramnagaidhlig.net>
Co-authored-by: Renaud Chaput <renchap@gmail.com>
Co-authored-by: Trevor Wolf <teeerevor@gmail.com>
Co-authored-by: наб <nabijaczleweli@nabijaczleweli.xyz>
Co-authored-by: mogaminsk <mgmnjp@icloud.com>
Co-authored-by: Nick Schonning <nschonni@gmail.com>
Co-authored-by: Eugen Rochko <eugen@zeonfederated.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Vyr Cossont <VyrCossont@users.noreply.github.com>
Co-authored-by: gol-cha <info@mevo.xyz>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: GitHub Actions <noreply@github.com>
Co-authored-by: Daniel M Brasil <danielmbrasil@protonmail.com>
Co-authored-by: Emelia Smith <ThisIsMissEm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants