diff --git a/app/models/commit.rb b/app/models/commit.rb index 128bc0958704b05468df6361cbac8396ba3be74d..8942b84ab74aabb50e35706c9ce7eefd7931fe5a 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -601,6 +601,10 @@ def has_encoded_file_paths? raw_diffs.any?(&:encoded_file_path) end + def valid_full_sha + id.match(Gitlab::Git::Commit::FULL_SHA_PATTERN).to_s + end + private def tipping_refs(ref_prefix, limit: 0) diff --git a/ee/app/models/security/project_security_exclusion.rb b/ee/app/models/security/project_security_exclusion.rb index cb8c177a5af86595b6c76be1b11852b994e3011d..818dd248c5621659f137c094877e0ba6c1bb950d 100644 --- a/ee/app/models/security/project_security_exclusion.rb +++ b/ee/app/models/security/project_security_exclusion.rb @@ -4,6 +4,13 @@ module Security class ProjectSecurityExclusion < Gitlab::Database::SecApplicationRecord self.inheritance_column = :_type_disabled + # Maximum number of path-based exclusions per project. This is an arbitrary limit aimed to + # prevent single project from having a huge number of path exclusions causing performance issues, + # and also to discourage users from using exclusions in favor of actually deleting removing secrets. + # + # See discussion: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/166511#note_2138495926. + MAX_PATH_EXCLUSIONS_PER_PROJECT = 10 + belongs_to :project enum scanner: { secret_push_protection: 0 } @@ -13,12 +20,37 @@ class ProjectSecurityExclusion < Gitlab::Database::SecApplicationRecord validates :active, inclusion: { in: [true, false] } validates :value, :description, length: { maximum: 255 } + validate :validate_push_protection_path_exclusions_limit, if: -> do + scanner == 'secret_push_protection' && type == 'path' + end + scope :by_scanner, ->(scanner) { where(scanner: scanner) } scope :by_type, ->(type) { where(type: type) } scope :by_status, ->(status) { where(active: status) } + scope :active, -> { by_status(true) } def audit_details attributes.slice('scanner', 'value', 'active', 'description').symbolize_keys end + + private + + def validate_push_protection_path_exclusions_limit + validate_push_protection_path_exclusions_count = project.security_exclusions + .by_scanner(:secret_push_protection) + .by_type(:path) + .where.not(id: id) + .count + + return unless validate_push_protection_path_exclusions_count >= MAX_PATH_EXCLUSIONS_PER_PROJECT + + errors.add( + :base, + format( + _("Cannot have more than %{maximum_path_exclusions} path exclusions for secret push protection per project"), + maximum_path_exclusions: MAX_PATH_EXCLUSIONS_PER_PROJECT + ) + ) + end end end diff --git a/ee/lib/gitlab/checks/secrets_check.rb b/ee/lib/gitlab/checks/secrets_check.rb index 7c54afbcb96a91c9db22af21750693a31d997391..17cf5ed0cb410614816c68999f53c832501665e7 100644 --- a/ee/lib/gitlab/checks/secrets_check.rb +++ b/ee/lib/gitlab/checks/secrets_check.rb @@ -39,6 +39,9 @@ class SecretsCheck < ::Gitlab::Checks::BaseBulkChecker DOCUMENTATION_PATH = 'user/application_security/secret_detection/secret_push_protection/index.html' DOCUMENTATION_PATH_ANCHOR = 'resolve-a-blocked-push' + # Maximum depth any path exclusion can have. + MAX_PATH_EXCLUSIONS_DEPTH = 20 + def validate! # Return early and do not perform the check: # 1. unless license is ultimate @@ -72,8 +75,8 @@ def validate! payloads = get_diffs # Pass payloads to gem for scanning. response = ::Gitlab::SecretDetection::ScanDiffs - .new(logger: secret_detection_logger) - .secrets_scan(payloads, timeout: logger.time_left) + .new(logger: secret_detection_logger) + .secrets_scan(payloads, timeout: logger.time_left) else payloads = ::Gitlab::Checks::ChangedBlobs.new( project, revisions, bytes_limit: PAYLOAD_BYTES_LIMIT + 1 @@ -81,13 +84,23 @@ def validate! # Filter out larger than PAYLOAD_BYTES_LIMIT blobs and binary blobs. payloads.reject! { |payload| payload.size > PAYLOAD_BYTES_LIMIT || payload.binary } + + # Only load exclusions if the feature flag is enabled. + exclusions = if Feature.enabled?(:secret_detection_project_level_exclusions, project) + active_exclusions + else + {} + end + # Pass payloads to gem for scanning. response = ::Gitlab::SecretDetection::Scan .new(logger: secret_detection_logger) - .secrets_scan(payloads, timeout: logger.time_left) + .secrets_scan(payloads, timeout: logger.time_left, exclusions: exclusions) end - # Handle the response depending on the status returned + + # Handle the response depending on the status returned. format_response(response) + # TODO: Perhaps have a separate message for each and better logging? rescue ::Gitlab::SecretDetection::Scan::RulesetParseError, ::Gitlab::SecretDetection::Scan::RulesetCompilationError, @@ -99,13 +112,8 @@ def validate! private - def http_or_ssh_protocol? - %w[http ssh].include?(changes_access.protocol) - end - - def use_diff_scan? - Feature.enabled?(:spp_scan_diffs, project) && http_or_ssh_protocol? - end + ############################## + # Project Eligibility Checks def run_pre_receive_secret_detection? Gitlab::CurrentSettings.current_application_settings.pre_receive_secret_detection_enabled && @@ -122,6 +130,17 @@ def enabled_for_dedicated_project? project.security_setting&.pre_receive_secret_detection_enabled end + ############### + # Scan Checks + + def http_or_ssh_protocol? + %w[http ssh].include?(changes_access.protocol) + end + + def use_diff_scan? + Feature.enabled?(:spp_scan_diffs, project) && http_or_ssh_protocol? + end + def includes_full_revision_history? Gitlab::Git.blank_ref?(changes_access.changes.first[:newrev]) end @@ -134,6 +153,9 @@ def skip_secret_detection_push_option? changes_access.push_options&.get(:secret_push_protection, :skip_all) end + ############################ + # Audits and Event Logging + def secret_detection_logger @secret_detection_logger ||= ::Gitlab::SecretDetectionLogger.build end @@ -165,6 +187,21 @@ def track_spp_skipped(skip_method) ) end + def track_secret_found(secret_type) + track_internal_event( + 'detect_secret_type_on_push', + user: changes_access.user_access.user, + project: changes_access.project, + namespace: changes_access.project.namespace, + additional_properties: { + label: secret_type + } + ) + end + + ####################### + # Load Payloads + def get_diffs diffs = [] # Get new commits @@ -194,6 +231,21 @@ def get_diffs diffs end + ####################### + # Format Scan Results + + def commits + @commit ||= changes_access.commits.map(&:valid_full_sha) + end + + def revisions + @revisions ||= changes_access + .changes + .pluck(:newrev) # rubocop:disable CodeReuse/ActiveRecord -- Array#pluck + .reject { |revision| ::Gitlab::Git.blank_ref?(revision) } + .compact + end + def format_response(response) # Try to retrieve file path and commit sha for the diffs found. if [ @@ -201,6 +253,10 @@ def format_response(response) ::Gitlab::SecretDetection::Status::FOUND_WITH_ERRORS ].include?(response.status) results = transform_findings(response) + + # If there is no findings in `response.results`, that means all findings + # were excluded in `transform_findings`, so we set status to no secrets found. + response.status = ::Gitlab::SecretDetection::Status::NOT_FOUND if response.results.empty? end case response.status @@ -236,14 +292,6 @@ def format_response(response) end end - def revisions - @revisions ||= changes_access - .changes - .pluck(:newrev) # rubocop:disable CodeReuse/ActiveRecord -- Array#pluck - .reject { |revision| ::Gitlab::Git.blank_ref?(revision) } - .compact - end - def build_secrets_found_message(results, with_errors: false) message = with_errors ? LOG_MESSAGES[:found_secrets_with_errors] : LOG_MESSAGES[:found_secrets] @@ -313,18 +361,7 @@ def build_blob_finding_message(finding) format(LOG_MESSAGES[:finding_message], finding.to_h) end - def track_secret_found(secret_type) - track_internal_event( - 'detect_secret_type_on_push', - user: changes_access.user_access.user, - project: changes_access.project, - namespace: changes_access.project.namespace, - additional_properties: { - label: secret_type - } - ) - end - + # rubocop:disable Metrics/CyclomaticComplexity -- Not easy to move complexity away into other methods. def transform_findings(response) # Let's group the findings by the blob id. findings_by_blobs = response.results.group_by(&:blob_id) @@ -335,8 +372,6 @@ def transform_findings(response) # Let's create a set to store ids of blobs found in tree entries. blobs_found_with_tree_entries = Set.new - commits = changes_access.commits.map { |commit| commit.id.match(/[a-f0-9]{40}([a-f0-9]{24})?/).to_s } - # Scanning had found secrets, let's try to look up their file path and commit id. This can be done # by using `GetTreeEntries()` RPC, and cross examining blobs with ones where secrets where found. commits.each do |revision| @@ -366,6 +401,19 @@ def transform_findings(response) # Skip if the blob doesn't have any findings. next unless findings_by_blobs[entry.id].present? + # Skip a tree entry if it's excluded from scanning by the user based on its file + # path. We unfortunately have to do this after scanning is done because we only get + # file paths when calling `GetTreeEntries()` RPC and not earlier. When diff scanning + # is available, we will likely be able move this check to the gem/secret detection service + # since paths will be available pre-scanning. + if entry_matches_excluded_path?(entry.path) + response.results.delete_if { |finding| finding.blob_id == entry.id } + + findings_by_blobs.delete(entry.id) + + next + end + new_entry = findings_by_blobs[entry.id].each_with_object({}) do |finding, hash| hash[entry.commit_id] ||= {} hash[entry.commit_id][entry.path] ||= [] @@ -391,6 +439,34 @@ def transform_findings(response) blobs: findings_by_blobs } end + # rubocop:enable Metrics/CyclomaticComplexity + + ############## + # Exclusions + + def active_exclusions + @active_exclusions ||= project + .security_exclusions + .by_scanner(:secret_push_protection) + .active + .select(:type, :value) + .group_by { |exclusion| exclusion.type.to_sym } + end + + def entry_matches_excluded_path?(path) + # Skip checking if tree entry match excluded paths when feature flag is disabled. + return false unless ::Feature.enabled?(:secret_detection_project_level_exclusions, project) + + # Skip paths that are too deep. + return false if path.count('/') > MAX_PATH_EXCLUSIONS_DEPTH + + # Skip any path exclusions past the maximum path exclusions count (i.e. 50 path exclusions). + active_exclusions[:path] + &.first(::Security::ProjectSecurityExclusion::MAX_PATH_EXCLUSIONS_PER_PROJECT) + &.any? do |exclusion| + File.fnmatch?(exclusion.value, path, File::FNM_DOTMATCH | File::FNM_EXTGLOB | File::FNM_PATHNAME) + end + end end end end diff --git a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb index 00b19bf89527a147502eca4a97f988eed237e5c1..625be1f65a1ac557c9508a210710ab362568df87 100644 --- a/ee/spec/lib/gitlab/checks/secrets_check_spec.rb +++ b/ee/spec/lib/gitlab/checks/secrets_check_spec.rb @@ -80,6 +80,16 @@ it_behaves_like 'scan skipped due to invalid status' it_behaves_like 'scan skipped when a commit has special bypass flag' it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed' + it_behaves_like 'scan discarded secrets because they match exclusions' + + # TODO: remove this context when `secret_detection_project_level_exclusions` FF is removed. + context 'when exclusions feature flag is disabled' do + before do + stub_feature_flags(secret_detection_project_level_exclusions: false) + end + + it_behaves_like 'scan does not discard excluded secrets' + end end end @@ -97,6 +107,7 @@ it_behaves_like 'scan skipped due to invalid status' it_behaves_like 'scan skipped when a commit has special bypass flag' it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed' + it_behaves_like 'scan discarded secrets because they match exclusions' context 'when the protocol is web' do subject(:secrets_check) { described_class.new(changes_access_web) } @@ -110,6 +121,16 @@ it_behaves_like 'scan skipped due to invalid status' it_behaves_like 'scan skipped when a commit has special bypass flag' it_behaves_like 'scan skipped when secret_push_protection.skip_all push option is passed' + it_behaves_like 'scan discarded secrets because they match exclusions' + end + + # TODO: remove this context when `secret_detection_project_level_exclusions` FF is removed. + context 'when exclusions feature flag is disabled' do + before do + stub_feature_flags(secret_detection_project_level_exclusions: false) + end + + it_behaves_like 'scan does not discard excluded secrets' end end end diff --git a/ee/spec/models/security/project_security_exclusion_spec.rb b/ee/spec/models/security/project_security_exclusion_spec.rb index 2112666aa39e4cf0e2a6203797773c971f838758..08f95415dfd1a7e7354c988d1209a018663ffe9c 100644 --- a/ee/spec/models/security/project_security_exclusion_spec.rb +++ b/ee/spec/models/security/project_security_exclusion_spec.rb @@ -14,6 +14,64 @@ it { is_expected.to validate_presence_of(:value) } it { is_expected.to validate_length_of(:value).is_at_most(255) } it { is_expected.to validate_length_of(:description).is_at_most(255) } + + describe '#validate_push_protection_path_exclusions_limit' do + let_it_be(:project) { create(:project) } + + context 'when exclusion is created or updated for the secret push protection scanner' do + context 'with `path` type' do + context 'when maximum number of path exclusions already exist' do + let_it_be(:rule_exclusion) { create(:project_security_exclusion, :with_rule, project: project) } + + before do + create_list(:project_security_exclusion, 10, :with_path, project: project) + end + + it 'does not allow adding more exclusions' do + exclusion = build(:project_security_exclusion, :with_path, project: project) + + expect(exclusion.save).to be_falsey + expect(exclusion.errors.full_messages).to eq( + ["Cannot have more than 10 path exclusions for secret push protection per project"] + ) + end + + it 'does not allow updating an exclusion of another type to `path` type' do + rule_exclusion.type = :path + + expect(rule_exclusion.save).to be_falsey + expect(rule_exclusion.errors.full_messages).to eq( + ["Cannot have more than 10 path exclusions for secret push protection per project"] + ) + end + end + + context 'when less than maximum number of path exclusions exist' do + before do + create_list(:project_security_exclusion, 9, :with_path, project: project) + end + + it 'creates the 10th path exclusion' do + exclusion = build(:project_security_exclusion, :with_path, project: project) + + expect(exclusion.save).to be_truthy + end + end + end + + context 'when exclusion created or updated is not of `path` type' do + before do + create_list(:project_security_exclusion, 10, :with_rule, project: project) + end + + it 'allows adding more exclusions' do + exclusion = build(:project_security_exclusion, :with_rule, project: project) + + expect(exclusion.save).to be_truthy + end + end + end + end end describe 'enums' do @@ -45,6 +103,12 @@ expect(described_class.by_status(true)).to match_array([exclusion_1, exclusion_3]) end end + + describe '.active' do + it 'returns the correct records' do + expect(described_class.active).to match_array([exclusion_1, exclusion_3]) + end + end end describe '#audit_details' do diff --git a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb index e6ed8c88120f247539e18b42e80fee675c15f573..705d34f658fc696150faa510b42d3c1a4c2affcd 100644 --- a/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb +++ b/ee/spec/support/shared_contexts/secrets_check_shared_contexts.rb @@ -47,9 +47,12 @@ end let_it_be(:commits) do - [ - Gitlab::Git::Commit.find(repository, new_commit) - ] + Commit.decorate( + [ + Gitlab::Git::Commit.find(repository, new_commit) + ], + project + ) end let(:expected_tree_args) do @@ -216,7 +219,7 @@ message = finding_message_header message += finding_message_path message += finding_message_occurrence_line - message += another_finding_message_header + message += format(log_messages[:finding_message_occurrence_header], { sha: commit_with_same_blob }) message += finding_message_path message += finding_message_occurrence_line message @@ -314,16 +317,6 @@ end end -RSpec.shared_context 'with gitaly commit client' do - let(:gitaly_commit_client) { instance_double(Gitlab::GitalyClient::CommitService) } - - before do - # We mock the gitaly commit client to have it return tree entries. - allow(repository).to receive(:gitaly_commit_client).and_return(gitaly_commit_client) - allow(gitaly_commit_client).to receive(:tree_entries).and_return([tree_entries, gitaly_pagination_cursor]) - end -end - def create_commit(blobs, message = 'Add a file') commit = repository.commit_files( user, @@ -344,3 +337,16 @@ def create_commit(blobs, message = 'Add a file') commit end + +def finding_message(sha, path, line_number, description) + message = format(log_messages[:finding_message_occurrence_header], { sha: sha }) + message += format(log_messages[:finding_message_occurrence_path], { path: path }) + message += format( + log_messages[:finding_message_occurrence_line], + { + line_number: line_number, + description: description + } + ) + message +end diff --git a/ee/spec/support/shared_examples/lib/gitlab/diffs_secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/diffs_secrets_check_shared_examples.rb index 6fccb289e70ae7a73dc17465319bdb3bac648dea..538d75cba75b939eebe64e0a3ae7109546ef39f2 100644 --- a/ee/spec/support/shared_examples/lib/gitlab/diffs_secrets_check_shared_examples.rb +++ b/ee/spec/support/shared_examples/lib/gitlab/diffs_secrets_check_shared_examples.rb @@ -522,10 +522,13 @@ end let(:commits) do - [ - Gitlab::Git::Commit.find(repository, new_commit), - Gitlab::Git::Commit.find(repository, another_new_commit) - ] + Commit.decorate( + [ + Gitlab::Git::Commit.find(repository, new_commit), + Gitlab::Git::Commit.find(repository, another_new_commit) + ], + project + ) end let(:successful_with_same_blob_in_multiple_commits_scan_response) do @@ -704,10 +707,13 @@ let(:another_blob_reference) { 'e10edae379797ad5649a65ad364f6c940ee5bbc3' } let(:commits) do - [ - Gitlab::Git::Commit.find(repository, new_commit), - Gitlab::Git::Commit.find(repository, another_new_commit) - ] + Commit.decorate( + [ + Gitlab::Git::Commit.find(repository, new_commit), + Gitlab::Git::Commit.find(repository, another_new_commit) + ], + project + ) end let(:another_diff_blob) do diff --git a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb index 396e48f367406725d073685215e85aca0e9faf38..a673484b95d271a6ab54fd54e53dd165baa8d896 100644 --- a/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb +++ b/ee/spec/support/shared_examples/lib/gitlab/secrets_check_shared_examples.rb @@ -88,7 +88,8 @@ expect(instance).to receive(:secrets_scan) .with( [new_blob], - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(passed_scan_response) @@ -205,7 +206,8 @@ expect(instance).to receive(:secrets_scan) .with( [new_blob], - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_scan_response) @@ -375,7 +377,8 @@ expect(instance).to receive(:secrets_scan) .with( [new_blob], - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_with_multiple_findings_scan_response) @@ -404,7 +407,7 @@ create_commit('.env' => "SECRET=glpat-JUST20LETTERSANDNUMB") # gitleaks:allow end - let_it_be(:another_new_commit) do + let_it_be(:commit_with_same_blob) do create_commit( { '.env' => "SECRET=glpat-JUST20LETTERSANDNUMB" }, # gitleaks:allow 'Same commit different message' @@ -432,22 +435,25 @@ name: '.env', path: '.env', flat_path: '.env', - commit_id: another_new_commit + commit_id: commit_with_same_blob ) ] end let(:changes) do [ - { oldrev: initial_commit, newrev: another_new_commit, ref: 'refs/heads/master' } + { oldrev: initial_commit, newrev: commit_with_same_blob, ref: 'refs/heads/master' } ] end let(:commits) do - [ - Gitlab::Git::Commit.find(repository, new_commit), - Gitlab::Git::Commit.find(repository, another_new_commit) - ] + Commit.decorate( + [ + Gitlab::Git::Commit.find(repository, new_commit), + Gitlab::Git::Commit.find(repository, commit_with_same_blob) + ], + project + ) end let(:successful_with_same_blob_in_multiple_commits_scan_response) do @@ -474,7 +480,7 @@ before do allow(repository).to receive(:new_commits) - .with([another_new_commit]) + .with([commit_with_same_blob]) .and_return(commits) end @@ -483,7 +489,8 @@ expect(instance).to receive(:secrets_scan) .with( [new_blob], - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_with_same_blob_in_multiple_commits_scan_response) @@ -497,7 +504,7 @@ .and_call_original expect(::Gitlab::Git::Tree).to receive(:tree_entries) - .with(**expected_tree_args.merge(sha: another_new_commit)) + .with(**expected_tree_args.merge(sha: commit_with_same_blob)) .once .and_return([tree_entries, gitaly_pagination_cursor]) .and_call_original @@ -555,7 +562,8 @@ expect(instance).to receive(:secrets_scan) .with( [new_blob], - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_with_multiple_findings_on_same_line_scan_response) @@ -616,7 +624,8 @@ expect(instance).to receive(:secrets_scan) .with( array_including(new_blob, new_blob2), - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once.and_call_original do |*args| expect(instance.secrets_scan(*args)).to eq(successful_with_multiple_files_findings_scan_response) @@ -652,7 +661,8 @@ allow(instance).to receive(:secrets_scan) .with( [new_blob], - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_scan_response) @@ -746,7 +756,8 @@ expect(instance).to receive(:secrets_scan) .with( array_including(new_blob, timed_out_blob, failed_to_scan_blob), - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .and_return(successful_scan_with_errors_response) end @@ -776,7 +787,8 @@ expect(instance).to receive(:secrets_scan) .with( array_including(new_blob, timed_out_blob, failed_to_scan_blob), - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .and_return(successful_scan_with_errors_response) end @@ -808,7 +820,8 @@ expect(instance).to receive(:secrets_scan) .with( array_including(new_blob, timed_out_blob, failed_to_scan_blob), - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .and_return(successful_scan_with_errors_response) end @@ -826,7 +839,8 @@ expect(instance).to receive(:secrets_scan) .with( array_including(new_blob, timed_out_blob, failed_to_scan_blob), - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_scan_with_errors_response) @@ -863,7 +877,8 @@ expect(instance).to receive(:secrets_scan) .with( array_including(new_blob, timed_out_blob, failed_to_scan_blob), - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_scan_with_errors_response) @@ -949,7 +964,8 @@ expect(instance).to receive(:secrets_scan) .with( array_including(new_blob, timed_out_blob, failed_to_scan_blob), - timeout: kind_of(Float) + timeout: kind_of(Float), + exclusions: kind_of(Hash) ) .once .and_return(successful_scan_with_multiple_findings_and_errors_response) @@ -1179,3 +1195,258 @@ subject { super().validate! } end end + +RSpec.shared_examples 'scan discarded secrets because they match exclusions' do + include_context 'secrets check context' + + context 'when exclusion is based on matching a file path' do + context 'with exactly maximum numer of path exclusions allowed' do + let_it_be(:commit_with_excluded_file_paths) do + create_commit( + 'file-exclusion-1.rb' => 'KEY=glpat-JUST20LETTERSANDNUMB', # gitleaks:allow + 'file-exclusion-2-skipped.rb' => 'TOKEN=glpat-JUST20LETTERSANDNUMB' # gitleaks:allow + ) + end + + let(:changes) do + [ + { oldrev: initial_commit, newrev: commit_with_excluded_file_paths, ref: 'refs/heads/master' } + ] + end + + before do + stub_const('::Security::ProjectSecurityExclusion::MAX_PATH_EXCLUSIONS_PER_PROJECT', 1) + + create(:project_security_exclusion, :active, :with_path, project: project, value: "file-exclusion-1.rb") + end + + it 'excludes secrets matching file paths up to the maximum allowed' do + expect(secret_detection_logger).to receive(:info) + .once + .with(message: log_messages[:found_secrets]) + + expect { secrets_check.validate! }.to raise_error do |error| + expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError) + expect(error.message).to include( + log_messages[:found_secrets], + finding_message( + commit_with_excluded_file_paths, + 'file-exclusion-2-skipped.rb', + 1, + 'GitLab Personal Access Token' + ), + log_messages[:found_secrets_post_message], + found_secrets_docs_link + ) + end + end + end + + context 'with less than or equal to the path exclusions limit' do + let_it_be(:commit_with_excluded_file_paths) do + create_commit( + # We support specifying a certain file path, e.g. `file-exclusion-1.txt`. + 'file-exclusion-1.txt' => 'SECRET=glpat-JUST20LETTERSANDNUMB', # gitleaks:allow + + # We also support simple globbing, e.g. `spec/**/*.rb`, so we try to ensure as many as possible are matched. + 'spec/file-exclusion-2.rb' => 'KEY=glpat-JUST20LETTERSANDNUMB', # gitleaks:allow + 'spec/fixtures/file-exclusion-3.rb' => 'PASS=glpat-JUST20LETTERSANDNUMB', # gitleaks:allow + 'spec/fixtures/reports/file-exclusion-4.rb' => 'KEY=glpat-JUST20LETTERSANDNUMB' # gitleaks:allow + ) + end + + let_it_be(:commit_with_not_excluded_file_path) do + create_commit('file-exclusion-4.txt' => 'TOKEN=glrt-JUST20LETTERSANDNUMB') # gitleaks:allow + end + + let(:changes) do + [ + { oldrev: initial_commit, newrev: commit_with_excluded_file_paths, ref: 'refs/heads/master' }, + { oldrev: initial_commit, newrev: commit_with_not_excluded_file_path, ref: 'refs/heads/master' } + ] + end + + before do + create( + :project_security_exclusion, + :active, + :with_path, + project: project, + value: 'file-exclusion-1.txt' + ) + + create( + :project_security_exclusion, + :active, + :with_path, + project: project, + value: 'spec/**/*.rb' + ) + end + + it 'excludes secrets matching file paths from findings' do + expect(secret_detection_logger).to receive(:info) + .once + .with(message: log_messages[:found_secrets]) + + expect { secrets_check.validate! }.to raise_error do |error| + expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError) + expect(error.message).to include( + log_messages[:found_secrets], + finding_message( + commit_with_not_excluded_file_path, + 'file-exclusion-4.txt', + 1, + 'GitLab Runner Authentication Token' + ), + log_messages[:found_secrets_post_message], + found_secrets_docs_link + ) + end + end + end + end + + context 'when exclusion is based on matching a rule from default ruleset' do + let_it_be(:commit_with_excluded_rule) do + create_commit('rule-exclusion-1.txt' => 'SECRET=glpat-JUST20LETTERSANDNUMB') # gitleaks:allow + end + + let_it_be(:commit_with_not_excluded_rule) do + create_commit('rule-exclusion-2.txt' => 'TOKEN=glrt-JUST20LETTERSANDNUMB') # gitleaks:allow + end + + let(:changes) do + [ + { oldrev: initial_commit, newrev: commit_with_excluded_rule, ref: 'refs/heads/master' }, + { oldrev: initial_commit, newrev: commit_with_not_excluded_rule, ref: 'refs/heads/master' } + ] + end + + before do + create(:project_security_exclusion, :active, :with_rule, project: project) + end + + it 'excludes secrets matching rule from findings' do + expect(secret_detection_logger).to receive(:info) + .once + .with(message: log_messages[:found_secrets]) + + expect { secrets_check.validate! }.to raise_error do |error| + expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError) + expect(error.message).to include( + log_messages[:found_secrets], + finding_message( + commit_with_not_excluded_rule, + 'rule-exclusion-2.txt', + 1, + 'GitLab Runner Authentication Token' + ), + log_messages[:found_secrets_post_message], + found_secrets_docs_link + ) + end + end + end + + context 'when exclusion is based on matching a raw value or string' do + let_it_be(:commit_with_excluded_value) do + create_commit('raw-value-exclusion-1.txt' => 'SECRET=glpat-01234567890123456789') # gitleaks:allow + end + + let_it_be(:commit_with_not_excluded_value) do + create_commit('raw-value-exclusion-2.txt' => 'TOKEN=glpat-JUST20LETTERSANDNUMB') # gitleaks:allow + end + + let(:changes) do + [ + { oldrev: initial_commit, newrev: commit_with_excluded_value, ref: 'refs/heads/master' }, + { oldrev: initial_commit, newrev: commit_with_not_excluded_value, ref: 'refs/heads/master' } + ] + end + + before do + create( + :project_security_exclusion, + :active, + :with_raw_value, + project: project, + value: 'glpat-01234567890123456789' # gitleaks:allow + ) + end + + it 'excludes secrets matching raw value from findings' do + expect(secret_detection_logger).to receive(:info) + .once + .with(message: log_messages[:found_secrets]) + + expect { secrets_check.validate! }.to raise_error do |error| + expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError) + expect(error.message).to include( + log_messages[:found_secrets], + finding_message( + commit_with_not_excluded_value, + 'raw-value-exclusion-2.txt', + 1, + 'GitLab Personal Access Token' + ), + log_messages[:found_secrets_post_message], + found_secrets_docs_link + ) + end + end + end +end + +# TODO: remove this shared example when `secret_detection_project_level_exclusions` feature flag is removed. +RSpec.shared_examples 'scan does not discard excluded secrets' do + include_context 'secrets check context' + + context 'with any type of exclusion' do + let_it_be(:commit_with_excluded_rule) do + create_commit('ff-disabled-exclusion-1.txt' => 'SECRET=glpat-JUST20LETTERSANDNUMB') # gitleaks:allow + end + + let_it_be(:commit_with_not_excluded_rule) do + create_commit('ff-disabled-exclusion-2.txt' => 'TOKEN=glrt-JUST20LETTERSANDNUMB') # gitleaks:allow + end + + let(:changes) do + [ + { oldrev: initial_commit, newrev: commit_with_excluded_rule, ref: 'refs/heads/master' }, + { oldrev: initial_commit, newrev: commit_with_not_excluded_rule, ref: 'refs/heads/master' } + ] + end + + before do + create(:project_security_exclusion, :active, :with_rule, project: project) + end + + it 'does not exclude secrets matching the rule from findings' do + expect(secret_detection_logger).to receive(:info) + .once + .with(message: log_messages[:found_secrets]) + + expect { secrets_check.validate! }.to raise_error do |error| + expect(error).to be_a(::Gitlab::GitAccess::ForbiddenError) + expect(error.message).to include( + log_messages[:found_secrets], + finding_message( + commit_with_excluded_rule, + 'ff-disabled-exclusion-1.txt', + 1, + 'GitLab Personal Access Token' + ), + finding_message( + commit_with_not_excluded_rule, + 'ff-disabled-exclusion-2.txt', + 1, + 'GitLab Runner Authentication Token' + ), + log_messages[:found_secrets_post_message], + found_secrets_docs_link + ) + end + end + end +end diff --git a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/response.rb b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/response.rb index a34fba7c0b60e4f7c00e6cf7192f39aaf5264a4d..e39c994889f96a6bc82d5ff6e88d81446892beec 100644 --- a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/response.rb +++ b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/response.rb @@ -7,7 +7,8 @@ module SecretDetection # +status+:: One of values from SecretDetection::Status indicating the scan operation's status # +results+:: Array of SecretDetection::Finding values. Default value is nil. class Response - attr_reader :status, :results + attr_reader :results + attr_accessor :status def initialize(status, results = nil) @status = status diff --git a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb index 1eb9ac167ff6318542fde7e509b7ab3e175da3fd..73d7dbcc47ab17e89bc4c3a0b33153b7df24b2ce 100644 --- a/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb +++ b/gems/gitlab-secret_detection/lib/gitlab/secret_detection/scan.rb @@ -58,6 +58,8 @@ def initialize(logger: Logger.new($stdout), ruleset_path: RULESET_FILE_PATH) # it instead groups the blobs into smaller array where each array contains blobs with cumulative size of # +MIN_CHUNK_SIZE_PER_PROC_BYTES+ bytes and each group runs in a separate sub-process. Default value # is true. + # +exclusions+:: A hash containing arrays of exclusions by their type. Types handled here are + # `raw_value` and `rule`. # # NOTE: # Running the scan in fork mode primarily focuses on reducing the memory consumption of the scan by @@ -76,7 +78,8 @@ def secrets_scan( blobs, timeout: DEFAULT_SCAN_TIMEOUT_SECS, blob_timeout: DEFAULT_BLOB_TIMEOUT_SECS, - subprocess: RUN_IN_SUBPROCESS + subprocess: RUN_IN_SUBPROCESS, + exclusions: {} ) return SecretDetection::Response.new(SecretDetection::Status::INPUT_ERROR) unless validate_scan_input(blobs) @@ -86,9 +89,9 @@ def secrets_scan( next SecretDetection::Response.new(SecretDetection::Status::NOT_FOUND) if matched_blobs.empty? secrets = if subprocess - run_scan_within_subprocess(matched_blobs, blob_timeout) + run_scan_within_subprocess(matched_blobs, blob_timeout, exclusions) else - run_scan(matched_blobs, blob_timeout) + run_scan(matched_blobs, blob_timeout, exclusions) end scan_status = overall_scan_status(secrets) @@ -155,21 +158,19 @@ def filter_by_keywords(blobs) matched_blobs.freeze end - def run_scan(blobs, blob_timeout) - found_secrets = blobs.flat_map do |blob| + def run_scan(blobs, blob_timeout, exclusions) + blobs.flat_map do |blob| Timeout.timeout(blob_timeout) do - find_secrets(blob) + find_secrets(blob, exclusions) end rescue Timeout::Error => e logger.error "Secret Detection scan timed out on the blob(id:#{blob.id}): #{e}" SecretDetection::Finding.new(blob.id, SecretDetection::Status::PAYLOAD_TIMEOUT) end - - found_secrets.freeze end - def run_scan_within_subprocess(blobs, blob_timeout) + def run_scan_within_subprocess(blobs, blob_timeout, exclusions) blob_sizes = blobs.map(&:size) grouped_blob_indicies = group_by_chunk_size(blob_sizes) @@ -182,7 +183,7 @@ def run_scan_within_subprocess(blobs, blob_timeout) ) do |grouped_blob| grouped_blob.flat_map do |blob| Timeout.timeout(blob_timeout) do - find_secrets(blob) + find_secrets(blob, exclusions) end rescue Timeout::Error => e logger.error "Secret Detection scan timed out on the blob(id:#{blob.id}): #{e}" @@ -195,10 +196,16 @@ def run_scan_within_subprocess(blobs, blob_timeout) end # finds secrets in the given blob with a timeout circuit breaker - def find_secrets(blob) + def find_secrets(blob, exclusions) secrets = [] blob.data.each_line.with_index do |line, index| + exclusions[:raw_value]&.each do |exclusion| + line.gsub!(exclusion.value, '') # remove excluded raw value from the line. + end + + next if line.empty? + patterns = pattern_matcher.match(line, exception: false) next unless patterns.any? @@ -206,6 +213,9 @@ def find_secrets(blob) line_number = index + 1 patterns.each do |pattern| type = rules[pattern]["id"] + + next if exclusions[:rule]&.any? { |exclusion| exclusion.value == type } + description = rules[pattern]["description"] secrets << SecretDetection::Finding.new( diff --git a/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb b/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb index b345a083aca3c9a759401c962087ae6e82c36cbc..98a200ecacfe063e005285642a965c0bec6c9164 100644 --- a/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb +++ b/gems/gitlab-secret_detection/spec/lib/gitlab/secret_detection/scan_spec.rb @@ -9,6 +9,10 @@ def new_blob(id:, data:) Struct.new(:id, :data).new(id, data) end + def create_exclusion(value:) + Struct.new(:value).new(value) + end + let(:ruleset) do { "title" => "gitleaks config", @@ -310,5 +314,102 @@ def new_blob(id:, data:) expect(scan.secrets_scan(all_large_blobs, blob_timeout: each_blob_timeout_secs)).to eq(expected_response) end end + + context "when using exclusions" do + let(:blobs) do + [ + new_blob(id: 111, data: "data with no secret"), + new_blob(id: 222, data: "GR134894145645645645645645645"), # gitleaks:allow + new_blob(id: 333, data: "GR134894145645645645645645789"), # gitleaks:allow + new_blob(id: 444, data: "GR134894112312312312312312312"), # gitleaks:allow + new_blob(id: 555, data: "glpat-12312312312312312312"), # gitleaks:allow, + new_blob( + id: 666, data: "test data\nglptt-1231231231231231231212312312312312312312\nline contd" # gitleaks:allow + ) + ] + end + + context "when excluding secrets based on raw values" do + let(:exclusions) do + { + raw_value: [ + create_exclusion(value: 'GR134894112312312312312312312'), # gitleaks:allow + create_exclusion(value: 'glpat-12312312312312312312') # gitleaks:allow + ] + } + end + + let(:valid_lines) do + [ + blobs[1].data, + blobs[2].data, + *blobs[5].data.lines + ] + end + + it "excludes values from being detected" do + expected_scan_status = Gitlab::SecretDetection::Status::FOUND + + expected_response = Gitlab::SecretDetection::Response.new( + expected_scan_status, + [ + Gitlab::SecretDetection::Finding.new( + blobs[1].id, + expected_scan_status, + 1, + ruleset['rules'][2]['id'], + ruleset['rules'][2]['description'] + ), + Gitlab::SecretDetection::Finding.new( + blobs[2].id, + expected_scan_status, + 1, + ruleset['rules'][2]['id'], + ruleset['rules'][2]['description'] + ), + Gitlab::SecretDetection::Finding.new( + blobs[5].id, + expected_scan_status, + 2, + ruleset['rules'][1]['id'], + ruleset['rules'][1]['description'] + ) + ] + ) + + expect(scan.secrets_scan(blobs, exclusions: exclusions)).to eq(expected_response) + end + end + + context "when excluding secrets based on rules from default ruleset" do + let(:exclusions) do + { + rule: [ + create_exclusion(value: "gitlab_runner_registration_token"), + create_exclusion(value: "gitlab_personal_access_token") + ] + } + end + + it 'filters out secrets matching excluded rules from detected findings' do + expected_scan_status = Gitlab::SecretDetection::Status::FOUND + + expected_response = Gitlab::SecretDetection::Response.new( + expected_scan_status, + [ + Gitlab::SecretDetection::Finding.new( + blobs[5].id, + expected_scan_status, + 2, + ruleset['rules'][1]['id'], + ruleset['rules'][1]['description'] + ) + ] + ) + + expect(scan.secrets_scan(blobs, exclusions: exclusions)).to eq(expected_response) + end + end + end end end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index a9416abbec4ee9d6e373f30deb2b15d5c3cb019b..323e5c31b7e9ed72b342332fe158de8abf8b3fa1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -10889,6 +10889,9 @@ msgstr "" msgid "Cannot delete the default organization" msgstr "" +msgid "Cannot have more than %{maximum_path_exclusions} path exclusions for secret push protection per project" +msgstr "" + msgid "Cannot have multiple Jira imports running at the same time" msgstr "" diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index 6357cc7bd1eed81c0d536d5781bfb4bd42ce13fa..33f027e31d5099cb31c40bb5a4d3a5e369dcb5c5 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -1058,4 +1058,28 @@ end end end + + describe '#valid_full_sha' do + before do + allow(commit).to receive(:id).and_return(value) + end + + let(:sha) { '5716ca5987cbf97d6bb54920bea6adde242d87e6' } + + context 'when commit id does not match the full sha pattern' do + let(:value) { sha[0, Gitlab::Git::Commit::SHA1_LENGTH - 1] } # doesn't match Gitlab::Git::Commit::FULL_SHA_PATTERN because length is less than 40 + + it 'returns nil' do + expect(commit.valid_full_sha).to be_empty + end + end + + context 'when commit id matches the full sha pattern' do + let(:value) { sha } + + it 'returns the sha as a string' do + expect(commit.valid_full_sha).to eq(sha) + end + end + end end