diff --git a/app/models/concerns/user_access_key_overrides.rb b/app/models/concerns/user_access_key_overrides.rb index 74be964cd1a..d84456606d8 100644 --- a/app/models/concerns/user_access_key_overrides.rb +++ b/app/models/concerns/user_access_key_overrides.rb @@ -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 @@ -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 diff --git a/app/models/profile.rb b/app/models/profile.rb index 43219c8b851..5953403c789 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -268,12 +268,12 @@ 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 @@ -281,13 +281,13 @@ def decrypt_pii(password) 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) @@ -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 @@ -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 diff --git a/app/services/encryption/encryptors/pii_encryptor.rb b/app/services/encryption/encryptors/pii_encryptor.rb index c577f04b9b0..24e72826d74 100644 --- a/app/services/encryption/encryptors/pii_encryptor.rb +++ b/app/services/encryption/encryptors/pii_encryptor.rb @@ -5,7 +5,16 @@ 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 @@ -13,11 +22,21 @@ class << self 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), @@ -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( ciphertext.encrypted_data, kms_encryption_context(user_uuid: user_uuid) ) diff --git a/app/services/encryption/password_verifier.rb b/app/services/encryption/password_verifier.rb index 46bd17174cc..50135682aeb 100644 --- a/app/services/encryption/password_verifier.rb +++ b/app/services/encryption/password_verifier.rb @@ -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, @@ -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, @@ -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) diff --git a/app/services/encryption/regional_ciphertext_pair.rb b/app/services/encryption/regional_ciphertext_pair.rb deleted file mode 100644 index 09db686c95b..00000000000 --- a/app/services/encryption/regional_ciphertext_pair.rb +++ /dev/null @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -Encryption::RegionalCiphertextPair = RedactedStruct.new( - :single_region_ciphertext, :multi_region_ciphertext, keyword_init: true -) do - def to_ary - [single_region_ciphertext, multi_region_ciphertext] - end - - def multi_or_single_region_ciphertext - multi_region_ciphertext.presence || single_region_ciphertext - end -end.freeze diff --git a/app/services/encryption/regional_encrypted_value_pair.rb b/app/services/encryption/regional_encrypted_value_pair.rb new file mode 100644 index 00000000000..bec161a031b --- /dev/null +++ b/app/services/encryption/regional_encrypted_value_pair.rb @@ -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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 7417fb9627e..eb1a0bd4c56 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -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, diff --git a/spec/services/encryption/encryptors/pii_encryptor_spec.rb b/spec/services/encryption/encryptors/pii_encryptor_spec.rb index 6d9f6b3a0ef..9a7fbf4b288 100644 --- a/spec/services/encryption/encryptors/pii_encryptor_spec.rb +++ b/spec/services/encryption/encryptors/pii_encryptor_spec.rb @@ -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 @@ -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, @@ -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 diff --git a/spec/services/encryption/password_verifier_spec.rb b/spec/services/encryption/password_verifier_spec.rb index 31bc2bf1162..81d14f2e2c1 100644 --- a/spec/services/encryption/password_verifier_spec.rb +++ b/spec/services/encryption/password_verifier_spec.rb @@ -64,12 +64,22 @@ password: password, user_uuid: user_uuid, ) - expect(JSON.parse(digest_pair.single_region_ciphertext, symbolize_names: true)).to match( + expect( + JSON.parse( + digest_pair.single_region_encrypted_value.to_s, + symbolize_names: true, + ), + ).to match( password_salt: salt, password_cost: IdentityConfig.store.scrypt_cost, encrypted_password: 'single_region_kms_ciphertext', ) - expect(JSON.parse(digest_pair.multi_region_ciphertext, symbolize_names: true)).to match( + expect( + JSON.parse( + digest_pair.multi_region_encrypted_value.to_s, + symbolize_names: true, + ), + ).to match( password_salt: salt, password_cost: IdentityConfig.store.scrypt_cost, encrypted_password: 'multi_region_kms_ciphertext', @@ -102,9 +112,15 @@ it 'returns false for nonsense' do result = subject.verify( - digest_pair: Encryption::RegionalCiphertextPair.new( - single_region_ciphertext: 'nonsense', - multi_region_ciphertext: 'nonsense on stilts', + digest_pair: Encryption::RegionalEncryptedValuePair.new( + single_region_encrypted_value: double( + Encryption::PasswordVerifier::PasswordDigest, + to_s: 'nonsense', + ), + multi_region_encrypted_value: double( + Encryption::PasswordVerifier::PasswordDigest, + to_s: 'nonsense on stilts', + ), ), password: password, user_uuid: user_uuid, @@ -115,9 +131,12 @@ end it 'allows verification of legacy UAK passwords' do - legacy_digest_pair = Encryption::RegionalCiphertextPair.new( - single_region_ciphertext: Encryption::UakPasswordVerifier.digest(password), - multi_region_ciphertext: nil, + legacy_digest_pair = Encryption::RegionalEncryptedValuePair.new( + single_region_encrypted_value: double( + Encryption::PasswordVerifier::PasswordDigest, + to_s: Encryption::UakPasswordVerifier.digest(password), + ), + multi_region_encrypted_value: nil, ) good_match_result = subject.verify( @@ -144,7 +163,7 @@ password: password, user_uuid: user_uuid, ) - test_digest_pair.multi_region_ciphertext = nil + test_digest_pair.multi_region_encrypted_value = nil correct_password_result = subject.verify( password: password, @@ -176,7 +195,7 @@ it 'returns false if the digest is fresh' do digest = subject.create_digest_pair( password: password, user_uuid: user_uuid, - ).single_region_ciphertext + ).single_region_encrypted_value.to_s result = subject.stale_digest?(digest)