Lazily evaluate regional encryptor KMS ciphertext #11876
Conversation
changelog: Internal, Performance, Lazily evaluate regional encryptor ciphertext
There was a problem hiding this comment.
Took a bit for me to digest this, but I think I'm following it.
Previously, we'd:
- Have some text we need to encrypt with KMS (either a password or PII blob)
- We'd encrypt the text with both the single region KMS key and multi region KMS key
- Save the multi-region key encrypted text to the database
These changes pull up the processing of the text so that we don't encrypt with both keys, which makes sense. I'm aware that there are elements that need to be decrypted with the single-region key, but it's not clear if there are any cases we're encrypting with the single-region key. Should we be instantiating the objects with the single-region key unless we're decrypting?
| def encrypted_data | ||
| self[:encrypted_data] || | ||
| kms_client.encrypt(aes_encrypted_ciphertext, user_kms_encryption_context) | ||
| end |
There was a problem hiding this comment.
This and encrypted_password give me some pause. It doesn't feel super clear that calling it will potentially take the action of encrypting. I'm not really sure what approach would be preferable. I'm not sure if separating it (e.g. something like encrypt_data!) so that the setting and getting are separate would be better?
🛠 Summary of changes
Updates encryptor classes to lazily evaluate regional KMS ciphertexts.
Since #9047, we've added support for multi-region KMS encryption, with fallback to single-region encryption in a few specific cases where multi-region is not possible (e.g. legacy personal keys). The fallback is such that we only ever use one of the two ciphertexts, but prior to the changes proposed here, we would always eagerly evaluate the ciphertext for both single-region and multi-region.
With these changes, we defer the KMS encryption until after we've evaluated which regional digest to work with, so that only a single KMS encryption occurs.
📜 Testing Plan
TBD