Skip to content
Closed
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::RegionalEncryptedValuePair.new(
single_region_encrypted_value: encrypted_password_digest,
multi_region_encrypted_value: 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::RegionalEncryptedValuePair.new(
single_region_encrypted_value: encrypted_recovery_code_digest,
multi_region_encrypted_value: 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::RegionalEncryptedValuePair.new(
single_region_encrypted_value: encrypted_pii,
multi_region_encrypted_value: 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::RegionalEncryptedValuePair.new(
single_region_encrypted_value: encrypted_pii_recovery,
multi_region_encrypted_value: 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
60 changes: 42 additions & 18 deletions app/services/encryption/encryptors/pii_encryptor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,38 @@ module Encryptors
class PiiEncryptor
include ::NewRelic::Agent::MethodTracer

Ciphertext = RedactedStruct.new(:encrypted_data, :salt, :cost, allowed_members: [:cost]) do
EncryptedValue = 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 encrypt_data!
self[:encrypted_data] =
kms_client.encrypt(aes_encrypted_ciphertext, user_kms_encryption_context)
nil
end

def to_s
{
encrypted_data: encode(encrypted_data),
Expand Down Expand Up @@ -51,29 +70,34 @@ 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_encrypted_value = EncryptedValue.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
single_region_encrypted_value.encrypt_data!

multi_region_kms_encrypted_ciphertext = multi_region_kms_client.encrypt(
aes_encrypted_ciphertext, kms_encryption_context(user_uuid: user_uuid)
multi_region_encrypted_value = EncryptedValue.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
multi_region_encrypted_value.encrypt_data!

RegionalCiphertextPair.new(
single_region_ciphertext: single_region_ciphertext,
multi_region_ciphertext: multi_region_ciphertext,
RegionalEncryptedValuePair.new(
single_region_encrypted_value:,
multi_region_encrypted_value:,
)
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_encrypted_value.to_s
ciphertext = EncryptedValue.parse_from_string(ciphertext_string)
aes_encrypted_ciphertext = multi_region_kms_client.decrypt(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

ciphertext.encrypted_data, kms_encryption_context(user_uuid: user_uuid)
)
Expand Down
41 changes: 24 additions & 17 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 encrypt_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,32 @@ 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,
single_region_encrypted_value = PasswordDigest.new(
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,

single_region_encrypted_value.encrypt_password!

multi_region_encrypted_value = PasswordDigest.new(
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,
)
multi_region_encrypted_value.encrypt_password!

RegionalEncryptedValuePair.new(single_region_encrypted_value:, multi_region_encrypted_value:)
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_encrypted_value.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_encrypted_value_pair.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# frozen_string_literal: true

Encryption::RegionalEncryptedValuePair = RedactedStruct.new(
:single_region_encrypted_value,
:multi_region_encrypted_value,
keyword_init: true,
) do
def to_ary
[single_region_encrypted_value, multi_region_encrypted_value]
end

def multi_or_single_region_encrypted_value
multi_region_encrypted_value.presence || single_region_encrypted_value
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 @@ -1332,7 +1332,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_encrypted_value.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::EncryptedValue 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::RegionalEncryptedValuePair.new(
single_region_encrypted_value: double(
Encryption::Encryptors::PiiEncryptor::EncryptedValue,
to_s: {
encrypted_data: Base64.strict_encode64('kms_ciphertext_sr'),
salt: salt,
cost: '800$8$1$',
}.to_json,
),
multi_region_encrypted_value: double(
Encryption::Encryptors::PiiEncryptor::EncryptedValue,
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::RegionalEncryptedValuePair.new(
single_region_encrypted_value: subject.encrypt(
'single-region-text', user_uuid: '123abc'
).single_region_ciphertext,
multi_region_ciphertext: nil,
).single_region_encrypted_value,
multi_region_encrypted_value: 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