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

AO3-6686 Reintroduce Rails 7 defaults (without dangerous cookie stuff) #4973

Merged
merged 12 commits into from
Dec 8, 2024

Conversation

brianjaustin
Copy link
Member

@brianjaustin brianjaustin commented Nov 30, 2024

Pull Request Checklist

Issue

https://otwarchive.atlassian.net/browse/AO3-6686

Purpose

Resurrects #4860 to upgrade us to Rails 7 defaults, except without touching cookies for the most part. The default format is (by default) updated to SHA256, but because of #4926 it will really be a no-op until the rotator is removed in a month or so. Better (correct-er) explanation in the comments.

brianjaustin and others added 8 commits November 30, 2024 19:17
These options just needed to wait until things like cookies had a chance to be rotated.
It's been a few months, so that should be enough time now.
# Conflicts:
#	spec/controllers/comments_controller_spec.rb
@Bilka2
Copy link
Contributor

Bilka2 commented Nov 30, 2024

The default format is (by default) updated to SHA256, but because of #4926 it will really be a no-op until the rotator is removed in a month or so.

This is not correct. The rotator will indeed rotate the cookies from SHA1 to SHA256 once it is deployed. So before this is deployed, it needs to be certain that all our other non-Rails code can deal with SHA256 cookies. (During the original issue Sep 20th this was indicated to be the case: "The worker doesn't care about the hash.")

The point of #4926 is to make sure that those rotated cookies can be read by the application code that is running on the servers right now. So if we have a situation where one application server is running this new code here and one application server is running the old code and a user gets pingponged between them, the following happens:

  1. User connects to application server running new code.
  2. The new rotator on that server rotates the old SHA1 cookie to SHA256 (yay, upgrade!). The user now has a SHA256 cookie and stays logged in.
  3. User connects to application server running old code.
  4. Due to #4926, the old rotator can read the upgraded SHA256 cookie and rotates it to SHA1 (aw, downgrade). The user now has a SHA1 cookie and stays logged in.
  5. Go to step 1.

This pingponging between code versions will happen during the rolling deploy. Since the cookie is always converted, no session/login issues should occur. So #4926 is what allows this running deploy and we do not need downtime for this PR.

Once the rolling deploy is over and all servers are running the new application code, steps 3 and 4 will no longer happen as there are no more servers running the old application code. That means that over time, all users will have their cookies rotate to SHA256, which is the intended upgrade. Once that has happened then the new rotator will be a no-op because all cookies will have the new format. Since the cookies last 14 days, that should be 14 days after deploy. However, since it is a no-op there is no time pressure or complication to keeping this new rotator around for longer and I would recommend to do so. See step 3 in the comment in the cookie rotator file.

This means this PR should go on a release on its own and needs extensive testing on staging during deploy to staging, similar to #4926. This PR here is the risky one and will explode in our faces if we misunderstood something. But if we did it all correctly then users won't even notice anything changed, despite the rolling deploy.

Copy link
Contributor

@Bilka2 Bilka2 left a comment

Choose a reason for hiding this comment

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

Just to make sure we have the right cookie setting I started Rails with this code and read Rails.application.config.active_support.key_generator_hash_digest_class from the console. It is SHA256 as expected.

Noting that we will need a strategy and Jira issue for the cookie serialisation change, since it is excluded here to reduce the risk of this change and do less cookie things at once.

@brianjaustin brianjaustin merged commit 04a605b into otwcode:master Dec 8, 2024
29 checks passed
@brianjaustin brianjaustin deleted the AO3-6686-new-defaults branch December 8, 2024 22:00
sarken added a commit to sarken/otwarchive that referenced this pull request Jan 10, 2025
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