diff --git a/.reek b/.reek index 2eb24e1fbf1..cd2da766a4a 100644 --- a/.reek +++ b/.reek @@ -69,7 +69,6 @@ LongParameterList: - Idv::ProoferJob#perform - Idv::VendorResult#initialize - JWT - - Pii::Attributes#self.new_from_encrypted RepeatedConditional: exclude: - Users::ResetPasswordsController diff --git a/app/models/profile.rb b/app/models/profile.rb index 4dea5a1b66c..98aeff5be39 100644 --- a/app/models/profile.rb +++ b/app/models/profile.rb @@ -33,39 +33,27 @@ def deactivate(reason) def decrypt_pii(password) Pii::Attributes.new_from_encrypted( encrypted_pii, - password: password, - salt: user.password_salt, - cost: user.password_cost + password: password ) end def recover_pii(personal_key) Pii::Attributes.new_from_encrypted( encrypted_pii_recovery, - password: personal_key, - salt: user.recovery_salt, - cost: user.recovery_cost + password: personal_key ) end def encrypt_pii(pii, password) ssn = pii.ssn self.ssn_signature = Pii::Fingerprinter.fingerprint(ssn) if ssn - self.encrypted_pii = pii.encrypted( - password: password, - salt: user.password_salt, - cost: user.password_cost - ) + self.encrypted_pii = pii.encrypted(password) encrypt_recovery_pii(pii) end def encrypt_recovery_pii(pii) personal_key = personal_key_generator.create - self.encrypted_pii_recovery = pii.encrypted( - password: personal_key_generator.normalize(personal_key), - salt: user.recovery_salt, - cost: user.recovery_cost - ) + self.encrypted_pii_recovery = pii.encrypted(personal_key_generator.normalize(personal_key)) @personal_key = personal_key end diff --git a/app/services/encryption/encryptors/pii_encryptor.rb b/app/services/encryption/encryptors/pii_encryptor.rb index d6cffd611d6..2dcb2ab529a 100644 --- a/app/services/encryption/encryptors/pii_encryptor.rb +++ b/app/services/encryption/encryptors/pii_encryptor.rb @@ -1,39 +1,66 @@ module Encryption module Encryptors class PiiEncryptor - include Pii::Encodable + Ciphertext = Struct.new(:encrypted_data, :salt, :cost) do + include Pii::Encodable + class << self + include Pii::Encodable + end - def initialize(password:, salt:, cost: nil) - cost ||= Figaro.env.scrypt_cost + 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']) + rescue JSON::ParserError + raise Pii::EncryptionError, 'ciphertext is not valid JSON' + end + + def to_s + { + encrypted_data: encode(encrypted_data), + salt: salt, + cost: cost, + }.to_json + end + + def self.extract_encrypted_data(parsed_json) + encoded_encrypted_data = parsed_json['encrypted_data'] + raise Pii::EncryptionError, 'ciphertext invalid' unless valid_base64_encoding?( + encoded_encrypted_data + ) + decode(encoded_encrypted_data) + end + end + + def initialize(password) + @password = password @aes_cipher = Pii::Cipher.new @kms_client = KmsClient.new - @scrypt_password_digest = build_scrypt_password(password, salt, cost).digest end def encrypt(plaintext) + salt = Devise.friendly_token[0, 20] + cost = Figaro.env.scrypt_cost + aes_encryption_key = scrypt_password_digest(salt: salt, cost: cost) aes_encrypted_ciphertext = aes_cipher.encrypt(plaintext, aes_encryption_key) kms_encrypted_ciphertext = kms_client.encrypt(aes_encrypted_ciphertext) - encode(kms_encrypted_ciphertext) + Ciphertext.new(kms_encrypted_ciphertext, salt, cost).to_s end - def decrypt(ciphertext) - raise Pii::EncryptionError, 'ciphertext invalid' unless valid_base64_encoding?(ciphertext) - decoded_ciphertext = decode(ciphertext) - aes_encrypted_ciphertext = kms_client.decrypt(decoded_ciphertext) + def decrypt(ciphertext_string) + ciphertext = Ciphertext.parse_from_string(ciphertext_string) + aes_encrypted_ciphertext = kms_client.decrypt(ciphertext.encrypted_data) + aes_encryption_key = scrypt_password_digest(salt: ciphertext.salt, cost: ciphertext.cost) aes_cipher.decrypt(aes_encrypted_ciphertext, aes_encryption_key) end private - attr_reader :aes_cipher, :kms_client, :scrypt_password_digest + attr_reader :password, :aes_cipher, :kms_client - def build_scrypt_password(password, salt, cost) + def scrypt_password_digest(salt:, cost:) scrypt_salt = cost + OpenSSL::Digest::SHA256.hexdigest(salt) scrypted = SCrypt::Engine.hash_secret password, scrypt_salt, 32 - SCrypt::Password.new(scrypted) - end - - def aes_encryption_key + scrypt_password_digest = SCrypt::Password.new(scrypted).digest scrypt_password_digest[0...32] end end diff --git a/app/services/pii/attributes.rb b/app/services/pii/attributes.rb index 02bb5823631..0a4778b98b3 100644 --- a/app/services/pii/attributes.rb +++ b/app/services/pii/attributes.rb @@ -18,12 +18,8 @@ def self.new_from_hash(hash) attrs end - def self.new_from_encrypted(encrypted, password:, salt:, cost:) - encryptor = Encryption::Encryptors::PiiEncryptor.new( - password: password, - salt: salt, - cost: cost - ) + def self.new_from_encrypted(encrypted, password:) + encryptor = Encryption::Encryptors::PiiEncryptor.new(password) decrypted = encryptor.decrypt(encrypted) new_from_json(decrypted) end @@ -39,12 +35,8 @@ def initialize(*args) assign_all_members end - def encrypted(password:, salt:, cost:) - encryptor = Encryption::Encryptors::PiiEncryptor.new( - password: password, - salt: salt, - cost: cost - ) + def encrypted(password) + encryptor = Encryption::Encryptors::PiiEncryptor.new(password) encryptor.encrypt(to_json) end diff --git a/saml_20180607094309.txt b/saml_20180607094309.txt new file mode 100644 index 00000000000..e69de29bb2d diff --git a/spec/controllers/users/sessions_controller_spec.rb b/spec/controllers/users/sessions_controller_spec.rb index 84bf4734687..5414e70873b 100644 --- a/spec/controllers/users/sessions_controller_spec.rb +++ b/spec/controllers/users/sessions_controller_spec.rb @@ -271,7 +271,9 @@ it 'deactivates profile if not de-cryptable' do user = create(:user, :signed_up) profile = create(:profile, :active, :verified, user: user, pii: { ssn: '1234' }) - profile.update!(encrypted_pii: Base64.strict_encode64('nonsense')) + profile.update!( + encrypted_pii: { encrypted_data: Base64.strict_encode64('nonsense') }.to_json + ) stub_analytics analytics_hash = { diff --git a/spec/services/encryption/encryptors/pii_encryptor_spec.rb b/spec/services/encryption/encryptors/pii_encryptor_spec.rb index 033b8be58a8..6caa35066f6 100644 --- a/spec/services/encryption/encryptors/pii_encryptor_spec.rb +++ b/spec/services/encryption/encryptors/pii_encryptor_spec.rb @@ -2,10 +2,9 @@ describe Encryption::Encryptors::PiiEncryptor do let(:password) { 'password' } - let(:salt) { 'n-pepa' } let(:plaintext) { 'Oooh baby baby' } - subject { described_class.new(password: password, salt: salt) } + subject { described_class.new(password) } describe '#encrypt' do it 'returns encrypted text' do @@ -15,6 +14,9 @@ end it 'uses the user access key encryptor to encrypt the plaintext' do + salt = '0' * 20 + allow(Devise).to receive(:friendly_token).and_return(salt) + scrypt_digest = '1' * 64 scrypt_password = instance_double(SCrypt::Password) @@ -37,7 +39,11 @@ ciphertext = subject.encrypt(plaintext) - expect(ciphertext).to eq(expected_ciphertext) + expect(ciphertext).to eq({ + encrypted_data: expected_ciphertext, + salt: salt, + cost: '800$8$1$', + }.to_json) end end @@ -51,12 +57,15 @@ it 'requires the same password used for encrypt' do ciphertext = subject.encrypt(plaintext) - new_encryptor = described_class.new(password: 'This is not the passowrd', salt: salt) + new_encryptor = described_class.new('This is not the passowrd') expect { new_encryptor.decrypt(ciphertext) }.to raise_error Pii::EncryptionError end it 'uses layered AES and KMS to decrypt the contents' do + salt = '0' * 20 + allow(Devise).to receive(:friendly_token).and_return(salt) + scrypt_digest = '1' * 64 scrypt_password = instance_double(SCrypt::Password) @@ -75,7 +84,11 @@ with('aes_ciphertext', scrypt_digest[0...32]). and_return(plaintext) - result = subject.decrypt(Base64.strict_encode64('kms_ciphertext')) + result = subject.decrypt({ + encrypted_data: Base64.strict_encode64('kms_ciphertext'), + salt: salt, + cost: '800$8$1$', + }.to_json) expect(result).to eq(plaintext) end diff --git a/spec/services/pii/attributes_spec.rb b/spec/services/pii/attributes_spec.rb index 3a0e0e78850..2bc19b365fb 100644 --- a/spec/services/pii/attributes_spec.rb +++ b/spec/services/pii/attributes_spec.rb @@ -3,8 +3,6 @@ describe Pii::Attributes do # let(:user_access_key) { Encryption::UserAccessKey.new(password: 'sekrit', salt: SecureRandom.uuid) } let(:password) { 'I am the password' } - let(:salt) { 'I am the salt' } - let(:cost) { '800$8$1$' } describe '#new_from_hash' do it 'initializes from plain Hash' do @@ -34,20 +32,16 @@ describe '#new_from_encrypted' do it 'inflates from encrypted string' do orig_attrs = described_class.new_from_hash(first_name: 'Jane') - encrypted_pii = orig_attrs.encrypted(password: password, salt: salt, cost: cost) - pii_attrs = described_class.new_from_encrypted( - encrypted_pii, password: password, salt: salt, cost: cost - ) + encrypted_pii = orig_attrs.encrypted(password) + pii_attrs = described_class.new_from_encrypted(encrypted_pii, password: password) expect(pii_attrs.first_name).to eq 'Jane' end it 'allows deprecated attributes that are no longer added to the hash schema' do deprecated_atts = described_class.new_from_hash(otp: '123abc') - encrypted_pii = deprecated_atts.encrypted(password: password, salt: salt, cost: cost) - pii_attrs = described_class.new_from_encrypted( - encrypted_pii, password: password, salt: salt, cost: cost - ) + encrypted_pii = deprecated_atts.encrypted(password) + pii_attrs = described_class.new_from_encrypted(encrypted_pii, password: password) expect(pii_attrs[:otp]).to eq('123abc') end @@ -71,7 +65,7 @@ it 'returns the object as encrypted string' do pii_attrs = described_class.new_from_hash(first_name: 'Jane') - encrypted = pii_attrs.encrypted(password: password, salt: salt, cost: cost) + encrypted = pii_attrs.encrypted(password) expect(encrypted).to_not match 'Jane' end end