From 2dbc4175b773a6c79c2a2dbbba8b3f62363713fe Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Nov 2018 15:11:36 +0100
Subject: Add `encrypted` strategy to persist encrypted tokens

---
 .../token_authenticatable_strategies/encrypted.rb  | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 app/models/concerns/token_authenticatable_strategies/encrypted.rb

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
new file mode 100644
index 00000000000..2b10d9dbd00
--- /dev/null
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+module TokenAuthenticatableStrategies
+  class Encrypted < Base
+    def find_token_authenticatable(token, unscoped = false)
+      return unless token
+
+      token_authenticatable = relation(unscoped)
+        .find_by(token_field_name => Gitlab::CryptoHelper.aes256_gcm_encrypt(token))
+
+      if @options[:fallback]
+        token_authenticatable ||= fallback_strategy.find_token_authenticatable(token)
+      end
+
+      token_authenticatable
+    end
+
+    def get_token(instance)
+      token = instance.cleartext_tokens.to_h[@token_field]
+      token ||= fallback_strategy.get_token(instance) if @options[:fallback]
+
+      token
+    end
+
+    def set_token(instance, token)
+      return unless token
+
+      instance.cleartext_tokens ||= {}
+      instance.cleartext_tokens[@token_field] = token
+      instance[token_field_name] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+      instance[@token_field] = nil if @options[:fallback] # TODO this seems wrong
+    end
+
+    protected
+
+    def fallback_strategy
+      @fallback_strategy ||= TokenAuthenticatableStrategies::Insecure.new(@klass, @token_field, @options)
+    end
+
+    def token_set?(instance)
+      token_digest = instance.read_attribute(token_field_name)
+      token_digest ||= instance.read_attribute(@token_field) if @options[:fallback]
+
+      token_digest.present?
+    end
+
+    def token_field_name
+      "#{@token_field}_encrypted"
+    end
+  end
+end
-- 
cgit v1.2.1


From 444062d9ee09fdcee03ef0f41611f355febb1158 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 13 Nov 2018 15:35:38 +0100
Subject: Do not use cleartext approach for encrypted tokens

---
 .../token_authenticatable_strategies/encrypted.rb        | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 2b10d9dbd00..985631119ba 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+          @parallelizable.with_indifferent_access
+
 module TokenAuthenticatableStrategies
   class Encrypted < Base
     def find_token_authenticatable(token, unscoped = false)
@@ -16,25 +18,23 @@ module TokenAuthenticatableStrategies
     end
 
     def get_token(instance)
-      token = instance.cleartext_tokens.to_h[@token_field]
+      raw_token = instance.read_attribute(token_field_name)
+      token = Gitlab::CryptoHelper.aes256_gcm_decrypt(raw_token)
       token ||= fallback_strategy.get_token(instance) if @options[:fallback]
-
-      token
     end
 
     def set_token(instance, token)
-      return unless token
+      raise ArgumentError unless token
 
-      instance.cleartext_tokens ||= {}
-      instance.cleartext_tokens[@token_field] = token
       instance[token_field_name] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
-      instance[@token_field] = nil if @options[:fallback] # TODO this seems wrong
+      # instance[@token_field] = nil if @options[:fallback] # TODO this seems wrong
     end
 
     protected
 
     def fallback_strategy
-      @fallback_strategy ||= TokenAuthenticatableStrategies::Insecure.new(@klass, @token_field, @options)
+      @fallback_strategy ||= TokenAuthenticatableStrategies::Insecure
+        .new(@klass, @token_field, @options)
     end
 
     def token_set?(instance)
-- 
cgit v1.2.1


From 10ea75396b8fe22e4b2fd1514e5d07e7bd97bf08 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Nov 2018 14:40:37 +0100
Subject: Remove token_field_name method from base class

---
 app/models/concerns/token_authenticatable_strategies/base.rb      | 4 ----
 app/models/concerns/token_authenticatable_strategies/encrypted.rb | 6 +++---
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index 413721d3e6c..9aa04ba3d62 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -65,9 +65,5 @@ module TokenAuthenticatableStrategies
     def token_set?(instance)
       raise NotImplementedError
     end
-
-    def token_field_name
-      @token_field
-    end
   end
 end
diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 985631119ba..c68ac399594 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -38,10 +38,10 @@ module TokenAuthenticatableStrategies
     end
 
     def token_set?(instance)
-      token_digest = instance.read_attribute(token_field_name)
-      token_digest ||= instance.read_attribute(@token_field) if @options[:fallback]
+      raw_token = instance.read_attribute(token_field_name)
+      raw_token ||= instance.read_attribute(@token_field) if @options[:fallback]
 
-      token_digest.present?
+      raw_token.present?
     end
 
     def token_field_name
-- 
cgit v1.2.1


From 10b8fd71f6f4dd9c96cd2555a6b115d7baafb91d Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Nov 2018 14:46:46 +0100
Subject: Refactor token authenticatable encrypted strategy

---
 .../token_authenticatable_strategies/base.rb       |  6 +++++
 .../token_authenticatable_strategies/encrypted.rb  | 27 +++++++++++-----------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index 9aa04ba3d62..ef5ed0e577e 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -1,6 +1,8 @@
 # frozen_string_literal: true
 
 module TokenAuthenticatableStrategies
+  attr_reader :klass, :token_field, :options
+
   class Base
     def initialize(klass, token_field, options)
       @klass = klass
@@ -36,6 +38,10 @@ module TokenAuthenticatableStrategies
       instance.save! if Gitlab::Database.read_write?
     end
 
+    def fallback?
+      options[:fallback] == true
+    end
+
     protected
 
     def write_new_token(instance)
diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index c68ac399594..822f0b1935c 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -7,45 +7,46 @@ module TokenAuthenticatableStrategies
     def find_token_authenticatable(token, unscoped = false)
       return unless token
 
+      encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
       token_authenticatable = relation(unscoped)
-        .find_by(token_field_name => Gitlab::CryptoHelper.aes256_gcm_encrypt(token))
+        .find_by(encrypted_field => encrypted_value)
 
-      if @options[:fallback]
-        token_authenticatable ||= fallback_strategy.find_token_authenticatable(token)
+      if fallback?
+        token_authenticatable ||= fallback_strategy
+          .find_token_authenticatable(token)
       end
 
       token_authenticatable
     end
 
     def get_token(instance)
-      raw_token = instance.read_attribute(token_field_name)
+      raw_token = instance.read_attribute(encrypted_field)
       token = Gitlab::CryptoHelper.aes256_gcm_decrypt(raw_token)
-      token ||= fallback_strategy.get_token(instance) if @options[:fallback]
+      token ||= fallback_strategy.get_token(instance) if fallback?
     end
 
     def set_token(instance, token)
-      raise ArgumentError unless token
+      raise ArgumentError unless token.present?
 
-      instance[token_field_name] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
-      # instance[@token_field] = nil if @options[:fallback] # TODO this seems wrong
+      instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
     end
 
     protected
 
     def fallback_strategy
       @fallback_strategy ||= TokenAuthenticatableStrategies::Insecure
-        .new(@klass, @token_field, @options)
+        .new(klass, token_field, options)
     end
 
     def token_set?(instance)
-      raw_token = instance.read_attribute(token_field_name)
-      raw_token ||= instance.read_attribute(@token_field) if @options[:fallback]
+      raw_token = instance.read_attribute(encrypted_field)
+      raw_token ||= instance.read_attribute(token_field) if fallback?
 
       raw_token.present?
     end
 
-    def token_field_name
-      "#{@token_field}_encrypted"
+    def encrypted_field
+      @encrypted_field ||= "#{@token_field}_encrypted"
     end
   end
 end
-- 
cgit v1.2.1


From ce22c7e10ffeb86f1dc6322521b281057d43e2aa Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Nov 2018 14:47:59 +0100
Subject: Remove text prepended to a class by a mistake

---
 app/models/concerns/token_authenticatable_strategies/encrypted.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 822f0b1935c..a9ecc10cee6 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -1,7 +1,5 @@
 # frozen_string_literal: true
 
-          @parallelizable.with_indifferent_access
-
 module TokenAuthenticatableStrategies
   class Encrypted < Base
     def find_token_authenticatable(token, unscoped = false)
-- 
cgit v1.2.1


From c04f56d3b449ed3cc9b3ecc611fe0bd663d1ee41 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Nov 2018 14:58:11 +0100
Subject: Add specs scaffold for encrypted token authenticatable

---
 .../token_authenticatable_strategies/encrypted.rb     |  3 ++-
 .../encrypted_spec.rb                                 | 19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 1 deletion(-)
 create mode 100644 spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index a9ecc10cee6..e2a5a973d4f 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -20,7 +20,8 @@ module TokenAuthenticatableStrategies
     def get_token(instance)
       raw_token = instance.read_attribute(encrypted_field)
       token = Gitlab::CryptoHelper.aes256_gcm_decrypt(raw_token)
-      token ||= fallback_strategy.get_token(instance) if fallback?
+
+      token || (fallback_strategy.get_token(instance) if fallback?)
     end
 
     def set_token(instance, token)
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
new file mode 100644
index 00000000000..34e5268b34d
--- /dev/null
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -0,0 +1,19 @@
+require 'spec_helper'
+
+describe TokenAuthenticatableStrategies::Encrypted do
+  let(:model) { double(:model) }
+  let(:options) { { fallback: true } }
+
+  subject do
+    described_class.new(model, 'some_field', options)
+  end
+
+  describe '#find_token_authenticatable' do
+  end
+
+  describe '#get_token' do
+  end
+
+  describe '#set_token' do
+  end
+end
-- 
cgit v1.2.1


From 9a830f1e0bb6c6ef87b704a7e43dee5aecbdc0cc Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 14 Nov 2018 15:24:00 +0100
Subject: Add unit tests for encrypted token authenticatable

---
 .../token_authenticatable_strategies/base.rb       |  4 +-
 .../encrypted_spec.rb                              | 51 ++++++++++++++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index ef5ed0e577e..ef1b2487cea 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
 module TokenAuthenticatableStrategies
-  attr_reader :klass, :token_field, :options
-
   class Base
+    attr_reader :klass, :token_field, :options
+
     def initialize(klass, token_field, options)
       @klass = klass
       @token_field = token_field
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index 34e5268b34d..e09da304cfb 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -2,18 +2,69 @@ require 'spec_helper'
 
 describe TokenAuthenticatableStrategies::Encrypted do
   let(:model) { double(:model) }
+  let(:instance) { double(:instance) }
   let(:options) { { fallback: true } }
 
+  let(:encrypted) do
+    Gitlab::CryptoHelper.aes256_gcm_encrypt('my-value')
+  end
+
   subject do
     described_class.new(model, 'some_field', options)
   end
 
   describe '#find_token_authenticatable' do
+    it 'finds a relevant resource by encrypted value' do
+      allow(model).to receive(:find_by)
+        .with('some_field_encrypted' => encrypted)
+        .and_return('encrypted resource')
+
+      expect(subject.find_token_authenticatable('my-value'))
+        .to eq 'encrypted resource'
+    end
+
+    it 'uses fallback strategy when token can not be found' do
+      allow_any_instance_of(TokenAuthenticatableStrategies::Insecure)
+        .to receive(:find_token_authenticatable)
+        .and_return('plaintext resource')
+
+      allow(model).to receive(:find_by)
+        .with('some_field_encrypted' => encrypted)
+        .and_return(nil)
+
+      expect(subject.find_token_authenticatable('my-value'))
+        .to eq 'plaintext resource'
+    end
   end
 
   describe '#get_token' do
+    it 'decrypts a token when encrypted token is present' do
+      allow(instance).to receive(:read_attribute)
+        .with('some_field_encrypted')
+        .and_return(encrypted)
+
+      expect(subject.get_token(instance)).to eq 'my-value'
+    end
+
+    it 'reads a plaintext token when encrypted token is not present' do
+      allow(instance).to receive(:read_attribute)
+        .with('some_field_encrypted')
+        .and_return(nil)
+
+      allow(instance).to receive(:read_attribute)
+        .with('some_field')
+        .and_return('cleartext value')
+
+      expect(subject.get_token(instance)).to eq 'cleartext value'
+    end
   end
 
   describe '#set_token' do
+    it 'writes encrypted token to a model instance' do
+      expect(instance).to receive(:[]=)
+        .with('some_field_encrypted', encrypted)
+
+      subject.set_token(instance, 'my-value')
+    end
   end
 end
-- 
cgit v1.2.1


From f1a74a656bcede06d082e351d6261e43a88a0df7 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 15 Nov 2018 15:17:32 +0100
Subject: Encrypt new instance runners registration tokens

---
 app/models/application_setting.rb                  |  2 +-
 app/models/concerns/token_authenticatable.rb       |  4 ++-
 .../token_authenticatable_strategies/base.rb       |  1 +
 .../token_authenticatable_strategies/encrypted.rb  |  1 +
 ...0140_add_encrypted_runners_token_to_settings.rb | 11 +++++++
 db/schema.rb                                       |  3 +-
 spec/models/concerns/token_authenticatable_spec.rb | 35 +++++++++++++++-------
 .../encrypted_spec.rb                              |  4 +--
 8 files changed, 46 insertions(+), 15 deletions(-)
 create mode 100644 db/migrate/20181115140140_add_encrypted_runners_token_to_settings.rb

diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index 207ffae873a..4319db42019 100644
--- a/app/models/application_setting.rb
+++ b/app/models/application_setting.rb
@@ -7,7 +7,7 @@ class ApplicationSetting < ActiveRecord::Base
   include IgnorableColumn
   include ChronicDurationAttribute
 
-  add_authentication_token_field :runners_registration_token
+  add_authentication_token_field :runners_registration_token, encrypted: true, fallback: true
   add_authentication_token_field :health_check_access_token
 
   DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s*     # comma or semicolon, optionally surrounded by whitespace
diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb
index 23a43aec677..f881d119a17 100644
--- a/app/models/concerns/token_authenticatable.rb
+++ b/app/models/concerns/token_authenticatable.rb
@@ -9,7 +9,7 @@ module TokenAuthenticatable
     private # rubocop:disable Lint/UselessAccessModifier
 
     def add_authentication_token_field(token_field, options = {})
-      @token_fields = [] unless @token_fields
+      @token_fields ||= []
       unique = options.fetch(:unique, true)
 
       if @token_fields.include?(token_field)
@@ -22,6 +22,8 @@ module TokenAuthenticatable
 
       strategy = if options[:digest]
                    TokenAuthenticatableStrategies::Digest.new(self, token_field, options)
+                 elsif options[:encrypted]
+                   TokenAuthenticatableStrategies::Encrypted.new(self, token_field, options)
                  else
                    TokenAuthenticatableStrategies::Insecure.new(self, token_field, options)
                  end
diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index ef1b2487cea..53b09ffd4d9 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -24,6 +24,7 @@ module TokenAuthenticatableStrategies
 
     def ensure_token(instance)
       write_new_token(instance) unless token_set?(instance)
+      get_token(instance)
     end
 
     # Returns a token, but only saves when the database is in read & write mode
diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index e2a5a973d4f..e6cf4f4ac58 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -28,6 +28,7 @@ module TokenAuthenticatableStrategies
       raise ArgumentError unless token.present?
 
       instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+      token
     end
 
     protected
diff --git a/db/migrate/20181115140140_add_encrypted_runners_token_to_settings.rb b/db/migrate/20181115140140_add_encrypted_runners_token_to_settings.rb
new file mode 100644
index 00000000000..36d9ad45b19
--- /dev/null
+++ b/db/migrate/20181115140140_add_encrypted_runners_token_to_settings.rb
@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddEncryptedRunnersTokenToSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :application_settings, :runners_registration_token_encrypted, :string
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 56137caf1d7..82e9c8f28e0 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181107054254) do
+ActiveRecord::Schema.define(version: 20181115140140) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -167,6 +167,7 @@ ActiveRecord::Schema.define(version: 20181107054254) do
     t.integer "diff_max_patch_bytes", default: 102400, null: false
     t.integer "archive_builds_in_seconds"
     t.string "commit_email_hostname"
+    t.string "runners_registration_token_encrypted"
   end
 
   create_table "audit_events", force: :cascade do |t|
diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb
index 782687516ae..0cdf430e9ab 100644
--- a/spec/models/concerns/token_authenticatable_spec.rb
+++ b/spec/models/concerns/token_authenticatable_spec.rb
@@ -21,44 +21,59 @@ end
 
 describe ApplicationSetting, 'TokenAuthenticatable' do
   let(:token_field) { :runners_registration_token }
+  let(:settings) { described_class.new }
+
   it_behaves_like 'TokenAuthenticatable'
 
   describe 'generating new token' do
     context 'token is not generated yet' do
       describe 'token field accessor' do
-        subject { described_class.new.send(token_field) }
+        subject { settings.send(token_field) }
+
         it { is_expected.not_to be_blank }
       end
 
-      describe 'ensured token' do
-        subject { described_class.new.send("ensure_#{token_field}") }
+      describe "ensure_runners_registration_token" do
+        subject { settings.send("ensure_#{token_field}") }
 
         it { is_expected.to be_a String }
         it { is_expected.not_to be_blank }
+
+        it 'does not persist token' do
+          expect(settings).not_to be_persisted
+        end
       end
 
-      describe 'ensured! token' do
-        subject { described_class.new.send("ensure_#{token_field}!") }
+      describe 'ensure_runners_registration_token!' do
+        subject { settings.send("ensure_#{token_field}!") }
+
+        it 'persists new token as an encrypted string' do
+          expect(subject).to eq settings.reload.runners_registration_token
+          expect(settings.read_attribute('runners_registration_token_encrypted'))
+            .to eq Gitlab::CryptoHelper.aes256_gcm_encrypt(subject)
+          expect(settings).to be_persisted
+        end
 
-        it 'persists new token' do
-          expect(subject).to eq described_class.current[token_field]
+        it 'does not persist token in a clear text' do
+          expect(subject).not_to eq settings.reload
+            .read_attribute('runners_registration_token_encrypted')
         end
       end
     end
 
     context 'token is generated' do
       before do
-        subject.send("reset_#{token_field}!")
+        settings.send("reset_#{token_field}!")
       end
 
       it 'persists a new token' do
-        expect(subject.send(:read_attribute, token_field)).to be_a String
+        expect(settings.runners_registration_token).to be_a String
       end
     end
   end
 
   describe 'setting new token' do
-    subject { described_class.new.send("set_#{token_field}", '0123456789') }
+    subject { settings.send("set_#{token_field}", '0123456789') }
 
     it { is_expected.to eq '0123456789' }
   end
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index e09da304cfb..851e04942c6 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -60,11 +60,11 @@ describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#set_token' do
-    it 'writes encrypted token to a model instance' do
+    it 'writes encrypted token to a model instance and returns it' do
       expect(instance).to receive(:[]=)
         .with('some_field_encrypted', encrypted)
 
-      subject.set_token(instance, 'my-value')
+      expect(subject.set_token(instance, 'my-value')).to eq 'my-value'
     end
   end
 end
-- 
cgit v1.2.1


From 3ee8e01923c9452afe9ae21c2135aa8a7eb82f89 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 15 Nov 2018 16:08:28 +0100
Subject: Extract token authenticatable strategy fabrication

---
 app/models/concerns/token_authenticatable.rb                 |  9 ++-------
 app/models/concerns/token_authenticatable_strategies/base.rb | 10 ++++++++++
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb
index f881d119a17..fd8b4d70532 100644
--- a/app/models/concerns/token_authenticatable.rb
+++ b/app/models/concerns/token_authenticatable.rb
@@ -20,13 +20,8 @@ module TokenAuthenticatable
 
       attr_accessor :cleartext_tokens
 
-      strategy = if options[:digest]
-                   TokenAuthenticatableStrategies::Digest.new(self, token_field, options)
-                 elsif options[:encrypted]
-                   TokenAuthenticatableStrategies::Encrypted.new(self, token_field, options)
-                 else
-                   TokenAuthenticatableStrategies::Insecure.new(self, token_field, options)
-                 end
+      strategy = TokenAuthenticatableStrategies::Base
+        .fabricate(self, token_field, options)
 
       if unique
         define_singleton_method("find_by_#{token_field}") do |token|
diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index 53b09ffd4d9..a722616756a 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -43,6 +43,16 @@ module TokenAuthenticatableStrategies
       options[:fallback] == true
     end
 
+    def self.fabricate(instance, field, options)
+      if options[:digest]
+       TokenAuthenticatableStrategies::Digest.new(instance, field, options)
+      elsif options[:encrypted]
+       TokenAuthenticatableStrategies::Encrypted.new(instance, field, options)
+      else
+       TokenAuthenticatableStrategies::Insecure.new(instance, field, options)
+      end
+    end
+
     protected
 
     def write_new_token(instance)
-- 
cgit v1.2.1


From 621071ca5b7ecb21b9baf6d9a2b32758371b7128 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 15 Nov 2018 16:12:07 +0100
Subject: Refactor and simplify token authenticatable concern

---
 app/models/concerns/token_authenticatable.rb | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb
index fd8b4d70532..ca5329a3615 100644
--- a/app/models/concerns/token_authenticatable.rb
+++ b/app/models/concerns/token_authenticatable.rb
@@ -9,21 +9,18 @@ module TokenAuthenticatable
     private # rubocop:disable Lint/UselessAccessModifier
 
     def add_authentication_token_field(token_field, options = {})
-      @token_fields ||= []
-      unique = options.fetch(:unique, true)
-
-      if @token_fields.include?(token_field)
+      if token_authenticatable_fields.include?(token_field)
         raise ArgumentError.new("#{token_field} already configured via add_authentication_token_field")
       end
 
-      @token_fields << token_field
+      token_authenticatable_fields.push(token_field)
 
       attr_accessor :cleartext_tokens
 
       strategy = TokenAuthenticatableStrategies::Base
         .fabricate(self, token_field, options)
 
-      if unique
+      if options.fetch(:unique, true)
         define_singleton_method("find_by_#{token_field}") do |token|
           strategy.find_token_authenticatable(token)
         end
@@ -51,5 +48,9 @@ module TokenAuthenticatable
         strategy.reset_token!(self)
       end
     end
+
+    def token_authenticatable_fields
+      @token_authenticatable_fields ||= []
+    end
   end
 end
-- 
cgit v1.2.1


From 0df989ba06606b675b19e32a74edf03f47a28fbb Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 16 Nov 2018 15:10:02 +0100
Subject: Add specs for token authenticable strategy factory method

---
 .../token_authenticatable_strategies/base.rb       |  6 ++--
 .../token_authenticatable_strategies/base_spec.rb  | 32 ++++++++++++++++++++++
 2 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 spec/models/concerns/token_authenticatable_strategies/base_spec.rb

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index a722616756a..1435d6a5751 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -45,11 +45,11 @@ module TokenAuthenticatableStrategies
 
     def self.fabricate(instance, field, options)
       if options[:digest]
-       TokenAuthenticatableStrategies::Digest.new(instance, field, options)
+        TokenAuthenticatableStrategies::Digest.new(instance, field, options)
       elsif options[:encrypted]
-       TokenAuthenticatableStrategies::Encrypted.new(instance, field, options)
+        TokenAuthenticatableStrategies::Encrypted.new(instance, field, options)
       else
-       TokenAuthenticatableStrategies::Insecure.new(instance, field, options)
+        TokenAuthenticatableStrategies::Insecure.new(instance, field, options)
       end
     end
 
diff --git a/spec/models/concerns/token_authenticatable_strategies/base_spec.rb b/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
new file mode 100644
index 00000000000..80df9c198fd
--- /dev/null
+++ b/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
@@ -0,0 +1,32 @@
+require 'spec_helper'
+
+describe TokenAuthenticatableStrategies::Base do
+  let(:instance) { double(:instance) }
+  let(:field) { double(:field) }
+
+  describe '.fabricate' do
+    context 'when digest stragegy is specified' do
+      it 'fabricates digest strategy object' do
+        strategy = described_class.fabricate(instance, field, digest: true)
+
+        expect(strategy).to be_a TokenAuthenticatableStrategies::Digest
+      end
+    end
+
+    context 'when encrypted strategy is specified' do
+      it 'fabricates encrypted strategy object' do
+        strategy = described_class.fabricate(instance, field, encrypted: true)
+
+        expect(strategy).to be_a TokenAuthenticatableStrategies::Encrypted
+      end
+    end
+
+    context 'when no strategy is specified' do
+      it 'fabricates insecure strategy object' do
+        strategy = described_class.fabricate(instance, field, something: true)
+
+        expect(strategy).to be_a TokenAuthenticatableStrategies::Insecure
+      end
+    end
+  end
+end
-- 
cgit v1.2.1


From fa33a2eedc4014ffbc450a74fcd112e663ac5b01 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 16 Nov 2018 15:16:37 +0100
Subject: Encrypt group / project runners registration tokens

---
 app/models/group.rb                                          |  2 +-
 app/models/project.rb                                        |  2 +-
 ...181116141415_add_encrypted_runners_token_to_namespaces.rb | 12 ++++++++++++
 ...20181116141504_add_encrypted_runners_token_to_projects.rb | 12 ++++++++++++
 db/schema.rb                                                 |  4 +++-
 5 files changed, 29 insertions(+), 3 deletions(-)
 create mode 100644 db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb
 create mode 100644 db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb

diff --git a/app/models/group.rb b/app/models/group.rb
index adb9169cfcd..e90b28bfa02 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -55,7 +55,7 @@ class Group < Namespace
 
   validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
 
-  add_authentication_token_field :runners_token
+  add_authentication_token_field :runners_token, encrypted: true, fallback: true
 
   after_create :post_create_hook
   after_destroy :post_destroy_hook
diff --git a/app/models/project.rb b/app/models/project.rb
index d87fc1e4b86..e2b65fab3ee 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -76,7 +76,7 @@ class Project < ActiveRecord::Base
   default_value_for :snippets_enabled, gitlab_config_features.snippets
   default_value_for :only_allow_merge_if_all_discussions_are_resolved, false
 
-  add_authentication_token_field :runners_token
+  add_authentication_token_field :runners_token, encrypted: true, fallback: true
 
   before_validation :mark_remote_mirrors_for_removal, if: -> { RemoteMirror.table_exists? }
 
diff --git a/db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb b/db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb
new file mode 100644
index 00000000000..a5a6373dd38
--- /dev/null
+++ b/db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class AddEncryptedRunnersTokenToNamespaces < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :namespaces, :runners_token_encrypted, :string
+    # TODO index
+  end
+end
diff --git a/db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb b/db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb
new file mode 100644
index 00000000000..32401629478
--- /dev/null
+++ b/db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class AddEncryptedRunnersTokenToProjects < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :projects, :runners_token_encrypted, :string
+    # TODO index
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 82e9c8f28e0..9fd4e05361c 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181115140140) do
+ActiveRecord::Schema.define(version: 20181116141504) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1409,6 +1409,7 @@ ActiveRecord::Schema.define(version: 20181115140140) do
     t.integer "two_factor_grace_period", default: 48, null: false
     t.integer "cached_markdown_version"
     t.string "runners_token"
+    t.string "runners_token_encrypted"
   end
 
   add_index "namespaces", ["created_at"], name: "index_namespaces_on_created_at", using: :btree
@@ -1753,6 +1754,7 @@ ActiveRecord::Schema.define(version: 20181115140140) do
     t.boolean "pages_https_only", default: true
     t.boolean "remote_mirror_available_overridden"
     t.integer "pool_repository_id", limit: 8
+    t.string "runners_token_encrypted"
   end
 
   add_index "projects", ["ci_id"], name: "index_projects_on_ci_id", using: :btree
-- 
cgit v1.2.1


From bc00b814d4eaf83a3a52c7693139eaed9b7c2b34 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 19 Nov 2018 15:01:37 +0100
Subject: Do not raise if encrypted tokens field does not exist

This is mostly important in specs for migration, where we are still
using factories, despite that we have a Rubocop cop that should prevent
doing that.
---
 .../concerns/token_authenticatable_strategies/encrypted.rb  | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index e6cf4f4ac58..71ef18a8d8f 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -15,6 +15,17 @@ module TokenAuthenticatableStrategies
       end
 
       token_authenticatable
+    rescue ActiveRecord::StatementInvalid
+      nil
+    end
+
+    def ensure_token(instance)
+      # TODO, tech debt, because some specs are testing migrations, but are still
+      # using factory bot to create resources, it might happen that a database
+      # schema does not have "#{token_name}_encrypted" field yet, however a bunch
+      # of models call `ensure_#{token_name}` in `before_save`.
+      #
+      super if instance.has_attribute?(encrypted_field)
     end
 
     def get_token(instance)
@@ -40,7 +51,7 @@ module TokenAuthenticatableStrategies
 
     def token_set?(instance)
       raw_token = instance.read_attribute(encrypted_field)
-      raw_token ||= instance.read_attribute(token_field) if fallback?
+      raw_token ||= (instance.read_attribute(token_field) if fallback?)
 
       raw_token.present?
     end
-- 
cgit v1.2.1


From f691b1fa08ac88c9bb4af819ffb49a9bf68df173 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 20 Nov 2018 14:29:46 +0100
Subject: Use falback to set token if encrypted field is missing

---
 .../concerns/token_authenticatable_strategies/encrypted.rb     | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 71ef18a8d8f..9c28530fbb6 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -24,8 +24,14 @@ module TokenAuthenticatableStrategies
       # using factory bot to create resources, it might happen that a database
       # schema does not have "#{token_name}_encrypted" field yet, however a bunch
       # of models call `ensure_#{token_name}` in `before_save`.
-      #
-      super if instance.has_attribute?(encrypted_field)
+
+      return super if instance.has_attribute?(encrypted_field)
+
+      if fallback?
+        fallback_strategy.ensure_token(instance)
+      else
+        raise ArgumentError, 'Encrypted field does not exist without fallback'
+      end
     end
 
     def get_token(instance)
-- 
cgit v1.2.1


From b32a99474b3c9e4e5a7f93116491c259418ff3bf Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 20 Nov 2018 15:25:24 +0100
Subject: Find a runner using encrypted project / group tokens

---
 lib/api/runner.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index 2f15f3a7d76..af205dc8a61 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -28,10 +28,10 @@ module API
           if runner_registration_token_valid?
             # Create shared runner. Requires admin access
             attributes.merge(runner_type: :instance_type)
-          elsif project = Project.find_by(runners_token: params[:token])
+          elsif project = Project.find_by_runners_token(params[:token])
             # Create a specific runner for the project
             attributes.merge(runner_type: :project_type, projects: [project])
-          elsif group = Group.find_by(runners_token: params[:token])
+          elsif group = Group.find_by_runners_token(params[:token])
             # Create a specific runner for the group
             attributes.merge(runner_type: :group_type, groups: [group])
           else
-- 
cgit v1.2.1


From 56e5a2a3abe6be7307a87125abe70f2775dcaee2 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 20 Nov 2018 15:25:45 +0100
Subject: Reset insecure token when setting an encrypted one

---
 app/models/concerns/token_authenticatable_strategies/encrypted.rb   | 6 +++++-
 .../concerns/token_authenticatable_strategies/encrypted_spec.rb     | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 9c28530fbb6..3d23eed164e 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -24,13 +24,16 @@ module TokenAuthenticatableStrategies
       # using factory bot to create resources, it might happen that a database
       # schema does not have "#{token_name}_encrypted" field yet, however a bunch
       # of models call `ensure_#{token_name}` in `before_save`.
+      #
+      # In that case we are using insecure strategy, but this should only happen
+      # in tests, because otherwise `encrypted_field` is going to exist.
 
       return super if instance.has_attribute?(encrypted_field)
 
       if fallback?
         fallback_strategy.ensure_token(instance)
       else
-        raise ArgumentError, 'Encrypted field does not exist without fallback'
+        raise ArgumentError, 'No fallback defined when encrypted field is missing!'
       end
     end
 
@@ -45,6 +48,7 @@ module TokenAuthenticatableStrategies
       raise ArgumentError unless token.present?
 
       instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+      instance[token_field] = nil
       token
     end
 
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index 851e04942c6..cbf7bf085ef 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -63,6 +63,8 @@ describe TokenAuthenticatableStrategies::Encrypted do
     it 'writes encrypted token to a model instance and returns it' do
       expect(instance).to receive(:[]=)
         .with('some_field_encrypted', encrypted)
+      expect(instance).to receive(:[]=)
+        .with('some_field', nil)
 
       expect(subject.set_token(instance, 'my-value')).to eq 'my-value'
     end
-- 
cgit v1.2.1


From 747a5c425a0ce1aa5e9005a03804f2ea513aa73b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 20 Nov 2018 15:26:38 +0100
Subject: Remove Rubocop override that is no longer necessary

---
 lib/api/runner.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/api/runner.rb b/lib/api/runner.rb
index af205dc8a61..c60d25b88cb 100644
--- a/lib/api/runner.rb
+++ b/lib/api/runner.rb
@@ -19,7 +19,6 @@ module API
         optional :tag_list, type: Array[String], desc: %q(List of Runner's tags)
         optional :maximum_timeout, type: Integer, desc: 'Maximum timeout set when this Runner will handle the job'
       end
-      # rubocop: disable CodeReuse/ActiveRecord
       post '/' do
         attributes = attributes_for_keys([:description, :active, :locked, :run_untagged, :tag_list, :maximum_timeout])
           .merge(get_runner_details_from_request)
@@ -46,7 +45,6 @@ module API
           render_validation_error!(runner)
         end
       end
-      # rubocop: enable CodeReuse/ActiveRecord
 
       desc 'Deletes a registered Runner' do
         http_codes [[204, 'Runner was deleted'], [403, 'Forbidden']]
-- 
cgit v1.2.1


From 0a611caaf9e92dd527b3200c52c1cd79958b8077 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 20 Nov 2018 15:41:06 +0100
Subject: Improve specs for hiding runners tokens in traces

---
 spec/models/ci/build_spec.rb                       | 26 ++++------------------
 .../shared_examples/ci_trace_shared_examples.rb    |  6 ++---
 2 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 6849bc6db7a..d32f3a0cead 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -769,33 +769,15 @@ describe Ci::Build do
     let(:subject) { build.hide_secrets(data) }
 
     context 'hide runners token' do
-      let(:data) { 'new token data'}
+      let(:data) { "new #{project.runners_token} data"}
 
-      before do
-        build.project.update(runners_token: 'token')
-      end
-
-      it { is_expected.to eq('new xxxxx data') }
+      it { is_expected.to match(/^new [x]+ data$/) }
     end
 
     context 'hide build token' do
-      let(:data) { 'new token data'}
-
-      before do
-        build.update(token: 'token')
-      end
-
-      it { is_expected.to eq('new xxxxx data') }
-    end
-
-    context 'hide build token' do
-      let(:data) { 'new token data'}
-
-      before do
-        build.update(token: 'token')
-      end
+      let(:data) { "new #{build.token} data"}
 
-      it { is_expected.to eq('new xxxxx data') }
+      it { is_expected.to match(/^new [x]+ data$/) }
     end
   end
 
diff --git a/spec/support/shared_examples/ci_trace_shared_examples.rb b/spec/support/shared_examples/ci_trace_shared_examples.rb
index 94e82b8ce90..a8bc7ccfbc9 100644
--- a/spec/support/shared_examples/ci_trace_shared_examples.rb
+++ b/spec/support/shared_examples/ci_trace_shared_examples.rb
@@ -180,10 +180,9 @@ shared_examples_for 'common trace features' do
     end
 
     context 'runners token' do
-      let(:token) { 'my_secret_token' }
+      let(:token) { build.project.runners_token }
 
       before do
-        build.project.update(runners_token: token)
         trace.set(token)
       end
 
@@ -193,10 +192,9 @@ shared_examples_for 'common trace features' do
     end
 
     context 'hides build token' do
-      let(:token) { 'my_secret_token' }
+      let(:token) { build.token }
 
       before do
-        build.update(token: token)
         trace.set(token)
       end
 
-- 
cgit v1.2.1


From 478c15fa8940d0669e57aa9e5c0a3dd927c76e5c Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 20 Nov 2018 16:15:09 +0100
Subject: Exclude runners tokens from GitLab project exports

---
 lib/gitlab/import_export/import_export.yml   | 4 ++++
 lib/gitlab/import_export/relation_factory.rb | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index 9790818ecaf..8a03b0e87b7 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/lib/gitlab/import_export/import_export.yml
@@ -101,6 +101,7 @@ excluded_attributes:
     - :import_error
     - :mirror
     - :runners_token
+    - :runners_token_encrypted
     - :repository_storage
     - :repository_read_only
     - :lfs_enabled
@@ -116,6 +117,9 @@ excluded_attributes:
     - :remote_mirror_available_overridden
     - :description_html
     - :repository_languages
+  namespaces:
+    - :runners_token
+    - :runners_token_encrypted
   prometheus_metrics:
     - :common
     - :identifier
diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb
index 2486b1e4921..ca574d51b63 100644
--- a/lib/gitlab/import_export/relation_factory.rb
+++ b/lib/gitlab/import_export/relation_factory.rb
@@ -31,7 +31,7 @@ module Gitlab
 
       EXISTING_OBJECT_CHECK = %i[milestone milestones label labels project_label project_labels group_label group_labels project_feature].freeze
 
-      TOKEN_RESET_MODELS = %w[Ci::Trigger Ci::Build ProjectHook].freeze
+      TOKEN_RESET_MODELS = %w[Project Namespace Group Ci::Trigger Ci::Build ProjectHook].freeze
 
       def self.create(*args)
         new(*args).create
-- 
cgit v1.2.1


From e491df5fc67a51d78e8d832a4919aff0033dc0bd Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 20 Nov 2018 16:30:39 +0100
Subject: Encrypt runners communication token in the database

---
 app/models/ci/runner.rb                                      |  9 ++++-----
 .../20181120151656_add_token_encrypted_to_ci_runners.rb      | 12 ++++++++++++
 db/schema.rb                                                 |  3 ++-
 lib/gitlab/import_export/import_export.yml                   |  3 +++
 lib/gitlab/import_export/relation_factory.rb                 |  3 ++-
 5 files changed, 23 insertions(+), 7 deletions(-)
 create mode 100644 db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 31330d0682e..a4645658c72 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -8,6 +8,9 @@ module Ci
     include RedisCacheable
     include ChronicDurationAttribute
     include FromUnion
+    include TokenAuthenticatable
+
+    add_authentication_token_field :token, encrypted: true, fallback: true
 
     enum access_level: {
       not_protected: 0,
@@ -39,7 +42,7 @@ module Ci
 
     has_one :last_build, ->() { order('id DESC') }, class_name: 'Ci::Build'
 
-    before_validation :set_default_values
+    before_save :ensure_token
 
     scope :active, -> { where(active: true) }
     scope :paused, -> { where(active: false) }
@@ -145,10 +148,6 @@ module Ci
       end
     end
 
-    def set_default_values
-      self.token = SecureRandom.hex(15) if self.token.blank?
-    end
-
     def assign_to(project, current_user = nil)
       if instance_type?
         self.runner_type = :project_type
diff --git a/db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb b/db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb
new file mode 100644
index 00000000000..8a9df4b44f8
--- /dev/null
+++ b/db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb
@@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class AddTokenEncryptedToCiRunners < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :ci_runners, :token_encrypted, :string
+    # TODO index
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 9fd4e05361c..0e26b8a2b26 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181116141504) do
+ActiveRecord::Schema.define(version: 20181120151656) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -543,6 +543,7 @@ ActiveRecord::Schema.define(version: 20181116141504) do
     t.string "ip_address"
     t.integer "maximum_timeout"
     t.integer "runner_type", limit: 2, null: false
+    t.string "token_encrypted"
   end
 
   add_index "ci_runners", ["contacted_at"], name: "index_ci_runners_on_contacted_at", using: :btree
diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml
index 8a03b0e87b7..a0a6ddcb0c4 100644
--- a/lib/gitlab/import_export/import_export.yml
+++ b/lib/gitlab/import_export/import_export.yml
@@ -158,6 +158,9 @@ excluded_attributes:
     - :encrypted_token_iv
     - :encrypted_url
     - :encrypted_url_iv
+  runners:
+    - :token
+    - :token_encrypted
 
 methods:
   labels:
diff --git a/lib/gitlab/import_export/relation_factory.rb b/lib/gitlab/import_export/relation_factory.rb
index ca574d51b63..441ca568003 100644
--- a/lib/gitlab/import_export/relation_factory.rb
+++ b/lib/gitlab/import_export/relation_factory.rb
@@ -8,6 +8,7 @@ module Gitlab
                     triggers: 'Ci::Trigger',
                     pipeline_schedules: 'Ci::PipelineSchedule',
                     builds: 'Ci::Build',
+                    runners: 'Ci::Runner',
                     hooks: 'ProjectHook',
                     merge_access_levels: 'ProtectedBranch::MergeAccessLevel',
                     push_access_levels: 'ProtectedBranch::PushAccessLevel',
@@ -31,7 +32,7 @@ module Gitlab
 
       EXISTING_OBJECT_CHECK = %i[milestone milestones label labels project_label project_labels group_label group_labels project_feature].freeze
 
-      TOKEN_RESET_MODELS = %w[Project Namespace Group Ci::Trigger Ci::Build ProjectHook].freeze
+      TOKEN_RESET_MODELS = %w[Project Namespace Ci::Trigger Ci::Build Ci::Runner ProjectHook].freeze
 
       def self.create(*args)
         new(*args).create
-- 
cgit v1.2.1


From 3578eb45f940ed3ddab742538bd1910b0bd8834f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 21 Nov 2018 11:28:06 +0100
Subject: Add database indices to encrypted runner tokens

---
 ...415_add_encrypted_runners_token_to_namespaces.rb |  1 -
 ...41504_add_encrypted_runners_token_to_projects.rb |  1 -
 ...81120151656_add_token_encrypted_to_ci_runners.rb |  1 -
 ...121101802_add_encrypted_runner_tokens_indices.rb | 21 +++++++++++++++++++++
 db/schema.rb                                        |  5 ++++-
 5 files changed, 25 insertions(+), 4 deletions(-)
 create mode 100644 db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb

diff --git a/db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb b/db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb
index a5a6373dd38..b92b1b50218 100644
--- a/db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb
+++ b/db/migrate/20181116141415_add_encrypted_runners_token_to_namespaces.rb
@@ -7,6 +7,5 @@ class AddEncryptedRunnersTokenToNamespaces < ActiveRecord::Migration
 
   def change
     add_column :namespaces, :runners_token_encrypted, :string
-    # TODO index
   end
 end
diff --git a/db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb b/db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb
index 32401629478..53e475bd180 100644
--- a/db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb
+++ b/db/migrate/20181116141504_add_encrypted_runners_token_to_projects.rb
@@ -7,6 +7,5 @@ class AddEncryptedRunnersTokenToProjects < ActiveRecord::Migration
 
   def change
     add_column :projects, :runners_token_encrypted, :string
-    # TODO index
   end
 end
diff --git a/db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb b/db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb
index 8a9df4b44f8..40db6b399ab 100644
--- a/db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb
+++ b/db/migrate/20181120151656_add_token_encrypted_to_ci_runners.rb
@@ -7,6 +7,5 @@ class AddTokenEncryptedToCiRunners < ActiveRecord::Migration
 
   def change
     add_column :ci_runners, :token_encrypted, :string
-    # TODO index
   end
 end
diff --git a/db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb b/db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb
new file mode 100644
index 00000000000..a4da24501a3
--- /dev/null
+++ b/db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+class AddEncryptedRunnerTokensIndices < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :namespaces, :runners_token_encrypted, unique: true
+    add_concurrent_index :projects, :runners_token_encrypted, unique: true
+    add_concurrent_index :ci_runners, :token_encrypted, unique: true
+  end
+
+  def down
+    remove_concurrent_index :ci_runners, :token_encrypted
+    remove_concurrent_index :projects, :runners_token_encrypted
+    remove_concurrent_index :namespaces, :runners_token_encrypted
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 41ad0e36849..b8875837471 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181120151656) do
+ActiveRecord::Schema.define(version: 20181121101802) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -527,6 +527,7 @@ ActiveRecord::Schema.define(version: 20181120151656) do
     t.index ["locked"], name: "index_ci_runners_on_locked", using: :btree
     t.index ["runner_type"], name: "index_ci_runners_on_runner_type", using: :btree
     t.index ["token"], name: "index_ci_runners_on_token", using: :btree
+    t.index ["token_encrypted"], name: "index_ci_runners_on_token_encrypted", unique: true, using: :btree
   end
 
   create_table "ci_stages", force: :cascade do |t|
@@ -1334,6 +1335,7 @@ ActiveRecord::Schema.define(version: 20181120151656) do
     t.index ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"}
     t.index ["require_two_factor_authentication"], name: "index_namespaces_on_require_two_factor_authentication", using: :btree
     t.index ["runners_token"], name: "index_namespaces_on_runners_token", unique: true, using: :btree
+    t.index ["runners_token_encrypted"], name: "index_namespaces_on_runners_token_encrypted", unique: true, using: :btree
     t.index ["type"], name: "index_namespaces_on_type", using: :btree
   end
 
@@ -1669,6 +1671,7 @@ ActiveRecord::Schema.define(version: 20181120151656) do
     t.index ["repository_storage", "created_at"], name: "idx_project_repository_check_partial", where: "(last_repository_check_at IS NULL)", using: :btree
     t.index ["repository_storage"], name: "index_projects_on_repository_storage", using: :btree
     t.index ["runners_token"], name: "index_projects_on_runners_token", using: :btree
+    t.index ["runners_token_encrypted"], name: "index_projects_on_runners_token_encrypted", unique: true, using: :btree
     t.index ["star_count"], name: "index_projects_on_star_count", using: :btree
     t.index ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
   end
-- 
cgit v1.2.1


From 64c23778547b14a6a8063280d07051eddf475e48 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 21 Nov 2018 11:46:36 +0100
Subject: Add migratable models for runners tokens migration

---
 lib/gitlab/background_migration/encrypt_columns.rb | 10 ++++----
 .../models/encrypt_columns/namespace.rb            | 28 ++++++++++++++++++++++
 .../models/encrypt_columns/project.rb              | 28 ++++++++++++++++++++++
 .../models/encrypt_columns/runner.rb               | 28 ++++++++++++++++++++++
 .../models/encrypt_columns/settings.rb             | 28 ++++++++++++++++++++++
 .../models/encrypt_columns/web_hook.rb             |  4 ++--
 6 files changed, 120 insertions(+), 6 deletions(-)
 create mode 100644 lib/gitlab/background_migration/models/encrypt_columns/namespace.rb
 create mode 100644 lib/gitlab/background_migration/models/encrypt_columns/project.rb
 create mode 100644 lib/gitlab/background_migration/models/encrypt_columns/runner.rb
 create mode 100644 lib/gitlab/background_migration/models/encrypt_columns/settings.rb

diff --git a/lib/gitlab/background_migration/encrypt_columns.rb b/lib/gitlab/background_migration/encrypt_columns.rb
index 0d333e47e7b..ba806c869c9 100644
--- a/lib/gitlab/background_migration/encrypt_columns.rb
+++ b/lib/gitlab/background_migration/encrypt_columns.rb
@@ -5,15 +5,17 @@ module Gitlab
     # EncryptColumn migrates data from an unencrypted column - `foo`, say - to
     # an encrypted column - `encrypted_foo`, say.
     #
+    # 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.
+    #
     # 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).
+    # shadowed), unless you want to define specific methods / accessors in the
+    # temporary model in `/models/encrypt_columns/your_model.rb`.
     #
-    # 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)
diff --git a/lib/gitlab/background_migration/models/encrypt_columns/namespace.rb b/lib/gitlab/background_migration/models/encrypt_columns/namespace.rb
new file mode 100644
index 00000000000..41f18979d76
--- /dev/null
+++ b/lib/gitlab/background_migration/models/encrypt_columns/namespace.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 `runners_token` column in `namespaces` table.
+        #
+        class Namespace < ActiveRecord::Base
+          include ::EachBatch
+
+          self.table_name = 'namespaces'
+          self.inheritance_column = :_type_disabled
+
+          def runners_token=(value)
+            self.runners_token_encrypted =
+              ::Gitlab::CryptoHelper.aes256_gcm_encrypt(value)
+          end
+
+          def self.encrypted_attributes
+            { runners_token: { attribute: :runners_token_encrypted } }
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/background_migration/models/encrypt_columns/project.rb b/lib/gitlab/background_migration/models/encrypt_columns/project.rb
new file mode 100644
index 00000000000..bfeae14584d
--- /dev/null
+++ b/lib/gitlab/background_migration/models/encrypt_columns/project.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 `runners_token` column in `projects` table.
+        #
+        class Project < ActiveRecord::Base
+          include ::EachBatch
+
+          self.table_name = 'projects'
+          self.inheritance_column = :_type_disabled
+
+          def runners_token=(value)
+            self.runners_token_encrypted =
+              ::Gitlab::CryptoHelper.aes256_gcm_encrypt(value)
+          end
+
+          def self.encrypted_attributes
+            { runners_token: { attribute: :runners_token_encrypted } }
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/background_migration/models/encrypt_columns/runner.rb b/lib/gitlab/background_migration/models/encrypt_columns/runner.rb
new file mode 100644
index 00000000000..425f9f6c346
--- /dev/null
+++ b/lib/gitlab/background_migration/models/encrypt_columns/runner.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` column in `ci_runners` table.
+        #
+        class Runner < ActiveRecord::Base
+          include ::EachBatch
+
+          self.table_name = 'ci_runners'
+          self.inheritance_column = :_type_disabled
+
+          def runners_token=(value)
+            self.token_encrypted =
+              ::Gitlab::CryptoHelper.aes256_gcm_encrypt(value)
+          end
+
+          def self.encrypted_attributes
+            { token: { attribute: :token_encrypted } }
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/lib/gitlab/background_migration/models/encrypt_columns/settings.rb b/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
new file mode 100644
index 00000000000..458f1202929
--- /dev/null
+++ b/lib/gitlab/background_migration/models/encrypt_columns/settings.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 `runners_token` column in `application_settings` table.
+        #
+        class Settings < ActiveRecord::Base
+          include ::EachBatch
+
+          self.table_name = 'application_settings'
+          self.inheritance_column = :_type_disabled
+
+          def runners_token=(value)
+            self.runners_token_encrypted =
+              ::Gitlab::CryptoHelper.aes256_gcm_encrypt(value)
+          end
+
+          def self.encrypted_attributes
+            { runners_token: { attribute: :runners_token_encrypted } }
+          end
+        end
+      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
index bb76eb8ed48..ccd9d4c6d44 100644
--- a/lib/gitlab/background_migration/models/encrypt_columns/web_hook.rb
+++ b/lib/gitlab/background_migration/models/encrypt_columns/web_hook.rb
@@ -15,12 +15,12 @@ module Gitlab
           attr_encrypted :token,
                          mode:      :per_attribute_iv,
                          algorithm: 'aes-256-gcm',
-                         key:       Settings.attr_encrypted_db_key_base_truncated
+                         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
+                         key:       ::Settings.attr_encrypted_db_key_base_truncated
         end
       end
     end
-- 
cgit v1.2.1


From c7a39ffa911f06ae60cc22ac237b6e82522a93b8 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 21 Nov 2018 12:35:40 +0100
Subject: Schedule background migration for encrypting runners tokens

---
 ...1121111200_schedule_runners_token_encryption.rb | 38 ++++++++++++++++++++++
 db/schema.rb                                       |  2 +-
 .../background_migration/encrypt_runners_tokens.rb | 20 ++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)
 create mode 100644 db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
 create mode 100644 lib/gitlab/background_migration/encrypt_runners_tokens.rb

diff --git a/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb b/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
new file mode 100644
index 00000000000..33403610d8e
--- /dev/null
+++ b/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+class ScheduleRunnersTokenEncryption < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  BATCH_SIZE = 10000
+  RANGE_SIZE = 100
+  MIGRATION = 'EncryptRunnersTokens'
+
+  MODELS = [
+    ::Gitlab::BackgroundMigration::Models::EncryptColumns::Settings,
+    ::Gitlab::BackgroundMigration::Models::EncryptColumns::Namespace,
+    ::Gitlab::BackgroundMigration::Models::EncryptColumns::Project,
+    ::Gitlab::BackgroundMigration::Models::EncryptColumns::Runner
+  ].freeze
+
+  disable_ddl_transaction!
+
+  def up
+    MODELS.each do |model|
+      model.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 = [model, model.encrypted_attributes.keys, *range]
+
+          BackgroundMigrationWorker.perform_in(delay, MIGRATION, args)
+        end
+      end
+    end
+  end
+
+  def down
+    # no-op
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index b8875837471..43415954f18 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20181121101802) do
+ActiveRecord::Schema.define(version: 20181121111200) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
diff --git a/lib/gitlab/background_migration/encrypt_runners_tokens.rb b/lib/gitlab/background_migration/encrypt_runners_tokens.rb
new file mode 100644
index 00000000000..4647301f1a9
--- /dev/null
+++ b/lib/gitlab/background_migration/encrypt_runners_tokens.rb
@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module BackgroundMigration
+    # EncryptColumn migrates data from an unencrypted column - `foo`, say - to
+    # an encrypted column - `encrypted_foo`, say.
+    #
+    # We only create a subclass here because we want to isolate this migration
+    # (migrating unencrypted runner registration tokens to encrypted columns)
+    # from other `EncryptColumns` migration. This class name is going to be
+    # serialized and stored in Redis and later picked by Sidekiq, so we need to
+    # create a separate class name in order to isolate these migration tasks.
+    #
+    # We can solve this differently, see tech debt issue:
+    #
+    # https://gitlab.com/gitlab-org/gitlab-ce/issues/54328
+    #
+    class EncryptRunnersTokens < EncryptColumns; end
+  end
+end
-- 
cgit v1.2.1


From 13cfd53dd982bcbcda53d5eb5899c3efc6e92654 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 21 Nov 2018 13:44:19 +0100
Subject: Add missing specs for crypto helper class

---
 spec/lib/gitlab/crypto_helper_spec.rb | 37 +++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 spec/lib/gitlab/crypto_helper_spec.rb

diff --git a/spec/lib/gitlab/crypto_helper_spec.rb b/spec/lib/gitlab/crypto_helper_spec.rb
new file mode 100644
index 00000000000..080d865b615
--- /dev/null
+++ b/spec/lib/gitlab/crypto_helper_spec.rb
@@ -0,0 +1,37 @@
+require 'spec_helper'
+
+describe Gitlab::CryptoHelper do
+  describe '.sha256' do
+    it 'generates SHA256 digest Base46 encoded' do
+      digest = described_class.sha256('some-value')
+
+      expect(digest).to match %r{^[A-Za-z0-9+/=]+$}
+      expect(digest).to eq digest.strip
+    end
+  end
+
+  describe '.aes256_gcm_encrypt' do
+    it 'is Base64 encoded string without new line character' do
+      encrypted = described_class.aes256_gcm_encrypt('some-value')
+
+      expect(encrypted).to match %r{^[A-Za-z0-9+/=]+$}
+      expect(encrypted).to eq encrypted.strip
+    end
+  end
+
+  describe '.aes256_gcm_decrypt' do
+    let(:encrypted) { described_class.aes256_gcm_encrypt('some-value') }
+
+    it 'correctly decrypts encrypted string' do
+      decrypted = described_class.aes256_gcm_decrypt(encrypted)
+
+      expect(decrypted).to eq 'some-value'
+    end
+
+    it 'decrypts a value when it ends with a new line character' do
+      decrypted = described_class.aes256_gcm_decrypt(encrypted + "\n")
+
+      expect(decrypted).to eq 'some-value'
+    end
+  end
+end
-- 
cgit v1.2.1


From 8a235c0c05efec1c8ee14c7454982dc2b8ca9464 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 21 Nov 2018 14:02:24 +0100
Subject: Do not append new line to Base64 encoded encrypted value

---
 lib/gitlab/crypto_helper.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/crypto_helper.rb b/lib/gitlab/crypto_helper.rb
index 68d0b5d8f8a..e85753c3a12 100644
--- a/lib/gitlab/crypto_helper.rb
+++ b/lib/gitlab/crypto_helper.rb
@@ -17,7 +17,7 @@ module Gitlab
 
     def aes256_gcm_encrypt(value)
       encrypted_token = Encryptor.encrypt(AES256_GCM_OPTIONS.merge(value: value))
-      Base64.encode64(encrypted_token)
+      Base64.strict_encode64(encrypted_token)
     end
 
     def aes256_gcm_decrypt(value)
-- 
cgit v1.2.1


From 777b6713bb473d2e09c8340ab9a96373fdbaae50 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Thu, 22 Nov 2018 15:35:49 +0100
Subject: Ensure that db encryption keys have proper bytesize

---
 config/settings.rb            |  8 ++++
 lib/gitlab/crypto_helper.rb   |  4 +-
 lib/gitlab/utils.rb           | 14 +++++++
 spec/config/settings_spec.rb  | 98 +++++++++++++++++++++++++++++++++++++++++++
 spec/lib/gitlab/utils_spec.rb | 38 +++++++++++++++++
 5 files changed, 160 insertions(+), 2 deletions(-)

diff --git a/config/settings.rb b/config/settings.rb
index 3f3481bb65d..1b94df785a7 100644
--- a/config/settings.rb
+++ b/config/settings.rb
@@ -95,6 +95,14 @@ class Settings < Settingslogic
       Gitlab::Application.secrets.db_key_base[0..31]
     end
 
+    def attr_encrypted_db_key_base_32
+      Gitlab::Utils.ensure_utf8_size(attr_encrypted_db_key_base, bytes: 32.bytes)
+    end
+
+    def attr_encrypted_db_key_base_12
+      Gitlab::Utils.ensure_utf8_size(attr_encrypted_db_key_base, bytes: 12.bytes)
+    end
+
     # This should be used for :per_attribute_salt_and_iv mode. There is no
     # need to truncate the key because the encryptor will use the salt to
     # generate a hash of the password:
diff --git a/lib/gitlab/crypto_helper.rb b/lib/gitlab/crypto_helper.rb
index e85753c3a12..87a03d9c58f 100644
--- a/lib/gitlab/crypto_helper.rb
+++ b/lib/gitlab/crypto_helper.rb
@@ -6,8 +6,8 @@ module Gitlab
 
     AES256_GCM_OPTIONS = {
       algorithm: 'aes-256-gcm',
-      key: Settings.attr_encrypted_db_key_base_truncated,
-      iv: Settings.attr_encrypted_db_key_base_truncated[0..11]
+      key: Settings.attr_encrypted_db_key_base_32,
+      iv: Settings.attr_encrypted_db_key_base_12
     }.freeze
 
     def sha256(value)
diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index 9e59137a2c0..ad2efa6b4e1 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -16,6 +16,20 @@ module Gitlab
       str.force_encoding(Encoding::UTF_8)
     end
 
+    def ensure_utf8_size(str, bytes:)
+      raise ArgumentError if str.empty? || bytes.negative?
+
+      truncated = str.each_char.each_with_object(String.new) do |char, object|
+        if object.bytesize + char.bytesize > bytes
+          break object
+        else
+          object.concat(char)
+        end
+      end
+
+      truncated + ("\0" * (bytes - truncated.bytesize))
+    end
+
     # Append path to host, making sure there's one single / in between
     def append_path(host, path)
       "#{host.to_s.sub(%r{\/+$}, '')}/#{path.to_s.sub(%r{^\/+}, '')}"
diff --git a/spec/config/settings_spec.rb b/spec/config/settings_spec.rb
index 83b2de47741..fdbd0cb8990 100644
--- a/spec/config/settings_spec.rb
+++ b/spec/config/settings_spec.rb
@@ -6,4 +6,102 @@ describe Settings do
       expect(described_class.omniauth.enabled).to be true
     end
   end
+
+  describe '.attr_encrypted_db_key_base_truncated' do
+    it 'is a string with maximum 32 bytes size' do
+      expect(described_class.attr_encrypted_db_key_base_truncated.bytesize)
+        .to be <= 32
+    end
+  end
+
+  describe '.attr_encrypted_db_key_base_12' do
+    context 'when db key base secret is less than 12 bytes' do
+      before do
+        allow(described_class)
+          .to receive(:attr_encrypted_db_key_base)
+          .and_return('a' * 10)
+      end
+
+      it 'expands db key base secret to 12 bytes' do
+        expect(described_class.attr_encrypted_db_key_base_12)
+          .to eq ('a' * 10) + ("\0" * 2)
+      end
+    end
+
+    context 'when key has multiple multi-byte UTF chars exceeding 12 bytes' do
+      before do
+        allow(described_class)
+          .to receive(:attr_encrypted_db_key_base)
+          .and_return('❤' * 18)
+      end
+
+      it 'does not use more than 32 bytes' do
+        db_key_base = described_class.attr_encrypted_db_key_base_12
+
+        expect(db_key_base).to eq ('❤' * 4)
+        expect(db_key_base.bytesize).to eq 12
+      end
+    end
+  end
+
+  describe '.attr_encrypted_db_key_base_32' do
+    context 'when db key base secret is less than 32 bytes' do
+      before do
+        allow(described_class)
+          .to receive(:attr_encrypted_db_key_base)
+          .and_return('a' * 10)
+      end
+
+      it 'expands db key base secret to 32 bytes' do
+        expanded_key_base = ('a' * 10) + ("\0" * 22)
+
+        expect(expanded_key_base.bytesize).to eq 32
+        expect(described_class.attr_encrypted_db_key_base_32)
+          .to eq expanded_key_base
+      end
+    end
+
+    context 'when db key base secret is 32 bytes' do
+      before do
+        allow(described_class)
+          .to receive(:attr_encrypted_db_key_base)
+          .and_return('a' * 32)
+      end
+
+      it 'returns original value' do
+        expect(described_class.attr_encrypted_db_key_base_32)
+          .to eq 'a' * 32
+      end
+    end
+
+    context 'when db key base contains multi-byte UTF character' do
+      before do
+        allow(described_class)
+          .to receive(:attr_encrypted_db_key_base)
+          .and_return('❤' * 6)
+      end
+
+      it 'does not use more than 32 bytes' do
+        db_key_base = described_class.attr_encrypted_db_key_base_32
+
+        expect(db_key_base).to eq '❤❤❤❤❤❤' + ("\0" * 14)
+        expect(db_key_base.bytesize).to eq 32
+      end
+    end
+
+    context 'when db key base multi-byte UTF chars exceeding 32 bytes' do
+      before do
+        allow(described_class)
+          .to receive(:attr_encrypted_db_key_base)
+          .and_return('❤' * 18)
+      end
+
+      it 'does not use more than 32 bytes' do
+        db_key_base = described_class.attr_encrypted_db_key_base_32
+
+        expect(db_key_base).to eq ('❤' * 10) + ("\0" * 2)
+        expect(db_key_base.bytesize).to eq 32
+      end
+    end
+  end
 end
diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb
index ad2c9d7f2af..9927ad41108 100644
--- a/spec/lib/gitlab/utils_spec.rb
+++ b/spec/lib/gitlab/utils_spec.rb
@@ -127,4 +127,42 @@ describe Gitlab::Utils do
       end
     end
   end
+
+  describe '.ensure_utf8_size' do
+    context 'string is has less bytes than expected' do
+      it 'backfills string with null characters' do
+        transformed = described_class.ensure_utf8_size('a' * 10, bytes: 32)
+
+        expect(transformed.bytesize).to eq 32
+        expect(transformed).to eq ('a' * 10) + ("\0" * 22)
+      end
+    end
+
+    context 'string size is exactly the one that is expected' do
+      it 'returns original value' do
+        transformed = described_class.ensure_utf8_size('a' * 32, bytes: 32)
+
+        expect(transformed).to eq 'a' * 32
+        expect(transformed.bytesize).to eq 32
+      end
+    end
+
+    context 'when string contains a few multi-byte UTF characters' do
+      it 'backfills string with null characters' do
+        transformed = described_class.ensure_utf8_size('❤' * 6, bytes: 32)
+
+        expect(transformed).to eq '❤❤❤❤❤❤' + ("\0" * 14)
+        expect(transformed.bytesize).to eq 32
+      end
+    end
+
+    context 'when string has multiple multi-byte UTF chars exceeding 32 bytes' do
+      it 'truncates string to 32 characters and backfills it if needed' do
+        transformed = described_class.ensure_utf8_size('❤' * 18, bytes: 32)
+
+        expect(transformed).to eq ('❤' * 10) + ("\0" * 2)
+        expect(transformed.bytesize).to eq 32
+      end
+    end
+  end
 end
-- 
cgit v1.2.1


From d1311119faeedaf163cfba7020dbcab33fb9308f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 10:03:43 +0100
Subject: Fix static analysis in utf8 helper methods

---
 lib/gitlab/utils.rb           | 2 +-
 spec/config/settings_spec.rb  | 6 +++---
 spec/lib/gitlab/utils_spec.rb | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index ad2efa6b4e1..41ed1dc5605 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -19,7 +19,7 @@ module Gitlab
     def ensure_utf8_size(str, bytes:)
       raise ArgumentError if str.empty? || bytes.negative?
 
-      truncated = str.each_char.each_with_object(String.new) do |char, object|
+      truncated = str.each_char.each_with_object(+'') do |char, object|
         if object.bytesize + char.bytesize > bytes
           break object
         else
diff --git a/spec/config/settings_spec.rb b/spec/config/settings_spec.rb
index fdbd0cb8990..f579ecacd3a 100644
--- a/spec/config/settings_spec.rb
+++ b/spec/config/settings_spec.rb
@@ -24,7 +24,7 @@ describe Settings do
 
       it 'expands db key base secret to 12 bytes' do
         expect(described_class.attr_encrypted_db_key_base_12)
-          .to eq ('a' * 10) + ("\0" * 2)
+          .to eq(('a' * 10) + ("\0" * 2))
       end
     end
 
@@ -38,7 +38,7 @@ describe Settings do
       it 'does not use more than 32 bytes' do
         db_key_base = described_class.attr_encrypted_db_key_base_12
 
-        expect(db_key_base).to eq ('❤' * 4)
+        expect(db_key_base).to eq('❤' * 4)
         expect(db_key_base.bytesize).to eq 12
       end
     end
@@ -99,7 +99,7 @@ describe Settings do
       it 'does not use more than 32 bytes' do
         db_key_base = described_class.attr_encrypted_db_key_base_32
 
-        expect(db_key_base).to eq ('❤' * 10) + ("\0" * 2)
+        expect(db_key_base).to eq(('❤' * 10) + ("\0" * 2))
         expect(db_key_base.bytesize).to eq 32
       end
     end
diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb
index 9927ad41108..7bf724bed6b 100644
--- a/spec/lib/gitlab/utils_spec.rb
+++ b/spec/lib/gitlab/utils_spec.rb
@@ -134,7 +134,7 @@ describe Gitlab::Utils do
         transformed = described_class.ensure_utf8_size('a' * 10, bytes: 32)
 
         expect(transformed.bytesize).to eq 32
-        expect(transformed).to eq ('a' * 10) + ("\0" * 22)
+        expect(transformed).to eq(('a' * 10) + ("\0" * 22))
       end
     end
 
@@ -160,7 +160,7 @@ describe Gitlab::Utils do
       it 'truncates string to 32 characters and backfills it if needed' do
         transformed = described_class.ensure_utf8_size('❤' * 18, bytes: 32)
 
-        expect(transformed).to eq ('❤' * 10) + ("\0" * 2)
+        expect(transformed).to eq(('❤' * 10) + ("\0" * 2))
         expect(transformed.bytesize).to eq 32
       end
     end
-- 
cgit v1.2.1


From d31a3873f4e25697bb65a00cb9cf7cdd0bdb6a5f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 10:12:09 +0100
Subject: Improve token strategy fallback and add more specs

---
 .../token_authenticatable_strategies/base.rb       |  4 ++++
 .../token_authenticatable_strategies/base_spec.rb  | 26 ++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index 1435d6a5751..c2c644558c0 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -40,6 +40,10 @@ module TokenAuthenticatableStrategies
     end
 
     def fallback?
+      unless options[:fallback].in?([true, false, nil])
+        raise ArgumentError, 'fallback: needs to be a boolean value!'
+      end
+
       options[:fallback] == true
     end
 
diff --git a/spec/models/concerns/token_authenticatable_strategies/base_spec.rb b/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
index 80df9c198fd..acf5c656ea9 100644
--- a/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
@@ -29,4 +29,30 @@ describe TokenAuthenticatableStrategies::Base do
       end
     end
   end
+
+  describe '#fallback?' do
+    context 'when fallback is set' do
+      it 'recognizes fallback setting' do
+        strategy = described_class.new(instance, field, fallback: true)
+
+        expect(strategy.fallback?).to be true
+      end
+    end
+
+    context 'when fallback is not a valid value' do
+      it 'raises an error' do
+        strategy = described_class.new(instance, field, fallback: 'something')
+
+        expect { strategy.fallback? }.to raise_error ArgumentError
+      end
+    end
+
+    context 'when fallback is not set' do
+      it 'raises an error' do
+        strategy = described_class.new(instance, field, {})
+
+        expect(strategy.fallback?).to eq false
+      end
+    end
+  end
 end
-- 
cgit v1.2.1


From 636b038e01c7064c6d1a88359f0370dbefc323e1 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 10:25:36 +0100
Subject: Only allow valid options when configuring tokens

---
 app/models/concerns/token_authenticatable_strategies/base.rb       | 4 ++++
 spec/models/concerns/token_authenticatable_strategies/base_spec.rb | 7 +++++++
 2 files changed, 11 insertions(+)

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index c2c644558c0..23ee34962c7 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -48,6 +48,10 @@ module TokenAuthenticatableStrategies
     end
 
     def self.fabricate(instance, field, options)
+      if options[:digest] && options[:encrypted]
+        raise ArgumentError, 'Incompatible options set!'
+      end
+
       if options[:digest]
         TokenAuthenticatableStrategies::Digest.new(instance, field, options)
       elsif options[:encrypted]
diff --git a/spec/models/concerns/token_authenticatable_strategies/base_spec.rb b/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
index acf5c656ea9..6605f1f5a5f 100644
--- a/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
@@ -28,6 +28,13 @@ describe TokenAuthenticatableStrategies::Base do
         expect(strategy).to be_a TokenAuthenticatableStrategies::Insecure
       end
     end
+
+    context 'when incompatible options are provided' do
+      it 'raises an error' do
+        expect { described_class.fabricate(instance, field, digest: true, encrypted: true) }
+          .to raise_error ArgumentError
+      end
+    end
   end
 
   describe '#fallback?' do
-- 
cgit v1.2.1


From 718ea942dc1b2ef749bf852a19a86f0928e4b36d Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 10:43:47 +0100
Subject: Improve test cases description in token-related specs

---
 .../concerns/token_authenticatable_strategies/encrypted.rb   |  8 +++-----
 .../background_migration/models/encrypt_columns/runner.rb    |  2 +-
 spec/models/ci/build_spec.rb                                 |  4 ++--
 .../token_authenticatable_strategies/encrypted_spec.rb       | 12 ++++++------
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 3d23eed164e..8e052a3ef68 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -15,8 +15,6 @@ module TokenAuthenticatableStrategies
       end
 
       token_authenticatable
-    rescue ActiveRecord::StatementInvalid
-      nil
     end
 
     def ensure_token(instance)
@@ -38,8 +36,8 @@ module TokenAuthenticatableStrategies
     end
 
     def get_token(instance)
-      raw_token = instance.read_attribute(encrypted_field)
-      token = Gitlab::CryptoHelper.aes256_gcm_decrypt(raw_token)
+      encrypted_token = instance.read_attribute(encrypted_field)
+      token = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
 
       token || (fallback_strategy.get_token(instance) if fallback?)
     end
@@ -61,7 +59,7 @@ module TokenAuthenticatableStrategies
 
     def token_set?(instance)
       raw_token = instance.read_attribute(encrypted_field)
-      raw_token ||= (instance.read_attribute(token_field) if fallback?)
+      raw_token ||= (fallback_strategy.get_token(instance) if fallback?)
 
       raw_token.present?
     end
diff --git a/lib/gitlab/background_migration/models/encrypt_columns/runner.rb b/lib/gitlab/background_migration/models/encrypt_columns/runner.rb
index 425f9f6c346..14ddce4b147 100644
--- a/lib/gitlab/background_migration/models/encrypt_columns/runner.rb
+++ b/lib/gitlab/background_migration/models/encrypt_columns/runner.rb
@@ -13,7 +13,7 @@ module Gitlab
           self.table_name = 'ci_runners'
           self.inheritance_column = :_type_disabled
 
-          def runners_token=(value)
+          def token=(value)
             self.token_encrypted =
               ::Gitlab::CryptoHelper.aes256_gcm_encrypt(value)
           end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 49b3d6ad959..4cdcae5f670 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -771,13 +771,13 @@ describe Ci::Build do
     context 'hide runners token' do
       let(:data) { "new #{project.runners_token} data"}
 
-      it { is_expected.to match(/^new [x]+ data$/) }
+      it { is_expected.to match(/^new x+ data$/) }
     end
 
     context 'hide build token' do
       let(:data) { "new #{build.token} data"}
 
-      it { is_expected.to match(/^new [x]+ data$/) }
+      it { is_expected.to match(/^new x+ data$/) }
     end
   end
 
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index cbf7bf085ef..4c074470f63 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -14,7 +14,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#find_token_authenticatable' do
-    it 'finds a relevant resource by encrypted value' do
+    it 'finds the encrypted resource by cleartext' do
       allow(model).to receive(:find_by)
         .with('some_field_encrypted' => encrypted)
         .and_return('encrypted resource')
@@ -23,8 +23,8 @@ describe TokenAuthenticatableStrategies::Encrypted do
         .to eq 'encrypted resource'
     end
 
-    it 'uses fallback strategy when token can not be found' do
-      allow_any_instance_of(TokenAuthenticatableStrategies::Insecure)
+    it 'uses fallback strategy when encrypted token cannot be found' do
+      allow(subject.send(:fallback_strategy))
         .to receive(:find_token_authenticatable)
         .and_return('plaintext resource')
 
@@ -38,7 +38,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#get_token' do
-    it 'decrypts a token when encrypted token is present' do
+    it 'returns decrypted token when an encrypted token is present' do
       allow(instance).to receive(:read_attribute)
         .with('some_field_encrypted')
         .and_return(encrypted)
@@ -46,7 +46,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
       expect(subject.get_token(instance)).to eq 'my-value'
     end
 
-    it 'reads a plaintext token when encrypted token is not present' do
+    it 'returns the plaintext token when encrypted token is not present' do
       allow(instance).to receive(:read_attribute)
         .with('some_field_encrypted')
         .and_return(nil)
@@ -60,7 +60,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#set_token' do
-    it 'writes encrypted token to a model instance and returns it' do
+    it 'writes encrypted token and removes plaintext token and returns it' do
       expect(instance).to receive(:[]=)
         .with('some_field_encrypted', encrypted)
       expect(instance).to receive(:[]=)
-- 
cgit v1.2.1


From 9ab50c86a9cc62f924509265886ce89d5ac47584 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 11:55:38 +0100
Subject: Add specs for runners tokens encryption migration

---
 .../models/encrypt_columns/settings.rb             | 10 ++-
 .../encrypt_runners_tokens_spec.rb                 | 79 ++++++++++++++++++++++
 2 files changed, 86 insertions(+), 3 deletions(-)
 create mode 100644 spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb

diff --git a/lib/gitlab/background_migration/models/encrypt_columns/settings.rb b/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
index 458f1202929..578d2ee7fbf 100644
--- a/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
+++ b/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
@@ -13,13 +13,17 @@ module Gitlab
           self.table_name = 'application_settings'
           self.inheritance_column = :_type_disabled
 
-          def runners_token=(value)
-            self.runners_token_encrypted =
+          def runners_registration_token=(value)
+            self.runners_registration_token_encrypted =
               ::Gitlab::CryptoHelper.aes256_gcm_encrypt(value)
           end
 
           def self.encrypted_attributes
-            { runners_token: { attribute: :runners_token_encrypted } }
+            {
+              runners_registration_token: {
+                attribute: :runners_registration_token_encrypted
+              }
+            }
           end
         end
       end
diff --git a/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb b/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
new file mode 100644
index 00000000000..b7f2fc73748
--- /dev/null
+++ b/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
@@ -0,0 +1,79 @@
+require 'spec_helper'
+
+describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema: 20181121111200 do
+  let(:settings) { table(:application_settings) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:runners) { table(:ci_runners) }
+
+  context 'when migrating application settings' do
+    before do
+      settings.create!(id: 1, runners_registration_token: 'plain-text-token1')
+    end
+
+    it 'migrates runners registration tokens' do
+      migrate!(:settings, :runners_registration_token, 1, 1)
+
+      encrypted_token = settings.first.runners_registration_token_encrypted
+      decrypted_token = ::Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
+
+      expect(decrypted_token).to eq 'plain-text-token1'
+      expect(settings.first.runners_registration_token).to be_nil
+    end
+  end
+
+  context 'when migrating namespaces' do
+    before do
+      namespaces.create!(id: 11, name: 'gitlab', path: 'gitlab-org', runners_token: 'my-token1')
+      namespaces.create!(id: 12, name: 'gitlab', path: 'gitlab-org', runners_token: 'my-token2')
+      namespaces.create!(id: 22, name: 'gitlab', path: 'gitlab-org', runners_token: 'my-token3')
+    end
+
+    it 'migrates runners registration tokens' do
+      migrate!(:namespace, :runners_token, 11, 22)
+
+      expect(namespaces.all.reload).to all(
+        have_attributes(runners_token: nil, runners_token_encrypted: be_a(String))
+      )
+    end
+  end
+
+  context 'when migrating projects' do
+    before do
+      namespaces.create!(id: 11, name: 'gitlab', path: 'gitlab-org')
+      projects.create!(id: 111, namespace_id: 11, name: 'gitlab', path: 'gitlab-ce', runners_token: 'my-token1')
+      projects.create!(id: 114, namespace_id: 11, name: 'gitlab', path: 'gitlab-ce', runners_token: 'my-token2')
+      projects.create!(id: 116, namespace_id: 11, name: 'gitlab', path: 'gitlab-ce', runners_token: 'my-token3')
+    end
+
+    it 'migrates runners registration tokens' do
+      migrate!(:project, :runners_token, 111, 116)
+
+      expect(projects.all.reload).to all(
+        have_attributes(runners_token: nil, runners_token_encrypted: be_a(String))
+      )
+    end
+  end
+
+  context 'when migrating runners' do
+    before do
+      runners.create!(id: 201, runner_type: 1, token: 'plain-text-token1')
+      runners.create!(id: 202, runner_type: 1, token: 'plain-text-token2')
+      runners.create!(id: 203, runner_type: 1, token: 'plain-text-token3')
+    end
+
+    it 'migrates runners communication tokens' do
+      migrate!(:runner, :token, 201, 203)
+
+      expect(runners.all.reload).to all(
+        have_attributes(token: nil, token_encrypted: be_a(String))
+      )
+    end
+  end
+
+  def migrate!(model, attribute, from, to)
+    model = "::Gitlab::BackgroundMigration::Models::EncryptColumns::#{model.to_s.capitalize}"
+
+    subject.perform(model, [attribute], from, to)
+  end
+end
-- 
cgit v1.2.1


From 627b4833c58b7bd9ffc852d4c76175366ab4f23b Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 13:05:41 +0100
Subject: Add test case for scheduling runners tokens migration

---
 spec/lib/gitlab/crypto_helper_spec.rb              |  1 -
 .../schedule_runners_token_encryption_spec.rb      | 31 ++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)
 create mode 100644 spec/migrations/schedule_runners_token_encryption_spec.rb

diff --git a/spec/lib/gitlab/crypto_helper_spec.rb b/spec/lib/gitlab/crypto_helper_spec.rb
index 080d865b615..bd3a38bfbaa 100644
--- a/spec/lib/gitlab/crypto_helper_spec.rb
+++ b/spec/lib/gitlab/crypto_helper_spec.rb
@@ -15,7 +15,6 @@ describe Gitlab::CryptoHelper do
       encrypted = described_class.aes256_gcm_encrypt('some-value')
 
       expect(encrypted).to match %r{^[A-Za-z0-9+/=]+$}
-      expect(encrypted).to eq encrypted.strip
     end
   end
 
diff --git a/spec/migrations/schedule_runners_token_encryption_spec.rb b/spec/migrations/schedule_runners_token_encryption_spec.rb
new file mode 100644
index 00000000000..728220d0a7f
--- /dev/null
+++ b/spec/migrations/schedule_runners_token_encryption_spec.rb
@@ -0,0 +1,31 @@
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20181121111200_schedule_runners_token_encryption')
+
+describe ScheduleRunnersTokenEncryption, :migration do
+  let(:settings) { table(:application_settings) }
+  let(:namespaces) { table(:namespaces) }
+  let(:projects) { table(:projects) }
+  let(:runners) { table(:ci_runners) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+
+    settings.create!(id: 1, runners_registration_token: 'plain-text-token1')
+    namespaces.create!(id: 11, name: 'gitlab', path: 'gitlab-org', runners_token: 'my-token1')
+    namespaces.create!(id: 12, name: 'gitlab', path: 'gitlab-org', runners_token: 'my-token2')
+    projects.create!(id: 111, namespace_id: 11, name: 'gitlab', path: 'gitlab-ce', runners_token: 'my-token1')
+    projects.create!(id: 114, namespace_id: 11, name: 'gitlab', path: 'gitlab-ce', runners_token: 'my-token2')
+    runners.create!(id: 201, runner_type: 1, token: 'plain-text-token1')
+    runners.create!(id: 202, runner_type: 1, token: 'plain-text-token2')
+  end
+
+  it 'schedules runners token encryption migration for multiple resources' do
+    Sidekiq::Testing.fake! do
+      Timecop.freeze do
+        migrate!
+
+        expect(BackgroundMigrationWorker.jobs.size).to eq 7
+      end
+    end
+  end
+end
-- 
cgit v1.2.1


From 1143411ae848fade4050d3d8319ba5ed88a1b1da Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 13:28:29 +0100
Subject: Reduce Sidekiq signature of scheduled tokens migration

---
 .../20181121111200_schedule_runners_token_encryption.rb    |  2 +-
 lib/gitlab/background_migration/encrypt_runners_tokens.rb  | 10 +++++++++-
 .../background_migration/encrypt_runners_tokens_spec.rb    | 14 ++++++--------
 spec/migrations/schedule_runners_token_encryption_spec.rb  |  7 +++++++
 4 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb b/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
index 33403610d8e..3a59217f07a 100644
--- a/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
+++ b/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
@@ -24,7 +24,7 @@ class ScheduleRunnersTokenEncryption < ActiveRecord::Migration
 
         relation.each_batch(of: RANGE_SIZE) do |relation|
           range = relation.pluck('MIN(id)', 'MAX(id)').first
-          args = [model, model.encrypted_attributes.keys, *range]
+          args = [model.name.demodulize.downcase, *range]
 
           BackgroundMigrationWorker.perform_in(delay, MIGRATION, args)
         end
diff --git a/lib/gitlab/background_migration/encrypt_runners_tokens.rb b/lib/gitlab/background_migration/encrypt_runners_tokens.rb
index 4647301f1a9..cb7a4c4d52e 100644
--- a/lib/gitlab/background_migration/encrypt_runners_tokens.rb
+++ b/lib/gitlab/background_migration/encrypt_runners_tokens.rb
@@ -15,6 +15,14 @@ module Gitlab
     #
     # https://gitlab.com/gitlab-org/gitlab-ce/issues/54328
     #
-    class EncryptRunnersTokens < EncryptColumns; end
+    class EncryptRunnersTokens < EncryptColumns
+      def perform(model, from, to)
+        resource = "::Gitlab::BackgroundMigration::Models::EncryptColumns::#{model.to_s.capitalize}"
+        model = resource.constantize
+        attributes = model.encrypted_attributes.keys
+
+        super(model, attributes, from, to)
+      end
+    end
   end
 end
diff --git a/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb b/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
index b7f2fc73748..fc95f51a822 100644
--- a/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
+++ b/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
@@ -12,7 +12,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
     end
 
     it 'migrates runners registration tokens' do
-      migrate!(:settings, :runners_registration_token, 1, 1)
+      migrate!(:settings, 1, 1)
 
       encrypted_token = settings.first.runners_registration_token_encrypted
       decrypted_token = ::Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
@@ -30,7 +30,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
     end
 
     it 'migrates runners registration tokens' do
-      migrate!(:namespace, :runners_token, 11, 22)
+      migrate!(:namespace, 11, 22)
 
       expect(namespaces.all.reload).to all(
         have_attributes(runners_token: nil, runners_token_encrypted: be_a(String))
@@ -47,7 +47,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
     end
 
     it 'migrates runners registration tokens' do
-      migrate!(:project, :runners_token, 111, 116)
+      migrate!(:project, 111, 116)
 
       expect(projects.all.reload).to all(
         have_attributes(runners_token: nil, runners_token_encrypted: be_a(String))
@@ -63,7 +63,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
     end
 
     it 'migrates runners communication tokens' do
-      migrate!(:runner, :token, 201, 203)
+      migrate!(:runner, 201, 203)
 
       expect(runners.all.reload).to all(
         have_attributes(token: nil, token_encrypted: be_a(String))
@@ -71,9 +71,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
     end
   end
 
-  def migrate!(model, attribute, from, to)
-    model = "::Gitlab::BackgroundMigration::Models::EncryptColumns::#{model.to_s.capitalize}"
-
-    subject.perform(model, [attribute], from, to)
+  def migrate!(model, from, to)
+    subject.perform(model, from, to)
   end
 end
diff --git a/spec/migrations/schedule_runners_token_encryption_spec.rb b/spec/migrations/schedule_runners_token_encryption_spec.rb
index 728220d0a7f..a1e4d36de1c 100644
--- a/spec/migrations/schedule_runners_token_encryption_spec.rb
+++ b/spec/migrations/schedule_runners_token_encryption_spec.rb
@@ -24,6 +24,13 @@ describe ScheduleRunnersTokenEncryption, :migration do
       Timecop.freeze do
         migrate!
 
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'settings', 1, 1)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'namespace', 11, 11)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'namespace', 12, 12)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'project', 111, 111)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'project', 114, 114)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'runner', 201, 201)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'runner', 202, 202)
         expect(BackgroundMigrationWorker.jobs.size).to eq 7
       end
     end
-- 
cgit v1.2.1


From 3dfbfa4e4f2ce962660dc534ac7a8c670049b506 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Fri, 23 Nov 2018 13:37:32 +0100
Subject: Adjust batch size for migrating runners token

---
 .../20181121111200_schedule_runners_token_encryption.rb    |  4 ++--
 spec/migrations/schedule_runners_token_encryption_spec.rb  | 14 +++++++-------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb b/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
index 3a59217f07a..753e052f7a7 100644
--- a/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
+++ b/db/post_migrate/20181121111200_schedule_runners_token_encryption.rb
@@ -5,7 +5,7 @@ class ScheduleRunnersTokenEncryption < ActiveRecord::Migration
 
   DOWNTIME = false
   BATCH_SIZE = 10000
-  RANGE_SIZE = 100
+  RANGE_SIZE = 2000
   MIGRATION = 'EncryptRunnersTokens'
 
   MODELS = [
@@ -20,7 +20,7 @@ class ScheduleRunnersTokenEncryption < ActiveRecord::Migration
   def up
     MODELS.each do |model|
       model.each_batch(of: BATCH_SIZE) do |relation, index|
-        delay = index * 2.minutes
+        delay = index * 4.minutes
 
         relation.each_batch(of: RANGE_SIZE) do |relation|
           range = relation.pluck('MIN(id)', 'MAX(id)').first
diff --git a/spec/migrations/schedule_runners_token_encryption_spec.rb b/spec/migrations/schedule_runners_token_encryption_spec.rb
index a1e4d36de1c..376d2795277 100644
--- a/spec/migrations/schedule_runners_token_encryption_spec.rb
+++ b/spec/migrations/schedule_runners_token_encryption_spec.rb
@@ -24,13 +24,13 @@ describe ScheduleRunnersTokenEncryption, :migration do
       Timecop.freeze do
         migrate!
 
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'settings', 1, 1)
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'namespace', 11, 11)
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'namespace', 12, 12)
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'project', 111, 111)
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'project', 114, 114)
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(2.minutes, 'runner', 201, 201)
-        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'runner', 202, 202)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'settings', 1, 1)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'namespace', 11, 11)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(8.minutes, 'namespace', 12, 12)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'project', 111, 111)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(8.minutes, 'project', 114, 114)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(4.minutes, 'runner', 201, 201)
+        expect(described_class::MIGRATION).to be_scheduled_delayed_migration(8.minutes, 'runner', 202, 202)
         expect(BackgroundMigrationWorker.jobs.size).to eq 7
       end
     end
-- 
cgit v1.2.1


From 37add27a00d38e4edaaec945ed9f44a123523884 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 27 Nov 2018 14:34:05 +0100
Subject: Improve token authenticable tests and exceptions

---
 app/models/concerns/token_authenticatable_strategies/base.rb      | 8 ++++----
 app/models/concerns/token_authenticatable_strategies/encrypted.rb | 2 +-
 lib/gitlab/utils.rb                                               | 3 ++-
 spec/lib/gitlab/crypto_helper_spec.rb                             | 5 +++--
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index 23ee34962c7..4c63c0dd629 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -47,17 +47,17 @@ module TokenAuthenticatableStrategies
       options[:fallback] == true
     end
 
-    def self.fabricate(instance, field, options)
+    def self.fabricate(model, field, options)
       if options[:digest] && options[:encrypted]
         raise ArgumentError, 'Incompatible options set!'
       end
 
       if options[:digest]
-        TokenAuthenticatableStrategies::Digest.new(instance, field, options)
+        TokenAuthenticatableStrategies::Digest.new(model, field, options)
       elsif options[:encrypted]
-        TokenAuthenticatableStrategies::Encrypted.new(instance, field, options)
+        TokenAuthenticatableStrategies::Encrypted.new(model, field, options)
       else
-        TokenAuthenticatableStrategies::Insecure.new(instance, field, options)
+        TokenAuthenticatableStrategies::Insecure.new(model, field, options)
       end
     end
 
diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 8e052a3ef68..c23d78b050a 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -46,7 +46,7 @@ module TokenAuthenticatableStrategies
       raise ArgumentError unless token.present?
 
       instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
-      instance[token_field] = nil
+      fallback_strategy.set_token(instance, nil) if fallback?
       token
     end
 
diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index 41ed1dc5605..96d2ed88b83 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -17,7 +17,8 @@ module Gitlab
     end
 
     def ensure_utf8_size(str, bytes:)
-      raise ArgumentError if str.empty? || bytes.negative?
+      raise ArgumentError, 'Empty string provided!' if str.empty?
+      raise ArgumentError, 'Negative string size provided!' if bytes.negative?
 
       truncated = str.each_char.each_with_object(+'') do |char, object|
         if object.bytesize + char.bytesize > bytes
diff --git a/spec/lib/gitlab/crypto_helper_spec.rb b/spec/lib/gitlab/crypto_helper_spec.rb
index bd3a38bfbaa..05cc6cf15de 100644
--- a/spec/lib/gitlab/crypto_helper_spec.rb
+++ b/spec/lib/gitlab/crypto_helper_spec.rb
@@ -5,7 +5,7 @@ describe Gitlab::CryptoHelper do
     it 'generates SHA256 digest Base46 encoded' do
       digest = described_class.sha256('some-value')
 
-      expect(digest).to match %r{^[A-Za-z0-9+/=]+$}
+      expect(digest).to match %r{\A[A-Za-z0-9+/=]+\z}
       expect(digest).to eq digest.strip
     end
   end
@@ -14,7 +14,8 @@ describe Gitlab::CryptoHelper do
     it 'is Base64 encoded string without new line character' do
       encrypted = described_class.aes256_gcm_encrypt('some-value')
 
-      expect(encrypted).to match %r{^[A-Za-z0-9+/=]+$}
+      expect(encrypted).to match %r{\A[A-Za-z0-9+/=]+\z}
+      expect(encrypted).not_to include "\n"
     end
   end
 
-- 
cgit v1.2.1


From 7d2b37bc48ca3963c241f384c8f2df6e937c74d6 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 27 Nov 2018 15:21:23 +0100
Subject: Fix resetting old token when fallback strategy is provided

---
 app/models/concerns/token_authenticatable_strategies/encrypted.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index c23d78b050a..ab64dfca820 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -46,7 +46,7 @@ module TokenAuthenticatableStrategies
       raise ArgumentError unless token.present?
 
       instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
-      fallback_strategy.set_token(instance, nil) if fallback?
+      instance[token_field] = nil if fallback?
       token
     end
 
-- 
cgit v1.2.1


From b7f35e893999b6cf1bc003747f28861be3412ab1 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 28 Nov 2018 11:43:17 +0100
Subject: Expire application settings after encrypting tokens

---
 app/models/concerns/token_authenticatable_strategies/encrypted.rb  | 3 +++
 lib/gitlab/background_migration/models/encrypt_columns/settings.rb | 5 +++++
 2 files changed, 8 insertions(+)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index ab64dfca820..c76cdc3bb90 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -25,6 +25,9 @@ module TokenAuthenticatableStrategies
       #
       # In that case we are using insecure strategy, but this should only happen
       # in tests, because otherwise `encrypted_field` is going to exist.
+      #
+      # Another use case is when we are caching resources / columns, like we do
+      # in case of ApplicationSetting.
 
       return super if instance.has_attribute?(encrypted_field)
 
diff --git a/lib/gitlab/background_migration/models/encrypt_columns/settings.rb b/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
index 578d2ee7fbf..7caed7f77aa 100644
--- a/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
+++ b/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
@@ -9,10 +9,15 @@ module Gitlab
         #
         class Settings < ActiveRecord::Base
           include ::EachBatch
+          include ::CacheableAttributes
 
           self.table_name = 'application_settings'
           self.inheritance_column = :_type_disabled
 
+          after_commit do
+            ApplicationSetting.expire
+          end
+
           def runners_registration_token=(value)
             self.runners_registration_token_encrypted =
               ::Gitlab::CryptoHelper.aes256_gcm_encrypt(value)
-- 
cgit v1.2.1


From 4c7665f2f930bba855646143684070544044de10 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 28 Nov 2018 12:00:17 +0100
Subject: Pad encryption keys with UTF-8 0 instead of \0 char

---
 lib/gitlab/utils.rb           | 2 +-
 spec/config/settings_spec.rb  | 8 ++++----
 spec/lib/gitlab/utils_spec.rb | 6 +++---
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index 96d2ed88b83..e0e8f598ba4 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -28,7 +28,7 @@ module Gitlab
         end
       end
 
-      truncated + ("\0" * (bytes - truncated.bytesize))
+      truncated + ('0' * (bytes - truncated.bytesize))
     end
 
     # Append path to host, making sure there's one single / in between
diff --git a/spec/config/settings_spec.rb b/spec/config/settings_spec.rb
index f579ecacd3a..c89b5f48dc0 100644
--- a/spec/config/settings_spec.rb
+++ b/spec/config/settings_spec.rb
@@ -24,7 +24,7 @@ describe Settings do
 
       it 'expands db key base secret to 12 bytes' do
         expect(described_class.attr_encrypted_db_key_base_12)
-          .to eq(('a' * 10) + ("\0" * 2))
+          .to eq(('a' * 10) + ('0' * 2))
       end
     end
 
@@ -53,7 +53,7 @@ describe Settings do
       end
 
       it 'expands db key base secret to 32 bytes' do
-        expanded_key_base = ('a' * 10) + ("\0" * 22)
+        expanded_key_base = ('a' * 10) + ('0' * 22)
 
         expect(expanded_key_base.bytesize).to eq 32
         expect(described_class.attr_encrypted_db_key_base_32)
@@ -84,7 +84,7 @@ describe Settings do
       it 'does not use more than 32 bytes' do
         db_key_base = described_class.attr_encrypted_db_key_base_32
 
-        expect(db_key_base).to eq '❤❤❤❤❤❤' + ("\0" * 14)
+        expect(db_key_base).to eq '❤❤❤❤❤❤' + ('0' * 14)
         expect(db_key_base.bytesize).to eq 32
       end
     end
@@ -99,7 +99,7 @@ describe Settings do
       it 'does not use more than 32 bytes' do
         db_key_base = described_class.attr_encrypted_db_key_base_32
 
-        expect(db_key_base).to eq(('❤' * 10) + ("\0" * 2))
+        expect(db_key_base).to eq(('❤' * 10) + ('0' * 2))
         expect(db_key_base.bytesize).to eq 32
       end
     end
diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb
index 7bf724bed6b..3579ed9a759 100644
--- a/spec/lib/gitlab/utils_spec.rb
+++ b/spec/lib/gitlab/utils_spec.rb
@@ -134,7 +134,7 @@ describe Gitlab::Utils do
         transformed = described_class.ensure_utf8_size('a' * 10, bytes: 32)
 
         expect(transformed.bytesize).to eq 32
-        expect(transformed).to eq(('a' * 10) + ("\0" * 22))
+        expect(transformed).to eq(('a' * 10) + ('0' * 22))
       end
     end
 
@@ -151,7 +151,7 @@ describe Gitlab::Utils do
       it 'backfills string with null characters' do
         transformed = described_class.ensure_utf8_size('❤' * 6, bytes: 32)
 
-        expect(transformed).to eq '❤❤❤❤❤❤' + ("\0" * 14)
+        expect(transformed).to eq '❤❤❤❤❤❤' + ('0' * 14)
         expect(transformed.bytesize).to eq 32
       end
     end
@@ -160,7 +160,7 @@ describe Gitlab::Utils do
       it 'truncates string to 32 characters and backfills it if needed' do
         transformed = described_class.ensure_utf8_size('❤' * 18, bytes: 32)
 
-        expect(transformed).to eq(('❤' * 10) + ("\0" * 2))
+        expect(transformed).to eq(('❤' * 10) + ('0' * 2))
         expect(transformed.bytesize).to eq 32
       end
     end
-- 
cgit v1.2.1


From 439d22b90fed46d16ebc26fd756f1459da370280 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Wed, 28 Nov 2018 13:18:10 +0100
Subject: Add changelog for runners token encryption fix

---
 changelogs/unreleased/fix-gb-encrypt-runners-tokens.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/fix-gb-encrypt-runners-tokens.yml

diff --git a/changelogs/unreleased/fix-gb-encrypt-runners-tokens.yml b/changelogs/unreleased/fix-gb-encrypt-runners-tokens.yml
new file mode 100644
index 00000000000..4ce4f96c1dd
--- /dev/null
+++ b/changelogs/unreleased/fix-gb-encrypt-runners-tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Encrypt runners tokens
+merge_request: 23412
+author:
+type: security
-- 
cgit v1.2.1


From f8dea2e214266c82541a6a1fbf34aa778d5ad216 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 3 Dec 2018 12:35:49 +0100
Subject: Implement migration strategy for token authenticatable

---
 .../token_authenticatable_strategies/base.rb       |   8 ++
 .../token_authenticatable_strategies/encrypted.rb  |  21 ++-
 .../encrypted_spec.rb                              | 154 ++++++++++++++++-----
 3 files changed, 144 insertions(+), 39 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index 4c63c0dd629..01fb194281a 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -47,6 +47,14 @@ module TokenAuthenticatableStrategies
       options[:fallback] == true
     end
 
+    def migrating?
+      unless options[:migrating].in?([true, false, nil])
+        raise ArgumentError, 'migrating: needs to be a boolean value!'
+      end
+
+      options[:migrating] == true
+    end
+
     def self.fabricate(model, field, options)
       if options[:digest] && options[:encrypted]
         raise ArgumentError, 'Incompatible options set!'
diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index c76cdc3bb90..4659109ca8f 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -2,14 +2,24 @@
 
 module TokenAuthenticatableStrategies
   class Encrypted < Base
+    def initialize(*)
+      super
+
+      if migrating? && fallback?
+        raise ArgumentError, '`fallback` and `migration` options are not compatible!'
+      end
+    end
+
     def find_token_authenticatable(token, unscoped = false)
       return unless token
 
-      encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
-      token_authenticatable = relation(unscoped)
-        .find_by(encrypted_field => encrypted_value)
+      unless migrating?
+        encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+        token_authenticatable = relation(unscoped)
+          .find_by(encrypted_field => encrypted_value)
+      end
 
-      if fallback?
+      if migrating? || fallback?
         token_authenticatable ||= fallback_strategy
           .find_token_authenticatable(token)
       end
@@ -39,6 +49,8 @@ module TokenAuthenticatableStrategies
     end
 
     def get_token(instance)
+      return fallback_strategy.get_token(instance) if migrating?
+
       encrypted_token = instance.read_attribute(encrypted_field)
       token = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
 
@@ -49,6 +61,7 @@ module TokenAuthenticatableStrategies
       raise ArgumentError unless token.present?
 
       instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+      instance[token_field] = token if migrating?
       instance[token_field] = nil if fallback?
       token
     end
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index 4c074470f63..7dced6d79eb 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -3,7 +3,6 @@ require 'spec_helper'
 describe TokenAuthenticatableStrategies::Encrypted do
   let(:model) { double(:model) }
   let(:instance) { double(:instance) }
-  let(:options) { { fallback: true } }
 
   let(:encrypted) do
     Gitlab::CryptoHelper.aes256_gcm_encrypt('my-value')
@@ -13,60 +12,145 @@ describe TokenAuthenticatableStrategies::Encrypted do
     described_class.new(model, 'some_field', options)
   end
 
+  describe '.new' do
+    context 'when fallback and migration strategies are set' do
+      let(:options) { { fallback: true, migrating: true } }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error ArgumentError, /not compatible/
+      end
+    end
+  end
+
   describe '#find_token_authenticatable' do
-    it 'finds the encrypted resource by cleartext' do
-      allow(model).to receive(:find_by)
-        .with('some_field_encrypted' => encrypted)
-        .and_return('encrypted resource')
+    context 'when using fallback strategy' do
+      let(:options) { { fallback: true } }
+
+      it 'finds the encrypted resource by cleartext' do
+        allow(model).to receive(:find_by)
+          .with('some_field_encrypted' => encrypted)
+          .and_return('encrypted resource')
+
+        expect(subject.find_token_authenticatable('my-value'))
+          .to eq 'encrypted resource'
+      end
 
-      expect(subject.find_token_authenticatable('my-value'))
-        .to eq 'encrypted resource'
+      it 'uses fallback strategy when encrypted token cannot be found' do
+        allow(subject.send(:fallback_strategy))
+          .to receive(:find_token_authenticatable)
+          .and_return('plaintext resource')
+
+        allow(model).to receive(:find_by)
+          .with('some_field_encrypted' => encrypted)
+          .and_return(nil)
+
+        expect(subject.find_token_authenticatable('my-value'))
+          .to eq 'plaintext resource'
+      end
     end
 
-    it 'uses fallback strategy when encrypted token cannot be found' do
-      allow(subject.send(:fallback_strategy))
-        .to receive(:find_token_authenticatable)
-        .and_return('plaintext resource')
+    context 'when using migration strategy' do
+      let(:options) { { migrating: true } }
+
+      it 'finds the cleartext resource by cleartext' do
+        allow(model).to receive(:find_by)
+          .with('some_field' => 'my-value')
+          .and_return('cleartext resource')
 
-      allow(model).to receive(:find_by)
-        .with('some_field_encrypted' => encrypted)
-        .and_return(nil)
+        expect(subject.find_token_authenticatable('my-value'))
+          .to eq 'cleartext resource'
+      end
 
-      expect(subject.find_token_authenticatable('my-value'))
-        .to eq 'plaintext resource'
+      it 'returns nil if resource cannot be found' do
+        allow(model).to receive(:find_by)
+          .with('some_field' => 'my-value')
+          .and_return(nil)
+
+        expect(subject.find_token_authenticatable('my-value'))
+          .to be_nil
+      end
     end
   end
 
   describe '#get_token' do
-    it 'returns decrypted token when an encrypted token is present' do
-      allow(instance).to receive(:read_attribute)
-        .with('some_field_encrypted')
-        .and_return(encrypted)
+    context 'when using fallback strategy' do
+      let(:options) { { fallback: true } }
+
+      it 'returns decrypted token when an encrypted token is present' do
+        allow(instance).to receive(:read_attribute)
+          .with('some_field_encrypted')
+          .and_return(encrypted)
+
+        expect(subject.get_token(instance)).to eq 'my-value'
+      end
 
-      expect(subject.get_token(instance)).to eq 'my-value'
+      it 'returns the plaintext token when encrypted token is not present' do
+        allow(instance).to receive(:read_attribute)
+          .with('some_field_encrypted')
+          .and_return(nil)
+
+        allow(instance).to receive(:read_attribute)
+          .with('some_field')
+          .and_return('cleartext value')
+
+        expect(subject.get_token(instance)).to eq 'cleartext value'
+      end
     end
 
-    it 'returns the plaintext token when encrypted token is not present' do
-      allow(instance).to receive(:read_attribute)
-        .with('some_field_encrypted')
-        .and_return(nil)
+    context 'when using migration strategy' do
+      let(:options) { { migrating: true } }
+
+      it 'returns cleartext token when an encrypted token is present' do
+        allow(instance).to receive(:read_attribute)
+          .with('some_field_encrypted')
+          .and_return(encrypted)
+
+        allow(instance).to receive(:read_attribute)
+          .with('some_field')
+          .and_return('my-cleartext-value')
+
+        expect(subject.get_token(instance)).to eq 'my-cleartext-value'
+      end
+
+      it 'returns the cleartext token when encrypted token is not present' do
+        allow(instance).to receive(:read_attribute)
+          .with('some_field_encrypted')
+          .and_return(nil)
 
-      allow(instance).to receive(:read_attribute)
-        .with('some_field')
-        .and_return('cleartext value')
+        allow(instance).to receive(:read_attribute)
+          .with('some_field')
+          .and_return('cleartext value')
 
-      expect(subject.get_token(instance)).to eq 'cleartext value'
+        expect(subject.get_token(instance)).to eq 'cleartext value'
+      end
     end
   end
 
   describe '#set_token' do
-    it 'writes encrypted token and removes plaintext token and returns it' do
-      expect(instance).to receive(:[]=)
-        .with('some_field_encrypted', encrypted)
-      expect(instance).to receive(:[]=)
-        .with('some_field', nil)
+    context 'when using fallback strategy' do
+      let(:options) { { fallback: true } }
+
+      it 'writes encrypted token and removes plaintext token and returns it' do
+        expect(instance).to receive(:[]=)
+          .with('some_field_encrypted', encrypted)
+        expect(instance).to receive(:[]=)
+          .with('some_field', nil)
+
+        expect(subject.set_token(instance, 'my-value')).to eq 'my-value'
+      end
+    end
+
+    context 'when using migration strategy' do
+      let(:options) { { migrating: true } }
+
+      it 'writes encrypted token and writes plaintext token' do
+        expect(instance).to receive(:[]=)
+          .with('some_field_encrypted', encrypted)
+        expect(instance).to receive(:[]=)
+          .with('some_field', 'my-value')
 
-      expect(subject.set_token(instance, 'my-value')).to eq 'my-value'
+        expect(subject.set_token(instance, 'my-value')).to eq 'my-value'
+      end
     end
   end
 end
-- 
cgit v1.2.1


From bba97f820330b4bbb5996af26b2aa3cae3ffe880 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 3 Dec 2018 13:21:36 +0100
Subject: Use proper scope when accessting application settings

---
 lib/gitlab/background_migration/models/encrypt_columns/settings.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/gitlab/background_migration/models/encrypt_columns/settings.rb b/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
index 7caed7f77aa..08ae35c0671 100644
--- a/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
+++ b/lib/gitlab/background_migration/models/encrypt_columns/settings.rb
@@ -15,7 +15,7 @@ module Gitlab
           self.inheritance_column = :_type_disabled
 
           after_commit do
-            ApplicationSetting.expire
+            ::ApplicationSetting.expire
           end
 
           def runners_registration_token=(value)
-- 
cgit v1.2.1


From 143b80bb37d9534ec0b48900d3ab3287c58f68f0 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 3 Dec 2018 13:22:24 +0100
Subject: Do not add indices for encrypted tokens in this iteration

---
 ...121101802_add_encrypted_runner_tokens_indices.rb | 21 ---------------------
 1 file changed, 21 deletions(-)
 delete mode 100644 db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb

diff --git a/db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb b/db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb
deleted file mode 100644
index a4da24501a3..00000000000
--- a/db/migrate/20181121101802_add_encrypted_runner_tokens_indices.rb
+++ /dev/null
@@ -1,21 +0,0 @@
-# frozen_string_literal: true
-
-class AddEncryptedRunnerTokensIndices < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def up
-    add_concurrent_index :namespaces, :runners_token_encrypted, unique: true
-    add_concurrent_index :projects, :runners_token_encrypted, unique: true
-    add_concurrent_index :ci_runners, :token_encrypted, unique: true
-  end
-
-  def down
-    remove_concurrent_index :ci_runners, :token_encrypted
-    remove_concurrent_index :projects, :runners_token_encrypted
-    remove_concurrent_index :namespaces, :runners_token_encrypted
-  end
-end
-- 
cgit v1.2.1


From 0f5073d9bb2dc5c7677c31605d66e2cc30f48d1f Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 3 Dec 2018 13:25:20 +0100
Subject: Migrate runners token first before fully switching

---
 app/models/ci/runner.rb | 2 +-
 app/models/group.rb     | 2 +-
 app/models/project.rb   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index a4645658c72..260348c97b2 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -10,7 +10,7 @@ module Ci
     include FromUnion
     include TokenAuthenticatable
 
-    add_authentication_token_field :token, encrypted: true, fallback: true
+    add_authentication_token_field :token, encrypted: true, migrating: true
 
     enum access_level: {
       not_protected: 0,
diff --git a/app/models/group.rb b/app/models/group.rb
index e90b28bfa02..02ddc8762af 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -55,7 +55,7 @@ class Group < Namespace
 
   validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 }
 
-  add_authentication_token_field :runners_token, encrypted: true, fallback: true
+  add_authentication_token_field :runners_token, encrypted: true, migrating: true
 
   after_create :post_create_hook
   after_destroy :post_destroy_hook
diff --git a/app/models/project.rb b/app/models/project.rb
index c6351e1c7fc..9563e145bb8 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -85,7 +85,7 @@ class Project < ActiveRecord::Base
   default_value_for :snippets_enabled, gitlab_config_features.snippets
   default_value_for :only_allow_merge_if_all_discussions_are_resolved, false
 
-  add_authentication_token_field :runners_token, encrypted: true, fallback: true
+  add_authentication_token_field :runners_token, encrypted: true, migrating: true
 
   before_validation :mark_remote_mirrors_for_removal, if: -> { RemoteMirror.table_exists? }
 
-- 
cgit v1.2.1


From fe4b5c98201a92ab74b1a0648e2d881feb306ee5 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 3 Dec 2018 13:40:45 +0100
Subject: Fix token encrypted strategy when migrating data

---
 .../token_authenticatable_strategies/encrypted.rb       |  8 +++++++-
 .../token_authenticatable_strategies/encrypted_spec.rb  | 17 +++++++++++++++++
 2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 4659109ca8f..35ee0193dc6 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -19,11 +19,17 @@ module TokenAuthenticatableStrategies
           .find_by(encrypted_field => encrypted_value)
       end
 
-      if migrating? || fallback?
+      if fallback? || migrating?
         token_authenticatable ||= fallback_strategy
           .find_token_authenticatable(token)
       end
 
+      if migrating?
+        encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+        token_authenticatable ||= relation(unscoped)
+          .find_by(encrypted_field => encrypted_value)
+      end
+
       token_authenticatable
     end
 
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index 7dced6d79eb..556182ee50e 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -66,9 +66,26 @@ describe TokenAuthenticatableStrategies::Encrypted do
           .with('some_field' => 'my-value')
           .and_return(nil)
 
+        allow(model).to receive(:find_by)
+          .with('some_field_encrypted' => encrypted)
+          .and_return(nil)
+
         expect(subject.find_token_authenticatable('my-value'))
           .to be_nil
       end
+
+      it 'finds by encrypted value if cleartext is not present' do
+        allow(model).to receive(:find_by)
+          .with('some_field' => 'my-value')
+          .and_return(nil)
+
+        allow(model).to receive(:find_by)
+          .with('some_field_encrypted' => encrypted)
+          .and_return('encrypted resource')
+
+        expect(subject.find_token_authenticatable('my-value'))
+          .to eq 'encrypted resource'
+      end
     end
   end
 
-- 
cgit v1.2.1


From e9abacedb01efdb127580dae54a6ffbe8c8c1399 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 3 Dec 2018 14:12:51 +0100
Subject: Refactor encrypted token strategy class

---
 .../token_authenticatable_strategies/encrypted.rb  | 57 ++++++++++++----------
 .../encrypted_spec.rb                              |  4 +-
 2 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 35ee0193dc6..1f752850aad 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -11,26 +11,18 @@ module TokenAuthenticatableStrategies
     end
 
     def find_token_authenticatable(token, unscoped = false)
-      return unless token
+      return if token.blank?
+      return find_by_encrypted_token(token, unscoped) if fully_encrypted?
 
-      unless migrating?
-        encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
-        token_authenticatable = relation(unscoped)
-          .find_by(encrypted_field => encrypted_value)
-      end
-
-      if fallback? || migrating?
-        token_authenticatable ||= fallback_strategy
-          .find_token_authenticatable(token)
-      end
-
-      if migrating?
-        encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
-        token_authenticatable ||= relation(unscoped)
-          .find_by(encrypted_field => encrypted_value)
+      if fallback?
+        find_by_encrypted_token(token, unscoped) ||
+          find_by_plaintext_token(token, unscoped)
+      elsif migrating?
+        find_by_plaintext_token(token, unscoped) ||
+          find_by_encrypted_token(token, unscoped)
+      else
+        raise ArgumentError, 'Unknown encryption strategy!'
       end
-
-      token_authenticatable
     end
 
     def ensure_token(instance)
@@ -47,20 +39,20 @@ module TokenAuthenticatableStrategies
 
       return super if instance.has_attribute?(encrypted_field)
 
-      if fallback?
-        fallback_strategy.ensure_token(instance)
+      if fully_encrypted?
+        raise ArgumentError, 'Using encrypted strategy when encrypted field is missing!'
       else
-        raise ArgumentError, 'No fallback defined when encrypted field is missing!'
+        insecure_strategy.ensure_token(instance)
       end
     end
 
     def get_token(instance)
-      return fallback_strategy.get_token(instance) if migrating?
+      return insecure_strategy.get_token(instance) if migrating?
 
       encrypted_token = instance.read_attribute(encrypted_field)
       token = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
 
-      token || (fallback_strategy.get_token(instance) if fallback?)
+      token || (insecure_strategy.get_token(instance) if fallback?)
     end
 
     def set_token(instance, token)
@@ -72,16 +64,29 @@ module TokenAuthenticatableStrategies
       token
     end
 
+    def fully_encrypted?
+      !migrating? && !fallback?
+    end
+
     protected
 
-    def fallback_strategy
-      @fallback_strategy ||= TokenAuthenticatableStrategies::Insecure
+    def find_by_plaintext_token(token, unscoped)
+      insecure_strategy.find_token_authenticatable(token, unscoped)
+    end
+
+    def find_by_encrypted_token(token, unscoped)
+      encrypted_value = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
+      relation(unscoped).find_by(encrypted_field => encrypted_value)
+    end
+
+    def insecure_strategy
+      @insecure_strategy ||= TokenAuthenticatableStrategies::Insecure
         .new(klass, token_field, options)
     end
 
     def token_set?(instance)
       raw_token = instance.read_attribute(encrypted_field)
-      raw_token ||= (fallback_strategy.get_token(instance) if fallback?)
+      raw_token ||= (insecure_strategy.get_token(instance) if fallback?)
 
       raw_token.present?
     end
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index 556182ee50e..f1e5810fa6a 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -35,8 +35,8 @@ describe TokenAuthenticatableStrategies::Encrypted do
           .to eq 'encrypted resource'
       end
 
-      it 'uses fallback strategy when encrypted token cannot be found' do
-        allow(subject.send(:fallback_strategy))
+      it 'uses insecure strategy when encrypted token cannot be found' do
+        allow(subject.send(:insecure_strategy))
           .to receive(:find_token_authenticatable)
           .and_return('plaintext resource')
 
-- 
cgit v1.2.1


From 239a4f72642b2df0cc057a17daadf1391332d118 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Mon, 3 Dec 2018 14:29:51 +0100
Subject: Use plaintext token when migration is not complete

---
 .../token_authenticatable_strategies/encrypted.rb       | 10 ++++++----
 lib/gitlab/background_migration/encrypt_columns.rb      |  8 +++++++-
 .../background_migration/encrypt_runners_tokens.rb      |  4 ++++
 .../background_migration/encrypt_runners_tokens_spec.rb |  8 ++++----
 .../token_authenticatable_strategies/encrypted_spec.rb  | 17 -----------------
 5 files changed, 21 insertions(+), 26 deletions(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 1f752850aad..2e65a2b6b22 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -12,16 +12,18 @@ module TokenAuthenticatableStrategies
 
     def find_token_authenticatable(token, unscoped = false)
       return if token.blank?
-      return find_by_encrypted_token(token, unscoped) if fully_encrypted?
+
+      if fully_encrypted?
+        return find_by_encrypted_token(token, unscoped)
+      end
 
       if fallback?
         find_by_encrypted_token(token, unscoped) ||
           find_by_plaintext_token(token, unscoped)
       elsif migrating?
-        find_by_plaintext_token(token, unscoped) ||
-          find_by_encrypted_token(token, unscoped)
+        find_by_plaintext_token(token, unscoped)
       else
-        raise ArgumentError, 'Unknown encryption strategy!'
+        raise ArgumentError, 'Unknown encryption phase!'
       end
     end
 
diff --git a/lib/gitlab/background_migration/encrypt_columns.rb b/lib/gitlab/background_migration/encrypt_columns.rb
index c7549da96a8..6ec021df6fb 100644
--- a/lib/gitlab/background_migration/encrypt_columns.rb
+++ b/lib/gitlab/background_migration/encrypt_columns.rb
@@ -38,6 +38,10 @@ module Gitlab
         end
       end
 
+      def clear_migrated_values?
+        true
+      end
+
       private
 
       # Build a hash of { attribute => encrypted column name }
@@ -74,7 +78,9 @@ module Gitlab
 
         if instance.changed?
           instance.save!
-          instance.update_columns(to_clear)
+          if clear_migrated_values?
+            instance.update_columns(to_clear)
+          end
         end
       end
 
diff --git a/lib/gitlab/background_migration/encrypt_runners_tokens.rb b/lib/gitlab/background_migration/encrypt_runners_tokens.rb
index cb7a4c4d52e..91e559a8765 100644
--- a/lib/gitlab/background_migration/encrypt_runners_tokens.rb
+++ b/lib/gitlab/background_migration/encrypt_runners_tokens.rb
@@ -23,6 +23,10 @@ module Gitlab
 
         super(model, attributes, from, to)
       end
+
+      def clear_migrated_values?
+        false
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb b/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
index fc95f51a822..9d4921968b3 100644
--- a/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
+++ b/spec/lib/gitlab/background_migration/encrypt_runners_tokens_spec.rb
@@ -18,7 +18,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
       decrypted_token = ::Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
 
       expect(decrypted_token).to eq 'plain-text-token1'
-      expect(settings.first.runners_registration_token).to be_nil
+      expect(settings.first.runners_registration_token).to eq 'plain-text-token1'
     end
   end
 
@@ -33,7 +33,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
       migrate!(:namespace, 11, 22)
 
       expect(namespaces.all.reload).to all(
-        have_attributes(runners_token: nil, runners_token_encrypted: be_a(String))
+        have_attributes(runners_token: be_a(String), runners_token_encrypted: be_a(String))
       )
     end
   end
@@ -50,7 +50,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
       migrate!(:project, 111, 116)
 
       expect(projects.all.reload).to all(
-        have_attributes(runners_token: nil, runners_token_encrypted: be_a(String))
+        have_attributes(runners_token: be_a(String), runners_token_encrypted: be_a(String))
       )
     end
   end
@@ -66,7 +66,7 @@ describe Gitlab::BackgroundMigration::EncryptRunnersTokens, :migration, schema:
       migrate!(:runner, 201, 203)
 
       expect(runners.all.reload).to all(
-        have_attributes(token: nil, token_encrypted: be_a(String))
+        have_attributes(token: be_a(String), token_encrypted: be_a(String))
       )
     end
   end
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index f1e5810fa6a..93cab80cb1f 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -66,26 +66,9 @@ describe TokenAuthenticatableStrategies::Encrypted do
           .with('some_field' => 'my-value')
           .and_return(nil)
 
-        allow(model).to receive(:find_by)
-          .with('some_field_encrypted' => encrypted)
-          .and_return(nil)
-
         expect(subject.find_token_authenticatable('my-value'))
           .to be_nil
       end
-
-      it 'finds by encrypted value if cleartext is not present' do
-        allow(model).to receive(:find_by)
-          .with('some_field' => 'my-value')
-          .and_return(nil)
-
-        allow(model).to receive(:find_by)
-          .with('some_field_encrypted' => encrypted)
-          .and_return('encrypted resource')
-
-        expect(subject.find_token_authenticatable('my-value'))
-          .to eq 'encrypted resource'
-      end
     end
   end
 
-- 
cgit v1.2.1


From 68780d29ad79627da94d805855254b43d354cfdf Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 4 Dec 2018 09:10:57 +0100
Subject: Fix token_set? method by checking migration status

---
 app/models/concerns/token_authenticatable_strategies/encrypted.rb | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 2e65a2b6b22..86f7f52f42c 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -88,7 +88,10 @@ module TokenAuthenticatableStrategies
 
     def token_set?(instance)
       raw_token = instance.read_attribute(encrypted_field)
-      raw_token ||= (insecure_strategy.get_token(instance) if fallback?)
+
+      unless fully_encrypted?
+        raw_token ||= insecure_strategy.get_token(instance)
+      end
 
       raw_token.present?
     end
-- 
cgit v1.2.1


From 06d3018f924093e56a1458bf1679c423743a5635 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 4 Dec 2018 10:10:04 +0100
Subject: Add a line before conditional in encrypt columns class

---
 lib/gitlab/background_migration/encrypt_columns.rb | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/gitlab/background_migration/encrypt_columns.rb b/lib/gitlab/background_migration/encrypt_columns.rb
index 6ec021df6fb..b9ad8267e37 100644
--- a/lib/gitlab/background_migration/encrypt_columns.rb
+++ b/lib/gitlab/background_migration/encrypt_columns.rb
@@ -78,6 +78,7 @@ module Gitlab
 
         if instance.changed?
           instance.save!
+
           if clear_migrated_values?
             instance.update_columns(to_clear)
           end
-- 
cgit v1.2.1


From c3d0f1294c237853b9d5a3b13f3ea7a2abb84eb4 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 4 Dec 2018 14:25:43 +0100
Subject: Remove runner token indices from db schema as well

---
 db/schema.rb | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/db/schema.rb b/db/schema.rb
index 5627d389677..d048be08d77 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -527,7 +527,6 @@ ActiveRecord::Schema.define(version: 20181126153547) do
     t.index ["locked"], name: "index_ci_runners_on_locked", using: :btree
     t.index ["runner_type"], name: "index_ci_runners_on_runner_type", using: :btree
     t.index ["token"], name: "index_ci_runners_on_token", using: :btree
-    t.index ["token_encrypted"], name: "index_ci_runners_on_token_encrypted", unique: true, using: :btree
   end
 
   create_table "ci_stages", force: :cascade do |t|
@@ -1348,7 +1347,6 @@ ActiveRecord::Schema.define(version: 20181126153547) do
     t.index ["path"], name: "index_namespaces_on_path_trigram", using: :gin, opclasses: {"path"=>"gin_trgm_ops"}
     t.index ["require_two_factor_authentication"], name: "index_namespaces_on_require_two_factor_authentication", using: :btree
     t.index ["runners_token"], name: "index_namespaces_on_runners_token", unique: true, using: :btree
-    t.index ["runners_token_encrypted"], name: "index_namespaces_on_runners_token_encrypted", unique: true, using: :btree
     t.index ["type"], name: "index_namespaces_on_type", using: :btree
   end
 
@@ -1699,7 +1697,6 @@ ActiveRecord::Schema.define(version: 20181126153547) do
     t.index ["repository_storage", "created_at"], name: "idx_project_repository_check_partial", where: "(last_repository_check_at IS NULL)", using: :btree
     t.index ["repository_storage"], name: "index_projects_on_repository_storage", using: :btree
     t.index ["runners_token"], name: "index_projects_on_runners_token", using: :btree
-    t.index ["runners_token_encrypted"], name: "index_projects_on_runners_token_encrypted", unique: true, using: :btree
     t.index ["star_count"], name: "index_projects_on_star_count", using: :btree
     t.index ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
   end
-- 
cgit v1.2.1


From a1bd34e9c04c79488dc20ad0af08b0c94bffe675 Mon Sep 17 00:00:00 2001
From: Grzegorz Bizon <grzesiek.bizon@gmail.com>
Date: Tue, 4 Dec 2018 14:32:03 +0100
Subject: Fix typo in encrypted token authenticatable strategy

---
 app/models/concerns/token_authenticatable_strategies/encrypted.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 86f7f52f42c..152491aa6e9 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
@@ -6,7 +6,7 @@ module TokenAuthenticatableStrategies
       super
 
       if migrating? && fallback?
-        raise ArgumentError, '`fallback` and `migration` options are not compatible!'
+        raise ArgumentError, '`fallback` and `migrating` options are not compatible!'
       end
     end
 
-- 
cgit v1.2.1