From 44715eab3456aa21bb975203aa3d2824356ba038 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Tue, 7 Jan 2025 14:19:25 -0600 Subject: [PATCH 1/5] Exclude projects or groups from CI Job Token authorizations compaction --- .../ci/job_token/authorizations_compactor.rb | 16 ++++++++-- .../authorizations_compactor_spec.rb | 29 +++++++++++++++++-- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/app/models/ci/job_token/authorizations_compactor.rb b/app/models/ci/job_token/authorizations_compactor.rb index 6485c2371bad8f..85c7d82d7fa93b 100644 --- a/app/models/ci/job_token/authorizations_compactor.rb +++ b/app/models/ci/job_token/authorizations_compactor.rb @@ -8,8 +8,9 @@ class AuthorizationsCompactor UnexpectedCompactionEntry = Class.new(StandardError) RedundantCompactionEntry = Class.new(StandardError) - def initialize(project_id) + def initialize(project_id, excluded_namespace_paths) @project_id = project_id + @excluded_namespace_paths = excluded_namespace_paths @allowlist_groups = [] @allowlist_projects = [] end @@ -28,14 +29,16 @@ def origin_project_traversal_ids origin_project_id_batches.each do |batch| projects = Project.where(id: batch).includes(:project_namespace) - origin_project_traversal_ids += projects.map { |p| p.project_namespace.traversal_ids } + origin_project_traversal_ids += projects.filter_map do |p| + p.project_namespace.traversal_ids unless namespace_path_excluded?(p.project_namespace.traversal_ids) + end end origin_project_traversal_ids end end - def compact(limit) + def compact(limit, _excluded_traversal_ids = []) compacted_traversal_ids = Gitlab::Utils::TraversalIdCompactor.compact(origin_project_traversal_ids, limit) Gitlab::Utils::TraversalIdCompactor.validate!(origin_project_traversal_ids, compacted_traversal_ids) @@ -51,6 +54,13 @@ def compact(limit) end end end + + def namespace_path_excluded?(namespace_path) + @excluded_namespace_paths.any? do |excluded_namespace_path| + namespace_path.size >= excluded_namespace_path.size && + namespace_path.first(excluded_namespace_path.size) == excluded_namespace_path + end + end end end end diff --git a/spec/models/ci/job_token/authorizations_compactor_spec.rb b/spec/models/ci/job_token/authorizations_compactor_spec.rb index 0611570e051409..857e3f616d9940 100644 --- a/spec/models/ci/job_token/authorizations_compactor_spec.rb +++ b/spec/models/ci/job_token/authorizations_compactor_spec.rb @@ -4,7 +4,8 @@ RSpec.describe Ci::JobToken::AuthorizationsCompactor, feature_category: :secrets_management do let_it_be(:accessed_project) { create(:project) } - let(:compactor) { described_class.new(accessed_project.id) } + let(:excluded_namespace_paths) { [] } + let(:compactor) { described_class.new(accessed_project.id, excluded_namespace_paths) } # [1, 21], ns1, p1 # [1, 2, 3], ns1, ns2, p2 @@ -111,6 +112,30 @@ end.not_to raise_error end end + + context 'with excluded_namespace_paths' do + describe 'when a single namespace path is excluded' do + let(:excluded_namespace_paths) { [ns6.traversal_ids] } + + it 'removes it from the compaction process' do + compactor.compact(4 - excluded_namespace_paths.count) + + expect(compactor.allowlist_groups).to match_array([ns2, ns4]) + expect(compactor.allowlist_projects).to match_array([pns1.project]) + end + end + + describe 'when a multiple namespace paths are excluded' do + let(:excluded_namespace_paths) { [ns6.traversal_ids, ns2.traversal_ids] } + + it 'removes it from the compaction process' do + compactor.compact(4 - excluded_namespace_paths.count) + + expect(compactor.allowlist_groups).to match_array([ns4]) + expect(compactor.allowlist_projects).to match_array([pns1.project]) + end + end + end end describe '#origin_traversal_ids' do @@ -118,7 +143,7 @@ accessed_project_control = create(:project) create(:ci_job_token_authorization, origin_project: pns1.project, accessed_project: accessed_project_control, last_authorized_at: 1.day.ago) - compactor_control = described_class.new(accessed_project_control.id) + compactor_control = described_class.new(accessed_project_control.id, []) control = ActiveRecord::QueryRecorder.new do compactor_control.origin_project_traversal_ids end -- GitLab From 9ecb87cbe094cc49ef6fef56dccc8683b7a7a7ff Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Wed, 8 Jan 2025 12:34:25 -0600 Subject: [PATCH 2/5] Adjust limit within the #compact method --- app/models/ci/job_token/authorizations_compactor.rb | 3 ++- spec/models/ci/job_token/authorizations_compactor_spec.rb | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/models/ci/job_token/authorizations_compactor.rb b/app/models/ci/job_token/authorizations_compactor.rb index 85c7d82d7fa93b..d5da13a2b1c8ef 100644 --- a/app/models/ci/job_token/authorizations_compactor.rb +++ b/app/models/ci/job_token/authorizations_compactor.rb @@ -38,7 +38,8 @@ def origin_project_traversal_ids end end - def compact(limit, _excluded_traversal_ids = []) + def compact(requested_limit) + limit = requested_limit - @excluded_namespace_paths.count compacted_traversal_ids = Gitlab::Utils::TraversalIdCompactor.compact(origin_project_traversal_ids, limit) Gitlab::Utils::TraversalIdCompactor.validate!(origin_project_traversal_ids, compacted_traversal_ids) diff --git a/spec/models/ci/job_token/authorizations_compactor_spec.rb b/spec/models/ci/job_token/authorizations_compactor_spec.rb index 857e3f616d9940..b85484f302845e 100644 --- a/spec/models/ci/job_token/authorizations_compactor_spec.rb +++ b/spec/models/ci/job_token/authorizations_compactor_spec.rb @@ -118,7 +118,7 @@ let(:excluded_namespace_paths) { [ns6.traversal_ids] } it 'removes it from the compaction process' do - compactor.compact(4 - excluded_namespace_paths.count) + compactor.compact(4) expect(compactor.allowlist_groups).to match_array([ns2, ns4]) expect(compactor.allowlist_projects).to match_array([pns1.project]) @@ -129,7 +129,7 @@ let(:excluded_namespace_paths) { [ns6.traversal_ids, ns2.traversal_ids] } it 'removes it from the compaction process' do - compactor.compact(4 - excluded_namespace_paths.count) + compactor.compact(4) expect(compactor.allowlist_groups).to match_array([ns4]) expect(compactor.allowlist_projects).to match_array([pns1.project]) -- GitLab From 9bca0be95599bda1f0c5eca0443bfc8efc596267 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Fri, 10 Jan 2025 13:14:10 -0600 Subject: [PATCH 3/5] Move excluded_namespace_paths from and input to a method in Ci::JobToken::AuthorizationsCompactor --- .../ci/job_token/authorizations_compactor.rb | 20 ++++-- .../authorizations_compactor_spec.rb | 62 ++++++++++++++++--- 2 files changed, 71 insertions(+), 11 deletions(-) diff --git a/app/models/ci/job_token/authorizations_compactor.rb b/app/models/ci/job_token/authorizations_compactor.rb index d5da13a2b1c8ef..6333a1b2ac40c4 100644 --- a/app/models/ci/job_token/authorizations_compactor.rb +++ b/app/models/ci/job_token/authorizations_compactor.rb @@ -3,14 +3,15 @@ module Ci module JobToken class AuthorizationsCompactor + include Gitlab::Utils::StrongMemoize + attr_reader :allowlist_groups, :allowlist_projects UnexpectedCompactionEntry = Class.new(StandardError) RedundantCompactionEntry = Class.new(StandardError) - def initialize(project_id, excluded_namespace_paths) + def initialize(project_id) @project_id = project_id - @excluded_namespace_paths = excluded_namespace_paths @allowlist_groups = [] @allowlist_projects = [] end @@ -39,7 +40,7 @@ def origin_project_traversal_ids end def compact(requested_limit) - limit = requested_limit - @excluded_namespace_paths.count + limit = requested_limit - excluded_namespace_paths.count compacted_traversal_ids = Gitlab::Utils::TraversalIdCompactor.compact(origin_project_traversal_ids, limit) Gitlab::Utils::TraversalIdCompactor.validate!(origin_project_traversal_ids, compacted_traversal_ids) @@ -57,11 +58,22 @@ def compact(requested_limit) end def namespace_path_excluded?(namespace_path) - @excluded_namespace_paths.any? do |excluded_namespace_path| + excluded_namespace_paths.any? do |excluded_namespace_path| namespace_path.size >= excluded_namespace_path.size && namespace_path.first(excluded_namespace_path.size) == excluded_namespace_path end end + + def excluded_namespace_paths + groups = Ci::JobToken::GroupScopeLink.where(source_project_id: @project_id) + projects = Ci::JobToken::ProjectScopeLink.where(source_project_id: @project_id) + + group_traversal_ids = groups.map { |g| g.target_group.traversal_ids } + project_traversal_ids = projects.map { |p| p.target_project.project_namespace.traversal_ids } + + group_traversal_ids + project_traversal_ids + end + strong_memoize_attr :excluded_namespace_paths end end end diff --git a/spec/models/ci/job_token/authorizations_compactor_spec.rb b/spec/models/ci/job_token/authorizations_compactor_spec.rb index b85484f302845e..e453f094aebd81 100644 --- a/spec/models/ci/job_token/authorizations_compactor_spec.rb +++ b/spec/models/ci/job_token/authorizations_compactor_spec.rb @@ -5,7 +5,7 @@ RSpec.describe Ci::JobToken::AuthorizationsCompactor, feature_category: :secrets_management do let_it_be(:accessed_project) { create(:project) } let(:excluded_namespace_paths) { [] } - let(:compactor) { described_class.new(accessed_project.id, excluded_namespace_paths) } + let(:compactor) { described_class.new(accessed_project.id) } # [1, 21], ns1, p1 # [1, 2, 3], ns1, ns2, p2 @@ -113,9 +113,11 @@ end end - context 'with excluded_namespace_paths' do - describe 'when a single namespace path is excluded' do - let(:excluded_namespace_paths) { [ns6.traversal_ids] } + context 'with exiting group scope links' do + describe 'when a single group exists' do + before do + create(:ci_job_token_group_scope_link, source_project: accessed_project, target_group: ns6) + end it 'removes it from the compaction process' do compactor.compact(4) @@ -125,8 +127,11 @@ end end - describe 'when a multiple namespace paths are excluded' do - let(:excluded_namespace_paths) { [ns6.traversal_ids, ns2.traversal_ids] } + describe 'when a multiple groups exist' do + before do + create(:ci_job_token_group_scope_link, source_project: accessed_project, target_group: ns6) + create(:ci_job_token_group_scope_link, source_project: accessed_project, target_group: ns2) + end it 'removes it from the compaction process' do compactor.compact(4) @@ -138,12 +143,55 @@ end end + context 'with exiting project scope links' do + describe 'when a single project exists' do + before do + create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns8.project) + end + + it 'removes it from the compaction process' do + compactor.compact(4) + + expect(compactor.allowlist_groups).to match_array([ns2, ns4]) + expect(compactor.allowlist_projects).to match_array([pns1.project]) + end + end + + describe 'when a multiple projects exist' do + before do + create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns8.project) + create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns2.project) + end + + it 'removes it from the compaction process' do + compactor.compact(6) + + expect(compactor.allowlist_groups).to match_array([ns2, ns4]) + expect(compactor.allowlist_projects).to match_array([pns1.project]) + end + end + end + + context 'with exiting group and project scope links' do + before do + create(:ci_job_token_group_scope_link, source_project: accessed_project, target_group: ns2) + create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns8.project) + end + + it 'removes it from the compaction process' do + compactor.compact(4) + + expect(compactor.allowlist_groups).to match_array([ns4]) + expect(compactor.allowlist_projects).to match_array([pns1.project]) + end + end + describe '#origin_traversal_ids' do it 'does not cause N+1 queries when loading projects' do accessed_project_control = create(:project) create(:ci_job_token_authorization, origin_project: pns1.project, accessed_project: accessed_project_control, last_authorized_at: 1.day.ago) - compactor_control = described_class.new(accessed_project_control.id, []) + compactor_control = described_class.new(accessed_project_control.id) control = ActiveRecord::QueryRecorder.new do compactor_control.origin_project_traversal_ids end -- GitLab From f89ce6f374fc6c3ed2fe86f7f674480a2d1d7596 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Fri, 10 Jan 2025 13:54:43 -0600 Subject: [PATCH 4/5] Add traversal_id methods to allowlist --- app/models/ci/job_token/allowlist.rb | 8 ++++++++ app/models/ci/job_token/authorizations_compactor.rb | 9 ++------- .../ci/job_token/authorizations_compactor_spec.rb | 12 ++++++++---- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/app/models/ci/job_token/allowlist.rb b/app/models/ci/job_token/allowlist.rb index 80a640b7919647..a6b919ced24b46 100644 --- a/app/models/ci/job_token/allowlist.rb +++ b/app/models/ci/job_token/allowlist.rb @@ -59,6 +59,14 @@ def add_group!(target_group, user:, default_permissions: true, policies: []) ) end + def project_link_traversal_ids + source_links.map { |p| p.target_project.project_namespace.traversal_ids } + end + + def group_link_traversal_ids + group_links.map { |g| g.target_group.traversal_ids } + end + def project_links Ci::JobToken::ProjectScopeLink .with_source(@source_project) diff --git a/app/models/ci/job_token/authorizations_compactor.rb b/app/models/ci/job_token/authorizations_compactor.rb index 6333a1b2ac40c4..88e0b07cdb7ffc 100644 --- a/app/models/ci/job_token/authorizations_compactor.rb +++ b/app/models/ci/job_token/authorizations_compactor.rb @@ -65,13 +65,8 @@ def namespace_path_excluded?(namespace_path) end def excluded_namespace_paths - groups = Ci::JobToken::GroupScopeLink.where(source_project_id: @project_id) - projects = Ci::JobToken::ProjectScopeLink.where(source_project_id: @project_id) - - group_traversal_ids = groups.map { |g| g.target_group.traversal_ids } - project_traversal_ids = projects.map { |p| p.target_project.project_namespace.traversal_ids } - - group_traversal_ids + project_traversal_ids + allowlist = Ci::JobToken::Allowlist.new(Project.find(@project_id)) + allowlist.project_link_traversal_ids + allowlist.group_link_traversal_ids end strong_memoize_attr :excluded_namespace_paths end diff --git a/spec/models/ci/job_token/authorizations_compactor_spec.rb b/spec/models/ci/job_token/authorizations_compactor_spec.rb index e453f094aebd81..70ad6454886be2 100644 --- a/spec/models/ci/job_token/authorizations_compactor_spec.rb +++ b/spec/models/ci/job_token/authorizations_compactor_spec.rb @@ -146,7 +146,8 @@ context 'with exiting project scope links' do describe 'when a single project exists' do before do - create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns8.project) + create(:ci_job_token_project_scope_link, source_project: accessed_project, direction: :inbound, + target_project: pns8.project) end it 'removes it from the compaction process' do @@ -159,8 +160,10 @@ describe 'when a multiple projects exist' do before do - create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns8.project) - create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns2.project) + create(:ci_job_token_project_scope_link, source_project: accessed_project, direction: :inbound, + target_project: pns8.project) + create(:ci_job_token_project_scope_link, source_project: accessed_project, direction: :inbound, + target_project: pns2.project) end it 'removes it from the compaction process' do @@ -175,7 +178,8 @@ context 'with exiting group and project scope links' do before do create(:ci_job_token_group_scope_link, source_project: accessed_project, target_group: ns2) - create(:ci_job_token_project_scope_link, source_project: accessed_project, target_project: pns8.project) + create(:ci_job_token_project_scope_link, source_project: accessed_project, direction: :inbound, + target_project: pns8.project) end it 'removes it from the compaction process' do -- GitLab From 2b543dd48a10b782fd56ed98352eb6e5a86badb0 Mon Sep 17 00:00:00 2001 From: Darby Frey Date: Tue, 14 Jan 2025 13:54:17 -0600 Subject: [PATCH 5/5] Performance and naming improvements --- app/models/ci/job_token/allowlist.rb | 6 ++++-- .../ci/job_token/authorizations_compactor.rb | 18 +++++++++--------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/models/ci/job_token/allowlist.rb b/app/models/ci/job_token/allowlist.rb index a6b919ced24b46..2f73adce424b67 100644 --- a/app/models/ci/job_token/allowlist.rb +++ b/app/models/ci/job_token/allowlist.rb @@ -60,11 +60,13 @@ def add_group!(target_group, user:, default_permissions: true, policies: []) end def project_link_traversal_ids - source_links.map { |p| p.target_project.project_namespace.traversal_ids } + project_links.includes(target_project: :project_namespace).map do |p| + p.target_project.project_namespace.traversal_ids + end end def group_link_traversal_ids - group_links.map { |g| g.target_group.traversal_ids } + group_links.includes(:target_group).map { |g| g.target_group.traversal_ids } end def project_links diff --git a/app/models/ci/job_token/authorizations_compactor.rb b/app/models/ci/job_token/authorizations_compactor.rb index 88e0b07cdb7ffc..49e7a8ade2fe6c 100644 --- a/app/models/ci/job_token/authorizations_compactor.rb +++ b/app/models/ci/job_token/authorizations_compactor.rb @@ -31,7 +31,7 @@ def origin_project_traversal_ids origin_project_id_batches.each do |batch| projects = Project.where(id: batch).includes(:project_namespace) origin_project_traversal_ids += projects.filter_map do |p| - p.project_namespace.traversal_ids unless namespace_path_excluded?(p.project_namespace.traversal_ids) + p.project_namespace.traversal_ids unless path_already_allowlisted?(p.project_namespace.traversal_ids) end end @@ -40,7 +40,7 @@ def origin_project_traversal_ids end def compact(requested_limit) - limit = requested_limit - excluded_namespace_paths.count + limit = requested_limit - existing_links_traversal_ids.count compacted_traversal_ids = Gitlab::Utils::TraversalIdCompactor.compact(origin_project_traversal_ids, limit) Gitlab::Utils::TraversalIdCompactor.validate!(origin_project_traversal_ids, compacted_traversal_ids) @@ -57,18 +57,18 @@ def compact(requested_limit) end end - def namespace_path_excluded?(namespace_path) - excluded_namespace_paths.any? do |excluded_namespace_path| - namespace_path.size >= excluded_namespace_path.size && - namespace_path.first(excluded_namespace_path.size) == excluded_namespace_path + def path_already_allowlisted?(namespace_path) + existing_links_traversal_ids.any? do |existing_links_traversal_id| + namespace_path.size >= existing_links_traversal_id.size && + namespace_path.first(existing_links_traversal_id.size) == existing_links_traversal_id end end - def excluded_namespace_paths + def existing_links_traversal_ids allowlist = Ci::JobToken::Allowlist.new(Project.find(@project_id)) - allowlist.project_link_traversal_ids + allowlist.group_link_traversal_ids + allowlist.group_link_traversal_ids + allowlist.project_link_traversal_ids end - strong_memoize_attr :excluded_namespace_paths + strong_memoize_attr :existing_links_traversal_ids end end end -- GitLab