From 0b2772e6759aa0af55aa4ca553299166d29b6f8c Mon Sep 17 00:00:00 2001 From: Matthew Draper Date: Thu, 24 Feb 2022 20:03:01 +1030 Subject: [PATCH] Don't require encryption credentials when using a custom key provider Instead of pre-validating the configuration, we now check for the required values when they're used. Co-authored-by: Alex Ghiculescu Co-authored-by: Jorge Manrubia Co-authored-by: John Hawthorn --- .../lib/active_record/encryption/config.rb | 8 +++++ .../active_record/encryption/configurable.rb | 4 +-- .../lib/active_record/encryption/context.rb | 13 ++++++-- .../active_record/encryption/key_generator.rb | 6 +++- .../lib/active_record/encryption/scheme.rb | 26 ++++----------- .../test/cases/encryption/config_test.rb | 21 ++++++++++++ .../encryption/encryptable_record_test.rb | 2 +- activerecord/test/cases/encryption/helper.rb | 28 ++++++++++------ .../test/cases/encryption/scheme_test.rb | 32 ------------------- activerecord/test/models/post_encrypted.rb | 2 +- 10 files changed, 73 insertions(+), 69 deletions(-) create mode 100644 activerecord/test/cases/encryption/config_test.rb diff --git a/activerecord/lib/active_record/encryption/config.rb b/activerecord/lib/active_record/encryption/config.rb index 46cccb3827..f31ae8df91 100644 --- a/activerecord/lib/active_record/encryption/config.rb +++ b/activerecord/lib/active_record/encryption/config.rb @@ -21,6 +21,14 @@ module ActiveRecord end end + %w(key_derivation_salt primary_key deterministic_key).each do |key| + silence_redefinition_of_method key + define_method(key) do + instance_variable_get(:"@#{key}").presence or + raise Errors::Configuration, "Missing Active Record encryption credential: active_record_encryption.#{key}" + end + end + private def set_defaults self.store_key_references = false diff --git a/activerecord/lib/active_record/encryption/configurable.rb b/activerecord/lib/active_record/encryption/configurable.rb index 03c022e769..b200b64b83 100644 --- a/activerecord/lib/active_record/encryption/configurable.rb +++ b/activerecord/lib/active_record/encryption/configurable.rb @@ -17,13 +17,11 @@ module ActiveRecord delegate name, to: :context end - def configure(primary_key:, deterministic_key:, key_derivation_salt:, **properties) # :nodoc: + def configure(primary_key: nil, deterministic_key: nil, key_derivation_salt: nil, **properties) # :nodoc: config.primary_key = primary_key config.deterministic_key = deterministic_key config.key_derivation_salt = key_derivation_salt - context.key_provider = ActiveRecord::Encryption::DerivedSecretKeyProvider.new(primary_key) - properties.each do |name, value| [:context, :config].each do |configurable_object_name| configurable_object = ActiveRecord::Encryption.send(configurable_object_name) diff --git a/activerecord/lib/active_record/encryption/context.rb b/activerecord/lib/active_record/encryption/context.rb index f0e3a554de..4c57f4e1ab 100644 --- a/activerecord/lib/active_record/encryption/context.rb +++ b/activerecord/lib/active_record/encryption/context.rb @@ -12,9 +12,7 @@ module ActiveRecord class Context PROPERTIES = %i[ key_provider key_generator cipher message_serializer encryptor frozen_encryption ] - PROPERTIES.each do |name| - attr_accessor name - end + attr_accessor(*PROPERTIES) def initialize set_defaults @@ -22,6 +20,11 @@ module ActiveRecord alias frozen_encryption? frozen_encryption + silence_redefinition_of_method :key_provider + def key_provider + @key_provider ||= build_default_key_provider + end + private def set_defaults self.frozen_encryption = false @@ -30,6 +33,10 @@ module ActiveRecord self.encryptor = ActiveRecord::Encryption::Encryptor.new self.message_serializer = ActiveRecord::Encryption::MessageSerializer.new end + + def build_default_key_provider + ActiveRecord::Encryption::DerivedSecretKeyProvider.new(ActiveRecord::Encryption.config.primary_key) + end end end end diff --git a/activerecord/lib/active_record/encryption/key_generator.rb b/activerecord/lib/active_record/encryption/key_generator.rb index 650eb86754..181aeeb1c4 100644 --- a/activerecord/lib/active_record/encryption/key_generator.rb +++ b/activerecord/lib/active_record/encryption/key_generator.rb @@ -30,10 +30,14 @@ module ActiveRecord # # The generated key will be salted with the value of +ActiveRecord::Encryption.key_derivation_salt+ def derive_key_from(password, length: key_length) - ActiveSupport::KeyGenerator.new(password).generate_key(ActiveRecord::Encryption.config.key_derivation_salt, length) + ActiveSupport::KeyGenerator.new(password).generate_key(key_derivation_salt, length) end private + def key_derivation_salt + @key_derivation_salt ||= ActiveRecord::Encryption.config.key_derivation_salt + end + def key_length @key_length ||= ActiveRecord::Encryption.cipher.key_length end diff --git a/activerecord/lib/active_record/encryption/scheme.rb b/activerecord/lib/active_record/encryption/scheme.rb index ea5211f6c6..175ed51070 100644 --- a/activerecord/lib/active_record/encryption/scheme.rb +++ b/activerecord/lib/active_record/encryption/scheme.rb @@ -45,10 +45,7 @@ module ActiveRecord end def key_provider - @key_provider ||= begin - validate_keys! - @key_provider_param || build_key_provider - end + @key_provider ||= @key_provider_param || build_key_provider || default_key_provider end def merge(other_scheme) @@ -74,26 +71,17 @@ module ActiveRecord raise Errors::Configuration, "key_provider: and key: can't be used simultaneously" if @key_provider_param && @key end - def validate_keys! - validate_credential :key_derivation_salt - validate_credential :primary_key, "needs to be configured to use non-deterministic encryption" unless @deterministic - validate_credential :deterministic_key, "needs to be configured to use deterministic encryption" if @deterministic - end - - def validate_credential(key, error_message = "is not configured") - unless ActiveRecord::Encryption.config.public_send(key).present? - raise Errors::Configuration, "#{key} #{error_message}. Please configure it via credential "\ - "active_record_encryption.#{key} or by setting config.active_record.encryption.#{key}" - end - end - def build_key_provider return DerivedSecretKeyProvider.new(@key) if @key.present? - if @deterministic && (deterministic_key = ActiveRecord::Encryption.config.deterministic_key) - DeterministicKeyProvider.new(deterministic_key) + if @deterministic + DeterministicKeyProvider.new(ActiveRecord::Encryption.config.deterministic_key) end end + + def default_key_provider + ActiveRecord::Encryption.key_provider + end end end end diff --git a/activerecord/test/cases/encryption/config_test.rb b/activerecord/test/cases/encryption/config_test.rb new file mode 100644 index 0000000000..8738d9d484 --- /dev/null +++ b/activerecord/test/cases/encryption/config_test.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require "cases/encryption/helper" + +class ActiveRecord::Encryption::ConfigTest < ActiveRecord::EncryptionTestCase + setup do + @config = ActiveRecord::Encryption::Config.new + end + + test "required keys will raise a config error when accessed but not set" do + @config.primary_key = nil + assert_raises ActiveRecord::Encryption::Errors::Configuration do + @config.primary_key + end + + @config.primary_key = "some key" + assert_nothing_raised do + @config.primary_key + end + end +end diff --git a/activerecord/test/cases/encryption/encryptable_record_test.rb b/activerecord/test/cases/encryption/encryptable_record_test.rb index 09d7623d9c..40f2e76a98 100644 --- a/activerecord/test/cases/encryption/encryptable_record_test.rb +++ b/activerecord/test/cases/encryption/encryptable_record_test.rb @@ -30,7 +30,7 @@ class ActiveRecord::Encryption::EncryptableRecordTest < ActiveRecord::Encryption post = EncryptedPost.create!(title: "The Starfleet is here!", body: "take cover!") post.reload.tags_count # accessing regular attributes works - assert_invalid_key_cant_read_attribute(post, :title) + assert_invalid_key_cant_read_attribute(post, :body) end test "ignores nil values" do diff --git a/activerecord/test/cases/encryption/helper.rb b/activerecord/test/cases/encryption/helper.rb index 32d71d1fed..cfb6607db4 100644 --- a/activerecord/test/cases/encryption/helper.rb +++ b/activerecord/test/cases/encryption/helper.rb @@ -17,7 +17,7 @@ module ActiveRecord::Encryption end def assert_invalid_key_cant_read_attribute(model, attribute_name) - if model.type_for_attribute(attribute_name).key_provider.present? + if model.type_for_attribute(attribute_name).key_provider.respond_to?(:keys=) assert_invalid_key_cant_read_attribute_with_custom_key_provider(model, attribute_name) else assert_invalid_key_cant_read_attribute_with_default_key_provider(model, attribute_name) @@ -91,11 +91,14 @@ module ActiveRecord::Encryption model.reload - attribute_type.key_provider.key = ActiveRecord::Encryption::Key.derive_from "other custom attribute secret" + original_keys = attribute_type.key_provider.keys + attribute_type.key_provider.keys = [ ActiveRecord::Encryption::Key.derive_from("other custom attribute secret") ] assert_raises ActiveRecord::Encryption::Errors::Decryption do model.public_send(attribute_name) end + ensure + attribute_type.key_provider.keys = original_keys end end @@ -141,22 +144,29 @@ end class ActiveRecord::EncryptionTestCase < ActiveRecord::TestCase include ActiveRecord::Encryption::EncryptionHelpers, ActiveRecord::Encryption::PerformanceHelpers - # , PerformanceHelpers - ENCRYPTION_ATTRIBUTES_TO_RESET = %i[ primary_key deterministic_key key_derivation_salt store_key_references - key_derivation_salt support_unencrypted_data encrypt_fixtures forced_encoding_for_deterministic_encryption ] + ENCRYPTION_PROPERTIES_TO_RESET = { + config: %i[ primary_key deterministic_key key_derivation_salt store_key_references + key_derivation_salt support_unencrypted_data encrypt_fixtures + forced_encoding_for_deterministic_encryption ], + context: %i[ key_provider ] + } setup do - ENCRYPTION_ATTRIBUTES_TO_RESET.each do |property| - instance_variable_set "@_original_#{property}", ActiveRecord::Encryption.config.public_send(property) + ENCRYPTION_PROPERTIES_TO_RESET.each do |key, properties| + properties.each do |property| + instance_variable_set "@_original_encryption_#{key}_#{property}", ActiveRecord::Encryption.public_send(key).public_send(property) + end end ActiveRecord::Encryption.config.previous_schemes.clear ActiveRecord::Encryption.encrypted_attribute_declaration_listeners&.clear end teardown do - ENCRYPTION_ATTRIBUTES_TO_RESET.each do |property| - ActiveRecord::Encryption.config.public_send("#{property}=", instance_variable_get("@_original_#{property}")) + ENCRYPTION_PROPERTIES_TO_RESET.each do |key, properties| + properties.each do |property| + ActiveRecord::Encryption.public_send(key).public_send("#{property}=", instance_variable_get("@_original_encryption_#{key}_#{property}")) + end end end end diff --git a/activerecord/test/cases/encryption/scheme_test.rb b/activerecord/test/cases/encryption/scheme_test.rb index 2f613a2b4e..55e2e3d1f1 100644 --- a/activerecord/test/cases/encryption/scheme_test.rb +++ b/activerecord/test/cases/encryption/scheme_test.rb @@ -13,38 +13,6 @@ class ActiveRecord::Encryption::SchemeTest < ActiveRecord::EncryptionTestCase assert_valid_declaration key_provider: ActiveRecord::Encryption::DerivedSecretKeyProvider.new("my secret") end - test "validates primary_key is set for non deterministic encryption" do - ActiveRecord::Encryption.config.primary_key = nil - - assert_raise ActiveRecord::Encryption::Errors::Configuration do - declare_and_use_class - end - - assert_nothing_raised do - declare_and_use_class deterministic: true - end - end - - test "validates deterministic_key is set for non deterministic encryption" do - ActiveRecord::Encryption.config.deterministic_key = nil - - assert_raise ActiveRecord::Encryption::Errors::Configuration do - declare_and_use_class deterministic: true - end - - assert_nothing_raised do - declare_and_use_class - end - end - - test "validates key_derivation_salt is set" do - ActiveRecord::Encryption.config.key_derivation_salt = nil - - assert_raise ActiveRecord::Encryption::Errors::Configuration do - declare_and_use_class - end - end - private def assert_invalid_declaration(**options) assert_raises ActiveRecord::Encryption::Errors::Configuration do diff --git a/activerecord/test/models/post_encrypted.rb b/activerecord/test/models/post_encrypted.rb index 7275f34d8e..8c15e0d09f 100644 --- a/activerecord/test/models/post_encrypted.rb +++ b/activerecord/test/models/post_encrypted.rb @@ -7,7 +7,7 @@ class EncryptedPost < Post # We want to modify the key for testing purposes class MutableDerivedSecretKeyProvider < ActiveRecord::Encryption::DerivedSecretKeyProvider - attr_accessor :key + attr_accessor :keys end encrypts :title