Skip to content

Commit

Permalink
Fix binary decryption on Postgres
Browse files Browse the repository at this point in the history
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 rails#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.
  • Loading branch information
djmb committed Aug 28, 2024
1 parent f0fd6ab commit 2a4050f
Show file tree
Hide file tree
Showing 5 changed files with 79 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
24 changes: 21 additions & 3 deletions activerecord/test/cases/encryption/encryptable_record_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
34 changes: 33 additions & 1 deletion activerecord/test/models/book_encrypted.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand All @@ -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)
Expand Down
8 changes: 8 additions & 0 deletions activerecord/test/models/traffic_light_encrypted.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 2a4050f

Please sign in to comment.