Merge remote-tracking branch 'security/security-mattkasa-608-incremental-token-rotation'

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

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

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? }
 
@@ -1870,6 +1877,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

config/feature_flags/development/groups_runners_token_prefix.yml

+---
+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

config/feature_flags/development/projects_runners_token_prefix.yml

+---
+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

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

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
 

spec/models/group_spec.rb

@@ -3190,4 +3190,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

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
 
@@ -8032,6 +8032,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)

spec/support/shared_examples/models/runners_token_prefix_shared_examples.rb

+# 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