-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Add option to configure digest in ActiveRecord Encryption, default to SHA256 #42929
Conversation
9fcb51f
to
a44166a
Compare
| `deterministic_key` | The key or list of keys used for deterministic encryption. It's preferred to configure it via a credential `active_record_encryption.deterministic_key`. | | ||
| `key_derivation_salt` | The salt used when deriving keys. It's preferred to configure it via a credential `active_record_encryption.key_derivation_salt`. | | ||
| `forced_encoding_for_deterministic_encryption` | The default encoding for attributes encrypted deterministically. You can disable forced encoding by setting this option to `nil`. It's `Encoding::UTF_8` by default. | | ||
| `hash_digest_class` | The digest algorithm used to derive keys. `OpenSSL::Digest::SHA256` by default. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big diff because I formatted the markdown table. It's only adding the last line (hash_digest_class
)
@jorgemanrubia - Thanks for looking into the #42922 issue report and turning a PR around so quickly! A couple of questions/comments:
|
eaa3175
to
3b70345
Compare
Yes, good idea! I added the iterations param as a fixed value for now.
This is the current case of HEY: it's loading 6.0 defaults but runs on Rails edge and it uses Active Record encryption. I imagine there can be more apps in this situation: early adopters of AR encryption that, after upgrading, would see existing encryption broken. They would have to add this to fix the problem: config.active_record.encryption.hash_digest_class = OpenSSL::Digest::SHA1 The new 6.0 default is intended to cover this case. |
Sorry in advance for hijacking the conversation to ask the following (I tried my best to find the answer on github and the ML but without success): will we also want |
4b7bc1b
to
85e5bae
Compare
ab40b2f
to
f7e34c1
Compare
hey @ghiculescu could we get this merged please? Thanks! |
12bde9a
to
44dc627
Compare
Rails 7 uses `SHA256` as the new default digest. With this change, Active Record Encryption will use `SHA256` too. This can be configured with: ```ruby config.active_record.encryption.hash_digest_class = OpenSSL::Digest::SHA1 ``` This sets the 6.0 default as `SHA1`, so that early apps that adopted encryption before the new default changed continued working without setting any config. Fix rails#42922
44dc627
to
896bfe9
Compare
Thanks for the review @rafaelfranca @marivaldo. I think I addressed all the comments. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@jorgemanrubia @ghiculescu @rafaelfranca @marivaldo It doesn't look like this was merged before the 7.0 release. Was the underlying issue handled elsewhere? Shouldn't this PR be reopened? |
Rails 7 uses
SHA256
as the new default digest. With this change, Active Record Encryption will enforce usingSHA256
too. Before, it was initializing withSHA1
due to the initialization sequence described in #42922.The new option can be configured with:
I added a separated option for Active Record encryption because we need it to be stable. If not, it could happen that, if we set a new default for Rails, it would invalidate existing encrypted data (which is essentially what happened when we changed the default digest for 6.1 apps using encryption). We use the digest to derive keys so, if it changes, keys change and existing data can't be decrypted.
SHA256
is the new default in Rails 7.0 so we adopt it for Active Record encryption, which is a 7.0 feature. I am also setting it retroactively toSHA1
for 6.0 defaults, so that early adopters of encryption before the default changed don't have to tweak the new config to decrypt with existing data.Fix #42922
cc @ghiculescu @boomer196