From 2a4050f450861a319a2fd59b93e9943c41f42c4d Mon Sep 17 00:00:00 2001 From: Donal McBreen Date: Tue, 27 Aug 2024 10:02:52 +0100 Subject: [PATCH] Fix binary decryption on Postgres When encrypting attributes we need to do it just before inserting the data into the database, so after any other serialization steps, e.g. for serialized types or normalization. And we need things to happen in reverse order when decrypting. With attribute decoration we end up with initial type nested in other types. To ensure that the encryption happens in the right place, the EncryptedAttributeType first serializes the value with the type it is wrapping and then encrypts it. And in reverse it decrypts then deserializes with the wrapped type. There's an assumption here, which is that the wrapped type doesn't need to do anything in between the database and the encryption layer - so any database specific casting is skipped. This works fine for String columns as there's nothing for them to do. It also works for binary columns for MySQL and SQLite. But not for PostgreSQL which needs to receive the data as Binary::Data and has to call `PG::Connection.unescape_bytea` when deserializing the data. The serialization part was fixed in https://github.com/rails/rails/pull/50920, where the encryption output is wrapped in Binary::Data, which let's the PostgreSQL adapter know to convert the value [here](https://github.com/rails/rails/blob/5a0b2fa5a3be6ffd49fa7f1c3544c1da403c9cb5/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb#L83). That PR however didn't fix deserializing the data when it comes back out of the database (it wasn't round-tripping the data properly in the tests). We need to deserialize binary types before decrypting them - and we'll have to just assume that the wrapped type can do that for us. This won't work for serialized types as they'll also attempt to convert the data with the coder which needs to happen after decryption, so we need to special case them and extract the subtype instead. This isn't ideal but it should work ok for all built in types. But if you have a custom binary type that does something that needs to happen both before and after decryption then it may not work as expected. I think that's a rare case though and it's more important to have encryption of binary types working on PostgreSQL. --- .../encryption/encrypted_attribute_type.rb | 11 +++++- ...ble_record_message_pack_serialized_test.rb | 11 +++--- .../encryption/encryptable_record_test.rb | 24 +++++++++++-- activerecord/test/models/book_encrypted.rb | 34 ++++++++++++++++++- .../test/models/traffic_light_encrypted.rb | 8 +++++ 5 files changed, 79 insertions(+), 9 deletions(-) diff --git a/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb b/activerecord/lib/active_record/encryption/encrypted_attribute_type.rb index 10948ddea91b6..14c32410537e2 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) - text_to_database_type decrypt_as_text(value) + text_to_database_type decrypt_as_text(database_type_to_text(value)) end def try_to_deserialize_with_previous_encrypted_types(value) @@ -170,6 +170,15 @@ def text_to_database_type(value) value end end + + def database_type_to_text(value) + if value && cast_type.binary? + binary_cast_type = cast_type.serialized? ? cast_type.subtype : cast_type + binary_cast_type.deserialize(value) + else + value + end + end end end end diff --git a/activerecord/test/cases/encryption/encryptable_record_message_pack_serialized_test.rb b/activerecord/test/cases/encryption/encryptable_record_message_pack_serialized_test.rb index d84c8191b0117..0818690c20877 100644 --- a/activerecord/test/cases/encryption/encryptable_record_message_pack_serialized_test.rb +++ b/activerecord/test/cases/encryption/encryptable_record_message_pack_serialized_test.rb @@ -5,19 +5,22 @@ require "models/book_encrypted" require "active_record/encryption/message_pack_message_serializer" -class ActiveRecord::Encryption::EncryptableRecordTest < ActiveRecord::EncryptionTestCase +class ActiveRecord::Encryption::EncryptableRecordMessagePackSerializedTest < ActiveRecord::EncryptionTestCase fixtures :encrypted_books test "binary data can be serialized with message pack" do all_bytes = (0..255).map(&:chr).join - assert_equal all_bytes, EncryptedBookWithBinaryMessagePackSerialized.create!(logo: all_bytes).logo + book = EncryptedBookWithBinaryMessagePackSerialized.create!(logo: all_bytes) + assert_encrypted_attribute(book, :logo, all_bytes) end test "binary data can be encrypted uncompressed and serialized with message pack" do + # Strings below 140 bytes are not compressed low_bytes = (0..127).map(&:chr).join high_bytes = (128..255).map(&:chr).join - assert_equal low_bytes, EncryptedBookWithBinaryMessagePackSerialized.create!(logo: low_bytes).logo - assert_equal high_bytes, EncryptedBookWithBinaryMessagePackSerialized.create!(logo: high_bytes).logo + + assert_encrypted_attribute(EncryptedBookWithBinaryMessagePackSerialized.create!(logo: low_bytes), :logo, low_bytes) + assert_encrypted_attribute(EncryptedBookWithBinaryMessagePackSerialized.create!(logo: high_bytes), :logo, high_bytes) end test "text columns cannot be serialized with message pack" do diff --git a/activerecord/test/cases/encryption/encryptable_record_test.rb b/activerecord/test/cases/encryption/encryptable_record_test.rb index 74f7fb490631f..663d4e9eed538 100644 --- a/activerecord/test/cases/encryption/encryptable_record_test.rb +++ b/activerecord/test/cases/encryption/encryptable_record_test.rb @@ -92,6 +92,12 @@ class ActiveRecord::Encryption::EncryptableRecordTest < ActiveRecord::Encryption assert_encrypted_attribute(traffic_light, :state, states) end + test "encrypts serialized attributes where encrypts is declared first" do + states = ["green", "red"] + traffic_light = EncryptedFirstTrafficLight.create!(state: states, long_state: states) + assert_encrypted_attribute(traffic_light, :state, states) + end + test "encrypts store attributes with accessors" do traffic_light = EncryptedTrafficLightWithStoreState.create!(color: "red", long_state: ["green", "red"]) assert_equal "red", traffic_light.color @@ -404,13 +410,14 @@ def name 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 + assert_encrypted_attribute EncryptedBookWithBinary.create!(logo: low_bytes), :logo, low_bytes + assert_encrypted_attribute EncryptedBookWithBinary.create!(logo: high_bytes), :logo, high_bytes 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 + assert_encrypted_attribute EncryptedBookWithSerializedFirstBinary.create!(logo: json_bytes), :logo, json_bytes + assert_encrypted_attribute EncryptedBookWithSerializedSecondBinary.create!(logo: json_bytes), :logo, json_bytes end test "can compress data with custom compressor" do @@ -423,6 +430,17 @@ def name assert_equal :text, EncryptedPost.type_for_attribute(:body).type end + test "encrypts normalized data" do + assert_encrypted_attribute EncryptedBookNormalizedFirst.create!(name: "Book"), :name, "book" + assert_encrypted_attribute EncryptedBookNormalizedSecond.create!(name: "Book"), :name, "book" + assert_encrypted_attribute EncryptedBookNormalizedFirst.create!(logo: "Book"), :logo, "book" + assert_encrypted_attribute EncryptedBookNormalizedSecond.create!(logo: "Book"), :logo, "book" + end + + test "encrypts attribute data" do + assert_encrypted_attribute EncryptedBookAttribute.create!(name: "2024-01-01"), :name, Date.new(2024, 1, 1) + 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 22e61c490a1a9..dcd58f07d6802 100644 --- a/activerecord/test/models/book_encrypted.rb +++ b/activerecord/test/models/book_encrypted.rb @@ -24,6 +24,31 @@ class EncryptedBookWithDowncaseName < ActiveRecord::Base encrypts :name, deterministic: true, downcase: true end +class EncryptedBookNormalizedFirst < ActiveRecord::Base + self.table_name = "encrypted_books" + + normalizes :name, with: ->(value) { value.to_s.downcase } + encrypts :name + normalizes :logo, with: ->(value) { value.to_s.downcase } + encrypts :logo +end + +class EncryptedBookNormalizedSecond < ActiveRecord::Base + self.table_name = "encrypted_books" + + encrypts :name + normalizes :name, with: ->(value) { value.to_s.downcase } + encrypts :logo + normalizes :logo, with: ->(value) { value.to_s.downcase } +end + +class EncryptedBookAttribute < ActiveRecord::Base + self.table_name = "encrypted_books" + + attribute :name, :date + encrypts :name +end + class EncryptedBookThatIgnoresCase < ActiveRecord::Base self.table_name = "encrypted_books" @@ -50,13 +75,20 @@ class EncryptedBookWithBinary < ActiveRecord::Base encrypts :logo end -class EncryptedBookWithSerializedBinary < ActiveRecord::Base +class EncryptedBookWithSerializedFirstBinary < ActiveRecord::Base self.table_name = "encrypted_books" serialize :logo, coder: JSON encrypts :logo end +class EncryptedBookWithSerializedSecondBinary < ActiveRecord::Base + self.table_name = "encrypted_books" + + encrypts :logo + serialize :logo, coder: JSON +end + class EncryptedBookWithCustomCompressor < ActiveRecord::Base module CustomCompressor def self.deflate(value) diff --git a/activerecord/test/models/traffic_light_encrypted.rb b/activerecord/test/models/traffic_light_encrypted.rb index aefbf216f251b..8926d09d7d8a0 100644 --- a/activerecord/test/models/traffic_light_encrypted.rb +++ b/activerecord/test/models/traffic_light_encrypted.rb @@ -7,6 +7,14 @@ class EncryptedTrafficLight < TrafficLight encrypts :state end +class EncryptedFirstTrafficLight < ActiveRecord::Base + self.table_name = "traffic_lights" + + encrypts :state + serialize :state, type: Array + serialize :long_state, type: Array +end + class EncryptedTrafficLightWithStoreState < TrafficLight store :state, accessors: %i[ color ], coder: ActiveRecord::Coders::JSON encrypts :state