Merge pull request #145 from jmazzi/bugfix/decrypt-plaintext

Fixes serializer issue which calls decrypt with the plaintext value

lib/crypt_keeper.rb

@@ -6,6 +6,7 @@
 require 'crypt_keeper/provider/base'
 require 'crypt_keeper/provider/aes_new'
 require 'crypt_keeper/provider/mysql_aes_new'
+require 'crypt_keeper/provider/postgres_base'
 require 'crypt_keeper/provider/postgres_pgp'
 require 'crypt_keeper/provider/postgres_pgp_public_key'
 

lib/crypt_keeper/helper.rb

@@ -3,10 +3,38 @@ module Helper
     module SQL
       private
 
-      # Private: Sanitize an sql query and then execute it
-      def escape_and_execute_sql(query)
+      # Private: Sanitize an sql query and then execute it.
+      #
+      # query - the sql query
+      # new_transaction - if the query should run inside a new transaction
+      #
+      # Returns the ActiveRecord response.
+      def escape_and_execute_sql(query, new_transaction: false)
         query = ::ActiveRecord::Base.send :sanitize_sql_array, query
-        ::ActiveRecord::Base.connection.execute(query).first
+
+        if CryptKeeper.silence_logs?
+          ::ActiveRecord::Base.logger.silence do
+            execute_sql(query, new_transaction: new_transaction)
+          end
+        else
+          execute_sql(query, new_transaction: new_transaction)
+        end
+      end
+
+      # Private: Executes the query.
+      #
+      # query - the sql query
+      # new_transaction - if the query should run inside a new transaction
+      #
+      # Returns an Array.
+      def execute_sql(query, new_transaction: false)
+        if new_transaction
+          ::ActiveRecord::Base.transaction(requires_new: true) do
+            ::ActiveRecord::Base.connection.execute(query).first
+          end
+        else
+          ::ActiveRecord::Base.connection.execute(query).first
+        end
       end
     end
 

lib/crypt_keeper/log_subscriber/mysql_aes.rb

@@ -14,8 +14,6 @@ def sql(event)
         payload = event.payload[:sql]
           .encode('UTF-8', 'binary', invalid: :replace, undef: :replace, replace: '')
 
-        return if CryptKeeper.silence_logs? && payload =~ filter
-
         event.payload[:sql] = payload.gsub(filter) do |_|
           "#{$1}([FILTERED])"
         end

lib/crypt_keeper/log_subscriber/postgres_pgp.rb

@@ -4,7 +4,7 @@
 module CryptKeeper
   module LogSubscriber
     module PostgresPgp
-      FILTER = /(\(*)pgp_(sym|pub)_(?<operation>decrypt|encrypt)(\(+.*\)+)/im
+      FILTER = /(\(*)(?<operation>pgp_sym_encrypt|pgp_sym_decrypt|pgp_pub_encrypt|pgp_pub_decrypt|pgp_key_id)(\(+.*\)+)/im
 
       # Public: Prevents sensitive data from being logged
       #
@@ -13,9 +13,6 @@ module PostgresPgp
       # Returns a boolean.
       def sql(event)
         payload = crypt_keeper_payload_parse(event.payload[:sql])
-
-        return if CryptKeeper.silence_logs? && payload =~ FILTER
-
         event.payload[:sql] = crypt_keeper_filter_postgres_log(payload)
         super(event)
       end

lib/crypt_keeper/provider/postgres_base.rb

@@ -0,0 +1,28 @@
+require 'crypt_keeper/log_subscriber/postgres_pgp'
+
+module CryptKeeper
+  module Provider
+    class PostgresBase < Base
+      include CryptKeeper::Helper::SQL
+      include CryptKeeper::LogSubscriber::PostgresPgp
+
+      INVALID_DATA_ERROR = "Wrong key or corrupt data".freeze
+
+      # Public: Checks if value is already encrypted.
+      #
+      # Returns boolean
+      def encrypted?(value)
+        begin
+          escape_and_execute_sql(["SELECT pgp_key_id(?)", value.to_s],
+            new_transaction: true)['pgp_key_id'].present?
+        rescue ActiveRecord::StatementInvalid => e
+          if e.message.include?(INVALID_DATA_ERROR)
+            false
+          else
+            raise
+          end
+        end
+      end
+    end
+  end
+end

lib/crypt_keeper/provider/postgres_pgp.rb

@@ -1,11 +1,6 @@
-require 'crypt_keeper/log_subscriber/postgres_pgp'
-
 module CryptKeeper
   module Provider
-    class PostgresPgp < Base
-      include CryptKeeper::Helper::SQL
-      include CryptKeeper::LogSubscriber::PostgresPgp
-
+    class PostgresPgp < PostgresBase
       attr_accessor :key
       attr_accessor :pgcrypto_options
 
@@ -37,8 +32,12 @@ def encrypt(value)
       # Returns a plaintext string
       def decrypt(value)
         rescue_invalid_statement do
-          escape_and_execute_sql(["SELECT pgp_sym_decrypt(?, ?)",
-            value, key])['pgp_sym_decrypt']
+          if encrypted?(value)
+            escape_and_execute_sql(["SELECT pgp_sym_decrypt(?, ?)",
+              value, key])['pgp_sym_decrypt']
+          else
+            value
+          end
         end
       end
 

lib/crypt_keeper/provider/postgres_pgp_public_key.rb

@@ -1,10 +1,6 @@
-require 'crypt_keeper/log_subscriber/postgres_pgp'
-
 module CryptKeeper
   module Provider
-    class PostgresPgpPublicKey < Base
-      include CryptKeeper::Helper::SQL
-
+    class PostgresPgpPublicKey < PostgresBase
       attr_accessor :key
 
       def initialize(options = {})
@@ -33,25 +29,13 @@ def encrypt(value)
       #
       # Returns a plaintext string
       def decrypt(value)
-        if @private_key.present?
-          escape_and_execute_sql(["SELECT pgp_pub_decrypt(?, dearmor(?), ?)", value, @private_key, @key])['pgp_pub_decrypt']
+        if @private_key.present? && encrypted?(value)
+          escape_and_execute_sql(["SELECT pgp_pub_decrypt(?, dearmor(?), ?)",
+            value, @private_key, @key])['pgp_pub_decrypt']
         else
           value
         end
       end
-
-      # Public: Attempts to extract a PGP key id. If it's successful, it returns true
-      #
-      # Returns boolean
-      def encrypted?(value)
-        begin
-          ActiveRecord::Base.transaction(requires_new: true) do
-            escape_and_execute_sql(["SELECT pgp_key_id(?)", value.to_s])['pgp_key_id'].present?
-          end
-        rescue ActiveRecord::StatementInvalid
-          false
-        end
-      end
     end
   end
 end

lib/crypt_keeper/version.rb

@@ -1,3 +1,3 @@
 module CryptKeeper
-  VERSION = "1.1.0"
+  VERSION = "1.1.1"
 end

spec/crypt_keeper/log_subscriber/mysql_aes_spec.rb

@@ -46,11 +46,5 @@
 
       should_log_scrubbed_query(input: input_query, output: output_query)
     end
-
-    it "skips logging if CryptKeeper.silence_logs is set" do
-      CryptKeeper.silence_logs = true
-
-      should_not_log_query(input_query)
-    end
   end
 end

spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb

@@ -13,44 +13,32 @@
       CryptKeeper::Provider::PostgresPgp.new key: 'secret'
     end
 
-    let(:input_query) do
-      "SELECT pgp_sym_encrypt('encrypt_value', 'encrypt_key'), pgp_sym_decrypt('decrypt_value', 'decrypt_key') FROM DUAL;"
-    end
-
-    let(:output_query) do
-      "SELECT encrypt([FILTERED]) FROM DUAL;"
-    end
-
-    let(:input_search_query) do
-      "SELECT \"sensitive_data\".* FROM \"sensitive_data\" WHERE ((pgp_sym_decrypt('f'), 'tool') = 'blah')) AND secret = 'testing'"
-    end
-
-    let(:output_search_query) do
-      "SELECT \"sensitive_data\".* FROM \"sensitive_data\" WHERE decrypt([FILTERED]) AND secret = 'testing'"
-    end
+    let(:queries) {
+      {
+        "SELECT pgp_sym_encrypt('encrypt_value', 'encrypt_key') FROM DUAL;" => "SELECT pgp_sym_encrypt([FILTERED]) FROM DUAL;",
+        "SELECT pgp_sym_decrypt('encrypt_value') FROM DUAL;" => "SELECT pgp_sym_decrypt([FILTERED]) FROM DUAL;",
+        "SELECT pgp_key_id('encrypt_value') FROM DUAL;" => "SELECT pgp_key_id([FILTERED]) FROM DUAL;",
+        "SELECT \"sensitive_data\".* FROM \"sensitive_data\" WHERE ((pgp_sym_decrypt('f'), 'tool') = 'blah')) AND secret = 'testing'" => "SELECT \"sensitive_data\".* FROM \"sensitive_data\" WHERE pgp_sym_decrypt([FILTERED]) AND secret = 'testing'",
+      }
+    }
 
     it "filters pgp functions" do
-      should_log_scrubbed_query(input: input_query, output: output_query)
+      queries.each do |k, v|
+        should_log_scrubbed_query(input: k, output: v)
+      end
     end
 
     it "filters pgp functions in lowercase" do
-      should_log_scrubbed_query(input: input_query.downcase, output: output_query.downcase.gsub(/filtered/, 'FILTERED'))
-    end
-
-    it "filters pgp functions when searching" do
-      should_log_scrubbed_query(input: input_search_query, output: output_search_query)
+      queries.each do |k, v|
+        should_log_scrubbed_query(input: k.downcase, output: v.downcase.gsub(/filtered/, 'FILTERED'))
+      end
     end
 
     it "forces string encodings" do
-      input_query = "SELECT pgp_sym_encrypt('hi \255', 'test') FROM DUAL;"
-
-      should_log_scrubbed_query(input: input_query, output: output_query)
-    end
-
-    it "skips logging if CryptKeeper.silence_logs is set" do
-      CryptKeeper.silence_logs = true
-
-      should_not_log_query(input_query)
+      queries.each do |k, v|
+        k = "#{k}\255"
+        should_log_scrubbed_query(input: k, output: v)
+      end
     end
   end
 
@@ -68,27 +56,23 @@
       CryptKeeper::Provider::PostgresPgpPublicKey.new key: 'secret', public_key: public_key, private_key: private_key
     end
 
-    let(:input_query) do
-      "SELECT pgp_pub_encrypt('test', dearmor('#{public_key}
-      '))"
-    end
-
-    let(:output_query) do
-      "SELECT encrypt([FILTERED])"
-    end
+    let(:queries) {
+      {
+        "SELECT pgp_pub_encrypt('test', dearmor('#{public_key}')) FROM DUAL;" => "SELECT pgp_pub_encrypt([FILTERED]) FROM DUAL;",
+        "SELECT pgp_pub_decrypt('test', dearmor('#{public_key}'), '#{private_key}') FROM DUAL;" => "SELECT pgp_pub_decrypt([FILTERED]) FROM DUAL;",
+      }
+    }
 
     it "filters pgp functions" do
-      should_log_scrubbed_query(input: input_query, output: output_query)
+      queries.each do |k, v|
+        should_log_scrubbed_query(input: k, output: v)
+      end
     end
 
     it "filters pgp functions in lowercase" do
-      should_log_scrubbed_query(input: input_query.downcase, output: output_query.downcase.gsub(/filtered/, 'FILTERED'))
-    end
-
-    it "skips logging if CryptKeeper.silence_logs is set" do
-      CryptKeeper.silence_logs = true
-
-      should_not_log_query(input_query)
+      queries.each do |k, v|
+        should_log_scrubbed_query(input: k.downcase, output: v.downcase.gsub(/filtered/, 'FILTERED'))
+      end
     end
   end
 end

spec/crypt_keeper/provider/postgres_pgp_public_key_spec.rb

@@ -47,6 +47,7 @@
   describe "#decrypt" do
     specify { expect(subject.decrypt(cipher_text)).to eq(plain_text) }
     specify { expect(subject.decrypt(integer_cipher_text)).to eq(integer_plain_text.to_s) }
+    specify { expect(subject.decrypt(plain_text)).to eq(plain_text) }
 
     it "does not decrypt w/o private key" do
       pgp = described_class.new key: ENCRYPTION_PASSWORD, public_key: public_key

spec/crypt_keeper/provider/postgres_pgp_spec.rb

@@ -44,6 +44,7 @@
   describe "#decrypt" do
     specify { expect(subject.decrypt(cipher_text)).to eq(plain_text) }
     specify { expect(subject.decrypt(integer_cipher_text)).to eq(integer_plain_text.to_s) }
+    specify { expect(subject.decrypt(plain_text)).to eq(plain_text) }
 
     it "filters StatementInvalid errors" do
       begin

spec/support/logging.rb

@@ -47,11 +47,8 @@ def debug(message)
       def should_log_scrubbed_query(input:, output:)
         queries = sql(input)
 
-        valid_input = queries.none? { |line| line.include? input }
-        expect(valid_input).to eq(true), "found unscrubbed SQL query logged!"
-
-        valid_output = queries.any? { |line| line.include? output }
-        expect(valid_output).to eq(true), "output query was not logged!"
+        expect(queries.join("\n")).to_not include(input)
+        expect(queries.join("\n")).to include(output)
       end
 
       # Public: Verifies that the given input query was not logged.