Merge branch 'use-fallback-strategy' into 'master'

Use optional encryption strategy for Runners Tokens

Closes #54859

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

app/models/application_setting.rb

@@ -7,7 +7,7 @@ class ApplicationSetting < ActiveRecord::Base
   include IgnorableColumn
   include ChronicDurationAttribute
 
-  add_authentication_token_field :runners_registration_token, encrypted: true, fallback: true
+  add_authentication_token_field :runners_registration_token, encrypted: -> { Feature.enabled?(:application_settings_tokens_optional_encryption) ? :optional : :required }
   add_authentication_token_field :health_check_access_token
 
   DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s*     # comma or semicolon, optionally surrounded by whitespace

app/models/ci/build.rb

@@ -138,7 +138,7 @@ module Ci
 
     acts_as_taggable
 
-    add_authentication_token_field :token, encrypted: true, fallback: true
+    add_authentication_token_field :token, encrypted: :optional
 
     before_save :update_artifacts_size, if: :artifacts_file_changed?
     before_save :ensure_token

app/models/ci/runner.rb

@@ -10,7 +10,7 @@ module Ci
     include FromUnion
     include TokenAuthenticatable
 
-    add_authentication_token_field :token, encrypted: true, migrating: true
+    add_authentication_token_field :token, encrypted: -> { Feature.enabled?(:ci_runners_tokens_optional_encryption) ? :optional : :required }
 
     enum access_level: {
       not_protected: 0,

app/models/concerns/token_authenticatable_strategies/base.rb

@@ -39,22 +39,6 @@ module TokenAuthenticatableStrategies
       instance.save! if Gitlab::Database.read_write?
     end
 
-    def fallback?
-      unless options[:fallback].in?([true, false, nil])
-        raise ArgumentError, 'fallback: needs to be a boolean value!'
-      end
-
-      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!'

app/models/concerns/token_authenticatable_strategies/encrypted.rb

@@ -2,28 +2,18 @@
 
 module TokenAuthenticatableStrategies
   class Encrypted < Base
-    def initialize(*)
-      super
-
-      if migrating? && fallback?
-        raise ArgumentError, '`fallback` and `migrating` options are not compatible!'
-      end
-    end
-
     def find_token_authenticatable(token, unscoped = false)
       return if token.blank?
 
-      if fully_encrypted?
-        return find_by_encrypted_token(token, unscoped)
-      end
-
-      if fallback?
+      if required?
+        find_by_encrypted_token(token, unscoped)
+      elsif optional?
         find_by_encrypted_token(token, unscoped) ||
           find_by_plaintext_token(token, unscoped)
       elsif migrating?
         find_by_plaintext_token(token, unscoped)
       else
-        raise ArgumentError, 'Unknown encryption phase!'
+        raise ArgumentError, "Unknown encryption strategy: #{encrypted_strategy}!"
       end
     end
 
@@ -41,8 +31,8 @@ module TokenAuthenticatableStrategies
 
       return super if instance.has_attribute?(encrypted_field)
 
-      if fully_encrypted?
-        raise ArgumentError, 'Using encrypted strategy when encrypted field is missing!'
+      if required?
+        raise ArgumentError, 'Using required encryption strategy when encrypted field is missing!'
       else
         insecure_strategy.ensure_token(instance)
       end
@@ -53,8 +43,7 @@ module TokenAuthenticatableStrategies
 
       encrypted_token = instance.read_attribute(encrypted_field)
       token = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
-
-      token || (insecure_strategy.get_token(instance) if fallback?)
+      token || (insecure_strategy.get_token(instance) if optional?)
     end
 
     def set_token(instance, token)
@@ -62,16 +51,35 @@ module TokenAuthenticatableStrategies
 
       instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
       instance[token_field] = token if migrating?
-      instance[token_field] = nil if fallback?
+      instance[token_field] = nil if optional?
       token
     end
 
-    def fully_encrypted?
-      !migrating? && !fallback?
+    def required?
+      encrypted_strategy == :required
+    end
+
+    def migrating?
+      encrypted_strategy == :migrating
+    end
+
+    def optional?
+      encrypted_strategy == :optional
     end
 
     protected
 
+    def encrypted_strategy
+      value = options[:encrypted]
+      value = value.call if value.is_a?(Proc)
+
+      unless value.in?([:required, :optional, :migrating])
+        raise ArgumentError, 'encrypted: needs to be a :required, :optional or :migrating!'
+      end
+
+      value
+    end
+
     def find_by_plaintext_token(token, unscoped)
       insecure_strategy.find_token_authenticatable(token, unscoped)
     end
@@ -89,7 +97,7 @@ module TokenAuthenticatableStrategies
     def token_set?(instance)
       raw_token = instance.read_attribute(encrypted_field)
 
-      unless fully_encrypted?
+      unless required?
         raw_token ||= insecure_strategy.get_token(instance)
       end
 

app/models/group.rb

@@ -56,7 +56,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, migrating: true
+  add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:groups_tokens_optional_encryption) ? :optional : :required }
 
   after_create :post_create_hook
   after_destroy :post_destroy_hook

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, migrating: true
+  add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:projects_tokens_optional_encryption) ? :optional : :required }
 
   before_validation :mark_remote_mirrors_for_removal, if: -> { RemoteMirror.table_exists? }
 

changelogs/unreleased/use-encrypted-runner-tokens.yml

+---
+title: Use encrypted runner tokens
+merge_request: 25532
+author:
+type: security

db/migrate/20190225160300_steal_encrypt_runners_tokens.rb

+# frozen_string_literal: true
+
+class StealEncryptRunnersTokens < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  # This cleans after `EncryptRunnersTokens`
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    Gitlab::BackgroundMigration.steal('EncryptRunnersTokens')
+  end
+
+  def down
+    # no-op
+  end
+end

db/migrate/20190225160301_add_runner_tokens_indexes.rb

+# frozen_string_literal: true
+
+class AddRunnerTokensIndexes < ActiveRecord::Migration[5.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  # It seems that `ci_runners.token_encrypted` and `projects.runners_token_encrypted`
+  # are non-unique
+
+  def up
+    add_concurrent_index :ci_runners, :token_encrypted
+    add_concurrent_index :projects, :runners_token_encrypted
+    add_concurrent_index :namespaces, :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, unique: true
+  end
+end

db/schema.rb

@@ -556,6 +556,7 @@ ActiveRecord::Schema.define(version: 20190301081611) 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", using: :btree
   end
 
   create_table "ci_stages", force: :cascade do |t|
@@ -1383,6 +1384,7 @@ ActiveRecord::Schema.define(version: 20190301081611) 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
 
@@ -1752,6 +1754,7 @@ ActiveRecord::Schema.define(version: 20190301081611) 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", 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

lib/gitlab/background_migration/encrypt_columns.rb

@@ -91,7 +91,8 @@ module Gitlab
 
         # No need to do anything if the plaintext is nil, or an encrypted
         # value already exists
-        return nil unless plaintext.present? && !ciphertext.present?
+        return unless plaintext.present?
+        return if ciphertext.present?
 
         # attr_encrypted will calculate and set the expected value for us
         instance.public_send("#{plain_column}=", plaintext) # rubocop:disable GitlabSecurity/PublicSend

spec/models/concerns/token_authenticatable_strategies/base_spec.rb

@@ -15,7 +15,7 @@ describe TokenAuthenticatableStrategies::Base do
 
     context 'when encrypted strategy is specified' do
       it 'fabricates encrypted strategy object' do
-        strategy = described_class.fabricate(instance, field, encrypted: true)
+        strategy = described_class.fabricate(instance, field, encrypted: :required)
 
         expect(strategy).to be_a TokenAuthenticatableStrategies::Encrypted
       end
@@ -23,7 +23,7 @@ describe TokenAuthenticatableStrategies::Base do
 
     context 'when no strategy is specified' do
       it 'fabricates insecure strategy object' do
-        strategy = described_class.fabricate(instance, field, something: true)
+        strategy = described_class.fabricate(instance, field, something: :required)
 
         expect(strategy).to be_a TokenAuthenticatableStrategies::Insecure
       end
@@ -31,35 +31,9 @@ describe TokenAuthenticatableStrategies::Base do
 
     context 'when incompatible options are provided' do
       it 'raises an error' do
-        expect { described_class.fabricate(instance, field, digest: true, encrypted: true) }
+        expect { described_class.fabricate(instance, field, digest: true, encrypted: :required) }
           .to raise_error ArgumentError
       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

spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb

@@ -12,19 +12,9 @@ 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
-    context 'when using fallback strategy' do
-      let(:options) { { fallback: true } }
+    context 'when using optional strategy' do
+      let(:options) { { encrypted: :optional } }
 
       it 'finds the encrypted resource by cleartext' do
         allow(model).to receive(:find_by)
@@ -50,7 +40,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
     end
 
     context 'when using migration strategy' do
-      let(:options) { { migrating: true } }
+      let(:options) { { encrypted: :migrating } }
 
       it 'finds the cleartext resource by cleartext' do
         allow(model).to receive(:find_by)
@@ -73,8 +63,8 @@ describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#get_token' do
-    context 'when using fallback strategy' do
-      let(:options) { { fallback: true } }
+    context 'when using optional strategy' do
+      let(:options) { { encrypted: :optional } }
 
       it 'returns decrypted token when an encrypted token is present' do
         allow(instance).to receive(:read_attribute)
@@ -98,7 +88,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
     end
 
     context 'when using migration strategy' do
-      let(:options) { { migrating: true } }
+      let(:options) { { encrypted: :migrating } }
 
       it 'returns cleartext token when an encrypted token is present' do
         allow(instance).to receive(:read_attribute)
@@ -127,8 +117,8 @@ describe TokenAuthenticatableStrategies::Encrypted do
   end
 
   describe '#set_token' do
-    context 'when using fallback strategy' do
-      let(:options) { { fallback: true } }
+    context 'when using optional strategy' do
+      let(:options) { { encrypted: :optional } }
 
       it 'writes encrypted token and removes plaintext token and returns it' do
         expect(instance).to receive(:[]=)
@@ -141,7 +131,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
     end
 
     context 'when using migration strategy' do
-      let(:options) { { migrating: true } }
+      let(:options) { { encrypted: :migrating } }
 
       it 'writes encrypted token and writes plaintext token' do
         expect(instance).to receive(:[]=)