Migrate from attr_encrypted to Rails 7

[david-a-wheeler]

We want to move to Rails 7, and simultaneously migrate from attr_encrypted to Rails 7's built-in encryption system.

In general we try to minimize our dependencies, and attr_encrypted's support seems weak now (they have "maintainer wanted" on their front page).

This unfortunately appears to be complicated. The good news is that there appears to be a guide for this: https://pagertree.com/2021/04/13/rails-7-attr-encrypted-migration/

See its site: https://github.com/attr-encrypted/attr_encrypted

[andrewfader]

@david-a-wheeler so on the checklist for this migration we have the following

• [x] upgrade to attr_encrypted 4.0
• [x] upgrade to rails 7
• [ ] create credential files with environment variables for different environments
• [ ] Modify our model to dynamically define attributes to use during our migration
• [ ] Create a new temporary_encrypted_email column
• [ ] Add a new column for our Rails 7 Active Record encrypts data
• [ ] Copy old encrypted data to new temporary column, and delete old column
• [ ] Run a migration to programmatically decrypt attr_encrypted temporary column and put it in Rails 7 Active Record encrypts column
• [ ] Delete temporary column

For the next task, presumably, we should set up the new secrets and credentials. How do we want to store the secrets? [edited checklist to reflect that we will use env vars]

[david-a-wheeler]

The easy solution is to use the existing secrets. We have no evidence of compromise, and every reason to believe they're still secret.

[david-a-wheeler]

We've been storing them in environment variables. That means that they are not exposed when source code is exposed, nor can they be exposed during the test processes (which builds the software but doesn't have access to those secrets).

Of course, environment variables means that other parts of the process have access to them. However, it's really hard to do anything else on Heroku. Other alternatives like "Secrets Keeper" also put the secrets into the process. Trying to put them in isolated hardware is a big step up in terms of difficulty.

[andrewfader]

Fine with me, I'll just need to figure out how to make that work with Rails 7. Rails 7 prescribes using the credentials files and the encrypted .yml.enc secret file with a master.key. I assume there is a way to make it work with env vars too.

[andrewfader]

Actually, it looks like I can put env vars in all the configs and encrypt that with a master key that itself will be a new env var then it'll all work

[andrewfader]

@david-a-wheeler before I get too far down this path, is it worth doing a gut check to confirm we really want to go ahead with the migration if attr_encrypted 4.0 is working OK right now?

[david-a-wheeler]

Fair enough. My current thinking is "yes, we should switch to using Rails' encryption system instead of using attr_encrypted given what I know now". But if there's a strong reason to avoid this, I would want to know!!

Here are the reasons I think we should remove attr_encrypted and use Rails' built-in encryption system instead:

1. We want to minimize the number of libraries we use. Every library we bring in might be subverted, stop getting maintained, etc. A Rails app has to use the Rails gems anyway; removing a gem that we don't have to use is an improvement. This change will also get rid of blind_index I believe, making the argument even stronger.
2. The attr_encrypted gem is not being maintained as well as we'd like. It stopped being maintained for a while; it seems to have picked up, but it's a drop in the bucket compared to Rails. Rails is well supported.
3. Moving the encryption system into Rails' built-in mechanisms means we're less likely to have future incompatibilities.

The only reason to not move is that it takes effort. But that's a one-time cost, with the benefits listed above, and it shouldn't take too much effort.

I investigated earlier, and as far as I can tell, all we need is supported by Rails 7.0. We need to support case-insensitive searching of encrypted emails, but it can do that. Eventually we'll want to encrypt passwords (as well as using iterated salted hash of them, that is, bcrypt); encrypting password hashes isn't strictly necessary, but it seems like a reasonable hardening step.

Thoughts?

[andrewfader]

OK. I think it's a pretty minor cost to continue using attr_encrypted, and a fairly minor benefit to adopting rails encryption. However, there's no major downside other than increased complexity to migrate and adopt the new system. Plus as you say as the cost of effort goes. It's not excessive, although, it is fairly involved, and possibly not the highest value thing for actual users.

Moving forward there are a couple of options we need to decide, see: https://guides.rubyonrails.org/active_record_encryption.html

1. should we use deterministic or non-deterministic encryption? The latter is stronger but limits queryability.
2. do we want to have a custom encryption provider to preserve compatibility with the old scheme? (https://guides.rubyonrails.org/active_record_encryption.html#support-for-previous-encryption-schemes) this might make some parts harder but some parts, namely, the migration, easier

[david-a-wheeler]

Our use of attr_encrypted meant we could not update Rails, which was a long-term problem I'd rather not repeat. Now that Rails has a built-in encryption mechanism, I expect use and support for attr_encrypt to decrease. I am extremely grateful to its developers, who created an awesome mechanism, but I think we must move off it to ensure long term support.

should we use deterministic or non-deterministic encryption? The latter is stronger but limits queryability.

My reading is that we need deterministic for encrypting email. Search by lowercase email needs to be indexed and fast.

When we encrypt hashed passwords (a future task) that could be non-deterministic.

do we want to have a custom encryption provider to preserve compatibility with the old scheme? (https://guides.rubyonrails.org/active_record_encryption.html#support-for-previous-encryption-schemes) this might make some parts harder but some parts, namely, the migration, easier

No. I want to minimize running code, and in general I want to use common conventions. That will keep long term maintenance effort low.

That means the migration execution time will take longer (we have to decrypt and re-encrypt every email address), but the database isn't that big and we can accept the one-time pause.

Thoughts?

[andrewfader]

@david-a-wheeler sounds good. We'll proceed with deterministic encryption for email, and we'll follow the Rails 7 conventions and defaults wherever possible.

[david-a-wheeler]

sounds good. We'll proceed with deterministic encryption for email, and we'll follow the Rails 7 conventions and defaults wherever possible.

Great! Sounds wonderful, and if you hit an unexpected roadblock let me know.

Please create a migration that simply moves the old data column, so we can later delete it. That way, if a migration only works halfway or otherwise has a hideous problem, we can revert to the data we have.

Following Rails 7 conventions/defaults wherever possible is great. This software was written using earlier versions of Rails, so if there are changes we should make to upgrade it to current conventions, we should probably do it (especially if it reduces code and/or configuration). I imagine those other changes will be separate PRs, but be on alert for that sort of thing.

[david-a-wheeler]

@andrewfader - nudge nudge :-). Let me know if there's anything you need from me to complete this (eventually I think there will be). Once we remove the gems attr_encrypted and blind_index we'll have 2 fewer dependencies and greatly reduce the risk of being unable to update in the future. It's the "can't update Rails" that was particularly concerning, and now that Rails has built-in support for encryption, I don't expect those gems' support for Rails to continue in a strong way. Now is the time to switch.

[andrewfader]

Will do! Busy couple of weeks but I'll pick it back up.

On Mon, May 29, 2023 at 6:46 PM David A. Wheeler @.***> wrote:

@andrewfader https://github.com/andrewfader - nudge nudge :-). Let me know if there's anything you need from me to complete this (eventually I think there will be). Once we remove the gems attr_encrypted and blind_index we'll have 2 fewer dependencies and greatly reduce the risk of being unable to update in the future. It's the "can't update Rails" that was particularly concerning, and now that Rails has built-in support for encryption, I don't expect those gems' support for Rails to continue in a strong way. Now is the time to switch.

— Reply to this email directly, view it on GitHub https://github.com/coreinfrastructure/best-practices-badge/issues/1816#issuecomment-1567573495, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEKFJBTRPSO7QXSPP4CPYTXIURKTANCNFSM5X7VD5TQ . You are receiving this because you were mentioned.Message ID: @.*** com>

[andrewfader]

Sorry for the delay... am diving back in.

On Tue, May 30, 2023 at 3:29 PM Andrew Fader @.***> wrote:

Will do! Busy couple of weeks but I'll pick it back up.

On Mon, May 29, 2023 at 6:46 PM David A. Wheeler @.***> wrote:

@andrewfader https://github.com/andrewfader - nudge nudge :-). Let me know if there's anything you need from me to complete this (eventually I think there will be). Once we remove the gems attr_encrypted and blind_index we'll have 2 fewer dependencies and greatly reduce the risk of being unable to update in the future. It's the "can't update Rails" that was particularly concerning, and now that Rails has built-in support for encryption, I don't expect those gems' support for Rails to continue in a strong way. Now is the time to switch.

— Reply to this email directly, view it on GitHub https://github.com/coreinfrastructure/best-practices-badge/issues/1816#issuecomment-1567573495, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEKFJBTRPSO7QXSPP4CPYTXIURKTANCNFSM5X7VD5TQ . You are receiving this because you were mentioned.Message ID: @.*** com>

[david-a-wheeler]

Understood. Welcome back!

[andrewfader]

Still working on this. Will try to wrap it up.

[david-a-wheeler]

Thanks. One less dependency, especially one that's security-critical, is a good thing.

We need to move the site to bestpractices.dev, but I'm trying to minimize the number of big changes. So if that could completed soon that'd be great.