Lazily evaluate regional encryptor KMS ciphertext#11953
Lazily evaluate regional encryptor KMS ciphertext#11953mitchellhenke wants to merge 4 commits intomainfrom
Conversation
changelog: Internal, Performance, Lazily evaluate regional encryptor ciphertext
mitchellhenke
changed the title
| @@ -0,0 +1,15 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| Encryption::RegionalDigestPair = RedactedStruct.new( | |||
There was a problem hiding this comment.
What's the reasoning for changing the name here to "digest"?
I think the way that we use this class neither "digest" nor "ciphertext" is technically correct. Within password encryption we use a hash function at which point we create a digest which is not reversible. The PII encryption however creates a reversible ciphertext.
Maybe we could change this to something like RegionalEncryptedValue?
There was a problem hiding this comment.
Yeah, that makes sense. I've renamed in 2211af1.
mitchellhenke
force-pushed
the
mitchellhenke|aduth/lazy-regional-ciphertext
branch
from
8579f09 to
2211af1
Compare
jmhooper
left a comment
jmhooper
left a comment
There was a problem hiding this comment.
I had a couple comments here. I'm wondering if the description on this pull request may be out of date? It looks like a refactor to change the way we manage things in the encryption module, but I don't see the optimization to stop doing KMS operations.
| include ::NewRelic::Agent::MethodTracer | ||
|
|
||
| Ciphertext = RedactedStruct.new(:encrypted_data, :salt, :cost, allowed_members: [:cost]) do | ||
| Digest = RedactedStruct.new( |
There was a problem hiding this comment.
I'll remark again here that this is not a digest since it is reversible.
| def decrypt(digest_pair, user_uuid: nil) | ||
| ciphertext_string = digest_pair.multi_or_single_region_encrypted_value.to_s | ||
| ciphertext = Digest.parse_from_string(ciphertext_string) | ||
| aes_encrypted_ciphertext = multi_region_kms_client.decrypt( |
There was a problem hiding this comment.
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.
I don't think I see where this change actually occurs. It looks here like the decryption selects a multi or single region value and then goes to KMS with just that value. I don't see where both values are being eagerly decrypted in the existing code.
|
Closing in favor of #11963 |
3 participants
🛠 Summary of changes
This resurrects #11876 with a few small changes, so I've copied the PR description from it.
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