From ff7b07697092a91c082a5bbb2e9c4d9cca356d6d Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Tue, 30 Jan 2024 09:47:26 +0000 Subject: [PATCH 1/3] Support encrypting binary columns ActiveRecord Encryption doesn't prevent you from encrypting binary columns but it doesn't have proper support for it either. When the data is fed through encrypt/decrypt it is converted to a String. This means that the the encryption layer is not transparent to binary data - which should be passed as Type::Binary::Data. As a result the data is not properly escaped in the SQL queries or deserialized correctly after decryption. However it just happens to work fine for MySQL and SQLite because the MessageSerializer doesn't use any characters that need to be encoded. However if you try to use a custom serializer that does then it breaks. PostgreSQL on the other hand does not work - because the Bytea type is passed a String rather than a Type::Binary::Data to deserialize, it attempts to unescape the data and either mangles it or raises an error if it contains null bytes. The commit fixes the issue, by reserializing the data after encryption and decryption. For text data that's a no-op, but for binary data we'll convert it back to a Type::Binary::Data. --- activerecord/CHANGELOG.md | 9 +++++++++ .../encryption/encrypted_attribute_type.rb | 16 +++++++++++++--- .../cases/encryption/encryptable_record_test.rb | 12 ++++++++++++ activerecord/test/models/book_encrypted.rb | 6 ++++++ activerecord/test/schema/schema.rb | 1 + 5 files changed, 41 insertions(+), 3 deletions(-) diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md index 8b37f79f077b7..f4efdf4aae2f0 100644 --- a/activerecord/CHANGELOG.md +++ b/activerecord/CHANGELOG.md @@ -1,3 +1,12 @@ +* Add support for encrypting binary columns + + Ensure encryption and decryption pass `Type::Binary::Data` around for binary data. + + Previously encrypting binary columns with the `ActiveRecord::Encryption::MessageSerializer` + incidentally worked for MySQL and SQLite, but not PostgreSQL. + + *Donal McBreen* + * Deprecated `ENV["SCHEMA_CACHE"]` in favor of `schema_cache_path` in the database configuration. *Rafael Mendonça França* diff --git a/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb b/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb index a44d78dbdaccc..66db77ac71517 100644 --- a/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb +++ b/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb @@ -85,11 +85,13 @@ def decrypt(value) with_context do unless value.nil? if @default && @default == value - value + decrypted_value = value else - encryptor.decrypt(value, **decryption_options) + decrypted_value = encryptor.decrypt(value, **decryption_options) end end + + reserialize(decrypted_value) end rescue ActiveRecord::Encryption::Errors::Base => error if previous_types_without_clean_text.blank? @@ -131,7 +133,7 @@ def serialize_with_current(value) def encrypt(value) with_context do - encryptor.encrypt(value, **encryption_options) + reserialize(encryptor.encrypt(value, **encryption_options)) end end @@ -150,6 +152,14 @@ def decryption_options def clean_text_scheme @clean_text_scheme ||= ActiveRecord::Encryption::Scheme.new(downcase: downcase?, encryptor: ActiveRecord::Encryption::NullEncryptor.new) end + + def reserialize(value) + if cast_type.binary? + cast_type.serialize(value) + else + value + end + end end end end diff --git a/activerecord/test/cases/encryption/encryptable_record_test.rb b/activerecord/test/cases/encryption/encryptable_record_test.rb index 14180f6b47e17..e88d564107a4b 100644 --- a/activerecord/test/cases/encryption/encryptable_record_test.rb +++ b/activerecord/test/cases/encryption/encryptable_record_test.rb @@ -394,6 +394,18 @@ def name assert_predicate OtherEncryptedPost.type_for_attribute(:title).scheme.previous_schemes, :one? end + test "binary data can be encrypted" do + all_bytes = (0..255).map(&:chr).join + assert_equal all_bytes, EncryptedBookWithBinary.create!(logo: all_bytes).logo + end + + test "binary data can be encrypted uncompressed" do + low_bytes = (0..127).map(&:chr).join + high_bytes = (128..255).map(&:chr).join + assert_equal low_bytes, EncryptedBookWithBinary.create!(logo: low_bytes).logo + assert_equal high_bytes, EncryptedBookWithBinary.create!(logo: high_bytes).logo + end + private def build_derived_key_provider_with(hash_digest_class) ActiveRecord::Encryption.with_encryption_context(key_generator: ActiveRecord::Encryption::KeyGenerator.new(hash_digest_class: hash_digest_class)) do diff --git a/activerecord/test/models/book_encrypted.rb b/activerecord/test/models/book_encrypted.rb index 0c4971379551e..349d6e51796eb 100644 --- a/activerecord/test/models/book_encrypted.rb +++ b/activerecord/test/models/book_encrypted.rb @@ -43,3 +43,9 @@ class EncryptedBookWithUnencryptedDataOptedIn < ActiveRecord::Base validates :name, uniqueness: true encrypts :name, deterministic: true, support_unencrypted_data: true end + +class EncryptedBookWithBinary < ActiveRecord::Base + self.table_name = "encrypted_books" + + encrypts :logo +end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 7d9c2f831f032..8c44978076299 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -163,6 +163,7 @@ t.string :format t.column :name, :string, default: "" t.column :original_name, :string + t.column :logo, :binary t.datetime :created_at t.datetime :updated_at From 53ca39fe6ee33c59194e0ee7554dae0b79c5d368 Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Fri, 2 Feb 2024 13:02:22 +0000 Subject: [PATCH 2/3] Extract decrypt_as_text/encrypt_as_text --- .../encryption/encrypted_attribute_type.rb | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb b/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb index 66db77ac71517..2d843f09b015f 100644 --- a/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb +++ b/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb @@ -81,17 +81,15 @@ def previous_type? @previous_type end - def decrypt(value) + def decrypt_as_text(value) with_context do unless value.nil? if @default && @default == value - decrypted_value = value + value else - decrypted_value = encryptor.decrypt(value, **decryption_options) + encryptor.decrypt(value, **decryption_options) end end - - reserialize(decrypted_value) end rescue ActiveRecord::Encryption::Errors::Base => error if previous_types_without_clean_text.blank? @@ -101,6 +99,10 @@ def decrypt(value) end end + def decrypt(value) + reserialize decrypt_as_text(value) + end + def try_to_deserialize_with_previous_encrypted_types(value) previous_types.each.with_index do |type, index| break type.deserialize(value) @@ -131,12 +133,16 @@ def serialize_with_current(value) encrypt(casted_value.to_s) unless casted_value.nil? end - def encrypt(value) + def encrypt_as_text(value) with_context do - reserialize(encryptor.encrypt(value, **encryption_options)) + encryptor.encrypt(value, **encryption_options) end end + def encrypt(value) + reserialize encrypt_as_text(value) + end + def encryptor ActiveRecord::Encryption.encryptor end From 38b7df7aa2629e27c1afa7a0d2eb2c961077a1aa Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Thu, 15 Feb 2024 10:56:06 +0000 Subject: [PATCH 3/3] Handle serialized binary data in encrypted columns Calling `serialize` is not always possible, because the column type might not expect to be serializing a String, for example when declared as serialzed or store attribute. With binary data the encryptor was passed an `ActiveModel::Type::Binary::Data`` and returned a `String``. In order to remain transparent we need to turn the data back into a `ActiveModel::Type::Binary::Data` before passing it on. We'll also rename `serialize`` to `text_to_database_type` to be a bit more descriptive. --- .../encryption/encrypted_attribute_type.rb | 10 +++++----- .../test/cases/encryption/encryptable_record_test.rb | 7 +++++++ activerecord/test/models/book_encrypted.rb | 7 +++++++ 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb b/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb index 2d843f09b015f..8da9ac31dfc9b 100644 --- a/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb +++ b/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb @@ -100,7 +100,7 @@ def decrypt_as_text(value) end def decrypt(value) - reserialize decrypt_as_text(value) + text_to_database_type decrypt_as_text(value) end def try_to_deserialize_with_previous_encrypted_types(value) @@ -140,7 +140,7 @@ def encrypt_as_text(value) end def encrypt(value) - reserialize encrypt_as_text(value) + text_to_database_type encrypt_as_text(value) end def encryptor @@ -159,9 +159,9 @@ def clean_text_scheme @clean_text_scheme ||= ActiveRecord::Encryption::Scheme.new(downcase: downcase?, encryptor: ActiveRecord::Encryption::NullEncryptor.new) end - def reserialize(value) - if cast_type.binary? - cast_type.serialize(value) + def text_to_database_type(value) + if value && cast_type.binary? + ActiveModel::Type::Binary::Data.new(value) else value end diff --git a/activerecord/test/cases/encryption/encryptable_record_test.rb b/activerecord/test/cases/encryption/encryptable_record_test.rb index e88d564107a4b..18affa72110fd 100644 --- a/activerecord/test/cases/encryption/encryptable_record_test.rb +++ b/activerecord/test/cases/encryption/encryptable_record_test.rb @@ -397,6 +397,8 @@ def name test "binary data can be encrypted" do all_bytes = (0..255).map(&:chr).join assert_equal all_bytes, EncryptedBookWithBinary.create!(logo: all_bytes).logo + assert_nil EncryptedBookWithBinary.create!(logo: nil).logo + assert_equal "", EncryptedBookWithBinary.create!(logo: "").logo end test "binary data can be encrypted uncompressed" do @@ -406,6 +408,11 @@ def name assert_equal high_bytes, EncryptedBookWithBinary.create!(logo: high_bytes).logo end + test "serialized binary data can be encrypted" do + json_bytes = (32..127).map(&:chr) + assert_equal json_bytes, EncryptedBookWithSerializedBinary.create!(logo: json_bytes).logo + end + private def build_derived_key_provider_with(hash_digest_class) ActiveRecord::Encryption.with_encryption_context(key_generator: ActiveRecord::Encryption::KeyGenerator.new(hash_digest_class: hash_digest_class)) do diff --git a/activerecord/test/models/book_encrypted.rb b/activerecord/test/models/book_encrypted.rb index 349d6e51796eb..942e26b210d39 100644 --- a/activerecord/test/models/book_encrypted.rb +++ b/activerecord/test/models/book_encrypted.rb @@ -49,3 +49,10 @@ class EncryptedBookWithBinary < ActiveRecord::Base encrypts :logo end + +class EncryptedBookWithSerializedBinary < ActiveRecord::Base + self.table_name = "encrypted_books" + + serialize :logo, coder: JSON + encrypts :logo +end