Merge branch '51021-more-attr-encrypted' into 'master'

Encrypt webhook tokens and URLs in the database

Closes #51021

See merge request gitlab-org/gitlab-ce!21645

10 files changed, 290 insertions, 0 deletions

diff --git a/app/models/hooks/web_hook.rb b/app/models/hooks/web_hook.rb
index 771a61b090f..68ba4b213b2 100644
--- a/app/models/hooks/web_hook.rb
+++ b/app/models/hooks/web_hook.rb
@@ -3,6 +3,16 @@
 class WebHook < ActiveRecord::Base
   include Sortable
 
+  attr_encrypted :token,
+                 mode:      :per_attribute_iv,
+                 algorithm: 'aes-256-gcm',
+                 key:       Settings.attr_encrypted_db_key_base_truncated
+
+  attr_encrypted :url,
+                 mode:      :per_attribute_iv,
+                 algorithm: 'aes-256-gcm',
+                 key:       Settings.attr_encrypted_db_key_base_truncated
+
   has_many :web_hook_logs, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
   validates :url, presence: true, public_url: { allow_localhost: lambda(&:allow_local_requests?),
@@ -27,4 +37,38 @@ class WebHook < ActiveRecord::Base
   def allow_local_requests?
     false
   end
+
+  # In 11.4, the web_hooks table has both `token` and `encrypted_token` fields.
+  # Ensure that the encrypted version always takes precedence if present.
+  alias_method :attr_encrypted_token, :token
+  def token
+    attr_encrypted_token.presence || read_attribute(:token)
+  end
+
+  # In 11.4, the web_hooks table has both `token` and `encrypted_token` fields.
+  # Pending a background migration to encrypt all fields, we should just clear
+  # the unencrypted value whenever the new value is set.
+  alias_method :'attr_encrypted_token=', :'token='
+  def token=(value)
+    self.attr_encrypted_token = value
+
+    write_attribute(:token, nil)
+  end
+
+  # In 11.4, the web_hooks table has both `url` and `encrypted_url` fields.
+  # Ensure that the encrypted version always takes precedence if present.
+  alias_method :attr_encrypted_url, :url
+  def url
+    attr_encrypted_url.presence || read_attribute(:url)
+  end
+
+  # In 11.4, the web_hooks table has both `url` and `encrypted_url` fields.
+  # Pending a background migration to encrypt all fields, we should just clear
+  # the unencrypted value whenever the new value is set.
+  alias_method :'attr_encrypted_url=', :'url='
+  def url=(value)
+    self.attr_encrypted_url = value
+
+    write_attribute(:url, nil)
+  end
 end
diff --git a/changelogs/unreleased/51021-more-attr-encrypted.yml b/changelogs/unreleased/51021-more-attr-encrypted.yml
new file mode 100644
index 00000000000..0e18c59f1bb
--- /dev/null
+++ b/changelogs/unreleased/51021-more-attr-encrypted.yml
@@ -0,0 +1,5 @@
+---
+title: Encrypt webhook tokens and URLs in the database
+merge_request: 21645
+author:
+type: security
diff --git a/db/migrate/20180910115836_add_attr_encrypted_columns_to_web_hook.rb b/db/migrate/20180910115836_add_attr_encrypted_columns_to_web_hook.rb
new file mode 100644
index 00000000000..72f5c8d653b
--- /dev/null
+++ b/db/migrate/20180910115836_add_attr_encrypted_columns_to_web_hook.rb
@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+class AddAttrEncryptedColumnsToWebHook < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :web_hooks, :encrypted_token, :string
+    add_column :web_hooks, :encrypted_token_iv, :string
+
+    add_column :web_hooks, :encrypted_url, :string
+    add_column :web_hooks, :encrypted_url_iv, :string
+  end
+end
diff --git a/db/post_migrate/20180914162043_encrypt_web_hooks_columns.rb b/db/post_migrate/20180914162043_encrypt_web_hooks_columns.rb
new file mode 100644
index 00000000000..05ec4864a9e
--- /dev/null
+++ b/db/post_migrate/20180914162043_encrypt_web_hooks_columns.rb
@@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+class EncryptWebHooksColumns < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  BATCH_SIZE = 10000
+  RANGE_SIZE = 100
+  MIGRATION = 'EncryptColumns'
+  COLUMNS = [:token, :url]
+
+  WebHook = ::Gitlab::BackgroundMigration::Models::EncryptColumns::WebHook
+
+  disable_ddl_transaction!
+
+  def up
+    WebHook.each_batch(of: BATCH_SIZE) do |relation, index|
+      delay = index * 2.minutes
+
+      relation.each_batch(of: RANGE_SIZE) do |relation|
+        range = relation.pluck('MIN(id)', 'MAX(id)').first
+        args = [WebHook, COLUMNS, *range]
+
+        BackgroundMigrationWorker.perform_in(delay, MIGRATION, args)
+      end
+    end
+  end
+
+  def down
+    # noop
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index ecb9d4391d7..13c6d65255e 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -2272,6 +2272,10 @@ ActiveRecord::Schema.define(version: 20180917172041) do
     t.boolean "job_events", default: false, null: false
     t.boolean "confidential_note_events"
     t.text "push_events_branch_filter"
+    t.string "encrypted_token"
+    t.string "encrypted_token_iv"
+    t.string "encrypted_url"
+    t.string "encrypted_url_iv"
   end
 
   add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
diff --git a/lib/gitlab/background_migration/encrypt_columns.rb b/lib/gitlab/background_migration/encrypt_columns.rb
new file mode 100644
index 00000000000..0d333e47e7b
--- /dev/null
+++ b/lib/gitlab/background_migration/encrypt_columns.rb
@@ -0,0 +1,80 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # EncryptColumn migrates data from an unencrypted column - `foo`, say - to
+    # an encrypted column - `encrypted_foo`, say.
+    #
+    # For this background migration to work, the table that is migrated _has_ to
+    # have an `id` column as the primary key. Additionally, the encrypted column
+    # should be managed by attr_encrypted, and map to an attribute with the same
+    # name as the unencrypted column (i.e., the unencrypted column should be
+    # shadowed).
+    #
+    # To avoid depending on a particular version of the model in app/, add a
+    # model to `lib/gitlab/background_migration/models/encrypt_columns` and use
+    # it in the migration that enqueues the jobs, so code can be shared.
+    class EncryptColumns
+      def perform(model, attributes, from, to)
+        model = model.constantize if model.is_a?(String)
+        attributes = expand_attributes(model, Array(attributes).map(&:to_sym))
+
+        model.transaction do
+          # Use SELECT ... FOR UPDATE to prevent the value being changed while
+          # we are encrypting it
+          relation = model.where(id: from..to).lock
+
+          relation.each do |instance|
+            encrypt!(instance, attributes)
+          end
+        end
+      end
+
+      private
+
+      # Build a hash of { attribute => encrypted column name }
+      def expand_attributes(klass, attributes)
+        expanded = attributes.flat_map do |attribute|
+          attr_config = klass.encrypted_attributes[attribute]
+          crypt_column_name = attr_config&.fetch(:attribute)
+
+          raise "Couldn't determine encrypted column for #{klass}##{attribute}" if
+            crypt_column_name.nil?
+
+          [attribute, crypt_column_name]
+        end
+
+        Hash[*expanded]
+      end
+
+      # Generate ciphertext for each column and update the database
+      def encrypt!(instance, attributes)
+        to_clear = attributes
+          .map { |plain, crypt| apply_attribute!(instance, plain, crypt) }
+          .compact
+          .flat_map { |plain| [plain, nil] }
+
+        to_clear = Hash[*to_clear]
+
+        if instance.changed?
+          instance.save!
+          instance.update_columns(to_clear)
+        end
+      end
+
+      def apply_attribute!(instance, plain_column, crypt_column)
+        plaintext = instance[plain_column]
+        ciphertext = instance[crypt_column]
+
+        # No need to do anything if the plaintext is nil, or an encrypted
+        # value already exists
+        return nil unless plaintext.present? && !ciphertext.present?
+
+        # attr_encrypted will calculate and set the expected value for us
+        instance.public_send("#{plain_column}=", plaintext) # rubocop:disable GitlabSecurity/PublicSend
+
+        plain_column
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/background_migration/models/encrypt_columns/web_hook.rb b/lib/gitlab/background_migration/models/encrypt_columns/web_hook.rb
new file mode 100644
index 00000000000..bb76eb8ed48
--- /dev/null
+++ b/lib/gitlab/background_migration/models/encrypt_columns/web_hook.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    module Models
+      module EncryptColumns
+        # This model is shared between synchronous and background migrations to
+        # encrypt the `token` and `url` columns
+        class WebHook < ActiveRecord::Base
+          include ::EachBatch
+
+          self.table_name = 'web_hooks'
+          self.inheritance_column = :_type_disabled
+
+          attr_encrypted :token,
+                         mode:      :per_attribute_iv,
+                         algorithm: 'aes-256-gcm',
+                         key:       Settings.attr_encrypted_db_key_base_truncated
+
+          attr_encrypted :url,
+                         mode:      :per_attribute_iv,
+                         algorithm: 'aes-256-gcm',
+                         key:       Settings.attr_encrypted_db_key_base_truncated
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index a19b3c88627..2bed470514b 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/lib/gitlab/import_export/import_export.yml
@@ -147,6 +147,12 @@ excluded_attributes:
     - :reference
     - :reference_html
     - :epic_id
+  hooks:
+    - :token
+    - :encrypted_token
+    - :encrypted_token_iv
+    - :encrypted_url
+    - :encrypted_url_iv
 
 methods:
   labels:
diff --git a/spec/lib/gitlab/background_migration/encrypt_columns_spec.rb b/spec/lib/gitlab/background_migration/encrypt_columns_spec.rb
new file mode 100644
index 00000000000..2a869446753
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/encrypt_columns_spec.rb
@@ -0,0 +1,69 @@
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::EncryptColumns, :migration, schema: 20180910115836 do
+  let(:model) { Gitlab::BackgroundMigration::Models::EncryptColumns::WebHook }
+  let(:web_hooks) { table(:web_hooks) }
+
+  let(:plaintext_attrs) do
+    {
+      'encrypted_token' => nil,
+      'encrypted_url' => nil,
+      'token' =>  'secret',
+      'url' => 'http://example.com?access_token=secret'
+    }
+  end
+
+  let(:encrypted_attrs) do
+    {
+      'encrypted_token' => be_present,
+      'encrypted_url' => be_present,
+      'token' => nil,
+      'url' => nil
+    }
+  end
+
+  describe '#perform' do
+    it 'encrypts columns for the specified range' do
+      hooks = web_hooks.create([plaintext_attrs] * 5).sort_by(&:id)
+
+      # Encrypt all but the first and last rows
+      subject.perform(model, [:token, :url], hooks[1].id, hooks[3].id)
+
+      hooks = web_hooks.where(id: hooks.map(&:id)).order(:id)
+
+      aggregate_failures do
+        expect(hooks[0]).to have_attributes(plaintext_attrs)
+        expect(hooks[1]).to have_attributes(encrypted_attrs)
+        expect(hooks[2]).to have_attributes(encrypted_attrs)
+        expect(hooks[3]).to have_attributes(encrypted_attrs)
+        expect(hooks[4]).to have_attributes(plaintext_attrs)
+      end
+    end
+
+    it 'acquires an exclusive lock for the update' do
+      relation = double('relation', each: nil)
+
+      expect(model).to receive(:where) { relation }
+      expect(relation).to receive(:lock) { relation }
+
+      subject.perform(model, [:token, :url], 1, 1)
+    end
+
+    it 'skips already-encrypted columns' do
+      values = {
+        'encrypted_token' => 'known encrypted token',
+        'encrypted_url' => 'known encrypted url',
+        'token' => 'token',
+        'url' => 'url'
+      }
+
+      hook = web_hooks.create(values)
+
+      subject.perform(model, [:token, :url], hook.id, hook.id)
+
+      hook.reload
+
+      expect(hook).to have_attributes(values)
+    end
+  end
+end
diff --git a/spec/models/hooks/web_hook_spec.rb b/spec/models/hooks/web_hook_spec.rb
index a4181631f01..a308ac6e33a 100644
--- a/spec/models/hooks/web_hook_spec.rb
+++ b/spec/models/hooks/web_hook_spec.rb
@@ -57,6 +57,12 @@ describe WebHook do
     end
   end
 
+  describe 'encrypted attributes' do
+    subject { described_class.encrypted_attributes.keys }
+
+    it { is_expected.to contain_exactly(:token, :url) }
+  end
+
   describe 'execute' do
     let(:data) { { key: 'value' } }
     let(:hook_name) { 'project hook' }