From 696bef428fae55095e3395bfe439c7ede67c5478 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Fri, 25 Feb 2022 16:56:41 +0000 Subject: Add latest changes from gitlab-org/security/gitlab@14-8-stable-ee --- .../token_authenticatable_strategies/encrypted.rb | 46 +++++---- app/models/group.rb | 20 +++- app/models/project.rb | 18 +++- .../development/groups_runners_token_prefix.yml | 8 ++ .../development/projects_runners_token_prefix.yml | 8 ++ spec/models/concerns/token_authenticatable_spec.rb | 103 +++++++++++++++++++++ .../encrypted_spec.rb | 45 +++++++++ spec/models/group_spec.rb | 8 ++ spec/models/project_spec.rb | 12 ++- .../models/runners_token_prefix_shared_examples.rb | 35 +++++++ 10 files changed, 283 insertions(+), 20 deletions(-) create mode 100644 config/feature_flags/development/groups_runners_token_prefix.yml create mode 100644 config/feature_flags/development/projects_runners_token_prefix.yml create mode 100644 spec/support/shared_examples/models/runners_token_prefix_shared_examples.rb diff --git a/app/models/concerns/token_authenticatable_strategies/encrypted.rb b/app/models/concerns/token_authenticatable_strategies/encrypted.rb index 50a2613bb10..e957d09fbc6 100644 --- a/app/models/concerns/token_authenticatable_strategies/encrypted.rb +++ b/app/models/concerns/token_authenticatable_strategies/encrypted.rb @@ -5,16 +5,18 @@ module TokenAuthenticatableStrategies def find_token_authenticatable(token, unscoped = false) return if token.blank? - 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 strategy: %{encrypted_strategy}!") % { encrypted_strategy: encrypted_strategy } - end + instance = 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 strategy: %{encrypted_strategy}!") % { encrypted_strategy: encrypted_strategy } + end + + instance if instance && matches_prefix?(instance, token) end def ensure_token(instance) @@ -41,9 +43,7 @@ module TokenAuthenticatableStrategies def get_token(instance) return insecure_strategy.get_token(instance) if migrating? - encrypted_token = instance.read_attribute(encrypted_field) - token = EncryptionHelper.decrypt_token(encrypted_token) - token || (insecure_strategy.get_token(instance) if optional?) + get_encrypted_token(instance) end def set_token(instance, token) @@ -69,6 +69,12 @@ module TokenAuthenticatableStrategies protected + def get_encrypted_token(instance) + encrypted_token = instance.read_attribute(encrypted_field) + token = EncryptionHelper.decrypt_token(encrypted_token) + token || (insecure_strategy.get_token(instance) if optional?) + end + def encrypted_strategy value = options[:encrypted] value = value.call if value.is_a?(Proc) @@ -95,14 +101,22 @@ module TokenAuthenticatableStrategies .new(klass, token_field, options) end + def matches_prefix?(instance, token) + prefix = options[:prefix] + prefix = prefix.call(instance) if prefix.is_a?(Proc) + prefix = '' unless prefix.is_a?(String) + + token.start_with?(prefix) + end + def token_set?(instance) - raw_token = instance.read_attribute(encrypted_field) + token = get_encrypted_token(instance) unless required? - raw_token ||= insecure_strategy.get_token(instance) + token ||= insecure_strategy.get_token(instance) end - raw_token.present? + token.present? && matches_prefix?(instance, token) end def encrypted_field diff --git a/app/models/group.rb b/app/models/group.rb index 53da70f47e5..a395861fbb6 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -20,6 +20,13 @@ class Group < Namespace include ChronicDurationAttribute include RunnerTokenExpirationInterval + extend ::Gitlab::Utils::Override + + # Prefix for runners_token which can be used to invalidate existing tokens. + # The value chosen here is GR (for Gitlab Runner) combined with the rotation + # date (20220225) decimal to hex encoded. + RUNNERS_TOKEN_PREFIX = 'GR1348941' + def self.sti_name 'Group' end @@ -115,7 +122,9 @@ class Group < Namespace message: Gitlab::Regex.group_name_regex_message }, if: :name_changed? - add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:groups_tokens_optional_encryption, default_enabled: true) ? :optional : :required } + add_authentication_token_field :runners_token, + encrypted: -> { Feature.enabled?(:groups_tokens_optional_encryption, default_enabled: true) ? :optional : :required }, + prefix: ->(instance) { instance.runners_token_prefix } after_create :post_create_hook after_destroy :post_destroy_hook @@ -669,6 +678,15 @@ class Group < Namespace ensure_runners_token! end + def runners_token_prefix + Feature.enabled?(:groups_runners_token_prefix, self, default_enabled: :yaml) ? RUNNERS_TOKEN_PREFIX : '' + end + + override :format_runners_token + def format_runners_token(token) + "#{runners_token_prefix}#{token}" + end + def project_creation_level super || ::Gitlab::CurrentSettings.default_project_creation end diff --git a/app/models/project.rb b/app/models/project.rb index 512c6ac1acb..de7dd42866f 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -89,6 +89,11 @@ class Project < ApplicationRecord DEFAULT_SQUASH_COMMIT_TEMPLATE = '%{title}' + # Prefix for runners_token which can be used to invalidate existing tokens. + # The value chosen here is GR (for Gitlab Runner) combined with the rotation + # date (20220225) decimal to hex encoded. + RUNNERS_TOKEN_PREFIX = 'GR1348941' + cache_markdown_field :description, pipeline: :description default_value_for :packages_enabled, true @@ -109,7 +114,9 @@ class Project < ApplicationRecord default_value_for :autoclose_referenced_issues, true default_value_for(:ci_config_path) { Gitlab::CurrentSettings.default_ci_config_path } - add_authentication_token_field :runners_token, encrypted: -> { Feature.enabled?(:projects_tokens_optional_encryption, default_enabled: true) ? :optional : :required } + add_authentication_token_field :runners_token, + encrypted: -> { Feature.enabled?(:projects_tokens_optional_encryption, default_enabled: true) ? :optional : :required }, + prefix: ->(instance) { instance.runners_token_prefix } before_validation :mark_remote_mirrors_for_removal, if: -> { RemoteMirror.table_exists? } @@ -1873,6 +1880,15 @@ class Project < ApplicationRecord ensure_runners_token! end + def runners_token_prefix + Feature.enabled?(:projects_runners_token_prefix, self, default_enabled: :yaml) ? RUNNERS_TOKEN_PREFIX : '' + end + + override :format_runners_token + def format_runners_token(token) + "#{runners_token_prefix}#{token}" + end + def pages_deployed? pages_metadatum&.deployed? end diff --git a/config/feature_flags/development/groups_runners_token_prefix.yml b/config/feature_flags/development/groups_runners_token_prefix.yml new file mode 100644 index 00000000000..87b87266673 --- /dev/null +++ b/config/feature_flags/development/groups_runners_token_prefix.yml @@ -0,0 +1,8 @@ +--- +name: groups_runners_token_prefix +introduced_by_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353805 +milestone: '14.9' +type: development +group: group::database +default_enabled: true diff --git a/config/feature_flags/development/projects_runners_token_prefix.yml b/config/feature_flags/development/projects_runners_token_prefix.yml new file mode 100644 index 00000000000..5dd21d115f6 --- /dev/null +++ b/config/feature_flags/development/projects_runners_token_prefix.yml @@ -0,0 +1,8 @@ +--- +name: projects_runners_token_prefix +introduced_by_url: +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/353805 +milestone: '14.9' +type: development +group: group::database +default_enabled: true diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb index 2e82a12a61a..8a9e0248ed3 100644 --- a/spec/models/concerns/token_authenticatable_spec.rb +++ b/spec/models/concerns/token_authenticatable_spec.rb @@ -428,3 +428,106 @@ RSpec.describe Ci::Runner, 'TokenAuthenticatable', :freeze_time do end end end + +RSpec.shared_examples 'prefixed token rotation' do + describe "ensure_runners_token" do + subject { instance.ensure_runners_token } + + context 'token is not set' do + it 'generates a new token' do + expect(subject).to match(/^#{instance.class::RUNNERS_TOKEN_PREFIX}/) + expect(instance).not_to be_persisted + end + end + + context 'token is set, but does not match the prefix' do + before do + instance.set_runners_token('abcdef') + end + + it 'generates a new token' do + expect(subject).to match(/^#{instance.class::RUNNERS_TOKEN_PREFIX}/) + expect(instance).not_to be_persisted + end + + context 'feature flag is disabled' do + before do + flag = "#{described_class.name.downcase.pluralize}_runners_token_prefix" + stub_feature_flags(flag => false) + end + + it 'leaves the token unchanged' do + expect { subject }.not_to change(instance, :runners_token) + expect(instance).not_to be_persisted + end + end + end + + context 'token is set and matches prefix' do + before do + instance.set_runners_token(instance.class::RUNNERS_TOKEN_PREFIX + '-abcdef') + end + + it 'leaves the token unchanged' do + expect { subject }.not_to change(instance, :runners_token) + expect(instance).not_to be_persisted + end + end + end + + describe 'ensure_runners_token!' do + subject { instance.ensure_runners_token! } + + context 'token is not set' do + it 'generates a new token' do + expect(subject).to match(/^#{instance.class::RUNNERS_TOKEN_PREFIX}/) + expect(instance).to be_persisted + end + end + + context 'token is set, but does not match the prefix' do + before do + instance.set_runners_token('abcdef') + end + + it 'generates a new token' do + expect(subject).to match(/^#{instance.class::RUNNERS_TOKEN_PREFIX}/) + expect(instance).to be_persisted + end + + context 'feature flag is disabled' do + before do + flag = "#{described_class.name.downcase.pluralize}_runners_token_prefix" + stub_feature_flags(flag => false) + end + + it 'leaves the token unchanged' do + expect { subject }.not_to change(instance, :runners_token) + end + end + end + + context 'token is set and matches prefix' do + before do + instance.set_runners_token(instance.class::RUNNERS_TOKEN_PREFIX + '-abcdef') + instance.save! + end + + it 'leaves the token unchanged' do + expect { subject }.not_to change(instance, :runners_token) + end + end + end +end + +RSpec.describe Project, 'TokenAuthenticatable' do + let(:instance) { build(:project, runners_token: nil) } + + it_behaves_like 'prefixed token rotation' +end + +RSpec.describe Group, 'TokenAuthenticatable' do + let(:instance) { build(:group, runners_token: nil) } + + it_behaves_like 'prefixed token rotation' +end diff --git a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb index 1772fd0ff95..458dfb47394 100644 --- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb +++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb @@ -32,6 +32,21 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do expect(subject.find_token_authenticatable('my-value')) .to eq 'encrypted resource' end + + context 'when a prefix is required' do + let(:options) { { encrypted: :required, prefix: 'GR1348941' } } + + it 'finds the encrypted resource by cleartext' do + allow(model).to receive(:where) + .and_return(model) + allow(model).to receive(:find_by) + .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv]) + .and_return('encrypted resource') + + expect(subject.find_token_authenticatable('my-value')) + .to be_nil + end + end end context 'when encryption is optional' do @@ -62,6 +77,21 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do expect(subject.find_token_authenticatable('my-value')) .to eq 'plaintext resource' end + + context 'when a prefix is required' do + let(:options) { { encrypted: :optional, prefix: 'GR1348941' } } + + it 'finds the encrypted resource by cleartext' do + allow(model).to receive(:where) + .and_return(model) + allow(model).to receive(:find_by) + .with('some_field_encrypted' => [encrypted, encrypted_with_static_iv]) + .and_return('encrypted resource') + + expect(subject.find_token_authenticatable('my-value')) + .to be_nil + end + end end context 'when encryption is migrating' do @@ -88,6 +118,21 @@ RSpec.describe TokenAuthenticatableStrategies::Encrypted do expect(subject.find_token_authenticatable('my-value')) .to be_nil end + + context 'when a prefix is required' do + let(:options) { { encrypted: :migrating, prefix: 'GR1348941' } } + + it 'finds the encrypted resource by cleartext' do + allow(model).to receive(:where) + .and_return(model) + allow(model).to receive(:find_by) + .with('some_field' => 'my-value') + .and_return('cleartext resource') + + expect(subject.find_token_authenticatable('my-value')) + .to be_nil + end + end end end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 4bc4df02c24..db71fa4535d 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -3149,4 +3149,12 @@ RSpec.describe Group do it_behaves_like 'no effective expiration interval' end end + + describe '#runners_token' do + let_it_be(:group) { create(:group) } + + subject { group } + + it_behaves_like 'it has a prefixable runners_token', :groups_runners_token_prefix + end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index c487c87db1c..14cc4dbbea8 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -782,8 +782,8 @@ RSpec.describe Project, factory_default: :keep do end it 'does not set an random token if one provided' do - project = FactoryBot.create(:project, runners_token: 'my-token') - expect(project.runners_token).to eq('my-token') + project = FactoryBot.create(:project, runners_token: "#{Project::RUNNERS_TOKEN_PREFIX}my-token") + expect(project.runners_token).to eq("#{Project::RUNNERS_TOKEN_PREFIX}my-token") end end @@ -7997,6 +7997,14 @@ RSpec.describe Project, factory_default: :keep do end end + describe '#runners_token' do + let_it_be(:project) { create(:project) } + + subject { project } + + it_behaves_like 'it has a prefixable runners_token', :projects_runners_token_prefix + end + private def finish_job(export_job) diff --git a/spec/support/shared_examples/models/runners_token_prefix_shared_examples.rb b/spec/support/shared_examples/models/runners_token_prefix_shared_examples.rb new file mode 100644 index 00000000000..bbce67ae7b9 --- /dev/null +++ b/spec/support/shared_examples/models/runners_token_prefix_shared_examples.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +RSpec.shared_examples 'it has a prefixable runners_token' do |feature_flag| + context 'feature flag enabled' do + before do + stub_feature_flags(feature_flag => [subject]) + end + + describe '#runners_token' do + it 'has a runners_token_prefix' do + expect(subject.runners_token_prefix).not_to be_empty + end + + it 'starts with the runners_token_prefix' do + expect(subject.runners_token).to start_with(subject.runners_token_prefix) + end + end + end + + context 'feature flag disabled' do + before do + stub_feature_flags(feature_flag => false) + end + + describe '#runners_token' do + it 'does not have a runners_token_prefix' do + expect(subject.runners_token_prefix).to be_empty + end + + it 'starts with the runners_token_prefix' do + expect(subject.runners_token).to start_with(subject.runners_token_prefix) + end + end + end +end -- cgit v1.2.1