Merge pull request #50 from FundingCircle/fix-batch-encryption-for-0.7

Handle blank values in batch encryption and decryption
FundingCircle · Dec 10, 2018 · bfbe76a · bfbe76a
2 parents b9c1aed + 662b0c5
commit bfbe76a
Show file tree

Hide file tree

Showing 3 changed files with 146 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Vault Rails Changelog
 
+## Unreleased
+
+BUG FIXES
+- Allow blank values like `nil` and empty string as input to batch encryption and decryption (forward ported from 0.6.5)
+- Handle the case when plaintexts/ciphertexts parameter of #vault_batch_encrypt/#vault_batch_decrypt is an array with only blank values (forward ported from 0.6.7)
+
 ## 0.7.2 (December 3, 2018)
 
 NEW FEATURES

diff --git a/lib/vault/rails.rb b/lib/vault/rails.rb
@@ -260,14 +260,18 @@ def vault_encrypt(path, key, plaintext, client, convergent)
       def vault_batch_encrypt(path, key, plaintexts, client)
         return [] if plaintexts.empty?
 
+        # Only present values can be encrypted by Vault. Empty values should be returned as they are.
+        non_empty_plaintexts = plaintexts.select { |plaintext| plaintext.present? }
+        return plaintexts if non_empty_plaintexts.empty? # nothing to encrypt
+
         route = File.join(path, 'encrypt', key)
 
         options = {
           convergent_encryption: true,
           derived: true
         }
 
-        batch_input = plaintexts.map do |plaintext|
+        batch_input = non_empty_plaintexts.map do |plaintext|
           {
             context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
             plaintext: Base64.strict_encode64(plaintext)
@@ -277,7 +281,11 @@ def vault_batch_encrypt(path, key, plaintexts, client)
         options.merge!(batch_input: batch_input)
 
         secret = client.logical.write(route, options)
-        secret.data[:batch_results].map { |result| result[:ciphertext] }
+        vault_results = secret.data[:batch_results].map { |result| result[:ciphertext] }
+
+        plaintexts.map do |plaintext|
+          plaintext.present? ? vault_results.shift : plaintext
+        end
       end
 
       # Perform decryption using Vault. This will raise exceptions if Vault is
@@ -302,20 +310,26 @@ def vault_decrypt(path, key, ciphertext, client, convergent)
       def vault_batch_decrypt(path, key, ciphertexts, client)
         return [] if ciphertexts.empty?
 
-        route = File.join(path, 'decrypt', key)
+        # Only present values can be decrypted by Vault. Empty values should be returned as they are.
+        non_empty_ciphertexts = ciphertexts.select { |ciphertext| ciphertext.present? }
+        return ciphertexts if non_empty_ciphertexts.empty?
 
+        route = File.join(path, 'decrypt', key)
 
-        batch_input = ciphertexts.map do |ciphertext|
+        batch_input = non_empty_ciphertexts.map do |ciphertext|
           {
             context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
             ciphertext: ciphertext
           }
         end
-
         options = { batch_input: batch_input }
 
         secret = client.logical.write(route, options)
-        secret.data[:batch_results].map { |result| Base64.strict_decode64(result[:plaintext]) }
+        vault_results = secret.data[:batch_results].map { |result| Base64.strict_decode64(result[:plaintext]) }
+
+        ciphertexts.map do |ciphertext|
+          ciphertext.present? ? vault_results.shift : ciphertext
+        end
       end
 
       # The symmetric key for the given params.
@@ -328,7 +342,7 @@ def memory_key_for(path, key)
       # newly encoded string.
       # @return [String]
       def force_encoding(str)
-        str.force_encoding(Vault::Rails.encoding).encode(Vault::Rails.encoding)
+        str.blank? ? str : str.force_encoding(Vault::Rails.encoding).encode(Vault::Rails.encoding)
       end
 
       private

diff --git a/spec/unit/rails_spec.rb b/spec/unit/rails_spec.rb
@@ -156,6 +156,65 @@
 
       expect(Vault::Rails.batch_encrypt('path', 'key', ['plaintext1', 'plaintext2'], Vault::Rails.client)).to eq(%w(ciphertext1 ciphertext2))
     end
+
+    context 'with only blank values' do
+      it 'does not make any calls to Vault and just return the plaintexts' do
+        expect(Vault::Rails.client.logical).not_to receive(:write)
+
+        plaintexts = ['', '', nil, '', nil, nil]
+        expect(Vault::Rails.batch_encrypt('path', 'key', plaintexts, Vault::Rails.client)).to eq(plaintexts)
+      end
+    end
+
+    context 'with presented blank values' do
+      it 'sends the correct parameters to vault client' do
+        expected_route = 'path/encrypt/key'
+        expected_options = {
+          batch_input: [
+            {
+              plaintext: Base64.strict_encode64('plaintext1'),
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+            {
+              plaintext: Base64.strict_encode64('plaintext2'),
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+          ],
+          convergent_encryption: true,
+          derived: true
+        }
+
+        expect(Vault::Rails.client.logical).to receive(:write)
+          .with(expected_route, expected_options)
+          .and_return(spy('Vault::Secret'))
+
+        Vault::Rails.batch_encrypt('path', 'key', ['plaintext1', '', 'plaintext2', '', nil, nil], Vault::Rails.client)
+      end
+
+      it 'parses the response from vault client correctly and keeps the order of records' do
+        expected_route = 'path/encrypt/key'
+        expected_options = {
+          batch_input: [
+            {
+              plaintext: Base64.strict_encode64('plaintext1'),
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+            {
+              plaintext: Base64.strict_encode64('plaintext2'),
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+          ],
+          convergent_encryption: true,
+          derived: true
+        }
+
+        allow(Vault::Rails.client.logical).to receive(:write)
+          .with(expected_route, expected_options)
+          .and_return(instance_double('Vault::Secret', data: {:batch_results=>[{:ciphertext=>'ciphertext1'}, {:ciphertext=>'ciphertext2'}]}))
+
+        expect(Vault::Rails.batch_encrypt('path', 'key', ['plaintext1', '', 'plaintext2', '', nil], Vault::Rails.client)).to eq(['ciphertext1', '', 'ciphertext2', '', nil])
+      end
+    end
   end
 
   describe '.batch_decrypt' do
@@ -207,5 +266,65 @@
 
       expect(Vault::Rails.batch_decrypt('path', 'key', ['ciphertext1', 'ciphertext2'], Vault::Rails.client)).to eq( %w(plaintext1 plaintext2)) # in that order
     end
+
+    context 'with only blank values' do
+      it 'does not make any calls to Vault and just return the ciphertexts' do
+        expect(Vault::Rails.client.logical).not_to receive(:write)
+
+        ciphertexts = ['', '', nil, '', nil, nil]
+
+        expect(Vault::Rails.batch_decrypt('path', 'key', ciphertexts, Vault::Rails.client)).to eq(ciphertexts)
+      end
+    end
+
+    context 'with presented blank values' do
+      it 'sends the correct parameters to vault client' do
+        expected_route = 'path/decrypt/key'
+        expected_options = {
+          batch_input: [
+            {
+              ciphertext: 'ciphertext1',
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+            {
+              ciphertext: 'ciphertext2',
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+          ],
+        }
+
+        expect(Vault::Rails.client.logical).to receive(:write)
+          .with(expected_route, expected_options)
+          .and_return(spy('Vault::Secret'))
+
+        Vault::Rails.batch_decrypt('path', 'key', ['ciphertext1', '', 'ciphertext2', nil, '', ''], Vault::Rails.client)
+      end
+
+      it 'parses the response from vault client correctly and keeps the order of records' do
+        expected_route = 'path/decrypt/key'
+        expected_options = {
+          batch_input: [
+            {
+              ciphertext: 'ciphertext1',
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+            {
+              ciphertext: 'ciphertext2',
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+            {
+              ciphertext: 'ciphertext3',
+              context: Base64.strict_encode64(Vault::Rails.convergent_encryption_context),
+            },
+          ],
+        }
+
+        allow(Vault::Rails.client.logical).to receive(:write)
+          .with(expected_route, expected_options)
+          .and_return(instance_double('Vault::Secret', data: {batch_results: [{plaintext: 'cGxhaW50ZXh0MQ=='}, {plaintext:'cGxhaW50ZXh0Mg=='}, {plaintext: 'cGxhaW50ZXh0Mw=='}]}))
+
+        expect(Vault::Rails.batch_decrypt('path', 'key', ['ciphertext1', '', nil, 'ciphertext2', '', 'ciphertext3'], Vault::Rails.client)).to eq( ['plaintext1', '', nil, 'plaintext2', '', 'plaintext3']) # in that order
+      end
+    end
   end
 end