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

Opt in to remaining Rails 7.1 defaults #30332

Merged
merged 6 commits into from
Jun 10, 2024

Conversation

mjankowski
Copy link
Contributor

I believe that the remainder of the options which were not already enabled here (in the file deleted in this PR) are safe to enable at this point, and we can flip over to the 7.1 defaults.

However - I know we also want to be deliberate with how people step through upgrades, and we may want the 4.3 release to include an update to Rails 7.1 (4.2.x is on Rails 7.0) but NOT immediately enable all these defaults.

Vaguely related previous PRs:

Opening this to step through the final remaining changes in this diff, and coordinate the interaction of release timing/versions with when to make this change.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I have not managed to identify which config change causes this, but this PR still makes TOTP secret decryption fail with:

ActiveRecord::Encryption::Errors::Decryption (ActiveRecord::Encryption::Errors::Decryption):

activerecord (7.1.3.4) lib/active_record/encryption/encryptor.rb:58:in `rescue in decrypt'
activerecord (7.1.3.4) lib/active_record/encryption/encryptor.rb:52:in `decrypt'
activerecord (7.1.3.4) lib/active_record/encryption/encrypted_attribute_type.rb:90:in `block in decrypt'
activerecord (7.1.3.4) lib/active_record/encryption/scheme.rb:69:in `with_context'
activerecord (7.1.3.4) lib/active_record/encryption/encrypted_attribute_type.rb:15:in `with_context'
activerecord (7.1.3.4) lib/active_record/encryption/encrypted_attribute_type.rb:85:in `decrypt'
activerecord (7.1.3.4) lib/active_record/encryption/encrypted_attribute_type.rb:36:in `deserialize'
activemodel (7.1.3.4) lib/active_model/attribute_set/builder.rb:52:in `block in fetch_value'
activemodel (7.1.3.4) lib/active_model/attribute_set/builder.rb:46:in `fetch'
activemodel (7.1.3.4) lib/active_model/attribute_set/builder.rb:46:in `fetch_value'

@ClearlyClaire
Copy link
Contributor

ClearlyClaire commented Jun 7, 2024

Rails.application.config.active_record.encryption.hash_digest_class = OpenSSL::Digest::SHA1 seems to fix it, but that's odd, as config.active_support.key_generator_hash_digest_class is set to OpenSSL::Digest::SHA256

does that mean we should have set this to SHA256 before we ever started using ActiveRecord Encryption but we are now stuck out of it?

config.active_record.encryption.support_sha1_for_non_deterministic_encryption = true does not seem to help

@ClearlyClaire
Copy link
Contributor

This seems to be linked to rails/rails#50604

Indeed, the following is not a sufficient patch:

diff --git a/config/initializers/active_record_encryption.rb b/config/initializers/active_record_encryption.rb
index 777bafc273..21e95f7506 100644
--- a/config/initializers/active_record_encryption.rb
+++ b/config/initializers/active_record_encryption.rb
@@ -32,4 +32,5 @@ Rails.application.configure do
   config.active_record.encryption.deterministic_key = ENV.fetch('ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY')
   config.active_record.encryption.key_derivation_salt = ENV.fetch('ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT')
   config.active_record.encryption.primary_key = ENV.fetch('ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY')
+  config.active_record.encryption.support_sha1_for_non_deterministic_encryption = true
 end

But if I in addition disable eager loading, it works:

diff --git a/config/environments/production.rb b/config/environments/production.rb
index a39843e956..f86ebdec94 100644
--- a/config/environments/production.rb
+++ b/config/environments/production.rb
@@ -12,7 +12,7 @@ Rails.application.configure do
   # your application in memory, allowing both threaded web servers
   # and those relying on copy on write to perform better.
   # Rake tasks automatically ignore this option for performance.
-  config.eager_load = true
+  config.eager_load = false
 
   # Full error reports are disabled and caching is turned on.
   config.consider_all_requests_local       = false

@ClearlyClaire
Copy link
Contributor

Using the suggestion from that issue, the following appears to work:

diff --git a/config/initializers/active_record_encryption.rb b/config/initializers/active_record_encryption.rb
index 777bafc273..64b6077976 100644
--- a/config/initializers/active_record_encryption.rb
+++ b/config/initializers/active_record_encryption.rb
@@ -32,4 +32,8 @@ Rails.application.configure do
   config.active_record.encryption.deterministic_key = ENV.fetch('ACTIVE_RECORD_ENCRYPTION_DETERMINISTIC_KEY')
   config.active_record.encryption.key_derivation_salt = ENV.fetch('ACTIVE_RECORD_ENCRYPTION_KEY_DERIVATION_SALT')
   config.active_record.encryption.primary_key = ENV.fetch('ACTIVE_RECORD_ENCRYPTION_PRIMARY_KEY')
+  config.active_record.encryption.support_sha1_for_non_deterministic_encryption = true
+
+  # Use configs now! (to be removed if using Rails 7.1.4+)
+  ActiveRecord::Encryption.configure(**config.active_record.encryption)
 end

@mjankowski
Copy link
Contributor Author

Interesting ... I had the fix from that issue (ActiveRecord::Encryption.configure(**config.active_record.encryption) line) in the rails 7.2 branch at some point, but was able to remove it eventually.

Added here, with a TODO note pointing to that issue.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

I have yet to extensively test it in a real environment, but it looks good so far.

@ClearlyClaire ClearlyClaire added this pull request to the merge queue Jun 10, 2024
Merged via the queue into mastodon:main with commit 0cf9121 Jun 10, 2024
33 checks passed
@mjankowski mjankowski deleted the rails-7-1-defaults-enable branch June 10, 2024 13:15
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

2 participants