Encryption: support support_unencrypted_data at a per-attribute level

[ghiculescu]

If ActiveRecord::Encryption.config.support_unencrypted_data == true, this allows you to do:

class User < ActiveRecord::Base
  encrypts :name, deterministic: true, support_unencrypted_data: false
  encrypts :email, deterministic: true
end

Now only the email column will allow unencrypted data (and if extend_queries is true, only email queries will get extended). Here is some back story on why you might want this.

[jorgemanrubia]

I have mixed feelings about this one @ghiculescu.

The query extension system is meant to make deterministically encrypted attributes work in queries, which is a core behavior you want in place if you use encryption. I think a global configuration flag makes sense, in case you want to completely disable the system. But I don't see the fine-grained configuration option justified. The case of gradually enabling this on a per-column basis doesn't feel enough justification to me. What's the benefit compared to enable the flag globally?

[ghiculescu]

You should extend queries if you know you have a mixture of encrypted and unencrypted data in a column you’re querying by.

If you know all data in a column is encrypted then you don’t need extended queries for that column. But if you enable the configuration globally then you still have the extra overhead added to all your queries.

This PR lets you enable the config globally and then selectively opt out of it on specific columns that don’t need it. For example if you backfill a column so that it’s fully encrypted then you can stop extended queries for just that column.

(Maybe I made it confusing by talking about opting-in per column in the PR body. That doesn’t do anything different to how it works now.)

[jorgemanrubia]

Thanks, I understand better the scenario you have in mind with your last comment. Notice that the system is also used when you use different encryption schemes (previous: option lets you define past encryption properties for the attribute).

My main concern is: the new option adds a little bit of additional complexity, both for people seeing how to use the framework and internally. Does the overhead-saving gain justifies the new config option? I haven't measured, but it would be surprising to me. Also, think that situations where you manage encrypted/unencrypted data should be a transient one. But, even if it was permanent, would the gain be justified?

[ghiculescu]

I’m not sure if that question is rhetorical, but for the record, yeah I think the gain is worth it.

As you say it’s hopefully a transient state, but some backfills can take a very long time or not happen at all for whatever reason.

The slightly more complex API makes up for it making for a smoother process of gradually encrypting everything. It makes the transition to fully using encryption less scary.

[joshuay03]

Looking great! Just some minor comments and questions.

[jorgemanrubia]

I’m not sure if that question is rhetorical, but for the record, yeah I think the gain is worth it.

@ghiculescu not rhetorical. Could you elaborate why you think it's worth it? By overhead you mean a performance overhead, right? What's the performance gain?

[jorgemanrubia]

It makes the transition to fully using encryption less scary.

Also interested in understanding how this option makes it less scary. Is it related to the referred overhead? Or to the psychological element in seeing the queries altered when using deterministic encryption + support for unencrypted data?

[ghiculescu]

@jorgemanrubia The backstory for this is that we have a big application that's over a decade old. I've been gradually adding encryption to parts of it over the last year. Other teams have also been adding encryption, both to existing features and new ones.

The current state is we have a reasonable number of encrypted columns (over 30, and I still have more I want to do). About a quarter of these are deterministic. Some are fully encrypted, because encryption was enabled when the column was added. Some are fully encrypted, because all the data has been backfilled. And some are not fully encrypted.

The eventual goal is to have every encrypted column be fully encrypted, and to turn supports_encypted_data. But this seems like it won't happen for a long time, because we continue to add encryption to existing columns and then gradually backfill them. So I'm stuck with supports_encrypted_data and extend_queries being true for the foreseeable future.

This leaves me with a few issues:

1. Without connecting to the production database and querying every row, I don't have a good source of truth on which columns are fully encrypted and which are not.
2. Once I do backfill a column, I'm still incurring the overhead that extend_queries adds for every query to that column. This isn't much (your tests ensure it's not more than 20% slower), but it adds up, particularly on heavily used columns.

With this PR, I can tick off both of these issues.

1. As soon as a backfill is complete, I can disable extend_queries just for that column. Now I have the code as a source of truth for this.
2. Disabling extend_queries speeds up querying of that column. (I'm not sure how much by, but it can't slow it down.) This is also handy for columns that are fully encrypted from day one and never need a backfill.

All this leads to my comment about scariness. Let's say one day we decide there's no more deterministic columns to encrypt, and we think we've backfilled everything. We deploy a change to production that turns extend_queries off at the application level. This is a scary change, because it's hard to know ahead of time if it will break things. By moving extend_queries to the column level we can shrink the blast radius; once every deterministic column has extend_queries: false, we know it's safe to turn the application configuration off.

ps. I think I should have led with this, my PR body was pretty basic. Sorry for the lack of context, and thanks for bearing with me.

[jorgemanrubia]

Thanks @ghiculescu, I understand context much better now 🙏. I agree that such support would help to gradually reach the situation where you can disable support for unencrypted data. I see the value in that.

I'd suggest the following:

• Instead of naming the option extend_queries make it support_unencrypted_data.
• When the option is set (not nil), use it here. That will make that EncryptedAttributeType#previous_types to exclude the clean_text_scheme, and would result in the system to extend deterministic queries not altering the query. As a side benefit, this would play well with the case where there are previous encryption schemes.

[ghiculescu]

I’ll make these follow ups tomorrow 👍

[jorgemanrubia]

Looks great @ghiculescu 👏

[jorgemanrubia]

Great work @ghiculescu, this is a nice addition 👏

[rafaelfranca]

Why is this in the public documentation of this module. And why we have @todo on it? Let's remove those from the public documentation.

[ghiculescu]

I'm not sure if this module should be public API at all. Could we make the whole thing :nodoc:?

I will move the TODOs and internal notes outside of the public docs for now.

[ghiculescu]

@jorgemanrubia do you have any thoughts on the public/private state of the docs here?

[jorgemanrubia]

Yes, those notes are meant to be internal comments, not part of public API. I think your change here was fine.

[ghiculescu]

But should this module be public at all?

[jorgemanrubia]

yeah I think it should

[ghiculescu]

@rafaelfranca Could you please take another look at this one?