Commit c5f1f7f3 authored by Kamil Trzciński's avatar Kamil Trzciński Committed by Grzegorz Bizon

Use encrypted runner tokens

This makes code to support encrypted runner tokens.
This code also finished previously started encryption
process.
parent f100c9ba
...@@ -7,7 +7,7 @@ class ApplicationSetting < ActiveRecord::Base ...@@ -7,7 +7,7 @@ class ApplicationSetting < ActiveRecord::Base
include IgnorableColumn include IgnorableColumn
include ChronicDurationAttribute 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 add_authentication_token_field :health_check_access_token
DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s* # comma or semicolon, optionally surrounded by whitespace DOMAIN_LIST_SEPARATOR = %r{\s*[,;]\s* # comma or semicolon, optionally surrounded by whitespace
......
...@@ -138,7 +138,7 @@ module Ci ...@@ -138,7 +138,7 @@ module Ci
acts_as_taggable 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 :update_artifacts_size, if: :artifacts_file_changed?
before_save :ensure_token before_save :ensure_token
......
...@@ -10,7 +10,7 @@ module Ci ...@@ -10,7 +10,7 @@ module Ci
include FromUnion include FromUnion
include TokenAuthenticatable 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: { enum access_level: {
not_protected: 0, not_protected: 0,
......
...@@ -39,22 +39,6 @@ module TokenAuthenticatableStrategies ...@@ -39,22 +39,6 @@ module TokenAuthenticatableStrategies
instance.save! if Gitlab::Database.read_write? instance.save! if Gitlab::Database.read_write?
end 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) def self.fabricate(model, field, options)
if options[:digest] && options[:encrypted] if options[:digest] && options[:encrypted]
raise ArgumentError, 'Incompatible options set!' raise ArgumentError, 'Incompatible options set!'
......
...@@ -2,28 +2,18 @@ ...@@ -2,28 +2,18 @@
module TokenAuthenticatableStrategies module TokenAuthenticatableStrategies
class Encrypted < Base 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) def find_token_authenticatable(token, unscoped = false)
return if token.blank? return if token.blank?
if fully_encrypted? if required?
return find_by_encrypted_token(token, unscoped) find_by_encrypted_token(token, unscoped)
end elsif optional?
if fallback?
find_by_encrypted_token(token, unscoped) || find_by_encrypted_token(token, unscoped) ||
find_by_plaintext_token(token, unscoped) find_by_plaintext_token(token, unscoped)
elsif migrating? elsif migrating?
find_by_plaintext_token(token, unscoped) find_by_plaintext_token(token, unscoped)
else else
raise ArgumentError, 'Unknown encryption phase!' raise ArgumentError, "Unknown encryption strategy: #{encrypted_strategy}!"
end end
end end
...@@ -41,8 +31,8 @@ module TokenAuthenticatableStrategies ...@@ -41,8 +31,8 @@ module TokenAuthenticatableStrategies
return super if instance.has_attribute?(encrypted_field) return super if instance.has_attribute?(encrypted_field)
if fully_encrypted? if required?
raise ArgumentError, 'Using encrypted strategy when encrypted field is missing!' raise ArgumentError, 'Using required encryption strategy when encrypted field is missing!'
else else
insecure_strategy.ensure_token(instance) insecure_strategy.ensure_token(instance)
end end
...@@ -53,8 +43,7 @@ module TokenAuthenticatableStrategies ...@@ -53,8 +43,7 @@ module TokenAuthenticatableStrategies
encrypted_token = instance.read_attribute(encrypted_field) encrypted_token = instance.read_attribute(encrypted_field)
token = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token) token = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_token)
token || (insecure_strategy.get_token(instance) if optional?)
token || (insecure_strategy.get_token(instance) if fallback?)
end end
def set_token(instance, token) def set_token(instance, token)
...@@ -62,16 +51,35 @@ module TokenAuthenticatableStrategies ...@@ -62,16 +51,35 @@ module TokenAuthenticatableStrategies
instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token) instance[encrypted_field] = Gitlab::CryptoHelper.aes256_gcm_encrypt(token)
instance[token_field] = token if migrating? instance[token_field] = token if migrating?
instance[token_field] = nil if fallback? instance[token_field] = nil if optional?
token token
end end
def fully_encrypted? def required?
!migrating? && !fallback? encrypted_strategy == :required
end
def migrating?
encrypted_strategy == :migrating
end
def optional?
encrypted_strategy == :optional
end end
protected 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) def find_by_plaintext_token(token, unscoped)
insecure_strategy.find_token_authenticatable(token, unscoped) insecure_strategy.find_token_authenticatable(token, unscoped)
end end
...@@ -89,7 +97,7 @@ module TokenAuthenticatableStrategies ...@@ -89,7 +97,7 @@ module TokenAuthenticatableStrategies
def token_set?(instance) def token_set?(instance)
raw_token = instance.read_attribute(encrypted_field) raw_token = instance.read_attribute(encrypted_field)
unless fully_encrypted? unless required?
raw_token ||= insecure_strategy.get_token(instance) raw_token ||= insecure_strategy.get_token(instance)
end end
......
...@@ -56,7 +56,7 @@ class Group < Namespace ...@@ -56,7 +56,7 @@ class Group < Namespace
validates :two_factor_grace_period, presence: true, numericality: { greater_than_or_equal_to: 0 } 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_create :post_create_hook
after_destroy :post_destroy_hook after_destroy :post_destroy_hook
......
...@@ -85,7 +85,7 @@ class Project < ActiveRecord::Base ...@@ -85,7 +85,7 @@ class Project < ActiveRecord::Base
default_value_for :snippets_enabled, gitlab_config_features.snippets default_value_for :snippets_enabled, gitlab_config_features.snippets
default_value_for :only_allow_merge_if_all_discussions_are_resolved, false 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? } before_validation :mark_remote_mirrors_for_removal, if: -> { RemoteMirror.table_exists? }
......
---
title: Use encrypted runner tokens
merge_request: 25532
author:
type: security
# 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
# 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
...@@ -556,6 +556,7 @@ ActiveRecord::Schema.define(version: 20190301081611) do ...@@ -556,6 +556,7 @@ ActiveRecord::Schema.define(version: 20190301081611) do
t.index ["locked"], name: "index_ci_runners_on_locked", using: :btree 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 ["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"], name: "index_ci_runners_on_token", using: :btree
t.index ["token_encrypted"], name: "index_ci_runners_on_token_encrypted", using: :btree
end end
create_table "ci_stages", force: :cascade do |t| create_table "ci_stages", force: :cascade do |t|
...@@ -1383,6 +1384,7 @@ ActiveRecord::Schema.define(version: 20190301081611) do ...@@ -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 ["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 ["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"], 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 t.index ["type"], name: "index_namespaces_on_type", using: :btree
end end
...@@ -1752,6 +1754,7 @@ ActiveRecord::Schema.define(version: 20190301081611) do ...@@ -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", "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 ["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"], 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 ["star_count"], name: "index_projects_on_star_count", using: :btree
t.index ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree t.index ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
end end
......
...@@ -91,7 +91,8 @@ module Gitlab ...@@ -91,7 +91,8 @@ module Gitlab
# No need to do anything if the plaintext is nil, or an encrypted # No need to do anything if the plaintext is nil, or an encrypted
# value already exists # 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 # attr_encrypted will calculate and set the expected value for us
instance.public_send("#{plain_column}=", plaintext) # rubocop:disable GitlabSecurity/PublicSend instance.public_send("#{plain_column}=", plaintext) # rubocop:disable GitlabSecurity/PublicSend
......
...@@ -15,7 +15,7 @@ describe TokenAuthenticatableStrategies::Base do ...@@ -15,7 +15,7 @@ describe TokenAuthenticatableStrategies::Base do
context 'when encrypted strategy is specified' do context 'when encrypted strategy is specified' do
it 'fabricates encrypted strategy object' 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 expect(strategy).to be_a TokenAuthenticatableStrategies::Encrypted
end end
...@@ -23,7 +23,7 @@ describe TokenAuthenticatableStrategies::Base do ...@@ -23,7 +23,7 @@ describe TokenAuthenticatableStrategies::Base do
context 'when no strategy is specified' do context 'when no strategy is specified' do
it 'fabricates insecure strategy object' 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 expect(strategy).to be_a TokenAuthenticatableStrategies::Insecure
end end
...@@ -31,35 +31,9 @@ describe TokenAuthenticatableStrategies::Base do ...@@ -31,35 +31,9 @@ describe TokenAuthenticatableStrategies::Base do
context 'when incompatible options are provided' do context 'when incompatible options are provided' do
it 'raises an error' 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 .to raise_error ArgumentError
end end
end 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 end
...@@ -12,19 +12,9 @@ describe TokenAuthenticatableStrategies::Encrypted do ...@@ -12,19 +12,9 @@ describe TokenAuthenticatableStrategies::Encrypted do
described_class.new(model, 'some_field', options) described_class.new(model, 'some_field', options)
end 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 describe '#find_token_authenticatable' do
context 'when using fallback strategy' do context 'when using optional strategy' do
let(:options) { { fallback: true } } let(:options) { { encrypted: :optional } }
it 'finds the encrypted resource by cleartext' do it 'finds the encrypted resource by cleartext' do
allow(model).to receive(:find_by) allow(model).to receive(:find_by)
...@@ -50,7 +40,7 @@ describe TokenAuthenticatableStrategies::Encrypted do ...@@ -50,7 +40,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
end end
context 'when using migration strategy' do context 'when using migration strategy' do
let(:options) { { migrating: true } } let(:options) { { encrypted: :migrating } }
it 'finds the cleartext resource by cleartext' do it 'finds the cleartext resource by cleartext' do
allow(model).to receive(:find_by) allow(model).to receive(:find_by)
...@@ -73,8 +63,8 @@ describe TokenAuthenticatableStrategies::Encrypted do ...@@ -73,8 +63,8 @@ describe TokenAuthenticatableStrategies::Encrypted do
end end
describe '#get_token' do describe '#get_token' do
context 'when using fallback strategy' do context 'when using optional strategy' do
let(:options) { { fallback: true } } let(:options) { { encrypted: :optional } }
it 'returns decrypted token when an encrypted token is present' do it 'returns decrypted token when an encrypted token is present' do
allow(instance).to receive(:read_attribute) allow(instance).to receive(:read_attribute)
...@@ -98,7 +88,7 @@ describe TokenAuthenticatableStrategies::Encrypted do ...@@ -98,7 +88,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
end end
context 'when using migration strategy' do 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 it 'returns cleartext token when an encrypted token is present' do
allow(instance).to receive(:read_attribute) allow(instance).to receive(:read_attribute)
...@@ -127,8 +117,8 @@ describe TokenAuthenticatableStrategies::Encrypted do ...@@ -127,8 +117,8 @@ describe TokenAuthenticatableStrategies::Encrypted do
end end
describe '#set_token' do describe '#set_token' do
context 'when using fallback strategy' do context 'when using optional strategy' do
let(:options) { { fallback: true } } let(:options) { { encrypted: :optional } }
it 'writes encrypted token and removes plaintext token and returns it' do it 'writes encrypted token and removes plaintext token and returns it' do
expect(instance).to receive(:[]=) expect(instance).to receive(:[]=)
...@@ -141,7 +131,7 @@ describe TokenAuthenticatableStrategies::Encrypted do ...@@ -141,7 +131,7 @@ describe TokenAuthenticatableStrategies::Encrypted do
end end
context 'when using migration strategy' do context 'when using migration strategy' do
let(:options) { { migrating: true } } let(:options) { { encrypted: :migrating } }
it 'writes encrypted token and writes plaintext token' do it 'writes encrypted token and writes plaintext token' do
expect(instance).to receive(:[]=) expect(instance).to receive(:[]=)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment