Commit 6ddefe7c authored by Nick Thomas's avatar Nick Thomas

Correctly handle data-loss scenarios when encrypting columns

If the EncryptColumns background migration runs in a sidekiq with a
stale view of the database schema, or when the purported destination
columns don't actually exist, data loss can result. Attempt to work
around these issues by reloading schema information before running
the migration, and raising errors if the model reports that any of its
source or destination columns are missing.
parent 0afce35d
---
title: Correctly handle data-loss scenarios when encrypting columns
merge_request: 23306
author:
type: fixed
module AttrEncrypted module AttrEncrypted
module Adapters module Adapters
module ActiveRecord module ActiveRecord
module DBConnectionQuerier module GitlabMonkeyPatches
# Prevent attr_encrypted from defining virtual accessors for encryption
# data when the code and schema are out of sync. See this issue for more
# details: https://github.com/attr-encrypted/attr_encrypted/issues/332
def attribute_instance_methods_as_symbols_available?
false
end
# Prevent attr_encrypted from checking out a database connection
# indefinitely. The result of this method is only used when the former
# is true, but it is called unconditionally, so there is still value to
# ensuring the connection is released
def attribute_instance_methods_as_symbols def attribute_instance_methods_as_symbols
# Use with_connection so the connection doesn't stay pinned to the thread. # Use with_connection so the connection doesn't stay pinned to the thread.
connected = ::ActiveRecord::Base.connection_pool.with_connection(&:active?) rescue false connected = ::ActiveRecord::Base.connection_pool.with_connection(&:active?) rescue false
...@@ -15,7 +26,16 @@ module AttrEncrypted ...@@ -15,7 +26,16 @@ module AttrEncrypted
end end
end end
end end
prepend DBConnectionQuerier
end end
end end
end end
# As of v3.1.0, the attr_encrypted gem defines the AttrEncrypted and
# AttrEncrypted::Adapters::ActiveRecord modules, and uses "extend" to mix them
# into the ActiveRecord::Base class. This intervention overrides utility methods
# defined by attr_encrypted to fix two bugs, as detailed above.
#
# The methods are used here: https://github.com/attr-encrypted/attr_encrypted/blob/3.1.0/lib/attr_encrypted.rb#L145-158
ActiveSupport.on_load(:active_record) do
extend AttrEncrypted::Adapters::ActiveRecord::GitlabMonkeyPatches
end
...@@ -17,6 +17,12 @@ module Gitlab ...@@ -17,6 +17,12 @@ module Gitlab
class EncryptColumns class EncryptColumns
def perform(model, attributes, from, to) def perform(model, attributes, from, to)
model = model.constantize if model.is_a?(String) model = model.constantize if model.is_a?(String)
# If sidekiq hasn't undergone a restart, its idea of what columns are
# present may be inaccurate, so ensure this is as fresh as possible
model.reset_column_information
model.define_attribute_methods
attributes = expand_attributes(model, Array(attributes).map(&:to_sym)) attributes = expand_attributes(model, Array(attributes).map(&:to_sym))
model.transaction do model.transaction do
...@@ -41,6 +47,14 @@ module Gitlab ...@@ -41,6 +47,14 @@ module Gitlab
raise "Couldn't determine encrypted column for #{klass}##{attribute}" if raise "Couldn't determine encrypted column for #{klass}##{attribute}" if
crypt_column_name.nil? crypt_column_name.nil?
raise "#{klass} source column: #{attribute} is missing" unless
klass.column_names.include?(attribute.to_s)
# Running the migration without the destination column being present
# leads to data loss
raise "#{klass} destination column: #{crypt_column_name} is missing" unless
klass.column_names.include?(crypt_column_name.to_s)
[attribute, crypt_column_name] [attribute, crypt_column_name]
end end
......
require 'spec_helper'
describe 'GitLab monkey-patches to AttrEncrypted' do
describe '#attribute_instance_methods_as_symbols_available?' do
it 'returns false' do
expect(ActiveRecord::Base.__send__(:attribute_instance_methods_as_symbols_available?)).to be_falsy
end
it 'does not define virtual attributes' do
klass = Class.new(ActiveRecord::Base) do
# We need some sort of table to work on
self.table_name = 'projects'
attr_encrypted :foo
end
instance = klass.new
aggregate_failures do
%w[
encrypted_foo encrypted_foo=
encrypted_foo_iv encrypted_foo_iv=
encrypted_foo_salt encrypted_foo_salt=
].each do |method_name|
expect(instance).not_to respond_to(method_name)
end
end
end
end
end
...@@ -65,5 +65,30 @@ describe Gitlab::BackgroundMigration::EncryptColumns, :migration, schema: 201809 ...@@ -65,5 +65,30 @@ describe Gitlab::BackgroundMigration::EncryptColumns, :migration, schema: 201809
expect(hook).to have_attributes(values) expect(hook).to have_attributes(values)
end end
it 'reloads the model column information' do
expect(model).to receive(:reset_column_information).and_call_original
expect(model).to receive(:define_attribute_methods).and_call_original
subject.perform(model, [:token, :url], 1, 1)
end
it 'fails if a source column is not present' do
columns = model.columns.reject { |c| c.name == 'url' }
allow(model).to receive(:columns) { columns }
expect do
subject.perform(model, [:token, :url], 1, 1)
end.to raise_error(/source column: url is missing/)
end
it 'fails if a destination column is not present' do
columns = model.columns.reject { |c| c.name == 'encrypted_url' }
allow(model).to receive(:columns) { columns }
expect do
subject.perform(model, [:token, :url], 1, 1)
end.to raise_error(/destination column: encrypted_url is missing/)
end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment