summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/models/application_setting.rb2
-rw-r--r--app/models/ci/build.rb2
-rw-r--r--app/models/ci/runner.rb2
-rw-r--r--app/models/concerns/token_authenticatable_strategies/base.rb16
-rw-r--r--app/models/concerns/token_authenticatable_strategies/encrypted.rb52
-rw-r--r--app/models/group.rb2
-rw-r--r--app/models/project.rb2
-rw-r--r--changelogs/unreleased/use-encrypted-runner-tokens.yml5
-rw-r--r--db/migrate/20190225160300_steal_encrypt_runners_tokens.rb19
-rw-r--r--db/migrate/20190225160301_add_runner_tokens_indexes.rb24
-rw-r--r--db/schema.rb3
-rw-r--r--lib/gitlab/background_migration/encrypt_columns.rb3
-rw-r--r--spec/models/concerns/token_authenticatable_strategies/base_spec.rb32
-rw-r--r--spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb28
14 files changed, 100 insertions, 92 deletions
diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb
index daadf9427ba..c5035797621 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, 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
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 3bfde8d0a77..f39441a1e5b 100644
--- a/app/models/ci/build.rb
+++ b/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
diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index d82e11bbb89..ce26ee168ef 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, migrating: true
+ add_authentication_token_field :token, encrypted: -> { Feature.enabled?(:ci_runners_tokens_optional_encryption) ? :optional : :required }
enum access_level: {
not_protected: 0,
diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index 01fb194281a..df14e6e4754 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/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!'
diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb
index 152491aa6e9..2c7fa2c5b3c 100644
--- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb
+++ b/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
diff --git a/app/models/group.rb b/app/models/group.rb
index 52f503404af..495bfe04499 100644
--- a/app/models/group.rb
+++ b/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
diff --git a/app/models/project.rb b/app/models/project.rb
index 00592c108db..6db88c146b4 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, 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? }
diff --git a/changelogs/unreleased/use-encrypted-runner-tokens.yml b/changelogs/unreleased/use-encrypted-runner-tokens.yml
new file mode 100644
index 00000000000..e01978557bf
--- /dev/null
+++ b/changelogs/unreleased/use-encrypted-runner-tokens.yml
@@ -0,0 +1,5 @@
+---
+title: Use encrypted runner tokens
+merge_request: 25532
+author:
+type: security
diff --git a/db/migrate/20190225160300_steal_encrypt_runners_tokens.rb b/db/migrate/20190225160300_steal_encrypt_runners_tokens.rb
new file mode 100644
index 00000000000..18c0d2a2e1b
--- /dev/null
+++ b/db/migrate/20190225160300_steal_encrypt_runners_tokens.rb
@@ -0,0 +1,19 @@
+# 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
diff --git a/db/migrate/20190225160301_add_runner_tokens_indexes.rb b/db/migrate/20190225160301_add_runner_tokens_indexes.rb
new file mode 100644
index 00000000000..3230c2809de
--- /dev/null
+++ b/db/migrate/20190225160301_add_runner_tokens_indexes.rb
@@ -0,0 +1,24 @@
+# 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
diff --git a/db/schema.rb b/db/schema.rb
index 2ddc8358433..c782524c391 100644
--- a/db/schema.rb
+++ b/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
diff --git a/lib/gitlab/background_migration/encrypt_columns.rb b/lib/gitlab/background_migration/encrypt_columns.rb
index b9ad8267e37..173543b7c25 100644
--- a/lib/gitlab/background_migration/encrypt_columns.rb
+++ b/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
diff --git a/spec/models/concerns/token_authenticatable_strategies/base_spec.rb b/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
index 6605f1f5a5f..2a0182b4294 100644
--- a/spec/models/concerns/token_authenticatable_strategies/base_spec.rb
+++ b/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
diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
index 93cab80cb1f..ca38f86c5ab 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/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(:[]=)
View Lorry Controller Status