Replace AesNew provider with ActiveSupport::MessageEncryptor

[itspriddle]

The AES gem is seemingly deprecated (chicks/aes#5).

This PR removes the AesNew provider and replaces it with a similar implementation using ActiveSupport::MessageEncryptor.

[itspriddle]

I need to check into the usage/security more, but we can also use the Rails secret here, so something like:

secret = options.fetch :secret do
  key  = options.fetch(:key)
  salt  = options.fetch(:salt)
  ::ActiveSupport::KeyGenerator.new(key).generate_key(salt)
end

@encryptor = ::ActiveSupport::MessageEncryptor.new(secret)

Which would allow simpler model setup:

class User < ApplicationRecord
  crypt_keeper :ssn, encryptor: :active_support, secret: Rails.application.secrets.secret_key_base
end

[jmazzi]

👍

[jmazzi]

Overall I like the changes. The one thing that is not clear to me as a migration path for AesNew users. Any ideas on how that might be done?

[jmazzi]

👍

[itspriddle]

Hm, good question. Let me think on it a bit and circle back.

…

On May 8, 2017, at 10:21 AM, Justin Mazzi ***@***.***> wrote: @jmazzi commented on this pull request. Overall I like the changes. The one thing that is not clear to me as a migration path for AesNew users. Any ideas on how that might be done? In lib/crypt_keeper/provider/active_support.rb <#140 (comment)>: > +module CryptKeeper + module Provider + class ActiveSupport < Base + attr_reader :encryptor + + # Public: Initializes the encryptor + # + # options - A hash, :key and :salt are required + # + # Returns nothing. + def initialize(options = {}) + key = options.fetch(:key) + salt = options.fetch(:salt) + + @Encryptor = ::ActiveSupport::MessageEncryptor.new \ + ::ActiveSupport::KeyGenerator.new(key).generate_key(salt) 👍 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#140 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBoy-5v5DzT8EM7q6EIEQsuoFrZBYpks5r3yTegaJpZM4NLCfD>.

[itspriddle]

General migration path:

1. Enable maintenance mode in any live apps
2. Backup database
3. Decrypt tables: TableName.decrypt_table!
4. Update to 2.0.0.rc1 in your app. Update the encryptor to use :active_support
5. Encrypt tables: TableName.encrypt_table!
6. Verify data can be decrypted: TableName.first
7. Disable maintenance mode if necessary

Rollback:

1. Enable maintenance mode
2. Backup database again
3. Restore first database dump, from before CryptKeeper 2.0.0.rc1
4. Verify data can be decrypted
5. Disable maintenance mode
6. Let us know what happened :(

[michaelrigart]

Hey @itspriddle ,

just tried out the ActiveSupport::MessageEncryptor provider.

When I run my tests with crypt_keeper enabled, I receive the following error on tests that need the encrypted fields:

ActiveSupport::MessageVerifier::InvalidSignature: ActiveSupport::MessageVerifier::InvalidSignature

Any thoughts what might cause these?

[itspriddle]

@michaelrigart Off hand, it could be an issue with the way ActiveSupport::MessageEncryptor uses a unique initialization vector on each call, which means encrypt("string") returns a unique cipher text every time it is called. So if you had manually copied a cipher text from the console or a previous test run, and had a test like assert_equal model.encrypted_field, "static cipher text", that is expected to fail.

Can you tell me your Ruby and Rails versions, and maybe provide a simplified model/test case I can use to try to reproduce?

[michaelrigart]

Hey @itspriddle

running the latest Rails version 5.1.4 and minitest .
The most simplest form you can test with, is creating a model with an encrypted field:

# frozen_string_literal: true

class Case < ApplicationRecord
  crypt_keeper :description,
               encryptor: :active_support,
               key: ENV['CRYPT_KEEPER_KEY'],
               salt: ENV['CRYPT_KEEPER_SALT'],
               encoding: Encoding::UTF_8

  validates :description, presence: true
end

And the following minitest. The error always occurs during the setup.

# frozen_string_literal: true

require 'test_helper'

class CaseTest < ActiveSupport::TestCase
  setup do
    @case = cases(:test)
  end

  test 'valid case' do
    assert @case.valid?
  end

  test 'invalid without a description' do
    @case.description = nil
    refute @case.valid?
    assert_not_nil @case.errors[:description]
  end
end

E

Error:
CaseTest#test_invalid_without_a_description:
ActiveSupport::MessageVerifier::InvalidSignature: ActiveSupport::MessageVerifier::InvalidSignature
    lib/crypt_keeper/provider/active_support.rb:38:in `decrypt'
    test/models/case_test.rb:7:in `block in <class:CaseTest>'

Your idea might be spot on. Going to look into this a bit further.

[scicco]

Hi @itspriddle, I've followed your general migration path and tested it locally. I got no problem with my data.

[jesperronn]

OK @jmazzi, @itspriddle would you decide whether to merge this pull-request?

[itspriddle]

@michaelrigart were you able to work out your test issues?

@jesperronn @jmazzi this should be good to go, but I'd like to hear back on the above issue first.

[michaelrigart]

@itspriddle : haven't got around it yet. I temporarily disable crypt_keeper on tests, since everything works on other environments

[itspriddle]

@michaelrigart 👍 thanks! We're actually doing the same in our projects' test suites.

@jmazzi I think this is good to go then. Any final concerns?

[jmazzi]

@itspriddle Nope, changes look good. Think the README is good enough for communicating that re-encryption is needed?

[itspriddle]

@jmazzi I think it could be louder. Will try to handle that tonight and see if we can't get this merged tomorrow.

[jesperronn]

To address your questions with regards to whether README is good enough to communicate changes, I had some thoughts.

I think it would help do describe the migration path from previous versions. A heading example could be "migrating from crypt_keeper 1.x to 2.x" or similar.

Contents: Could be based of the content of previous comment from @itspriddle #140 (comment)

General migration path:

1. Enable maintenance mode in any live apps
2. Backup database
3. Decrypt tables: TableName.decrypt_table!
4. Update to 2.0.0.rc1 in your app. Update the encryptor to use :active_support
5. Encrypt tables: `TableName.encrypt_table!`
6. Verify data can be decrypted: `TableName.first`
7. Disable maintenance mode if necessary


Rollback:

1. Enable maintenance mode
2. Backup database again
3. Restore first database dump, from before CryptKeeper 2.0.0.rc1
4. Verify data can be decrypted
5. Disable maintenance mode
6. Let us know what happened :(

As I see it, just add this to the readme after this pull request has been merged.

[itspriddle]

Hey all, thanks for the patience while we sorted this out! 2.0.0.rc1 is released with these changes. Please let us know if you hit any issues!