From 90e006ac69a25bb1692609e70ed92f69715228df Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Mon, 15 Jul 2024 13:30:44 -0400 Subject: [PATCH] Audit group modifications to CI_JOB_TOKEN This add audit logging for modifications (additions, removals) of groups in the CI_JOB_TOKEN allow list, bringing these in line with project modifications to the same list. Resolves: https://gitlab.com/gitlab-org/gitlab/-/issues/467840 Changelog: added EE: true --- .../ci/job_token_scope/add_group_service.rb | 2 + doc/user/compliance/audit_event_types.md | 2 + .../ci/job_token_scope/add_group_service.rb | 36 ++++++++ .../job_token_scope/remove_group_service.rb | 36 ++++++++ .../types/secure_ci_job_token_group_added.yml | 9 ++ .../secure_ci_job_token_group_removed.yml | 9 ++ .../add_group_or_project_spec.rb | 78 +++++++++++++++++ .../ci/job_token_scope/remove_group_spec.rb | 87 +++++++++++++++++++ .../ci/job_token_scope/remove_project_spec.rb | 13 ++- 9 files changed, 264 insertions(+), 8 deletions(-) create mode 100644 ee/app/services/ee/ci/job_token_scope/add_group_service.rb create mode 100644 ee/app/services/ee/ci/job_token_scope/remove_group_service.rb create mode 100644 ee/config/audit_events/types/secure_ci_job_token_group_added.yml create mode 100644 ee/config/audit_events/types/secure_ci_job_token_group_removed.yml create mode 100644 ee/spec/graphql/ee/mutations/ci/job_token_scope/add_group_or_project_spec.rb create mode 100644 ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_group_spec.rb diff --git a/app/services/ci/job_token_scope/add_group_service.rb b/app/services/ci/job_token_scope/add_group_service.rb index 6e83f8b9b63118..09269c9d69d0a9 100644 --- a/app/services/ci/job_token_scope/add_group_service.rb +++ b/app/services/ci/job_token_scope/add_group_service.rb @@ -29,3 +29,5 @@ def allowlist end end end + +Ci::JobTokenScope::AddGroupService.prepend_mod_with('Ci::JobTokenScope::AddGroupService') diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index a4b262b33ec7c8..71f3cddeaf4731 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -552,6 +552,8 @@ Audit event types belong to the following product categories. | Name | Description | Saved to database | Streamed | Introduced in | Scope | |:------------|:------------|:------------------|:---------|:--------------|:--------------| +| [`secure_ci_job_token_group_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/159754) | Event triggered when group added to CI_JOB_TOKEN scope | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.3](https://gitlab.com/gitlab-org/gitlab/-/issues/467840) | Project | +| [`secure_ci_job_token_group_removed`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/159754) | Event triggered when group removed from CI_JOB_TOKEN scope | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [17.3](https://gitlab.com/gitlab-org/gitlab/-/issues/467840) | Project | | [`secure_ci_job_token_inbound_disabled`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115350) | Event triggered when CI_JOB_TOKEN permissions disabled for inbound | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/338255) | Project | | [`secure_ci_job_token_inbound_enabled`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115350) | Event triggered when CI_JOB_TOKEN permissions enabled for inbound | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/338255) | Project | | [`secure_ci_job_token_project_added`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/115350) | Event triggered when project added to inbound CI_JOB_TOKEN scope | **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.0](https://gitlab.com/gitlab-org/gitlab/-/issues/338255) | Project | diff --git a/ee/app/services/ee/ci/job_token_scope/add_group_service.rb b/ee/app/services/ee/ci/job_token_scope/add_group_service.rb new file mode 100644 index 00000000000000..d0ab23f4634cac --- /dev/null +++ b/ee/app/services/ee/ci/job_token_scope/add_group_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module EE + module Ci + module JobTokenScope + module AddGroupService + extend ::Gitlab::Utils::Override + + override :execute + def execute(target_group) + super.tap do |response| + audit(project, target_group, current_user) if response.success? + end + end + + private + + def audit(scope, target, author) + audit_message = + "Group #{target.full_path} was added to list of allowed groups for #{scope.full_path}" + event_name = 'secure_ci_job_token_group_added' + + audit_context = { + name: event_name, + author: author, + scope: scope, + target: target, + message: audit_message + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end + end +end diff --git a/ee/app/services/ee/ci/job_token_scope/remove_group_service.rb b/ee/app/services/ee/ci/job_token_scope/remove_group_service.rb new file mode 100644 index 00000000000000..001e9892665010 --- /dev/null +++ b/ee/app/services/ee/ci/job_token_scope/remove_group_service.rb @@ -0,0 +1,36 @@ +# frozen_string_literal: true + +module EE + module Ci + module JobTokenScope + module RemoveGroupService + extend ::Gitlab::Utils::Override + + override :execute + def execute(target_group) + super.tap do |response| + audit(project, target_group, current_user) if response.success? + end + end + + private + + def audit(scope, target, author) + audit_message = + "Group #{target.full_path} was removed from list of allowed groups for #{scope.full_path}" + event_name = 'secure_ci_job_token_group_removed' + + audit_context = { + name: event_name, + author: author, + scope: scope, + target: target, + message: audit_message + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end + end +end diff --git a/ee/config/audit_events/types/secure_ci_job_token_group_added.yml b/ee/config/audit_events/types/secure_ci_job_token_group_added.yml new file mode 100644 index 00000000000000..b4be2074e68015 --- /dev/null +++ b/ee/config/audit_events/types/secure_ci_job_token_group_added.yml @@ -0,0 +1,9 @@ +name: secure_ci_job_token_group_added +description: Event triggered when group added to CI_JOB_TOKEN scope +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/467840 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/159754 +feature_category: "verify_security" +milestone: "17.3" +saved_to_database: true +streamed: true +scope: [Project] diff --git a/ee/config/audit_events/types/secure_ci_job_token_group_removed.yml b/ee/config/audit_events/types/secure_ci_job_token_group_removed.yml new file mode 100644 index 00000000000000..b86b737d1f7d43 --- /dev/null +++ b/ee/config/audit_events/types/secure_ci_job_token_group_removed.yml @@ -0,0 +1,9 @@ +name: secure_ci_job_token_group_removed +description: Event triggered when group removed from CI_JOB_TOKEN scope +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/467840 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/159754 +feature_category: "verify_security" +milestone: "17.3" +saved_to_database: true +streamed: true +scope: [Project] diff --git a/ee/spec/graphql/ee/mutations/ci/job_token_scope/add_group_or_project_spec.rb b/ee/spec/graphql/ee/mutations/ci/job_token_scope/add_group_or_project_spec.rb new file mode 100644 index 00000000000000..9f3deeaf9bc622 --- /dev/null +++ b/ee/spec/graphql/ee/mutations/ci/job_token_scope/add_group_or_project_spec.rb @@ -0,0 +1,78 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Mutations::Ci::JobTokenScope::AddGroupOrProject, feature_category: :continuous_integration do + include GraphqlHelpers + + describe '#resolve' do + let(:project) do + create(:project, ci_outbound_job_token_scope_enabled: true) + end + + let(:target_group) { create(:group) } + + let(:target_group_path) { target_group.full_path } + let(:project_path) { project.full_path } + let(:input) do + { + project_path: project.full_path, + target_path: target_group_path + } + end + + let(:current_user) { create(:user) } + + let(:expected_audit_context) do + { + name: event_name, + author: current_user, + scope: project, + target: target_group, + message: expected_audit_message + } + end + + let(:call_add_group) do + ctx = { current_user: current_user } + mutation = graphql_mutation(described_class, input) + GitlabSchema.execute(mutation.query, context: ctx, variables: mutation.variables).to_h + end + + context 'when adding group validate it triggers audits' do + before do + project.add_maintainer(current_user) + target_group.add_guest(current_user) + end + + let(:expected_audit_message) do + "Group #{target_group_path} was added to list of allowed groups for #{project_path}" + end + + let(:event_name) { 'secure_ci_job_token_group_added' } + + context 'when user adds target group to the job token scope' do + it 'logs an audit event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including(expected_audit_context)) + + call_add_group + end + + context 'and service returns an error' do + it 'does not log an audit event' do + expect_next_instance_of(::Ci::JobTokenScope::AddGroupService) do |service| + expect(service) + .to receive(:validate_group_add!) + .with(project, target_group, current_user) + .and_raise(ActiveRecord::RecordNotUnique) + end + + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + call_add_group + end + end + end + end + end +end diff --git a/ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_group_spec.rb b/ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_group_spec.rb new file mode 100644 index 00000000000000..3b46d7a99c5eae --- /dev/null +++ b/ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_group_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Mutations::Ci::JobTokenScope::RemoveGroup, feature_category: :continuous_integration do + include GraphqlHelpers + + describe '#resolve' do + let(:project) do + create(:project, ci_outbound_job_token_scope_enabled: true) + end + + let(:target_group) { create(:group) } + + let!(:link) do + create(:ci_job_token_group_scope_link, + source_project: project, + target_group: target_group) + end + + let(:links_relation) { Ci::JobToken::GroupScopeLink.with_source(project).with_target(target_group) } + + let(:target_group_path) { target_group.full_path } + let(:project_path) { project.full_path } + let(:input) { { project_path: project.full_path, target_group_path: target_group_path } } + let(:current_user) { create(:user) } + + let(:expected_audit_context) do + { + name: event_name, + author: current_user, + scope: project, + target: target_group, + message: expected_audit_message + } + end + + let(:call_remove_group) do + ctx = { current_user: current_user } + mutation = graphql_mutation(described_class, input) + GitlabSchema.execute(mutation.query, context: ctx, variables: mutation.variables).to_h + end + + context 'when removing group validate it triggers audits' do + before do + project.add_maintainer(current_user) + target_group.add_guest(current_user) + end + + context 'when user removes target group to the job token scope' do + let(:expected_audit_message) do + "Group #{target_group_path} was removed from list of allowed groups for #{project_path}" + end + + let(:event_name) { 'secure_ci_job_token_group_removed' } + + let(:service) do + instance_double('Ci::JobTokenScope::RemoveGroupService') + end + + it 'logs an audit event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(hash_including(expected_audit_context)) + + call_remove_group + end + + context 'and service returns an error' do + let(:mutation_args) do + { project_path: project.full_path, target_group_path: target_group_path } + end + + it 'does not log an audit event' do + expect_next_instance_of(::Ci::JobTokenScope::RemoveGroupService) do |service| + expect(service) + .to receive(:validate_group_remove!) + .and_raise(::Ci::JobTokenScope::EditScopeValidations::ValidationError) + end + + expect(::Gitlab::Audit::Auditor).not_to receive(:audit) + + call_remove_group + end + end + end + end + end +end diff --git a/ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_project_spec.rb b/ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_project_spec.rb index 22eb3f21f7ce86..1c944d9ad2e620 100644 --- a/ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_project_spec.rb +++ b/ee/spec/graphql/ee/mutations/ci/job_token_scope/remove_project_spec.rb @@ -69,14 +69,11 @@ context 'and service returns an error' do it 'does not log an audit event' do - expect(::Ci::JobTokenScope::RemoveProjectService).to receive(:new).with( - project, - current_user - ).and_return(service) - expect(service) - .to receive(:execute) - .with(target_project, :inbound) - .and_return(ServiceResponse.error(message: 'The error message')) + expect_next_instance_of(::Ci::JobTokenScope::RemoveProjectService) do |service| + expect(service) + .to receive(:validate_edit!) + .and_raise(::Ci::JobTokenScope::EditScopeValidations::ValidationError) + end expect(::Gitlab::Audit::Auditor).not_to receive(:audit) -- GitLab