From 606b14fe898a29942ff00313622aaf61ab69f10e Mon Sep 17 00:00:00 2001
From: Kyle Edwards <kyle.edwards@kitware.com>
Date: Tue, 14 Sep 2021 16:07:42 -0400
Subject: [PATCH 1/3] Add expires_at option to authenticatable tokens

Authenticatable token fields can now specify an :expires_at option with
a procedure that returns an expiration date/time for the token. If the
procedure returns nil, there is no expiration.

Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/30942
---
 app/models/concerns/token_authenticatable.rb  | 12 +++++++
 .../token_authenticatable_strategies/base.rb  | 36 ++++++++++++++++++-
 lib/api/support/token_with_expiration.rb      | 24 +++++++++++++
 .../encrypted_spec.rb                         | 10 ++++++
 4 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 lib/api/support/token_with_expiration.rb

diff --git a/app/models/concerns/token_authenticatable.rb b/app/models/concerns/token_authenticatable.rb
index 34c8630bb90fd4..f44ad8ebe90c2f 100644
--- a/app/models/concerns/token_authenticatable.rb
+++ b/app/models/concerns/token_authenticatable.rb
@@ -64,6 +64,18 @@ def add_authentication_token_field(token_field, options = {})
       mod.define_method("format_#{token_field}") do |token|
         token
       end
+
+      mod.define_method("#{token_field}_expires_at") do
+        strategy.expires_at(self)
+      end
+
+      mod.define_method("#{token_field}_expired?") do
+        strategy.expired?(self)
+      end
+
+      mod.define_method("#{token_field}_with_expiration") do
+        strategy.token_with_expiration(self)
+      end
     end
 
     def token_authenticatable_module
diff --git a/app/models/concerns/token_authenticatable_strategies/base.rb b/app/models/concerns/token_authenticatable_strategies/base.rb
index f72a41f06b1179..2cec4ab460ef70 100644
--- a/app/models/concerns/token_authenticatable_strategies/base.rb
+++ b/app/models/concerns/token_authenticatable_strategies/base.rb
@@ -7,6 +7,7 @@ class Base
     def initialize(klass, token_field, options)
       @klass = klass
       @token_field = token_field
+      @expires_at_field = "#{token_field}_expires_at"
       @options = options
     end
 
@@ -44,6 +45,25 @@ def reset_token!(instance)
       instance.save! if Gitlab::Database.read_write?
     end
 
+    def expires_at(instance)
+      instance.read_attribute(@expires_at_field)
+    end
+
+    def expired?(instance)
+      return false unless expirable? && token_expiration_enforced?
+
+      exp = expires_at(instance)
+      !!exp && Time.current > exp
+    end
+
+    def expirable?
+      !!@options[:expires_at]
+    end
+
+    def token_with_expiration(instance)
+      API::Support::TokenWithExpiration.new(self, instance)
+    end
+
     def self.fabricate(model, field, options)
       if options[:digest] && options[:encrypted]
         raise ArgumentError, _('Incompatible options set!')
@@ -64,6 +84,10 @@ def write_new_token(instance)
       new_token = generate_available_token
       formatted_token = format_token(instance, new_token)
       set_token(instance, formatted_token)
+
+      if expirable?
+        instance[@expires_at_field] = @options[:expires_at].to_proc.call(instance)
+      end
     end
 
     def unique
@@ -82,11 +106,21 @@ def generate_token
     end
 
     def relation(unscoped)
-      unscoped ? @klass.unscoped : @klass
+      unscoped ? @klass.unscoped : @klass.where(not_expired)
     end
 
     def token_set?(instance)
       raise NotImplementedError
     end
+
+    def token_expiration_enforced?
+      return true unless @options[:expiration_enforced?]
+
+      @options[:expiration_enforced?].to_proc.call(@klass)
+    end
+
+    def not_expired
+      Arel.sql("#{@expires_at_field} IS NULL OR #{@expires_at_field} >= NOW()") if expirable? && token_expiration_enforced?
+    end
   end
 end
diff --git a/lib/api/support/token_with_expiration.rb b/lib/api/support/token_with_expiration.rb
new file mode 100644
index 00000000000000..2cbd562c6081bd
--- /dev/null
+++ b/lib/api/support/token_with_expiration.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module API
+  module Support
+    class TokenWithExpiration
+      def initialize(strategy, instance)
+        @strategy = strategy
+        @instance = instance
+      end
+
+      def token
+        @strategy.get_token(@instance)
+      end
+
+      def token_expires_at
+        @strategy.expires_at(@instance)
+      end
+
+      def expirable?
+        @strategy.expirable?
+      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 b311e302a310d6..1772fd0ff95703 100644
--- a/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
+++ b/spec/models/concerns/token_authenticatable_strategies/encrypted_spec.rb
@@ -23,6 +23,8 @@
       let(:options) { { encrypted: :required } }
 
       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')
@@ -36,6 +38,8 @@
       let(:options) { { encrypted: :optional } }
 
       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')
@@ -49,6 +53,8 @@
           .to receive(:find_token_authenticatable)
           .and_return('plaintext resource')
 
+        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(nil)
@@ -62,6 +68,8 @@
       let(:options) { { encrypted: :migrating } }
 
       it 'finds the cleartext 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')
@@ -71,6 +79,8 @@
       end
 
       it 'returns nil if resource cannot be found' do
+        allow(model).to receive(:where)
+          .and_return(model)
         allow(model).to receive(:find_by)
           .with('some_field' => 'my-value')
           .and_return(nil)
-- 
GitLab


From 7596013889e20a71bfecffd36bb2adde7030f23e Mon Sep 17 00:00:00 2001
From: Kyle Edwards <kyle.edwards@kitware.com>
Date: Thu, 3 Feb 2022 13:46:10 -0500
Subject: [PATCH 2/3] Feature Flags: Add enforce_runner_token_expires_at
 feature flag

Changelog: added
Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/30942
---
 .../development/enforce_runner_token_expires_at.yml       | 8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 config/feature_flags/development/enforce_runner_token_expires_at.yml

diff --git a/config/feature_flags/development/enforce_runner_token_expires_at.yml b/config/feature_flags/development/enforce_runner_token_expires_at.yml
new file mode 100644
index 00000000000000..a1cb3bdcfdd4c2
--- /dev/null
+++ b/config/feature_flags/development/enforce_runner_token_expires_at.yml
@@ -0,0 +1,8 @@
+---
+name: enforce_runner_token_expires_at
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/78557
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/352008
+milestone: '14.8'
+type: development
+group: group::runner
+default_enabled: false
-- 
GitLab


From 7111b825b414618e21504691a3a8d43e3d4da51b Mon Sep 17 00:00:00 2001
From: Kyle Edwards <kyle.edwards@kitware.com>
Date: Mon, 10 Jan 2022 17:30:31 -0500
Subject: [PATCH 3/3] CI Runners: Enforce token expiration

Changelog: added
Issue: https://gitlab.com/gitlab-org/gitlab/-/issues/30942
---
 app/models/ci/runner.rb                       |  31 ++-
 spec/models/ci/runner_spec.rb                 | 186 +++++++++++++++++-
 .../concerns/token_authenticatable_spec.rb    | 138 +++++++++++++
 .../api/ci/runner/runners_verify_post_spec.rb |  24 +++
 4 files changed, 376 insertions(+), 3 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index 0f8a4be03a7ba7..26f2c873899f58 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -13,7 +13,7 @@ class Runner < Ci::ApplicationRecord
     include TaggableQueries
     include Presentable
 
-    add_authentication_token_field :token, encrypted: :optional
+    add_authentication_token_field :token, encrypted: :optional, expires_at: :compute_token_expiration, expiration_enforced?: :token_expiration_enforced?
 
     enum access_level: {
       not_protected: 0,
@@ -479,6 +479,21 @@ def namespace_ids
       end
     end
 
+    def compute_token_expiration
+      case runner_type
+      when 'instance_type'
+        compute_token_expiration_instance
+      when 'group_type'
+        compute_token_expiration_group
+      when 'project_type'
+        compute_token_expiration_project
+      end
+    end
+
+    def self.token_expiration_enforced?
+      Feature.enabled?(:enforce_runner_token_expires_at, default_enabled: :yaml)
+    end
+
     private
 
     EXECUTOR_NAME_TO_TYPES = {
@@ -498,6 +513,20 @@ def namespace_ids
 
     EXECUTOR_TYPE_TO_NAMES = EXECUTOR_NAME_TO_TYPES.invert.freeze
 
+    def compute_token_expiration_instance
+      return unless expiration_interval = Gitlab::CurrentSettings.runner_token_expiration_interval
+
+      expiration_interval.seconds.from_now
+    end
+
+    def compute_token_expiration_group
+      ::Group.where(id: runner_namespaces.map(&:namespace_id)).map(&:effective_runner_token_expiration_interval).compact.min&.from_now
+    end
+
+    def compute_token_expiration_project
+      Project.where(id: runner_projects.map(&:project_id)).map(&:effective_runner_token_expiration_interval).compact.min&.from_now
+    end
+
     def cleanup_runner_queue
       Gitlab::Redis::SharedState.with do |redis|
         redis.del(runner_queue_key)
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index 08bbdfd8185ec0..76e312beef8cf4 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -3,6 +3,8 @@
 require 'spec_helper'
 
 RSpec.describe Ci::Runner do
+  include StubGitlabCalls
+
   it_behaves_like 'having unique enum values'
 
   it_behaves_like 'it has loose foreign keys' do
@@ -1301,9 +1303,11 @@ def does_db_update
     end
 
     it 'supports ordering by the token expiration' do
-      runner1 = create(:ci_runner, token_expires_at: 1.year.from_now)
+      runner1 = create(:ci_runner)
+      runner1.update!(token_expires_at: 1.year.from_now)
       runner2 = create(:ci_runner)
-      runner3 = create(:ci_runner, token_expires_at: 1.month.from_now)
+      runner3 = create(:ci_runner)
+      runner3.update!(token_expires_at: 1.month.from_now)
 
       runners = described_class.order_by('token_expires_at_asc')
       expect(runners).to eq([runner3, runner1, runner2])
@@ -1522,4 +1526,182 @@ def does_db_update
       )
     end
   end
+
+  describe '#token_expires_at', :freeze_time do
+    shared_examples 'expiring token' do |interval:|
+      it 'expires' do
+        expect(runner.token_expires_at).to eq(interval.from_now)
+      end
+    end
+
+    shared_examples 'non-expiring token' do
+      it 'does not expire' do
+        expect(runner.token_expires_at).to be_nil
+      end
+    end
+
+    context 'no expiration' do
+      let(:runner) { create(:ci_runner) }
+
+      it_behaves_like 'non-expiring token'
+    end
+
+    context 'system-wide shared expiration' do
+      before do
+        stub_application_setting(runner_token_expiration_interval: 5.days.to_i)
+      end
+
+      let(:runner) { create(:ci_runner) }
+
+      it_behaves_like 'expiring token', interval: 5.days
+    end
+
+    context 'system-wide group expiration' do
+      before do
+        stub_application_setting(group_runner_token_expiration_interval: 5.days.to_i)
+      end
+
+      let(:runner) { create(:ci_runner) }
+
+      it_behaves_like 'non-expiring token'
+    end
+
+    context 'system-wide project expiration' do
+      before do
+        stub_application_setting(project_runner_token_expiration_interval: 5.days.to_i)
+      end
+
+      let(:runner) { create(:ci_runner) }
+
+      it_behaves_like 'non-expiring token'
+    end
+
+    context 'group expiration' do
+      let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 6.days.to_i) }
+      let(:group) { create(:group, namespace_settings: group_settings) }
+      let(:runner) { create(:ci_runner, :group, groups: [group]) }
+
+      it_behaves_like 'expiring token', interval: 6.days
+    end
+
+    context 'human-readable group expiration' do
+      let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval_human_readable: '7 days') }
+      let(:group) { create(:group, namespace_settings: group_settings) }
+      let(:runner) { create(:ci_runner, :group, groups: [group]) }
+
+      it_behaves_like 'expiring token', interval: 7.days
+    end
+
+    context 'project expiration' do
+      let(:project) { create(:project, runner_token_expiration_interval: 4.days.to_i).tap(&:save!) }
+      let(:runner) { create(:ci_runner, :project, projects: [project]) }
+
+      it_behaves_like 'expiring token', interval: 4.days
+    end
+
+    context 'human-readable project expiration' do
+      let(:project) { create(:project, runner_token_expiration_interval_human_readable: '5 days').tap(&:save!) }
+      let(:runner) { create(:ci_runner, :project, projects: [project]) }
+
+      it_behaves_like 'expiring token', interval: 5.days
+    end
+
+    context 'multiple projects' do
+      let(:project1) { create(:project, runner_token_expiration_interval: 8.days.to_i).tap(&:save!) }
+      let(:project2) { create(:project, runner_token_expiration_interval: 7.days.to_i).tap(&:save!) }
+      let(:project3) { create(:project, runner_token_expiration_interval: 9.days.to_i).tap(&:save!) }
+      let(:runner) { create(:ci_runner, :project, projects: [project1, project2, project3]) }
+
+      it_behaves_like 'expiring token', interval: 7.days
+    end
+
+    context 'with project runner token expiring' do
+      let_it_be(:project) { create(:project, runner_token_expiration_interval: 4.days.to_i).tap(&:save!) }
+
+      context 'project overrides system' do
+        before do
+          stub_application_setting(project_runner_token_expiration_interval: 5.days.to_i)
+        end
+
+        let(:runner) { create(:ci_runner, :project, projects: [project]) }
+
+        it_behaves_like 'expiring token', interval: 4.days
+      end
+
+      context 'system overrides project' do
+        before do
+          stub_application_setting(project_runner_token_expiration_interval: 3.days.to_i)
+        end
+
+        let(:runner) { create(:ci_runner, :project, projects: [project]) }
+
+        it_behaves_like 'expiring token', interval: 3.days
+      end
+    end
+
+    context 'with group runner token expiring' do
+      let_it_be(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 4.days.to_i) }
+      let_it_be(:group) { create(:group, namespace_settings: group_settings) }
+
+      context 'group overrides system' do
+        before do
+          stub_application_setting(group_runner_token_expiration_interval: 5.days.to_i)
+        end
+
+        let(:runner) { create(:ci_runner, :group, groups: [group]) }
+
+        it_behaves_like 'expiring token', interval: 4.days
+      end
+
+      context 'system overrides group' do
+        before do
+          stub_application_setting(group_runner_token_expiration_interval: 3.days.to_i)
+        end
+
+        let(:runner) { create(:ci_runner, :group, groups: [group]) }
+
+        it_behaves_like 'expiring token', interval: 3.days
+      end
+    end
+
+    context "with group's project runner token expiring" do
+      let_it_be(:parent_group_settings) { create(:namespace_settings, subgroup_runner_token_expiration_interval: 2.days.to_i) }
+      let_it_be(:parent_group) { create(:group, namespace_settings: parent_group_settings) }
+
+      context 'parent group overrides subgroup' do
+        let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 3.days.to_i) }
+        let(:group) { create(:group, parent: parent_group, namespace_settings: group_settings) }
+        let(:runner) { create(:ci_runner, :group, groups: [group]) }
+
+        it_behaves_like 'expiring token', interval: 2.days
+      end
+
+      context 'subgroup overrides parent group' do
+        let(:group_settings) { create(:namespace_settings, runner_token_expiration_interval: 1.day.to_i) }
+        let(:group) { create(:group, parent: parent_group, namespace_settings: group_settings) }
+        let(:runner) { create(:ci_runner, :group, groups: [group]) }
+
+        it_behaves_like 'expiring token', interval: 1.day
+      end
+    end
+
+    context "with group's project runner token expiring" do
+      let_it_be(:group_settings) { create(:namespace_settings, project_runner_token_expiration_interval: 2.days.to_i) }
+      let_it_be(:group) { create(:group, namespace_settings: group_settings) }
+
+      context 'group overrides project' do
+        let(:project) { create(:project, group: group, runner_token_expiration_interval: 3.days.to_i).tap(&:save!) }
+        let(:runner) { create(:ci_runner, :project, projects: [project]) }
+
+        it_behaves_like 'expiring token', interval: 2.days
+      end
+
+      context 'project overrides group' do
+        let(:project) { create(:project, group: group, runner_token_expiration_interval: 1.day.to_i).tap(&:save!) }
+        let(:runner) { create(:ci_runner, :project, projects: [project]) }
+
+        it_behaves_like 'expiring token', interval: 1.day
+      end
+    end
+  end
 end
diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb
index 4bdb3e0a32a825..2e82a12a61a40c 100644
--- a/spec/models/concerns/token_authenticatable_spec.rb
+++ b/spec/models/concerns/token_authenticatable_spec.rb
@@ -289,4 +289,142 @@
       expect(build.read_attribute('token')).to be_nil
     end
   end
+
+  describe '#token_with_expiration' do
+    describe '#expirable?' do
+      subject { build.token_with_expiration.expirable? }
+
+      it { is_expected.to eq(false) }
+    end
+  end
+end
+
+RSpec.describe Ci::Runner, 'TokenAuthenticatable', :freeze_time do
+  let_it_be(:non_expirable_runner) { create(:ci_runner) }
+  let_it_be(:non_expired_runner) { create(:ci_runner).tap { |r| r.update!(token_expires_at: 5.seconds.from_now) } }
+  let_it_be(:expired_runner) { create(:ci_runner).tap { |r| r.update!(token_expires_at: 5.seconds.ago) } }
+
+  describe '#token_expired?' do
+    subject { runner.token_expired? }
+
+    context 'when enforce_runner_token_expires_at feature flag is disabled' do
+      before do
+        stub_feature_flags(enforce_runner_token_expires_at: false)
+      end
+
+      context 'when runner has no token expiration' do
+        let(:runner) { non_expirable_runner }
+
+        it { is_expected.to eq(false) }
+      end
+
+      context 'when runner token is not expired' do
+        let(:runner) { non_expired_runner }
+
+        it { is_expected.to eq(false) }
+      end
+
+      context 'when runner token is expired' do
+        let(:runner) { expired_runner }
+
+        it { is_expected.to eq(false) }
+      end
+    end
+
+    context 'when enforce_runner_token_expires_at feature flag is enabled' do
+      before do
+        stub_feature_flags(enforce_runner_token_expires_at: true)
+      end
+
+      context 'when runner has no token expiration' do
+        let(:runner) { non_expirable_runner }
+
+        it { is_expected.to eq(false) }
+      end
+
+      context 'when runner token is not expired' do
+        let(:runner) { non_expired_runner }
+
+        it { is_expected.to eq(false) }
+      end
+
+      context 'when runner token is expired' do
+        let(:runner) { expired_runner }
+
+        it { is_expected.to eq(true) }
+      end
+    end
+  end
+
+  describe '#token_with_expiration' do
+    describe '#token' do
+      subject { non_expired_runner.token_with_expiration.token }
+
+      it { is_expected.to eq(non_expired_runner.token) }
+    end
+
+    describe '#token_expires_at' do
+      subject { non_expired_runner.token_with_expiration.token_expires_at }
+
+      it { is_expected.to eq(non_expired_runner.token_expires_at) }
+    end
+
+    describe '#expirable?' do
+      subject { non_expired_runner.token_with_expiration.expirable? }
+
+      it { is_expected.to eq(true) }
+    end
+  end
+
+  describe '.find_by_token' do
+    subject { Ci::Runner.find_by_token(runner.token) }
+
+    context 'when enforce_runner_token_expires_at feature flag is disabled' do
+      before do
+        stub_feature_flags(enforce_runner_token_expires_at: false)
+      end
+
+      context 'when runner has no token expiration' do
+        let(:runner) { non_expirable_runner }
+
+        it { is_expected.to eq(non_expirable_runner) }
+      end
+
+      context 'when runner token is not expired' do
+        let(:runner) { non_expired_runner }
+
+        it { is_expected.to eq(non_expired_runner) }
+      end
+
+      context 'when runner token is expired' do
+        let(:runner) { expired_runner }
+
+        it { is_expected.to eq(expired_runner) }
+      end
+    end
+
+    context 'when enforce_runner_token_expires_at feature flag is enabled' do
+      before do
+        stub_feature_flags(enforce_runner_token_expires_at: true)
+      end
+
+      context 'when runner has no token expiration' do
+        let(:runner) { non_expirable_runner }
+
+        it { is_expected.to eq(non_expirable_runner) }
+      end
+
+      context 'when runner token is not expired' do
+        let(:runner) { non_expired_runner }
+
+        it { is_expected.to eq(non_expired_runner) }
+      end
+
+      context 'when runner token is expired' do
+        let(:runner) { expired_runner }
+
+        it { is_expected.to be_nil }
+      end
+    end
+  end
 end
diff --git a/spec/requests/api/ci/runner/runners_verify_post_spec.rb b/spec/requests/api/ci/runner/runners_verify_post_spec.rb
index 4680076acae732..038e126deaa835 100644
--- a/spec/requests/api/ci/runner/runners_verify_post_spec.rb
+++ b/spec/requests/api/ci/runner/runners_verify_post_spec.rb
@@ -49,6 +49,30 @@
           let(:expected_params) { { client_id: "runner/#{runner.id}" } }
         end
       end
+
+      context 'when non-expired token is provided' do
+        subject { post api('/runners/verify'), params: { token: runner.token } }
+
+        it 'verifies Runner credentials' do
+          runner["token_expires_at"] = 10.days.from_now
+          runner.save!
+          subject
+
+          expect(response).to have_gitlab_http_status(:ok)
+        end
+      end
+
+      context 'when expired token is provided' do
+        subject { post api('/runners/verify'), params: { token: runner.token } }
+
+        it 'does not verify Runner credentials' do
+          runner["token_expires_at"] = 10.days.ago
+          runner.save!
+          subject
+
+          expect(response).to have_gitlab_http_status(:forbidden)
+        end
+      end
     end
   end
 end
-- 
GitLab