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

Rails 7.0 : finalise la migration des defaults #10712

Merged

Conversation

colinux
Copy link
Member

@colinux colinux commented Aug 22, 2024

On avait attendu à l'époque pour se laisser l'opportunité au cas où de rollback car ces changements sont non rétro compatibles (et on avait jamais finalisé).

  • cache au format rails 7
  • algo par défaut de chiffrement des clés SHA1 => SHA256

Conséquemment :

  • on fait une rotation des cookies pour pas déconnecter tout le monde (le code du rotator vient direct de la doc)
  • task pour faire faire la rotation des attributs chiffrés (on a que Procedure#api_particulier_token_ concerné`)

On pourra décabler la rotation dans quelques temps.

Les messages signés ne sont pas concernés, la valeur par défaut restant au SHA1.

Testé manuellement :

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.57%. Comparing base (a784b01) to head (5859ea4).
Report is 95 commits behind head on main.

Files with missing lines Patch % Lines
...ce/rotate_api_particulier_token_encryption_task.rb 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #10712      +/-   ##
==========================================
- Coverage   84.57%   84.57%   -0.01%     
==========================================
  Files        1118     1119       +1     
  Lines       24753    24765      +12     
  Branches     4611     4611              
==========================================
+ Hits        20935    20945      +10     
- Misses       3818     3820       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@colinux colinux force-pushed the rails-7-finalize-defaults branch from 11d136a to 5859ea4 Compare August 28, 2024 11:59
@colinux colinux marked this pull request as ready for review August 29, 2024 11:48

# Remove after all encrypted attributes have been rotated.
legacy_key = ActiveSupport::KeyGenerator.new(password, hash_digest_class: OpenSSL::Digest::SHA1).generate_key(salt, len)
@encryptor.rotate legacy_key
Copy link
Contributor

@mfo mfo Sep 3, 2024

Choose a reason for hiding this comment

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

til: https://api.rubyonrails.org/v7.1.0/classes/ActiveSupport/MessageEncryptor.html – vraiment cool j'avais vraiment pas creusé comme API :-)

Copy link
Contributor

@mfo mfo left a comment

Choose a reason for hiding this comment

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

impeccable ! juste les tests autour de la rotation de encrypt/descrypt ou j'ai l'impression qu'on re-teste le MessageEncryptor de rails, mais a la limite on pourra les faire sauter une fois les cookies rotate et les migrations passées

@colinux colinux added this pull request to the merge queue Sep 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 9, 2024
@colinux colinux added this pull request to the merge queue Sep 9, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 9, 2024
@colinux colinux added this pull request to the merge queue Sep 9, 2024
Merged via the queue into demarches-simplifiees:main with commit 7c30ab8 Sep 9, 2024
18 checks passed
@colinux colinux deleted the rails-7-finalize-defaults branch September 9, 2024 08:17
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.

2 participants