Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ActiveRecord eagerly validates encryption configuration unnecessarily #46647

Closed
brynary opened this issue Dec 5, 2022 · 6 comments
Closed

ActiveRecord eagerly validates encryption configuration unnecessarily #46647

brynary opened this issue Dec 5, 2022 · 6 comments

Comments

@brynary
Copy link
Contributor

brynary commented Dec 5, 2022

I am using AWS KMS for encryption with ActiveRecord::Encryption using a simple custom Key Provider (pasted below). As such, KMS generates my data keys and my intent is to not use the build-in ActiveRecord Key Generator. However, ActiveRecord is insisting I configure the primary_key, deterministic_key, and key_derivation_salt.

Steps to reproduce

  1. Create a new Rails 7.0.4 application
  2. Define a custom Key Provider and use it within a model: encrypts :secret_field, key_provider: CustomKeyProvider.new
  3. Rails raises an exception when loading the

Expected behavior

Because I've chosen to replace Rails' built-in key generation, I would not expect to need to configure the encryption options in order to use ActiveRecord::Encryption. Configuring my application with a Rails primary_key creates confusion about exactly which key(s) are being used to encrypt my data.

Given that encryption configuration appears to be optional in general, I would expect configuration errors to be raised lazily e.g. if and only if the Rails-default Key Provider was asked to generate a key without being properly configured.

Actual behavior

activerecord-7.0.4/lib/active_record/encryption/scheme.rb:85:in `validate_credential': key_derivation_salt is not configured. Please configure it via credential active_record_encryption.key_derivation_salt or by setting config.active_record.encryption.key_derivation_salt (ActiveRecord::Encryption::Errors::Configuration)

System configuration

Rails version: 7.0.4

Ruby version: 3.1.2

Custom Key Provider

require "aws-sdk-kms"

class KmsKeyProvider
  KEY_SPEC = "AES_256"

  attr_reader :key_id
  attr_reader :encryption_context

  def initialize(key_id, encryption_context = {})
    @key_id = key_id
    @encryption_context = encryption_context
  end

  def kms_client
    @kms_client ||= Aws::KMS::Client.new
  end

  def encryption_key
    response =
      kms_client.generate_data_key(
        key_id: key_id,
        key_spec: KEY_SPEC,
        encryption_context: encryption_context
      )

    ActiveRecord::Encryption::Key
      .new(response.plaintext)
      .tap do |key|
        key.public_tags.encrypted_data_key = response.ciphertext_blob
      end
  end

  def decryption_keys(encrypted_message)
    encrypted_data_key = encrypted_message.headers.encrypted_data_key
    secret =
      kms_client.decrypt(
        ciphertext_blob: encrypted_data_key,
        encryption_context: encryption_context,
        key_id: key_id
      ).plaintext
    secret ? [ActiveRecord::Encryption::Key.new(secret)] : []
  end
end
@ghiculescu
Copy link
Member

I think #44540 was meant to fix this. Do you still get the issue if you target the Rails main branch?

@brynary
Copy link
Contributor Author

brynary commented Dec 5, 2022

Ah, sorry I did a search but had missed that one. Let me try!

@ghiculescu
Copy link
Member

Reading through that PR it looks like it may have introduced some issues (see also #44873) if you have already used AR Encryption in production. Just be wary of that.

@brynary
Copy link
Contributor Author

brynary commented Dec 5, 2022

Thanks for the heads up. Bit of a silly question.. It looks like that PR was merged into main in February. Does that mean it would've hit a release yet?

@ghiculescu
Copy link
Member

Rails 7.0 was released last year, so only select bug fixes automatically make it into 7.0.x releases. In this case it doesn't look like it's been backported so it is not in a release.

It will definitely be in the next major (7.1).

https://guides.rubyonrails.org/maintenance_policy.html explains how all this works. Generally the easiest way to check if it's going to be included in an existing release is to look at the -stable branch, so https://github.com/rails/rails/tree/7-0-stable in this case, and see if the relevant changes are there.

@ghiculescu
Copy link
Member

I'm going to close this as it seems like the issue is resolved. Please open another issue if you come across any other problems!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants