diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index f0c4d94766849aa5ea82c79765a31f7ba6bf186c..7fab424ac936c1f1602648185e52166a75ef7921 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -120,6 +120,7 @@ From there, you can see the following actions: - Project access token was successfully created or revoked ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/230007) in GitLab 13.9) - Failed attempt to create or revoke a project access token ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/230007) in GitLab 13.9) - When default branch changes for a project ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/52339) in GitLab 13.9) +- Created, updated, or deleted DAST profiles, DAST scanner profiles, and DAST site profiles ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217872)) Project events can also be accessed via the [Project Audit Events API](../api/audit_events.md#project-audit-events). diff --git a/doc/user/application_security/dast/index.md b/doc/user/application_security/dast/index.md index 4a96efc3d72a30634e8f92fb1bcab898028f33ae..eb4faca63f96a34b104d56ecc9023786f7217d4b 100644 --- a/doc/user/application_security/dast/index.md +++ b/doc/user/application_security/dast/index.md @@ -957,6 +957,7 @@ Alternatively, you can use the CI/CD variable `SECURE_ANALYZERS_PREFIX` to overr > - The saved scans feature was [introduced](https://gitlab.com/groups/gitlab-org/-/epics/5100) in GitLab 13.9. > - The option to select a branch was [introduced](https://gitlab.com/groups/gitlab-org/-/epics/4847) in GitLab 13.10. > - DAST branch selection [feature flag removed](https://gitlab.com/gitlab-org/gitlab/-/issues/322672) in GitLab 13.11. +> - Auditing for DAST profile management was [introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217872) in GitLab 14.1. An on-demand DAST scan runs outside the DevOps life cycle. Changes in your repository don't trigger the scan. You must start it manually. @@ -1280,6 +1281,13 @@ If a scanner profile is linked to a security policy, a user cannot delete the pr page. See [Scan Policies](../policies/index.md) for more information. +### Auditing + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/217872) in GitLab 14.1. + +The creation, updating, and deletion of DAST profiles, DAST scanner profiles, +and DAST site profiles are included in the [audit log](../../../administration/audit_events.md). + ## Reports The DAST tool outputs a report file in JSON format by default. However, this tool can also generate reports in diff --git a/ee/app/models/dast_scanner_profile.rb b/ee/app/models/dast_scanner_profile.rb index 3d599afbbe079b9a8beaded891ddab0d04d76adb..93cb684546ae775ebb3163e299bbfa70007413af 100644 --- a/ee/app/models/dast_scanner_profile.rb +++ b/ee/app/models/dast_scanner_profile.rb @@ -17,6 +17,12 @@ class DastScannerProfile < ApplicationRecord active: 2 } + def self.names(scanner_profile_ids) + find(*scanner_profile_ids).pluck(:name) + rescue ActiveRecord::RecordNotFound + [] + end + def ci_variables ::Gitlab::Ci::Variables::Collection.new.tap do |variables| variables.append(key: 'DAST_SPIDER_MINS', value: String(spider_timeout)) if spider_timeout diff --git a/ee/app/models/dast_site_profile.rb b/ee/app/models/dast_site_profile.rb index 1b061d690ddfc04c7b94839377da3bf13c549d34..2b19997cc70b1ea1b227ba0dee7a96f9a154cc39 100644 --- a/ee/app/models/dast_site_profile.rb +++ b/ee/app/models/dast_site_profile.rb @@ -31,6 +31,12 @@ class DastSiteProfile < ApplicationRecord delegate :dast_site_validation, to: :dast_site, allow_nil: true + def self.names(site_profile_ids) + find(*site_profile_ids).pluck(:name) + rescue ActiveRecord::RecordNotFound + [] + end + def ci_variables url = dast_site.url diff --git a/ee/app/services/app_sec/dast/profiles/audit/update_service.rb b/ee/app/services/app_sec/dast/profiles/audit/update_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..8260346640d4e37bd78bc057c2fadaf0e6d70b13 --- /dev/null +++ b/ee/app/services/app_sec/dast/profiles/audit/update_service.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +module AppSec + module Dast + module Profiles + module Audit + class UpdateService < BaseContainerService + def execute + params[:new_params].each do |property, new_value| + old_value = params[:old_params][property] + + next if old_value == new_value + + ::Gitlab::Audit::Auditor.audit( + name: 'dast_profile_update', + author: current_user, + scope: container, + target: params[:dast_profile], + message: audit_message(property, new_value, old_value) + ) + end + end + + private + + def audit_message(property, new_value, old_value) + case property + when :dast_scanner_profile_id + old_value, new_value = DastScannerProfile.names([old_value, new_value]) + property = :dast_scanner_profile + when :dast_site_profile_id + old_value, new_value = DastSiteProfile.names([old_value, new_value]) + property = :dast_site_profile + end + + "Changed DAST profile #{property} from #{old_value} to #{new_value}" + end + end + end + end + end +end diff --git a/ee/app/services/app_sec/dast/profiles/create_service.rb b/ee/app/services/app_sec/dast/profiles/create_service.rb index ef9ea4d245844e48c13478087337b9e6fce240fa..93660242590f8b80aae58c32d52d94e7e76e93da 100644 --- a/ee/app/services/app_sec/dast/profiles/create_service.rb +++ b/ee/app/services/app_sec/dast/profiles/create_service.rb @@ -16,6 +16,8 @@ def execute dast_scanner_profile: dast_scanner_profile ) + create_audit_event(dast_profile) + return ServiceResponse.success(payload: { dast_profile: dast_profile, pipeline_url: nil }) unless params.fetch(:run_after_create) response = ::DastOnDemandScans::CreateService.new( @@ -46,6 +48,16 @@ def dast_site_profile def dast_scanner_profile @dast_scanner_profile ||= params.fetch(:dast_scanner_profile) end + + def create_audit_event(profile) + ::Gitlab::Audit::Auditor.audit( + name: 'dast_profile_create', + author: current_user, + scope: container, + target: profile, + message: "Added DAST profile" + ) + end end end end diff --git a/ee/app/services/app_sec/dast/profiles/destroy_service.rb b/ee/app/services/app_sec/dast/profiles/destroy_service.rb index 7e4b0d50dccb5148321c00d59a7cd49e1c70b4d2..2a99a9271ff0b9abd0b91df6c6d7d1dd9af91234 100644 --- a/ee/app/services/app_sec/dast/profiles/destroy_service.rb +++ b/ee/app/services/app_sec/dast/profiles/destroy_service.rb @@ -9,6 +9,8 @@ def execute return ServiceResponse.error(message: 'Profile parameter missing') unless dast_profile return ServiceResponse.error(message: 'Profile failed to delete') unless dast_profile.destroy + create_audit_event + ServiceResponse.success(payload: dast_profile) end @@ -28,6 +30,16 @@ def unauthorized def dast_profile params[:dast_profile] end + + def create_audit_event + ::Gitlab::Audit::Auditor.audit( + name: 'dast_profile_destroy', + author: current_user, + scope: container, + target: dast_profile, + message: "Removed DAST profile" + ) + end end end end diff --git a/ee/app/services/app_sec/dast/profiles/update_service.rb b/ee/app/services/app_sec/dast/profiles/update_service.rb index cb876219dc5f6422cce522ef0216a829b351088c..d44f366f39c83c4eee89768253bf4867fef1c2ce 100644 --- a/ee/app/services/app_sec/dast/profiles/update_service.rb +++ b/ee/app/services/app_sec/dast/profiles/update_service.rb @@ -9,8 +9,17 @@ class UpdateService < BaseContainerService def execute return unauthorized unless allowed? return error('Profile parameter missing') unless dast_profile + + auditor = AppSec::Dast::Profiles::Audit::UpdateService.new(container: container, current_user: current_user, params: { + dast_profile: dast_profile, + new_params: dast_profile_params, + old_params: dast_profile.attributes.symbolize_keys + }) + return error(dast_profile.errors.full_messages) unless dast_profile.update(dast_profile_params) + auditor.execute + return success(dast_profile: dast_profile, pipeline_url: nil) unless params[:run_after_update] response = create_scan(dast_profile) diff --git a/ee/spec/models/dast_scanner_profile_spec.rb b/ee/spec/models/dast_scanner_profile_spec.rb index 5563859c3ecc9a2147cc311a4509d3e1d8911214..c090cce866d8f73d6bf1443622f65efb6ad6be87 100644 --- a/ee/spec/models/dast_scanner_profile_spec.rb +++ b/ee/spec/models/dast_scanner_profile_spec.rb @@ -35,6 +35,25 @@ end end + describe '.names' do + it 'returns the names for the DAST scanner profiles with the given IDs' do + first_profile = create(:dast_scanner_profile, name: 'First profile') + second_profile = create(:dast_scanner_profile, name: 'Second profile') + + names = described_class.names([first_profile.id, second_profile.id]) + + expect(names).to contain_exactly('First profile', 'Second profile') + end + + context 'when a profile is not found' do + it 'rescues the error and returns an empty array' do + names = described_class.names([0]) + + expect(names).to be_empty + end + end + end + describe '#ci_variables' do let(:collection) { subject.ci_variables } diff --git a/ee/spec/models/dast_site_profile_spec.rb b/ee/spec/models/dast_site_profile_spec.rb index da4b98485dd78693def5e88e3ef8bbb398ce8c8b..f57516827987af403985dc92df6a844028ce1c42 100644 --- a/ee/spec/models/dast_site_profile_spec.rb +++ b/ee/spec/models/dast_site_profile_spec.rb @@ -132,6 +132,25 @@ it { is_expected.to define_enum_for(:target_type).with_values(**target_types) } end + describe '.names' do + it 'returns the names for the DAST site profiles with the given IDs' do + first_profile = create(:dast_site_profile, name: 'First profile') + second_profile = create(:dast_site_profile, name: 'Second profile') + + names = described_class.names([first_profile.id, second_profile.id]) + + expect(names).to contain_exactly('First profile', 'Second profile') + end + + context 'when a profile is not found' do + it 'rescues the error and returns an empty array' do + names = described_class.names([0]) + + expect(names).to be_empty + end + end + end + describe 'instance methods' do describe '#destroy!' do context 'when the associated dast_site has no dast_site_profiles' do diff --git a/ee/spec/services/app_sec/dast/profiles/audit/update_service_spec.rb b/ee/spec/services/app_sec/dast/profiles/audit/update_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..f33ccdf8d7c24203bd53a9bdf2033e4e0a474a56 --- /dev/null +++ b/ee/spec/services/app_sec/dast/profiles/audit/update_service_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AppSec::Dast::Profiles::Audit::UpdateService do + let_it_be(:dast_profile) { create(:dast_profile) } + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + describe '#execute' do + it 'creates audit events for the changed properties', :aggregate_failures do + auditor = described_class.new(container: project, current_user: user, params: { + dast_profile: dast_profile, + new_params: { name: 'New name' }, + old_params: { name: 'Old name' } + }) + + auditor.execute + + audit_event = AuditEvent.find_by(author_id: user.id) + expect(audit_event.author).to eq(user) + expect(audit_event.entity).to eq(project) + expect(audit_event.target_id).to eq(dast_profile.id) + expect(audit_event.target_type).to eq('Dast::Profile') + expect(audit_event.target_details).to eq(dast_profile.name) + expect(audit_event.details).to eq({ + author_name: user.name, + custom_message: 'Changed DAST profile name from Old name to New name', + target_id: dast_profile.id, + target_type: 'Dast::Profile', + target_details: dast_profile.name + }) + end + + it 'uses names instead of IDs for the changed scanner and site profile messages' do + new_scanner_profile = create(:dast_scanner_profile) + old_scanner_profile = create(:dast_scanner_profile) + new_site_profile = create(:dast_site_profile) + old_site_profile = create(:dast_site_profile) + + auditor = described_class.new(container: project, current_user: user, params: { + dast_profile: dast_profile, + new_params: { dast_scanner_profile_id: new_scanner_profile.id, dast_site_profile_id: new_site_profile.id }, + old_params: { dast_scanner_profile_id: old_scanner_profile.id, dast_site_profile_id: old_site_profile.id } + }) + + auditor.execute + + audit_events = AuditEvent.where(author_id: user.id) + messages = audit_events.map(&:details).pluck(:custom_message) + expect(messages).to contain_exactly( + "Changed DAST profile dast_scanner_profile from #{old_scanner_profile.name} to #{new_scanner_profile.name}", + "Changed DAST profile dast_site_profile from #{old_site_profile.name} to #{new_site_profile.name}" + ) + end + + it 'does not exceed the maximum permitted number of queries' do + new_scanner_profile = create(:dast_scanner_profile) + old_scanner_profile = create(:dast_scanner_profile) + new_site_profile = create(:dast_site_profile) + old_site_profile = create(:dast_site_profile) + + new_params = { + branch_name: 'new-branch', + dast_scanner_profile_id: new_scanner_profile.id, + dast_site_profile_id: new_site_profile.id, + description: 'New description', + name: 'New name' + } + old_params = { + branch_name: 'old-branch', + dast_scanner_profile_id: old_scanner_profile.id, + dast_site_profile_id: old_site_profile.id, + description: 'Old description', + name: 'Old name' + } + + auditor = described_class.new(container: project, current_user: user, params: { + dast_profile: dast_profile, new_params: new_params, old_params: old_params + }) + + recorder = ActiveRecord::QueryRecorder.new do + auditor.execute + end + + expect(recorder.count).to be <= 18 + end + end +end diff --git a/ee/spec/services/app_sec/dast/profiles/create_service_spec.rb b/ee/spec/services/app_sec/dast/profiles/create_service_spec.rb index 614ce26621be944b1922fc15e99766528caa6567..6e6c18ff9e19642c88345f2a32d672063b9e4e01 100644 --- a/ee/spec/services/app_sec/dast/profiles/create_service_spec.rb +++ b/ee/spec/services/app_sec/dast/profiles/create_service_spec.rb @@ -48,6 +48,27 @@ expect { subject }.to change { Dast::Profile.count }.by(1) end + it 'audits the creation' do + profile = subject.payload[:dast_profile] + + audit_event = AuditEvent.find_by(author_id: developer.id) + + aggregate_failures do + expect(audit_event.author).to eq(developer) + expect(audit_event.entity).to eq(project) + expect(audit_event.target_id).to eq(profile.id) + expect(audit_event.target_type).to eq('Dast::Profile') + expect(audit_event.target_details).to eq(profile.name) + expect(audit_event.details).to eq({ + author_name: developer.name, + custom_message: 'Added DAST profile', + target_id: profile.id, + target_type: 'Dast::Profile', + target_details: profile.name + }) + end + end + context 'when param run_after_create: true' do let(:params) { default_params.merge(run_after_create: true) } diff --git a/ee/spec/services/app_sec/dast/profiles/destroy_service_spec.rb b/ee/spec/services/app_sec/dast/profiles/destroy_service_spec.rb index a25b36c19597a6882e14cd002e76c52a713b9ad1..809438ed36c72bb8b72506622ff092e21ecaa95b 100644 --- a/ee/spec/services/app_sec/dast/profiles/destroy_service_spec.rb +++ b/ee/spec/services/app_sec/dast/profiles/destroy_service_spec.rb @@ -60,6 +60,27 @@ expect(subject.payload).to be_a(Dast::Profile) end + it 'audits the deletion' do + profile = subject.payload + + audit_event = AuditEvent.find_by(author_id: user.id) + + aggregate_failures do + expect(audit_event.author).to eq(user) + expect(audit_event.entity).to eq(project) + expect(audit_event.target_id).to eq(profile.id) + expect(audit_event.target_type).to eq('Dast::Profile') + expect(audit_event.target_details).to eq(profile.name) + expect(audit_event.details).to eq({ + author_name: user.name, + custom_message: 'Removed DAST profile', + target_id: profile.id, + target_type: 'Dast::Profile', + target_details: profile.name + }) + end + end + context 'when the dast_profile fails to destroy' do it 'communicates failure' do allow(dast_profile).to receive(:destroy).and_return(false) diff --git a/ee/spec/services/app_sec/dast/profiles/update_service_spec.rb b/ee/spec/services/app_sec/dast/profiles/update_service_spec.rb index 3b1341f0691a66601c81df03c712a2605ff6828e..0c25dc06c44fb82bbacb1fefae2d89b0bac06994 100644 --- a/ee/spec/services/app_sec/dast/profiles/update_service_spec.rb +++ b/ee/spec/services/app_sec/dast/profiles/update_service_spec.rb @@ -76,6 +76,37 @@ end end + it 'audits the update', :aggregate_failures do + old_profile_attrs = { + description: dast_profile.description, + name: dast_profile.name, + scanner_profile_name: dast_profile.dast_scanner_profile.name, + site_profile_name: dast_profile.dast_site_profile.name + } + + subject + + new_profile = dast_profile.reload + audit_events = AuditEvent.where(author_id: user.id) + + audit_events.each do |event| + expect(event.author).to eq(user) + expect(event.entity).to eq(project) + expect(event.target_id).to eq(new_profile.id) + expect(event.target_type).to eq('Dast::Profile') + expect(event.target_details).to eq(new_profile.name) + end + + messages = audit_events.map(&:details).pluck(:custom_message) + expected_messages = [ + "Changed DAST profile dast_scanner_profile from #{old_profile_attrs[:scanner_profile_name]} to #{dast_scanner_profile.name}", + "Changed DAST profile dast_site_profile from #{old_profile_attrs[:site_profile_name]} to #{dast_site_profile.name}", + "Changed DAST profile name from #{old_profile_attrs[:name]} to #{new_profile.name}", + "Changed DAST profile description from #{old_profile_attrs[:description]} to #{new_profile.description}" + ] + expect(messages).to match_array(expected_messages) + end + context 'when param run_after_update: true' do let(:params) { default_params.merge(run_after_update: true) }