diff options
author | Kamil TrzciĆski <ayufan@ayufan.eu> | 2019-03-06 15:18:53 +0300 |
---|---|---|
committer | Grzegorz Bizon <grzegorz@gitlab.com> | 2019-03-06 15:18:53 +0300 |
commit | c5f1f7f3dbd5e7094ae3f30823d6c87b7a72121d (patch) | |
tree | 0123c4e12a3a79d69c3c791c9cc797e577f5c822 | |
parent | f100c9ba158a0ab6f4edaa1de73e107737d4a9d0 (diff) |
Use encrypted runner tokens
This makes code to support encrypted runner tokens.
This code also finished previously started encryption
process.
-rw-r--r-- | app/models/application_setting.rb | 2 | ||||
-rw-r--r-- | app/models/ci/build.rb | 2 | ||||
-rw-r--r-- | app/models/ci/runner.rb | 2 | ||||
-rw-r--r-- | app/models/concerns/token_authenticatable_strategies/base.rb | 16 | ||||
-rw-r--r-- | app/models/concerns/token_authenticatable_strategies/encrypted.rb | 52 | ||||
-rw-r--r-- | app/models/group.rb | 2 | ||||
-rw-r--r-- | app/models/project.rb | 2 | ||||
-rw-r--r-- | changelogs/unreleased/use-encrypted-runner-tokens.yml | 5 | ||||
-rw-r--r-- | db/migrate/20190225160300_steal_encrypt_runners_tokens.rb | 19 | ||||
-rw-r--r-- | db/migrate/20190225160301_add_runner_tokens_indexes.rb | 24 | ||||
-rw-r--r-- | db/schema.rb | 3 | ||||
-rw-r--r-- | lib/gitlab/background_migration/encrypt_columns.rb | 3 | ||||
-rw-r--r-- | spec/models/concerns/token_authenticatable_strategies/base_spec.rb | 32 | ||||
-rw-r--r-- | spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb | 28 |
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(:[]=) |