Skip to content
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

Lazily evaluate regional encryptor KMS ciphertext #11876

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions app/models/concerns/user_access_key_overrides.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,9 @@ def password=(new_password)
end

def password_regional_digest_pair
Encryption::RegionalCiphertextPair.new(
single_region_ciphertext: encrypted_password_digest,
multi_region_ciphertext: encrypted_password_digest_multi_region,
Encryption::RegionalDigestPair.new(
single_region_digest: encrypted_password_digest,
multi_region_digest: encrypted_password_digest_multi_region,
)
end

Expand All @@ -58,9 +58,9 @@ def personal_key=(new_personal_key)
end

def recovery_code_regional_digest_pair
Encryption::RegionalCiphertextPair.new(
single_region_ciphertext: encrypted_recovery_code_digest,
multi_region_ciphertext: encrypted_recovery_code_digest_multi_region,
Encryption::RegionalDigestPair.new(
single_region_digest: encrypted_recovery_code_digest,
multi_region_digest: encrypted_recovery_code_digest_multi_region,
)
end

Expand Down
20 changes: 10 additions & 10 deletions app/models/profile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -268,26 +268,26 @@ def reject_for_fraud(notify_user:)
def decrypt_pii(password)
encryptor = Encryption::Encryptors::PiiEncryptor.new(password)

encrypted_pii_ciphertext_pair = Encryption::RegionalCiphertextPair.new(
single_region_ciphertext: encrypted_pii,
multi_region_ciphertext: encrypted_pii_multi_region,
encrypted_pii_digest_pair = Encryption::RegionalDigestPair.new(
single_region_digest: encrypted_pii,
multi_region_digest: encrypted_pii_multi_region,
)

decrypted_json = encryptor.decrypt(encrypted_pii_ciphertext_pair, user_uuid: user.uuid)
decrypted_json = encryptor.decrypt(encrypted_pii_digest_pair, user_uuid: user.uuid)
Pii::Attributes.new_from_json(decrypted_json)
end

# @return [Pii::Attributes]
def recover_pii(personal_key)
encryptor = Encryption::Encryptors::PiiEncryptor.new(personal_key)

encrypted_pii_recovery_ciphertext_pair = Encryption::RegionalCiphertextPair.new(
single_region_ciphertext: encrypted_pii_recovery,
multi_region_ciphertext: encrypted_pii_recovery_multi_region,
encrypted_pii_recovery_digest_pair = Encryption::RegionalDigestPair.new(
single_region_digest: encrypted_pii_recovery,
multi_region_digest: encrypted_pii_recovery_multi_region,
)

decrypted_recovery_json = encryptor.decrypt(
encrypted_pii_recovery_ciphertext_pair, user_uuid: user.uuid
encrypted_pii_recovery_digest_pair, user_uuid: user.uuid
)
return nil if JSON.parse(decrypted_recovery_json).nil?
Pii::Attributes.new_from_json(decrypted_recovery_json)
Expand All @@ -300,7 +300,7 @@ def encrypt_pii(pii, password)
encryptor = Encryption::Encryptors::PiiEncryptor.new(password)
self.encrypted_pii, self.encrypted_pii_multi_region = encryptor.encrypt(
pii.to_json, user_uuid: user.uuid
)
).map(&:to_s)
encrypt_recovery_pii(pii)
end

Expand All @@ -312,7 +312,7 @@ def encrypt_recovery_pii(pii, personal_key: nil)
)
self.encrypted_pii_recovery, self.encrypted_pii_recovery_multi_region = encryptor.encrypt(
pii.to_json, user_uuid: user.uuid
)
).map(&:to_s)
@personal_key = personal_key
end

Expand Down
56 changes: 37 additions & 19 deletions app/services/encryption/encryptors/pii_encryptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,37 @@ module Encryptors
class PiiEncryptor
include ::NewRelic::Agent::MethodTracer

Ciphertext = RedactedStruct.new(:encrypted_data, :salt, :cost, allowed_members: [:cost]) do
Digest = RedactedStruct.new(
:encrypted_data,
:aes_encrypted_ciphertext,
:user_kms_encryption_context,
:kms_client,
:salt,
:cost,
keyword_init: true,
allowed_members: [:cost],
) do
include Encodable
class << self
include Encodable
end

def self.parse_from_string(ciphertext_string)
parsed_json = JSON.parse(ciphertext_string)
new(extract_encrypted_data(parsed_json), parsed_json['salt'], parsed_json['cost'])
new(
encrypted_data: extract_encrypted_data(parsed_json),
salt: parsed_json['salt'],
cost: parsed_json['cost'],
)
rescue JSON::ParserError
raise EncryptionError, 'ciphertext is not valid JSON'
end

def encrypted_data
self[:encrypted_data] ||
kms_client.encrypt(aes_encrypted_ciphertext, user_kms_encryption_context)
end
Comment on lines +34 to +37
Copy link
Contributor

@mitchellhenke mitchellhenke Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


def to_s
{
encrypted_data: encode(encrypted_data),
Expand Down Expand Up @@ -51,29 +69,29 @@ def encrypt(plaintext, user_uuid: nil)
cost = IdentityConfig.store.scrypt_cost
aes_encryption_key = scrypt_password_digest(salt: salt, cost: cost)
aes_encrypted_ciphertext = aes_cipher.encrypt(plaintext, aes_encryption_key)
single_region_kms_encrypted_ciphertext = single_region_kms_client.encrypt(
aes_encrypted_ciphertext, kms_encryption_context(user_uuid: user_uuid)
user_kms_encryption_context = kms_encryption_context(user_uuid: user_uuid)
single_region_digest = Digest.new(
kms_client: single_region_kms_client,
aes_encrypted_ciphertext:,
user_kms_encryption_context:,
salt:,
cost:,
)
single_region_ciphertext = Ciphertext.new(
single_region_kms_encrypted_ciphertext, salt, cost
).to_s

multi_region_kms_encrypted_ciphertext = multi_region_kms_client.encrypt(
aes_encrypted_ciphertext, kms_encryption_context(user_uuid: user_uuid)
multi_region_digest = Digest.new(
kms_client: multi_region_kms_client,
aes_encrypted_ciphertext:,
user_kms_encryption_context:,
salt:,
cost:,
)
multi_region_ciphertext = Ciphertext.new(
multi_region_kms_encrypted_ciphertext, salt, cost
).to_s

RegionalCiphertextPair.new(
single_region_ciphertext: single_region_ciphertext,
multi_region_ciphertext: multi_region_ciphertext,
)
RegionalDigestPair.new(single_region_digest:, multi_region_digest:)
end

def decrypt(ciphertext_pair, user_uuid: nil)
ciphertext_string = ciphertext_pair.multi_or_single_region_ciphertext
ciphertext = Ciphertext.parse_from_string(ciphertext_string)
def decrypt(digest_pair, user_uuid: nil)
ciphertext_string = digest_pair.multi_or_single_region_digest.to_s
ciphertext = Digest.parse_from_string(ciphertext_string)
aes_encrypted_ciphertext = multi_region_kms_client.decrypt(
ciphertext.encrypted_data, kms_encryption_context(user_uuid: user_uuid)
)
Expand Down
34 changes: 19 additions & 15 deletions app/services/encryption/password_verifier.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ class PasswordVerifier
include ::NewRelic::Agent::MethodTracer

PasswordDigest = RedactedStruct.new(
:kms_client,
:scrypted_password,
:user_kms_encryption_context,
:encrypted_password,
:encryption_key,
:password_salt,
Expand All @@ -18,6 +21,11 @@ def self.parse_from_string(digest_string)
raise EncryptionError, 'digest contains invalid json'
end

def encrypted_password
self[:encrypted_password] ||
kms_client.encrypt(scrypted_password, user_kms_encryption_context)
end

def to_s
{
encrypted_password: encrypted_password,
Expand Down Expand Up @@ -45,33 +53,29 @@ def create_digest_pair(password:, user_uuid:)
salt = SecureRandom.hex(32)
cost = IdentityConfig.store.scrypt_cost
scrypted_password = scrypt_password_digest(salt: salt, cost: cost, password: password)
user_kms_encryption_context = kms_encryption_context(user_uuid:)

single_region_encrypted_password = single_region_kms_client.encrypt(
scrypted_password, kms_encryption_context(user_uuid: user_uuid)
)
single_region_digest = PasswordDigest.new(
encrypted_password: single_region_encrypted_password,
kms_client: single_region_kms_client,
scrypted_password:,
user_kms_encryption_context:,
password_salt: salt,
password_cost: cost,
).to_s

multi_region_encrypted_password = multi_region_kms_client.encrypt(
scrypted_password, kms_encryption_context(user_uuid: user_uuid)
)

multi_region_digest = PasswordDigest.new(
encrypted_password: multi_region_encrypted_password,
kms_client: multi_region_kms_client,
scrypted_password:,
user_kms_encryption_context:,
password_salt: salt,
password_cost: cost,
).to_s

RegionalCiphertextPair.new(
single_region_ciphertext: single_region_digest,
multi_region_ciphertext: multi_region_digest,
)

RegionalDigestPair.new(single_region_digest:, multi_region_digest:)
end

def verify(password:, digest_pair:, user_uuid:, log_context:)
digest = digest_pair.multi_or_single_region_ciphertext
digest = digest_pair.multi_or_single_region_digest.to_s
password_digest = PasswordDigest.parse_from_string(digest)
if password_digest.uak_password_digest?
return verify_uak_digest(password, digest, user_uuid, log_context)
Expand Down
13 changes: 0 additions & 13 deletions app/services/encryption/regional_ciphertext_pair.rb

This file was deleted.

15 changes: 15 additions & 0 deletions app/services/encryption/regional_digest_pair.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

Encryption::RegionalDigestPair = RedactedStruct.new(
:single_region_digest,
:multi_region_digest,
keyword_init: true,
) do
def to_ary
[single_region_digest, multi_region_digest]
end

def multi_or_single_region_digest
multi_region_digest.presence || single_region_digest
end
end.freeze
2 changes: 1 addition & 1 deletion spec/models/user_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1293,7 +1293,7 @@ def it_should_not_send_survey
encrypted_pii_recovery, encrypted_pii_recovery_multi_region =
Encryption::Encryptors::PiiEncryptor.new(
personal_key,
).encrypt('null', user_uuid: user.uuid).single_region_ciphertext
).encrypt('null', user_uuid: user.uuid).single_region_digest.to_s

create(
:profile,
Expand Down
52 changes: 29 additions & 23 deletions spec/services/encryption/encryptors/pii_encryptor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@

subject { described_class.new(password) }

describe Encryption::Encryptors::PiiEncryptor::Ciphertext do
describe Encryption::Encryptors::PiiEncryptor::Digest do
describe '.parse_from_string' do
it 'does not blow up with unknown/new keys' do
blob = Encryption::Encryptors::PiiEncryptor::Ciphertext.new('encrypted_data').to_s
blob = described_class.new(encrypted_data: 'encrypted_data').to_s
str = JSON.parse(blob).merge(some_new_field: 'some_new_field').to_json

ciphertext = Encryption::Encryptors::PiiEncryptor::Ciphertext.parse_from_string(str)
ciphertext = described_class.parse_from_string(str)
expect(ciphertext.encrypted_data).to eq('encrypted_data')
end
end
Expand Down Expand Up @@ -60,18 +60,18 @@
.with('aes_ciphertext', { 'context' => 'pii-encryption', 'user_uuid' => 'uuid-123-abc' })
.and_return('multi_region_kms_ciphertext')

ciphertext_single_region, ciphertext_multi_region = subject.encrypt(
digest_single_region, digest_multi_region = subject.encrypt(
plaintext, user_uuid: 'uuid-123-abc'
)

expect(ciphertext_single_region).to eq(
expect(digest_single_region.to_s).to eq(
{
encrypted_data: Base64.strict_encode64('single_region_kms_ciphertext'),
salt: salt,
cost: '800$8$1$',
}.to_json,
)
expect(ciphertext_multi_region).to eq(
expect(digest_multi_region.to_s).to eq(
{
encrypted_data: Base64.strict_encode64('multi_region_kms_ciphertext'),
salt: salt,
Expand Down Expand Up @@ -118,33 +118,39 @@
.with('aes_ciphertext', decoded_scrypt_digest)
.and_return(plaintext)

ciphertext_pair = Encryption::RegionalCiphertextPair.new(
single_region_ciphertext: {
encrypted_data: Base64.strict_encode64('kms_ciphertext_sr'),
salt: salt,
cost: '800$8$1$',
}.to_json,
multi_region_ciphertext: {
encrypted_data: Base64.strict_encode64('kms_ciphertext_mr'),
salt: salt,
cost: '800$8$1$',
}.to_json,
digest_pair = Encryption::RegionalDigestPair.new(
single_region_digest: double(
Encryption::Encryptors::PiiEncryptor::Digest,
to_s: {
encrypted_data: Base64.strict_encode64('kms_ciphertext_sr'),
salt: salt,
cost: '800$8$1$',
}.to_json,
),
multi_region_digest: double(
Encryption::Encryptors::PiiEncryptor::Digest,
to_s: {
encrypted_data: Base64.strict_encode64('kms_ciphertext_mr'),
salt: salt,
cost: '800$8$1$',
}.to_json,
),
)

result = subject.decrypt(ciphertext_pair, user_uuid: 'uuid-123-abc')
result = subject.decrypt(digest_pair, user_uuid: 'uuid-123-abc')

expect(result).to eq(plaintext)
end

it 'uses the single region ciphertext if the multi-region ciphertext is nil' do
test_ciphertext_pair = Encryption::RegionalCiphertextPair.new(
single_region_ciphertext: subject.encrypt(
test_digest_pair = Encryption::RegionalDigestPair.new(
single_region_digest: subject.encrypt(
'single-region-text', user_uuid: '123abc'
).single_region_ciphertext,
multi_region_ciphertext: nil,
).single_region_digest,
multi_region_digest: nil,
)

result = subject.decrypt(test_ciphertext_pair, user_uuid: '123abc')
result = subject.decrypt(test_digest_pair, user_uuid: '123abc')

expect(result).to eq('single-region-text')
end
Expand Down
Loading