From 0b81bf218fa78cf4e555d3206551cb024da8eb4a Mon Sep 17 00:00:00 2001 From: Daniel Prause Date: Tue, 29 Jul 2025 09:13:08 +0200 Subject: [PATCH 1/6] Remove old encrypted token implementation --- .rubocop_todo/database/avoid_scope_to.yml | 1 - .rubocop_todo/rspec/be_eq.yml | 1 - app/models/ci/trigger.rb | 17 ++------ .../encrypt_ci_trigger_token.rb | 41 ------------------- spec/models/ci/trigger_spec.rb | 17 +------- 5 files changed, 4 insertions(+), 73 deletions(-) delete mode 100644 lib/gitlab/background_migration/encrypt_ci_trigger_token.rb diff --git a/.rubocop_todo/database/avoid_scope_to.yml b/.rubocop_todo/database/avoid_scope_to.yml index 46342c3d3ddb9d..a17d090acc4afa 100644 --- a/.rubocop_todo/database/avoid_scope_to.yml +++ b/.rubocop_todo/database/avoid_scope_to.yml @@ -41,7 +41,6 @@ Database/AvoidScopeTo: - 'lib/gitlab/background_migration/destroy_invalid_group_members.rb' - 'lib/gitlab/background_migration/destroy_invalid_members.rb' - 'lib/gitlab/background_migration/destroy_invalid_project_members.rb' - - 'lib/gitlab/background_migration/encrypt_ci_trigger_token.rb' - 'lib/gitlab/background_migration/expire_o_auth_tokens.rb' - 'lib/gitlab/background_migration/fix_bad_sharding_key_id_on_project_ci_runners.rb' - 'lib/gitlab/background_migration/limit_namespace_visibility_by_organization_visibility.rb' diff --git a/.rubocop_todo/rspec/be_eq.yml b/.rubocop_todo/rspec/be_eq.yml index f8ae4431e5109e..60487c60341e75 100644 --- a/.rubocop_todo/rspec/be_eq.yml +++ b/.rubocop_todo/rspec/be_eq.yml @@ -680,7 +680,6 @@ RSpec/BeEq: - 'spec/lib/gitlab/auth/saml/origin_validator_spec.rb' - 'spec/lib/gitlab/avatar_cache_spec.rb' - 'spec/lib/gitlab/background_migration/backfill_vs_code_settings_version_spec.rb' - - 'spec/lib/gitlab/background_migration/encrypt_ci_trigger_token_spec.rb' - 'spec/lib/gitlab/background_migration/job_coordinator_spec.rb' - 'spec/lib/gitlab/background_migration/update_users_set_external_if_service_account_spec.rb' - 'spec/lib/gitlab/background_migration_spec.rb' diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 030f0b2953d052..f707163f7a31b2 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -24,15 +24,10 @@ class Trigger < Ci::ApplicationRecord validate :expires_at_before_instance_max_expiry_date, on: :create - attr_encrypted :encrypted_token_tmp, - attribute: :encrypted_token, - mode: :per_attribute_iv, - algorithm: 'aes-256-gcm', - key: :db_key_base_32, - encode: false - before_validation :set_default_values - before_save :copy_token_to_encrypted_token + + ignore_column :encrypted_token, remove_with: '18.4', remove_after: '2025-09-30' + ignore_column :encrypted_token_iv, remove_with: '18.4', remove_after: '2025-09-30' # rubocop:disable Gitlab/TokenWithoutPrefix -- we are doing this ourselves here since ensure_token # does not work as expected @@ -115,12 +110,6 @@ def expires_at_before_instance_max_expiry_date format(_("must be before %{expiry_date}"), expiry_date: max_expiry_date) ) end - - private - - def copy_token_to_encrypted_token - self.encrypted_token_tmp = token - end end end diff --git a/lib/gitlab/background_migration/encrypt_ci_trigger_token.rb b/lib/gitlab/background_migration/encrypt_ci_trigger_token.rb deleted file mode 100644 index e602fdf2053f9c..00000000000000 --- a/lib/gitlab/background_migration/encrypt_ci_trigger_token.rb +++ /dev/null @@ -1,41 +0,0 @@ -# frozen_string_literal: true - -module Gitlab - module BackgroundMigration - # Migration to make sure that all the prevously saved tokens have their encrypted values in the db. - class EncryptCiTriggerToken < Gitlab::BackgroundMigration::BatchedMigrationJob - feature_category :continuous_integration - scope_to ->(relation) { relation.where(encrypted_token: nil) } - operation_name :update - # Class that is imitating Ci::Trigger - class CiTrigger < ::Ci::ApplicationRecord - include Gitlab::EncryptedAttribute - - ALGORITHM = 'aes-256-gcm' - - self.table_name = 'ci_triggers' - - attr_encrypted :encrypted_token_tmp, - attribute: :encrypted_token, - mode: :per_attribute_iv, - algorithm: 'aes-256-gcm', - key: :db_key_base_32, - encode: false - - before_save :copy_token_to_encrypted_token - - def copy_token_to_encrypted_token - self.encrypted_token_tmp = token - end - end - - def perform - each_sub_batch do |sub_batch| - sub_batch.each do |trigger| - Gitlab::BackgroundMigration::EncryptCiTriggerToken::CiTrigger.find(trigger.id).save! - end - end - end - end - end -end diff --git a/spec/models/ci/trigger_spec.rb b/spec/models/ci/trigger_spec.rb index 866de73f9c36e5..6e654c81f0c479 100644 --- a/spec/models/ci/trigger_spec.rb +++ b/spec/models/ci/trigger_spec.rb @@ -100,10 +100,6 @@ end end - it_behaves_like 'encrypted attribute', :encrypted_token_tmp, :db_key_base_32 do - let(:record) { create(:ci_trigger_without_token, project: project) } - end - describe '.with_token' do context 'when ff lookup for encrypted token is enabled' do let_it_be(:project) { create(:project) } @@ -330,10 +326,6 @@ trigger = create(:ci_trigger_without_token, project: project) expect(trigger.token).not_to be_nil - expect(trigger.encrypted_token).not_to be_nil - expect(trigger.encrypted_token_iv).not_to be_nil - - expect(trigger.reload.encrypted_token_tmp).to eq(trigger.token) expect(trigger.token_encrypted).not_to be_nil end end @@ -343,10 +335,6 @@ trigger = create(:ci_trigger, project: project) expect(trigger.token).not_to be_nil - expect(trigger.encrypted_token).not_to be_nil - expect(trigger.encrypted_token_iv).not_to be_nil - - expect(trigger.reload.encrypted_token_tmp).to eq(trigger.token) expect(trigger.token_encrypted).not_to be_nil end end @@ -355,10 +343,7 @@ it 'encrypts the given token' do trigger = create(:ci_trigger, project: project, token: "token") expect { trigger.update!(token: "new token") } - .to change { trigger.encrypted_token } - .and change { trigger.encrypted_token_iv } - .and change { trigger.encrypted_token_tmp }.from("token").to("new token") - .and change { trigger.token }.from("token").to("new token") + .to change { trigger.token }.from("token").to("new token") .and change { trigger.token_encrypted } end end -- GitLab From 8213e8cfadf071bca55d20f978fff26c52ed65a2 Mon Sep 17 00:00:00 2001 From: Daniel Prause Date: Tue, 29 Jul 2025 09:20:12 +0200 Subject: [PATCH 2/6] Apply 1 suggestion(s) to 1 file(s) Co-authored-by: GitLab Duo --- app/models/ci/trigger.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index f707163f7a31b2..5f7651408a82bd 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -26,8 +26,8 @@ class Trigger < Ci::ApplicationRecord before_validation :set_default_values - ignore_column :encrypted_token, remove_with: '18.4', remove_after: '2025-09-30' - ignore_column :encrypted_token_iv, remove_with: '18.4', remove_after: '2025-09-30' + ignore_column :encrypted_token, remove_with: '18.3', remove_after: '2025-07-29' + ignore_column :encrypted_token_iv, remove_with: '18.3', remove_after: '2025-07-29' # rubocop:disable Gitlab/TokenWithoutPrefix -- we are doing this ourselves here since ensure_token # does not work as expected -- GitLab From dd5b4f2bf05eb73208649c1f44cecedf717e3936 Mon Sep 17 00:00:00 2001 From: Daniel Prause Date: Tue, 5 Aug 2025 08:34:13 +0200 Subject: [PATCH 3/6] Remove unused spec --- ..._triggers_token_to_token_encrypted_spec.rb | 28 ------------------- 1 file changed, 28 deletions(-) diff --git a/spec/migrations/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted_spec.rb b/spec/migrations/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted_spec.rb index eee0f868e69a36..c6c1c9e03a7ce0 100644 --- a/spec/migrations/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted_spec.rb +++ b/spec/migrations/20250709140240_queue_migrate_ci_triggers_token_to_token_encrypted_spec.rb @@ -9,15 +9,6 @@ let!(:migration) { described_class::MIGRATION } describe '#up' do - shared_examples 'finalizes the migration' do - it 'finalizes the migration' do - allow_next_instance_of(Gitlab::Database::BackgroundMigration::BatchedMigrationRunner) do |runner| - expect(runner).to receive(:finalize).with('QueueMigrateCiTriggersTokenToTokenEncrypted', :ci_triggers, :id, - []) - end - end - end - context 'with migration present' do let!(:ci_trigger_token_encryption_migration) do batched_migrations.create!( @@ -52,25 +43,6 @@ ) end end - - context 'with different migration statuses' do - using RSpec::Parameterized::TableSyntax - - where(:status, :description) do - 0 | 'paused' - 1 | 'active' - 4 | 'failed' - 5 | 'finalizing' - end - - with_them do - before do - ci_trigger_token_encryption_migration.update!(status: status) - end - - it_behaves_like 'finalizes the migration' - end - end end end -- GitLab From c4d303a2d9276f949bbca133600975e121077868 Mon Sep 17 00:00:00 2001 From: Daniel Prause Date: Wed, 6 Aug 2025 16:20:06 +0200 Subject: [PATCH 4/6] Remove unused spec --- .../encrypt_ci_trigger_token_spec.rb | 63 ------------------- 1 file changed, 63 deletions(-) delete mode 100644 spec/lib/gitlab/background_migration/encrypt_ci_trigger_token_spec.rb diff --git a/spec/lib/gitlab/background_migration/encrypt_ci_trigger_token_spec.rb b/spec/lib/gitlab/background_migration/encrypt_ci_trigger_token_spec.rb deleted file mode 100644 index c447134e7f9410..00000000000000 --- a/spec/lib/gitlab/background_migration/encrypt_ci_trigger_token_spec.rb +++ /dev/null @@ -1,63 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe Gitlab::BackgroundMigration::EncryptCiTriggerToken, feature_category: :continuous_integration do - before do - stub_feature_flags(encrypted_trigger_token_lookup: false) - end - - let(:ci_triggers) do - table(:ci_triggers, database: :ci) do |ci_trigger| - ci_trigger.send :attr_encrypted, :encrypted_token_tmp, - attribute: :encrypted_token, - mode: :per_attribute_iv, - key: Settings.db_key_base_keys_32_bytes.first, - algorithm: 'aes-256-gcm', - encode: false - end - end - - let(:without_encryption) { ci_triggers.create!(token: "token", owner_id: 1, project_id: 1) } - let(:without_encryption_2) { ci_triggers.create!(token: "token 2", owner_id: 1, project_id: 1) } - let(:with_encryption) do - ci_triggers.create!(token: 'token 3', owner_id: 1, encrypted_token_tmp: 'token 3', project_id: 1) - end - - let(:start_id) { ci_triggers.minimum(:id) } - let(:end_id) { ci_triggers.maximum(:id) } - - let(:migration_attrs) do - { - start_id: start_id, - end_id: end_id, - batch_table: :ci_triggers, - batch_column: :id, - sub_batch_size: 1, - pause_ms: 0, - connection: Ci::ApplicationRecord.connection - } - end - - it 'ensures all unencrypted tokens are encrypted' do - expect(without_encryption.encrypted_token).to eq(nil) - expect(without_encryption_2.encrypted_token).to eq(nil) - expect(with_encryption.encrypted_token).not_to be_nil - - described_class.new(**migration_attrs).perform - - updated_triggers = [without_encryption, without_encryption_2] - updated_triggers.each do |stale_trigger| - db_trigger = Ci::Trigger.find(stale_trigger.id) - expect(db_trigger.encrypted_token).not_to be_nil - expect(db_trigger.encrypted_token_iv).not_to be_nil - expect(db_trigger.token).to eq(db_trigger.encrypted_token_tmp) - end - - already_encrypted_token = Ci::Trigger.find(with_encryption.id) - expect(already_encrypted_token.encrypted_token).to eq(with_encryption.encrypted_token) - expect(already_encrypted_token.encrypted_token_iv).to eq(with_encryption.encrypted_token_iv) - expect(already_encrypted_token.token).to eq(already_encrypted_token.encrypted_token_tmp) - expect(with_encryption.token).to eq(with_encryption.encrypted_token_tmp) - end -end -- GitLab From 2a03c4cefc383dbba7046795a8fb7044bd1af16f Mon Sep 17 00:00:00 2001 From: Daniel Prause Date: Wed, 6 Aug 2025 17:14:42 +0200 Subject: [PATCH 5/6] Remove compact since reject blank is in place --- app/models/ci/trigger.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 5f7651408a82bd..4d124c9f7de571 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -51,7 +51,7 @@ class Trigger < Ci::ApplicationRecord end scope :with_token, ->(tokens) { - tokens = Array.wrap(tokens).compact.reject(&:blank?) + tokens = Array.wrap(tokens).reject(&:blank?) if Feature.enabled?(:encrypted_trigger_token_lookup, :instance) encrypted_tokens = tokens.map { |token| Ci::Trigger.encode(token) } where(token_encrypted: encrypted_tokens) -- GitLab From 0be86ce55651df77dc6bb440a7f9e8737e104317 Mon Sep 17 00:00:00 2001 From: Daniel Prause Date: Fri, 8 Aug 2025 11:03:14 +0200 Subject: [PATCH 6/6] Adjust removal date for ignored columns --- app/models/ci/trigger.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/ci/trigger.rb b/app/models/ci/trigger.rb index 4d124c9f7de571..cc4088c8a254aa 100644 --- a/app/models/ci/trigger.rb +++ b/app/models/ci/trigger.rb @@ -26,8 +26,8 @@ class Trigger < Ci::ApplicationRecord before_validation :set_default_values - ignore_column :encrypted_token, remove_with: '18.3', remove_after: '2025-07-29' - ignore_column :encrypted_token_iv, remove_with: '18.3', remove_after: '2025-07-29' + ignore_column :encrypted_token, remove_with: '18.4', remove_after: '2025-09-30' + ignore_column :encrypted_token_iv, remove_with: '18.4', remove_after: '2025-09-30' # rubocop:disable Gitlab/TokenWithoutPrefix -- we are doing this ourselves here since ensure_token # does not work as expected -- GitLab