From 3c62f522739a13af919f0f745d3d09a12eee30c7 Mon Sep 17 00:00:00 2001 From: Fabio Kreusch Date: Wed, 31 May 2017 16:23:22 -0300 Subject: [PATCH 1/6] Fixes serializer issue which calls decrypt with the plaintext value Sometimes, the Rails serializer will call the decryptor again on a value that was already decrypted, which will raise. This fixes that by first making sure the value to decrypt is in fact an encrypted value. If it is not, just return it. --- lib/crypt_keeper/provider/postgres_pgp.rb | 9 +++++++++ lib/crypt_keeper/provider/postgres_pgp_public_key.rb | 9 +++++++++ .../provider/postgres_pgp_public_key_spec.rb | 1 + spec/crypt_keeper/provider/postgres_pgp_spec.rb | 1 + 4 files changed, 20 insertions(+) diff --git a/lib/crypt_keeper/provider/postgres_pgp.rb b/lib/crypt_keeper/provider/postgres_pgp.rb index c994aba..0b732e1 100644 --- a/lib/crypt_keeper/provider/postgres_pgp.rb +++ b/lib/crypt_keeper/provider/postgres_pgp.rb @@ -37,6 +37,15 @@ def encrypt(value) # Returns a plaintext string def decrypt(value) rescue_invalid_statement do + begin + # check if the value is an encrypted value by querying its key id + escape_and_execute_sql(["SELECT pgp_key_id(?)", value]) + rescue ActiveRecord::StatementInvalid + # 'Wrong key or corrupt data', means the value is not an encrypted + # value, so just the plaintext value + return value + end + escape_and_execute_sql(["SELECT pgp_sym_decrypt(?, ?)", value, key])['pgp_sym_decrypt'] end diff --git a/lib/crypt_keeper/provider/postgres_pgp_public_key.rb b/lib/crypt_keeper/provider/postgres_pgp_public_key.rb index ce0d5d8..a0669a2 100644 --- a/lib/crypt_keeper/provider/postgres_pgp_public_key.rb +++ b/lib/crypt_keeper/provider/postgres_pgp_public_key.rb @@ -34,6 +34,15 @@ def encrypt(value) # Returns a plaintext string def decrypt(value) if @private_key.present? + begin + # check if the value is an encrypted value by querying its key id + escape_and_execute_sql(["SELECT pgp_key_id(?)", value]) + rescue ActiveRecord::StatementInvalid + # 'Wrong key or corrupt data', means the value is not an encrypted + # value, so just the plaintext value + return value + end + escape_and_execute_sql(["SELECT pgp_pub_decrypt(?, dearmor(?), ?)", value, @private_key, @key])['pgp_pub_decrypt'] else value diff --git a/spec/crypt_keeper/provider/postgres_pgp_public_key_spec.rb b/spec/crypt_keeper/provider/postgres_pgp_public_key_spec.rb index 63f7313..f971e7e 100644 --- a/spec/crypt_keeper/provider/postgres_pgp_public_key_spec.rb +++ b/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 diff --git a/spec/crypt_keeper/provider/postgres_pgp_spec.rb b/spec/crypt_keeper/provider/postgres_pgp_spec.rb index e763814..fc0cf33 100644 --- a/spec/crypt_keeper/provider/postgres_pgp_spec.rb +++ b/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 From 9fd9521dccbbe89f7afccc13a96419452a470812 Mon Sep 17 00:00:00 2001 From: Fabio Kreusch Date: Tue, 13 Jun 2017 16:15:01 -0300 Subject: [PATCH 2/6] Refactors encrypted? --- lib/crypt_keeper.rb | 1 + lib/crypt_keeper/provider/postgres_base.rb | 23 +++++++++++++ lib/crypt_keeper/provider/postgres_pgp.rb | 22 ++++--------- .../provider/postgres_pgp_public_key.rb | 33 +++---------------- 4 files changed, 34 insertions(+), 45 deletions(-) create mode 100644 lib/crypt_keeper/provider/postgres_base.rb diff --git a/lib/crypt_keeper.rb b/lib/crypt_keeper.rb index bdca203..7306f66 100644 --- a/lib/crypt_keeper.rb +++ b/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' diff --git a/lib/crypt_keeper/provider/postgres_base.rb b/lib/crypt_keeper/provider/postgres_base.rb new file mode 100644 index 0000000..e8f4a38 --- /dev/null +++ b/lib/crypt_keeper/provider/postgres_base.rb @@ -0,0 +1,23 @@ +require 'crypt_keeper/log_subscriber/postgres_pgp' + +module CryptKeeper + module Provider + class PostgresBase < Base + include CryptKeeper::Helper::SQL + include CryptKeeper::LogSubscriber::PostgresPgp + + # Public: Checks if value is already encrypted. + # + # 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 diff --git a/lib/crypt_keeper/provider/postgres_pgp.rb b/lib/crypt_keeper/provider/postgres_pgp.rb index 0b732e1..a653817 100644 --- a/lib/crypt_keeper/provider/postgres_pgp.rb +++ b/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,17 +32,12 @@ def encrypt(value) # Returns a plaintext string def decrypt(value) rescue_invalid_statement do - begin - # check if the value is an encrypted value by querying its key id - escape_and_execute_sql(["SELECT pgp_key_id(?)", value]) - rescue ActiveRecord::StatementInvalid - # 'Wrong key or corrupt data', means the value is not an encrypted - # value, so just the plaintext value - return value + if encrypted?(value) + escape_and_execute_sql(["SELECT pgp_sym_decrypt(?, ?)", + value, key])['pgp_sym_decrypt'] + else + value end - - escape_and_execute_sql(["SELECT pgp_sym_decrypt(?, ?)", - value, key])['pgp_sym_decrypt'] end end diff --git a/lib/crypt_keeper/provider/postgres_pgp_public_key.rb b/lib/crypt_keeper/provider/postgres_pgp_public_key.rb index a0669a2..a2e5f6c 100644 --- a/lib/crypt_keeper/provider/postgres_pgp_public_key.rb +++ b/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,34 +29,13 @@ def encrypt(value) # # Returns a plaintext string def decrypt(value) - if @private_key.present? - begin - # check if the value is an encrypted value by querying its key id - escape_and_execute_sql(["SELECT pgp_key_id(?)", value]) - rescue ActiveRecord::StatementInvalid - # 'Wrong key or corrupt data', means the value is not an encrypted - # value, so just the plaintext value - return value - end - - 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 From 9a32d26ba20859b0627cbf120f235f9bf982ab18 Mon Sep 17 00:00:00 2001 From: Fabio Kreusch Date: Tue, 13 Jun 2017 16:38:13 -0300 Subject: [PATCH 3/6] Checks for the specific exception --- lib/crypt_keeper/provider/postgres_base.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/crypt_keeper/provider/postgres_base.rb b/lib/crypt_keeper/provider/postgres_base.rb index e8f4a38..90cfef9 100644 --- a/lib/crypt_keeper/provider/postgres_base.rb +++ b/lib/crypt_keeper/provider/postgres_base.rb @@ -6,6 +6,8 @@ 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 @@ -14,8 +16,12 @@ def encrypted?(value) 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 + rescue ActiveRecord::StatementInvalid => e + if e.message.include?(INVALID_DATA_ERROR) + false + else + raise + end end end end From 684e5a8b4879d844856b93a7ec833859b41b4774 Mon Sep 17 00:00:00 2001 From: Fabio Kreusch Date: Wed, 14 Jun 2017 10:33:50 -0300 Subject: [PATCH 4/6] Refactor log filter to include pgp_key_id --- .../log_subscriber/postgres_pgp.rb | 2 +- .../log_subscriber/postgres_pgp_spec.rb | 72 +++++++++---------- spec/support/logging.rb | 7 +- 3 files changed, 39 insertions(+), 42 deletions(-) diff --git a/lib/crypt_keeper/log_subscriber/postgres_pgp.rb b/lib/crypt_keeper/log_subscriber/postgres_pgp.rb index dbd65aa..ffeb254 100644 --- a/lib/crypt_keeper/log_subscriber/postgres_pgp.rb +++ b/lib/crypt_keeper/log_subscriber/postgres_pgp.rb @@ -4,7 +4,7 @@ module CryptKeeper module LogSubscriber module PostgresPgp - FILTER = /(\(*)pgp_(sym|pub)_(?decrypt|encrypt)(\(+.*\)+)/im + FILTER = /(\(*)(?pgp_sym_encrypt|pgp_sym_decrypt|pgp_pub_encrypt|pgp_pub_decrypt|pgp_key_id)(\(+.*\)+)/im # Public: Prevents sensitive data from being logged # diff --git a/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb b/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb index 0b22e7b..d73f74f 100644 --- a/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb +++ b/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb @@ -13,44 +13,40 @@ 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) + queries.each do |k, v| + k = "#{k}\255" + should_log_scrubbed_query(input: k, output: v) + end 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_not_log_query(k) + end end end @@ -68,27 +64,31 @@ 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')) + queries.each do |k, v| + should_log_scrubbed_query(input: k.downcase, output: v.downcase.gsub(/filtered/, 'FILTERED')) + end 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_not_log_query(k) + end end end end diff --git a/spec/support/logging.rb b/spec/support/logging.rb index c4bb5c5..d9431d6 100644 --- a/spec/support/logging.rb +++ b/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. From b6d574dc438ad8dab50a33b2b1ffcd99e5022bd2 Mon Sep 17 00:00:00 2001 From: Fabio Kreusch Date: Wed, 14 Jun 2017 14:41:04 -0300 Subject: [PATCH 5/6] Use ActiveRecord::Base.logger.silence to silence when using CryptKeeper.silence_logs? --- lib/crypt_keeper/helper.rb | 34 +++++++++++++++++-- lib/crypt_keeper/log_subscriber/mysql_aes.rb | 2 -- .../log_subscriber/postgres_pgp.rb | 3 -- lib/crypt_keeper/provider/postgres_base.rb | 5 ++- .../log_subscriber/mysql_aes_spec.rb | 6 ---- .../log_subscriber/postgres_pgp_spec.rb | 16 --------- 6 files changed, 33 insertions(+), 33 deletions(-) diff --git a/lib/crypt_keeper/helper.rb b/lib/crypt_keeper/helper.rb index b6d6d3c..8085b6a 100644 --- a/lib/crypt_keeper/helper.rb +++ b/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 diff --git a/lib/crypt_keeper/log_subscriber/mysql_aes.rb b/lib/crypt_keeper/log_subscriber/mysql_aes.rb index 9dd98cd..1039c01 100644 --- a/lib/crypt_keeper/log_subscriber/mysql_aes.rb +++ b/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 diff --git a/lib/crypt_keeper/log_subscriber/postgres_pgp.rb b/lib/crypt_keeper/log_subscriber/postgres_pgp.rb index ffeb254..ab24218 100644 --- a/lib/crypt_keeper/log_subscriber/postgres_pgp.rb +++ b/lib/crypt_keeper/log_subscriber/postgres_pgp.rb @@ -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 diff --git a/lib/crypt_keeper/provider/postgres_base.rb b/lib/crypt_keeper/provider/postgres_base.rb index 90cfef9..0a42dc1 100644 --- a/lib/crypt_keeper/provider/postgres_base.rb +++ b/lib/crypt_keeper/provider/postgres_base.rb @@ -13,9 +13,8 @@ class PostgresBase < Base # 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 + 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 diff --git a/spec/crypt_keeper/log_subscriber/mysql_aes_spec.rb b/spec/crypt_keeper/log_subscriber/mysql_aes_spec.rb index bc6a543..781f66c 100644 --- a/spec/crypt_keeper/log_subscriber/mysql_aes_spec.rb +++ b/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 diff --git a/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb b/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb index d73f74f..a6f7e23 100644 --- a/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb +++ b/spec/crypt_keeper/log_subscriber/postgres_pgp_spec.rb @@ -40,14 +40,6 @@ should_log_scrubbed_query(input: k, output: v) end end - - it "skips logging if CryptKeeper.silence_logs is set" do - CryptKeeper.silence_logs = true - - queries.each do |k, v| - should_not_log_query(k) - end - end end context "Public key encryption" do @@ -82,13 +74,5 @@ should_log_scrubbed_query(input: k.downcase, output: v.downcase.gsub(/filtered/, 'FILTERED')) end end - - it "skips logging if CryptKeeper.silence_logs is set" do - CryptKeeper.silence_logs = true - - queries.each do |k, v| - should_not_log_query(k) - end - end end end From 3a4d604edbf1bb42480c373cd53223d55d04b96a Mon Sep 17 00:00:00 2001 From: Fabio Kreusch Date: Thu, 22 Jun 2017 10:37:48 -0300 Subject: [PATCH 6/6] Version bump --- lib/crypt_keeper/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/crypt_keeper/version.rb b/lib/crypt_keeper/version.rb index aaaca39..648ec92 100644 --- a/lib/crypt_keeper/version.rb +++ b/lib/crypt_keeper/version.rb @@ -1,3 +1,3 @@ module CryptKeeper - VERSION = "1.1.0" + VERSION = "1.1.1" end