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.

Sign up for GitHub

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

Jump to bottom

Zero downtime 2.0 #35

Merged
merged 8 commits into from
Apr 12, 2018
Merged

Zero downtime 2.0 #35

merged 8 commits into from
Apr 12, 2018

Conversation

nattfodd
Copy link
Contributor

@nattfodd nattfodd commented Apr 8, 2018 •
edited
Loading

This pull request addresses the following issues: #33, #24

Idea

To make this happen you need to create a column, that contains current version of the encrypted field. For example, if you have attr_encrypted :ssn, you need to create in your table another column: ssn_version, with some default value - let it be 20180401000000 for example.

Assuming, you want to change encryption now. You create a new initializer file config/initializers/transcryptor.rb. And describe there the history of your changes:

Transcryptor::Migration.draw do
  define_encryption User,
    field: :ssn,
    options: {
      key:       '67c3800d1572d9d964a6ff3bd821ed02',
      algorithm: 'aes-256-gcm'
    },
    version: 20180401000000,
    version_field: :encrypted_ssn_version # optional

  define_encryption User,
    field: :ssn,
    options: {
      key:       '0726c4d149fa59523bc47d592151584b',
      algorithm: 'id-aes192-GCM'
    },
    version: 20180401000001

  define_encryption User,
    field: :ssn,
    options: {
      key:       '94dd7e2c40a3d51a8dd0a9137356a18e',
      algorithm: 'RC2-64-CBC'
    },
    version: 20180401000002
end

Complete example of usage can be found here: https://github.com/nattfodd/transcryptor_sample_app/pull/1/files

Pros:

  • No changes in model are needed from the client side (all changes are generated automagically by Transcryptor)
  • Versatile, as it only depends on ***_version field to perform as many migrations, as need

TODOs:

  • Make sure new records receive the latest encryption and do not involve in migration
  • Make sure it doesn't break attr_encrypted default behaviour
  • Add tests

ronaldtse
ronaldtse reviewed Apr 8, 2018
View reviewed changes
Copy link
Contributor

@ronaldtse ronaldtse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nattfodd looks great indeed — in fact it might be more useful if we separate the data migration scheme and the attr_encryption “processor”, for example, so when one day we switch to another non-attr_encryption gem, we can still take advantage of the data migration scheme!

@nattfodd
Copy link
Contributor Author

nattfodd commented Apr 8, 2018

@nattfodd looks great indeed — in fact it might be more useful if we separate the data migration scheme and the attr_encryption “processor”, for example, so when one day we switch to another non-attr_encryption gem, we can still take advantage of the data migration scheme!

Do you think it fits the scope of Transcryptor gem? Maybe it makes sense to extract migration logic into small separate gem and use it in transcryptor?

@ronaldtse
Copy link
Contributor

Yes that’s what I meant too. Feel free to proceed.

ribose-jeffreylau
ribose-jeffreylau approved these changes Apr 9, 2018
View reviewed changes
Copy link
Contributor

@ribose-jeffreylau ribose-jeffreylau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@ribose-jeffreylau ribose-jeffreylau mentioned this pull request Apr 9, 2018
Open
@ronaldtse
Copy link
Contributor

@nattfodd I believe these enhancements can be implemented after this PR is merged:

  • make data migration a separate gem
  • make data migration gem a Rails engine to automatically load data migration files, e.g., allowing 1 file per model
  • extract functionality related to attr_encryptor into a "transform processor" that is used to perform the migrations
  • make a rake task to help perform data migration, e.g., after deployment of code, we can start multiple processes to quickly finish the data migrations. Perhaps also add a process indicator (e.g., a SELECT statement that shows how many data cells are still pending migration.
  • allow adding and removing the application of attr_encryptor, e.g., removing encryption of a column, adding encryption to a column.

houndci-bot
houndci-bot reviewed Apr 9, 2018
View reviewed changes
spec/transcryptor/migration_spec.rb
let(:id) { 3 }

it 'does not change encrypted field' do
expect { MigrationSpec.find(id) }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/AmbiguousBlockAssociation: Parenthesize the param change { encrypted_columns_for.call(id) } to make sure that the block will be associated with the change method call.

spec/transcryptor/migration_spec.rb
key: '94dd7e2c40a3d51a8dd0a9137356a18e',
algorithm: 'RC2-64-CBC'
},
version: 20180401000002
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.

spec/transcryptor/migration_spec.rb
key: '67c3800d1572d9d964a6ff3bd821ed02',
algorithm: 'aes-256-gcm'
},
version: 20180401000000
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/NumericLiterals: Use underscores(_) as decimal mark and separate every 3 digits with them.

spec/transcryptor/migration_spec.rb
end
end

context 'when migration is used' do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/BlockLength: Block has too many lines. [43/25]

spec/transcryptor/migration_spec.rb
let(:id) { 1 }

it 'does not migrate encrypted fields' do
expect { MigrationSpec.find(id) }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/AmbiguousBlockAssociation: Parenthesize the param change { encrypted_columns_for.call(id) } to make sure that the block will be associated with the change method call.

spec/transcryptor/migration_spec.rb
algorithm: 'RC2-64-CBC'
end

describe Transcryptor::Migration do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/BlockLength: Block has too many lines. [78/25]

lib/transcryptor/migration/specify_latest_version.rb
def specify_latest_version!
migrated_fields = Transcryptor::Migration.migrations[self.class]
migrated_fields.each do |field, versions|
latest_v = versions[:latest_version].to_i
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/ExtraSpacing: Unnecessary spacing detected.
Layout/SpaceAroundOperators: Operator = should be surrounded by a single space.

lib/transcryptor/migration.rb
# user.ssn_20180401000000
# user.ssn_20180401000001
# user.ssn_20180401000002
def patch_models!
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/MethodLength: Method has too many lines. [11/10]

@nattfodd
Copy link
Contributor Author

nattfodd commented Apr 12, 2018 •
edited
Loading

I believe the PR is ready to be merged (besides those nasty hound comments, which is difficult to fix).

Having direction from unencrypted to encrypted and vice versa isn't still clear for me, since encrypted field is called encrypted_ssn in the database, and it would be weird to have raw unencrypted value in the column with such name. Same for universal migrations.

Let's discuss it in a different Pull Request when it will be subject to discuss.

@ronaldtse
Copy link
Contributor

ronaldtse commented Apr 12, 2018 •
edited
Loading

@nattfodd sounds good, please feel free to merge it with rebase.

Regarding unencrypted to encrypted, I suspect the following naming pairs make better sense?

Data column Version column
ssn ssn_version
encrypted_ssn encrypted_ssn_version

So the database will:

  1. Start with an :ssn column
  2. Add a database migration to add :encrypted_ssn, :encrypted_ssn_version columns (and other necessary attr_encrypted columns) (and run it)
  3. Add a data migration to migrate :ssn data to :encrypted_ssn

Perhaps something like this is reasonable?

@nattfodd
Copy link
Contributor Author

@nattfodd sounds good, please feel free to merge it with rebase.

I don't have access to write to the repository :)

@ronaldtse
Copy link
Contributor

Hmm.. I checked settings and it shouldn't be the case though. Anyway I'll merge it here.

@ronaldtse ronaldtse merged commit 0ce2f1e into riboseinc:master Apr 12, 2018
@ronaldtse
Copy link
Contributor

Ha, just found out why 😉 Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronaldtse ronaldtse ronaldtse left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@ribose-jeffreylau ribose-jeffreylau ribose-jeffreylau approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nattfodd @ronaldtse @ribose-jeffreylau @houndci-bot