Merge branch 'improve_vuln_tracking-backend_use_fingerprints' into 'master'

Improve Vulnerability Tracking: Use Signatures [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!54608

app/models/concerns/vulnerability_finding_helpers.rb

+# frozen_string_literal: true
+
+module VulnerabilityFindingHelpers
+  extend ActiveSupport::Concern
+end
+
+VulnerabilityFindingHelpers.prepend_if_ee('EE::VulnerabilityFindingHelpers')

app/models/concerns/vulnerability_finding_signature_helpers.rb

+# frozen_string_literal: true
+
+module VulnerabilityFindingSignatureHelpers
+  extend ActiveSupport::Concern
+end
+
+VulnerabilityFindingSignatureHelpers.prepend_if_ee('EE::VulnerabilityFindingSignatureHelpers')

ee/app/finders/security/findings_finder.rb

@@ -47,10 +47,13 @@ module Security
       report_finding = report_finding_for(security_finding)
       return Vulnerabilities::Finding.new unless report_finding
 
-      finding_data = report_finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :links)
+      finding_data = report_finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :links, :signatures)
       identifiers = report_finding.identifiers.map do |identifier|
         Vulnerabilities::Identifier.new(identifier.to_hash)
       end
+      signatures = report_finding.signatures.map do |signature|
+        Vulnerabilities::FindingSignature.new(signature.to_hash)
+      end
 
       Vulnerabilities::Finding.new(finding_data).tap do |finding|
         finding.location_fingerprint = report_finding.location.fingerprint
@@ -59,6 +62,7 @@ module Security
         finding.sha = pipeline.sha
         finding.scanner = security_finding.scanner
         finding.identifiers = identifiers
+        finding.signatures = signatures
       end
     end
 

ee/app/finders/security/pipeline_vulnerabilities_finder.rb

@@ -75,7 +75,7 @@ module Security
     def normalize_report_findings(report_findings, vulnerabilities)
       report_findings.map do |report_finding|
         finding_hash = report_finding.to_hash
-          .except(:compare_key, :identifiers, :location, :scanner, :links)
+          .except(:compare_key, :identifiers, :location, :scanner, :links, :signatures)
 
         finding = Vulnerabilities::Finding.new(finding_hash)
         # assigning Vulnerabilities to Findings to enable the computed state
@@ -90,6 +90,9 @@ module Security
         finding.identifiers = report_finding.identifiers.map do |identifier|
           Vulnerabilities::Identifier.new(identifier.to_hash)
         end
+        finding.signatures = report_finding.signatures.map do |signature|
+          Vulnerabilities::FindingSignature.new(signature.to_hash)
+        end
 
         finding
       end
@@ -111,18 +114,36 @@ module Security
     end
 
     def dismissal_feedback?(finding)
-      dismissal_feedback_by_fingerprint[finding.project_fingerprint]
+      if ::Feature.enabled?(:vulnerability_finding_signatures, pipeline.project) && !finding.signatures.empty?
+        dismissal_feedback_by_finding_signatures(finding)
+      else
+        dismissal_feedback_by_project_fingerprint(finding)
+      end
+    end
+
+    def all_dismissal_feedbacks
+      strong_memoize(:all_dismissal_feedbacks) do
+        pipeline.project
+          .vulnerability_feedback
+          .for_dismissal
+      end
+    end
+
+    def dismissal_feedback_by_finding_signatures(finding)
+      potential_uuids = Set.new([*finding.signature_uuids, finding.uuid].compact)
+      all_dismissal_feedbacks.any? { |dismissal| potential_uuids.include?(dismissal.finding_uuid) }
     end
 
     def dismissal_feedback_by_fingerprint
       strong_memoize(:dismissal_feedback_by_fingerprint) do
-        pipeline.project
-                .vulnerability_feedback
-                .for_dismissal
-                .group_by(&:project_fingerprint)
+        all_dismissal_feedbacks.group_by(&:project_fingerprint)
       end
     end
 
+    def dismissal_feedback_by_project_fingerprint(finding)
+      dismissal_feedback_by_fingerprint[finding.project_fingerprint]
+    end
+
     def confidence_levels
       Array(params.fetch(:confidence, Vulnerabilities::Finding.confidences.keys))
     end

ee/app/models/concerns/ee/vulnerability_finding_helpers.rb

+# frozen_string_literal: true
+
+module EE
+  module VulnerabilityFindingHelpers
+    extend ActiveSupport::Concern
+
+    def matches_signatures(other_signatures, other_uuid)
+      other_signature_types = other_signatures.index_by(&:algorithm_type)
+
+      # highest first
+      match_result = nil
+      signatures.sort_by(&:priority).reverse_each do |signature|
+        matching_other_signature = other_signature_types[signature.algorithm_type]
+        next if matching_other_signature.nil?
+
+        match_result = matching_other_signature == signature
+        break
+      end
+
+      if match_result.nil?
+        [uuid, *signature_uuids].include?(other_uuid)
+      else
+        match_result
+      end
+    end
+
+    def signature_uuids
+      signatures.map do |signature|
+        hex_sha = signature.signature_hex
+        ::Security::VulnerabilityUUID.generate(
+          report_type: report_type,
+          location_fingerprint: hex_sha,
+          primary_identifier_fingerprint: primary_identifier&.fingerprint,
+          project_id: project_id
+        )
+      end
+    end
+  end
+end

ee/app/models/concerns/ee/vulnerability_finding_signature_helpers.rb

+# frozen_string_literal: true
+
+module EE
+  module VulnerabilityFindingSignatureHelpers
+    extend ActiveSupport::Concern
+
+    # If the location object describes a physical location within a file
+    # (filename + line numbers), the 'location' algorithm_type should be used
+    #
+    # If the location object describes arbitrary data, then the 'hash'
+    # algorithm_type should be used.
+    PRIORITIES = {
+      scope_offset: 3,
+      location: 2,
+      hash: 1
+    }.with_indifferent_access.freeze
+
+    class_methods do
+      def priority(algorithm_type)
+        raise ArgumentError.new("No priority for #{algorithm_type.inspect}") unless PRIORITIES.key?(algorithm_type)
+
+        PRIORITIES[algorithm_type]
+      end
+    end
+
+    def priority
+      self.class.priority(algorithm_type)
+    end
+  end
+end

ee/app/models/license.rb

@@ -180,6 +180,7 @@ class License < ApplicationRecord
     subepics
     threat_monitoring
     vulnerability_auto_fix
+    vulnerability_finding_signatures
   ]
   EEU_FEATURES.freeze
 

ee/app/models/vulnerabilities/finding.rb

@@ -5,6 +5,7 @@ module Vulnerabilities
     include ShaAttribute
     include ::Gitlab::Utils::StrongMemoize
     include Presentable
+    include ::VulnerabilityFindingHelpers
 
     # https://gitlab.com/groups/gitlab-org/-/epics/3148
     # https://gitlab.com/gitlab-org/gitlab/-/issues/214563#note_370782508 is why the table names are not renamed
@@ -332,12 +333,16 @@ module Vulnerabilities
       end
     end
 
-    alias_method :==, :eql? # eql? is necessary in some cases like array intersection
+    alias_method :==, :eql?
 
     def eql?(other)
-      other.report_type == report_type &&
-        other.location_fingerprint == location_fingerprint &&
-        other.first_fingerprint == first_fingerprint
+      return false unless other.report_type == report_type && other.primary_identifier_fingerprint == primary_identifier_fingerprint
+
+      if ::Feature.enabled?(:vulnerability_finding_signatures, project)
+        matches_signatures(other.signatures, other.uuid)
+      else
+        other.location_fingerprint == location_fingerprint
+      end
     end
 
     # Array.difference (-) method uses hash and eql? methods to do comparison
@@ -348,7 +353,7 @@ module Vulnerabilities
       # when Finding is persisted and identifiers are not preloaded.
       return super if persisted? && !identifiers.loaded?
 
-      report_type.hash ^ location_fingerprint.hash ^ first_fingerprint.hash
+      report_type.hash ^ location_fingerprint.hash ^ primary_identifier_fingerprint.hash
     end
 
     def severity_value
@@ -380,7 +385,7 @@ module Vulnerabilities
 
     protected
 
-    def first_fingerprint
+    def primary_identifier_fingerprint
       identifiers.first&.fingerprint
     end
 

ee/app/models/vulnerabilities/finding_signature.rb

@@ -5,11 +5,23 @@ module Vulnerabilities
     self.table_name = 'vulnerability_finding_signatures'
 
     include BulkInsertSafe
+    include VulnerabilityFindingSignatureHelpers
 
     belongs_to :finding, foreign_key: 'finding_id', inverse_of: :signatures, class_name: 'Vulnerabilities::Finding'
 
     enum algorithm_type: { hash: 1, location: 2, scope_offset: 3 }, _prefix: :algorithm
 
     validates :finding, presence: true
+
+    def signature_hex
+      signature_sha.unpack1("H*")
+    end
+
+    def eql?(other)
+      other.algorithm_type == algorithm_type &&
+        other.signature_sha == signature_sha
+    end
+
+    alias_method :==, :eql?
   end
 end

ee/app/services/security/store_report_service.rb

@@ -61,17 +61,21 @@ module Security
         return
       end
 
-      vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links)
+      vulnerability_params = finding.to_hash.except(:compare_key, :identifiers, :location, :scanner, :scan, :links, :signatures)
       entity_params = Gitlab::Json.parse(vulnerability_params&.dig(:raw_metadata)).slice('description', 'message', 'solution', 'cve', 'location')
       # Vulnerabilities::Finding (`vulnerability_occurrences`)
       vulnerability_finding = vulnerability_findings_by_uuid[finding.uuid] ||
-        create_new_vulnerability_finding(finding, vulnerability_params.merge(entity_params))
+        find_or_create_vulnerability_finding(finding, vulnerability_params.merge(entity_params))
 
       update_vulnerability_scanner(finding) unless Feature.enabled?(:optimize_sql_query_for_security_report, project)
 
       update_vulnerability_finding(vulnerability_finding, vulnerability_params)
       reset_remediations_for(vulnerability_finding, finding)
-      update_finding_signatures(finding, vulnerability_finding)
+
+      if ::Feature.enabled?(:vulnerability_finding_signatures, project)
+        update_feedbacks(vulnerability_finding, vulnerability_params[:uuid])
+        update_finding_signatures(finding, vulnerability_finding)
+      end
 
       # The maximum number of identifiers is not used in validation
       # we just want to ignore the rest if a finding has more than that.
@@ -86,8 +90,16 @@ module Security
       create_vulnerability(vulnerability_finding, pipeline)
     end
 
+    def find_or_create_vulnerability_finding(finding, create_params)
+      if ::Feature.enabled?(:vulnerability_finding_signatures, project)
+        find_or_create_vulnerability_finding_with_signatures(finding, create_params)
+      else
+        find_or_create_vulnerability_finding_with_location(finding, create_params)
+      end
+    end
+
     # rubocop: disable CodeReuse/ActiveRecord
-    def create_new_vulnerability_finding(finding, create_params)
+    def find_or_create_vulnerability_finding_with_location(finding, create_params)
       find_params = {
         scanner: scanners_objects[finding.scanner.key],
         primary_identifier: identifiers_objects[finding.primary_identifier.key],
@@ -120,6 +132,56 @@ module Security
       end
     end
 
+    def get_matched_findings(finding, normalized_signatures, find_params)
+      project.vulnerability_findings.where(**find_params).filter do |vf|
+        vf.matches_signatures(normalized_signatures, finding.uuid)
+      end
+    end
+
+    def find_or_create_vulnerability_finding_with_signatures(finding, create_params)
+      find_params = {
+        # this isn't taking prioritization into account (happens in the filter
+        # block below), but it *does* limit the number of findings we have to sift through
+        location_fingerprint: [finding.location.fingerprint, *finding.signatures.map(&:signature_hex)],
+        scanner: scanners_objects[finding.scanner.key],
+        primary_identifier: identifiers_objects[finding.primary_identifier.key]
+      }
+
+      normalized_signatures = finding.signatures.map do |signature|
+        ::Vulnerabilities::FindingSignature.new(signature.to_hash)
+      end
+
+      matched_findings = get_matched_findings(finding, normalized_signatures, find_params)
+
+      begin
+        vulnerability_finding = matched_findings.first
+        if vulnerability_finding.nil?
+          find_params[:uuid] = finding.uuid
+          vulnerability_finding = project
+            .vulnerability_findings
+            .create_with(create_params)
+            .find_or_initialize_by(find_params)
+        end
+
+        vulnerability_finding.uuid = finding.uuid
+
+        vulnerability_finding.location_fingerprint = if finding.signatures.empty?
+                                                       finding.location.fingerprint
+                                                     else
+                                                       finding.signatures.max_by(&:priority).signature_hex
+                                                     end
+
+        vulnerability_finding.location = create_params.dig(:location)
+        vulnerability_finding.save!
+
+        vulnerability_finding
+      rescue ActiveRecord::RecordNotUnique
+        get_matched_findings(finding, normalized_signatures, find_params).first
+      rescue ActiveRecord::RecordInvalid => e
+        Gitlab::ErrorTracking.track_and_raise_exception(e, create_params: create_params&.dig(:raw_metadata))
+      end
+    end
+
     def update_vulnerability_scanner(finding)
       scanner = scanners_objects[finding.scanner.key]
       scanner.update!(finding.scanner.to_hash)
@@ -223,6 +285,14 @@ module Security
     end
     # rubocop: enable CodeReuse/ActiveRecord
 
+    def update_feedbacks(vulnerability_finding, new_uuid)
+      vulnerability_finding.load_feedback.each do |feedback|
+        feedback.finding_uuid = new_uuid
+        feedback.vulnerability_data = vulnerability_finding.raw_metadata
+        feedback.save!
+      end
+    end
+
     def update_finding_signatures(finding, vulnerability_finding)
       to_update = {}
       to_create = []
@@ -240,12 +310,12 @@ module Security
         next if poro_signature.nil?
 
         poro_signatures.delete(signature.algorithm_type)
-        to_update[signature.id] = poro_signature.to_h
+        to_update[signature.id] = poro_signature.to_hash
       end
 
       # any remaining poro signatures left are new
       poro_signatures.values.each do |poro_signature|
-        attributes = poro_signature.to_h.merge(finding_id: vulnerability_finding.id)
+        attributes = poro_signature.to_hash.merge(finding_id: vulnerability_finding.id)
         to_create << ::Vulnerabilities::FindingSignature.new(attributes: attributes, created_at: Time.zone.now, updated_at: Time.zone.now)
       end
 

ee/config/feature_flags/development/vulnerability_finding_signatures.yml

+---
+name: vulnerability_finding_signatures
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/54608
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/322044
+milestone: '13.11'
+type: development
+group: group::vulnerability research
+default_enabled: false

ee/lib/gitlab/ci/parsers/security/common.rb

@@ -7,13 +7,14 @@ module Gitlab
         class Common
           SecurityReportParserError = Class.new(Gitlab::Ci::Parsers::ParserError)
 
-          def self.parse!(json_data, report)
-            new(json_data, report).parse!
+          def self.parse!(json_data, report, vulnerability_finding_signatures_enabled = false)
+            new(json_data, report, vulnerability_finding_signatures_enabled).parse!
           end
 
-          def initialize(json_data, report)
+          def initialize(json_data, report, vulnerability_finding_signatures_enabled = false)
             @json_data = json_data
             @report = report
+            @vulnerability_finding_signatures_enabled = vulnerability_finding_signatures_enabled
           end
 
           def parse!
@@ -80,11 +81,20 @@ module Gitlab
             links = create_links(data['links'])
             location = create_location(data['location'] || {})
             remediations = create_remediations(data['remediations'])
-            signatures = create_signatures(tracking_data(data))
+            signatures = create_signatures(location, tracking_data(data))
+
+            if @vulnerability_finding_signatures_enabled && !signatures.empty?
+              # NOT the signature_sha - the compare key is hashed
+              # to create the project_fingerprint
+              highest_priority_signature = signatures.max_by(&:priority)
+              uuid = calculate_uuid_v5(identifiers.first, highest_priority_signature.signature_hex)
+            else
+              uuid = calculate_uuid_v5(identifiers.first, location&.fingerprint)
+            end
 
             report.add_finding(
               ::Gitlab::Ci::Reports::Security::Finding.new(
-                uuid: calculate_uuid_v5(identifiers.first, location),
+                uuid: uuid,
                 report_type: report.type,
                 name: finding_name(data, identifiers, location),
                 compare_key: data['cve'] || '',
@@ -99,14 +109,17 @@ module Gitlab
                 raw_metadata: data.to_json,
                 metadata_version: report_version,
                 details: data['details'] || {},
-                signatures: signatures))
+                signatures: signatures,
+                project_id: report.project_id,
+                vulnerability_finding_signatures_enabled: @vulnerability_finding_signatures_enabled))
           end
 
-          def create_signatures(data)
-            return [] if data.nil? || data['items'].nil?
+          def create_signatures(location, tracking)
+            tracking ||= { 'items' => [] }
 
             signature_algorithms = Hash.new { |hash, key| hash[key] = [] }
-            data['items'].each do |item|
+
+            tracking['items'].each do |item|
               next unless item.key?('signatures')
 
               item['signatures'].each do |signature|
@@ -117,14 +130,16 @@ module Gitlab
 
             signature_algorithms.map do |algorithm, values|
               value = values.join('|')
-              begin
-                signature = ::Gitlab::Ci::Reports::Security::FindingSignature.new(
-                  algorithm_type: algorithm,
-                  signature_value: value
-                )
-                signature.valid? ? signature : nil
-              rescue ArgumentError => e
-                Gitlab::ErrorTracking.track_and_raise_for_dev_exception(e)
+              signature = ::Gitlab::Ci::Reports::Security::FindingSignature.new(
+                algorithm_type: algorithm,
+                signature_value: value
+              )
+
+              if signature.valid?
+                signature
+              else
+                e = SecurityReportParserError.new("Vulnerability tracking signature is not valid: #{signature}")
+                Gitlab::ErrorTracking.track_exception(e)
                 nil
               end
             end.compact
@@ -201,11 +216,11 @@ module Gitlab
             "#{identifier.name} in #{location&.fingerprint_path}"
           end
 
-          def calculate_uuid_v5(primary_identifier, location)
+          def calculate_uuid_v5(primary_identifier, location_fingerprint)
             uuid_v5_name_components = {
               report_type: report.type,
               primary_identifier_fingerprint: primary_identifier&.fingerprint,
-              location_fingerprint: location&.fingerprint,
+              location_fingerprint: location_fingerprint,
               project_id: report.project_id
             }
 

ee/lib/gitlab/ci/reports/security/finding.rb

@@ -5,6 +5,8 @@ module Gitlab
     module Reports
       module Security
         class Finding
+          include ::VulnerabilityFindingHelpers
+
           UNSAFE_SEVERITIES = %w[unknown high critical].freeze
 
           attr_reader :compare_key
@@ -25,10 +27,11 @@ module Gitlab
           attr_reader :remediations
           attr_reader :details
           attr_reader :signatures
+          attr_reader :project_id
 
           delegate :file_path, :start_line, :end_line, to: :location
 
-          def initialize(compare_key:, identifiers:, links: [], remediations: [], location:, metadata_version:, name:, raw_metadata:, report_type:, scanner:, scan:, uuid:, confidence: nil, severity: nil, details: {}, signatures: []) # rubocop:disable Metrics/ParameterLists
+          def initialize(compare_key:, identifiers:, links: [], remediations: [], location:, metadata_version:, name:, raw_metadata:, report_type:, scanner:, scan:, uuid:, confidence: nil, severity: nil, details: {}, signatures: [], project_id: nil, vulnerability_finding_signatures_enabled: false) # rubocop:disable Metrics/ParameterLists
             @compare_key = compare_key
             @confidence = confidence
             @identifiers = identifiers
@@ -45,6 +48,8 @@ module Gitlab
             @remediations = remediations
             @details = details
             @signatures = signatures
+            @project_id = project_id
+            @vulnerability_finding_signatures_enabled = vulnerability_finding_signatures_enabled
 
             @project_fingerprint = generate_project_fingerprint
           end
@@ -66,6 +71,7 @@ module Gitlab
               severity
               uuid
               details
+              signatures
             ].each_with_object({}) do |key, hash|
               hash[key] = public_send(key) # rubocop:disable GitlabSecurity/PublicSend
             end
@@ -85,13 +91,22 @@ module Gitlab
           end
 
           def eql?(other)
-            report_type == other.report_type &&
-              location.fingerprint == other.location.fingerprint &&
-              primary_fingerprint == other.primary_fingerprint
+            return false unless report_type == other.report_type && primary_identifier_fingerprint == other.primary_identifier_fingerprint
+
+            if @vulnerability_finding_signatures_enabled
+              matches_signatures(other.signatures, other.uuid)
+            else
+              location.fingerprint == other.location.fingerprint
+            end
           end
 
           def hash
-            report_type.hash ^ location.fingerprint.hash ^ primary_fingerprint.hash
+            if @vulnerability_finding_signatures_enabled && !signatures.empty?
+              highest_signature = signatures.max_by(&:priority)
+              report_type.hash ^ highest_signature.signature_hex.hash ^ primary_identifier_fingerprint.hash
+            else
+              report_type.hash ^ location.fingerprint.hash ^ primary_identifier_fingerprint.hash
+            end
           end
 
           def valid?
@@ -104,7 +119,7 @@ module Gitlab
             end
           end
 
-          def primary_fingerprint
+          def primary_identifier_fingerprint
             primary_identifier&.fingerprint
           end
 

ee/lib/gitlab/ci/reports/security/finding_signature.rb

@@ -5,6 +5,8 @@ module Gitlab
     module Reports
       module Security
         class FindingSignature
+          include VulnerabilityFindingSignatureHelpers
+
           attr_accessor :algorithm_type, :signature_value
 
           def initialize(params = {})
@@ -12,11 +14,19 @@ module Gitlab
             @signature_value = params.dig(:signature_value)
           end
 
+          def priority
+            ::Vulnerabilities::FindingSignature.priority(algorithm_type)
+          end
+
           def signature_sha
             Digest::SHA1.digest(signature_value)
           end
 
-          def to_h
+          def signature_hex
+            signature_sha.unpack1("H*")
+          end
+
+          def to_hash
             {
               algorithm_type: algorithm_type,
               signature_sha: signature_sha
@@ -26,6 +36,13 @@ module Gitlab
           def valid?
             ::Vulnerabilities::FindingSignature.algorithm_types.key?(algorithm_type)
           end
+
+          def eql?(other)
+            other.algorithm_type == algorithm_type &&
+              other.signature_sha == signature_sha
+          end
+
+          alias_method :==, :eql?
         end
       end
     end

ee/lib/gitlab/ci/reports/security/locations/container_scanning.rb

@@ -18,12 +18,12 @@ module Gitlab
               @package_version = package_version
             end
 
-            private
-
             def fingerprint_data
               "#{docker_image_name_without_tag}:#{package_name}"
             end
 
+            private
+
             def docker_image_name_without_tag
               base_name, version = image.split(':')
 

ee/lib/gitlab/ci/reports/security/locations/coverage_fuzzing.rb

@@ -16,8 +16,6 @@ module Gitlab
               @crash_state = crash_state
             end
 
-            private
-
             def fingerprint_data
               "#{crash_type}:#{crash_state}"
             end

ee/lib/gitlab/ci/reports/security/locations/dast.rb

@@ -20,8 +20,6 @@ module Gitlab
 
             alias_method :fingerprint_path, :path
 
-            private
-
             def fingerprint_data
               "#{path}:#{method_name}:#{param}"
             end

ee/lib/gitlab/ci/reports/security/locations/dependency_scanning.rb

@@ -18,8 +18,6 @@ module Gitlab
               @package_version = package_version
             end
 
-            private
-
             def fingerprint_data
               "#{file_path}:#{package_name}"
             end

ee/lib/gitlab/ci/reports/security/locations/sast.rb

@@ -22,8 +22,6 @@ module Gitlab
               @start_line = start_line
             end
 
-            private
-
             def fingerprint_data
               "#{file_path}:#{start_line}:#{end_line}"
             end

ee/lib/gitlab/ci/reports/security/locations/secret_detection.rb

@@ -22,8 +22,6 @@ module Gitlab
               @start_line = start_line
             end
 
-            private
-
             def fingerprint_data
               "#{file_path}:#{start_line}:#{end_line}"
             end

ee/lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb

@@ -7,13 +7,17 @@ module Gitlab
         class VulnerabilityReportsComparer
           include Gitlab::Utils::StrongMemoize
 
-          attr_reader :base_report, :head_report
+          attr_reader :base_report, :head_report, :added, :fixed
 
           ACCEPTABLE_REPORT_AGE = 1.week
 
           def initialize(base_report, head_report)
             @base_report = base_report
             @head_report = head_report
+
+            @added = []
+            @fixed = []
+            calculate_changes
           end
 
           def base_report_created_at
@@ -30,16 +34,30 @@ module Gitlab
             ACCEPTABLE_REPORT_AGE.ago > @base_report.created_at
           end
 
-          def added
-            strong_memoize(:added) do
-              head_report.findings - base_report.findings
-            end
-          end
+          def calculate_changes
+            base_findings = base_report.findings
+            head_findings = head_report.findings
+
+            head_findings_hash = head_findings.index_by(&:object_id)
 
-          def fixed
-            strong_memoize(:fixed) do
-              base_report.findings - head_report.findings
+            # This is slow - O(N^2). If we didn't need to worry about one high
+            # priority fingerprint that doesn't match overruling a lower
+            # priority fingerprint that does match, we'd be able to do some
+            # set operations here
+            base_findings.each do |base_finding|
+              still_exists = false
+              head_findings.each do |head_finding|
+                next unless base_finding.eql?(head_finding)
+
+                still_exists = true
+                head_findings_hash.delete(head_finding.object_id)
+                break
+              end
+
+              @fixed << base_finding unless still_exists
             end
+
+            @added = head_findings_hash.values
           end
         end
       end

ee/spec/factories/ci/reports/security/findings.rb

@@ -39,6 +39,7 @@ FactoryBot.define do
         project_id: n
       )
     end
+    vulnerability_finding_signatures_enabled { false }
 
     skip_create
 

ee/spec/factories/vulnerabilities/feedback.rb

@@ -17,6 +17,7 @@ FactoryBot.define do
     category { 'sast' }
     project_fingerprint { generate(:project_fingerprint) }
     vulnerability_data { { category: 'sast' } }
+    finding_uuid { Gitlab::UUID.v5(SecureRandom.hex) }
 
     trait :dismissal do
       feedback_type { 'dismissal' }

ee/spec/finders/security/pipeline_vulnerabilities_finder_spec.rb

@@ -83,12 +83,12 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
         # use the same number of queries, regardless of the number of findings
         # contained in the pipeline report.
 
-        sast_findings = pipeline.security_reports.reports['sast'].findings
+        container_scanning_findings = pipeline.security_reports.reports['container_scanning'].findings
         dep_findings = pipeline.security_reports.reports['dependency_scanning'].findings
-        # this test is invalid if we don't have more sast findings than dep findings
-        expect(sast_findings.count).to be > dep_findings.count
+        # this test is invalid if we don't have more container_scanning findings than dep findings
+        expect(container_scanning_findings.count).to be > dep_findings.count
 
-        (sast_findings + dep_findings).each do |report_finding|
+        (container_scanning_findings + dep_findings).each do |report_finding|
           # create a finding and a vulnerability for each report finding
           # (the vulnerability is created with the :confirmed trait)
           create(:vulnerabilities_finding,
@@ -103,7 +103,7 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
 
         # should be the same number of queries between different report types
         expect do
-          described_class.new(pipeline: pipeline, params: { report_type: %w[sast] }).execute
+          described_class.new(pipeline: pipeline, params: { report_type: %w[container_scanning] }).execute
         end.to issue_same_number_of_queries_as {
           described_class.new(pipeline: pipeline, params: { report_type: %w[dependency_scanning] }).execute
         }
@@ -117,11 +117,11 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
         orig_security_reports = pipeline.security_reports
         new_finding = create(:ci_reports_security_finding)
         expect do
-          described_class.new(pipeline: pipeline, params: { report_type: %w[sast] }).execute
+          described_class.new(pipeline: pipeline, params: { report_type: %w[container_scanning] }).execute
         end.to issue_same_number_of_queries_as {
-          orig_security_reports.reports['sast'].add_finding(new_finding)
+          orig_security_reports.reports['container_scanning'].add_finding(new_finding)
           allow(pipeline).to receive(:security_reports).and_return(orig_security_reports)
-          described_class.new(pipeline: pipeline, params: { report_type: %w[sast] }).execute
+          described_class.new(pipeline: pipeline, params: { report_type: %w[container_scanning] }).execute
         }
       end
     end
@@ -130,9 +130,11 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
       context 'when sast' do
         let(:params) { { report_type: %w[sast] } }
         let(:sast_report_fingerprints) {pipeline.security_reports.reports['sast'].findings.map(&:location).map(&:fingerprint) }
+        let(:sast_report_uuids) {pipeline.security_reports.reports['sast'].findings.map(&:uuid) }
 
         it 'includes only sast' do
           expect(subject.findings.map(&:location_fingerprint)).to match_array(sast_report_fingerprints)
+          expect(subject.findings.map(&:uuid)).to match_array(sast_report_uuids)
           expect(subject.findings.count).to eq(sast_count)
         end
       end
@@ -172,52 +174,107 @@ RSpec.describe Security::PipelineVulnerabilitiesFinder do
       let(:ds_finding) { pipeline.security_reports.reports["dependency_scanning"].findings.first }
       let(:sast_finding) { pipeline.security_reports.reports["sast"].findings.first }
 
-      let!(:feedback) do
-        [
-          create(
-            :vulnerability_feedback,
-            :dismissal,
-            :dependency_scanning,
-            project: project,
-            pipeline: pipeline,
-            project_fingerprint: ds_finding.project_fingerprint,
-            vulnerability_data: ds_finding.raw_metadata
-          ),
-          create(
-            :vulnerability_feedback,
-            :dismissal,
-            :sast,
-            project: project,
-            pipeline: pipeline,
-            project_fingerprint: sast_finding.project_fingerprint,
-            vulnerability_data: sast_finding.raw_metadata
-          )
-        ]
-      end
+      context 'when vulnerability_finding_signatures feature flag is disabled' do
+        let!(:feedback) do
+          [
+            create(
+              :vulnerability_feedback,
+              :dismissal,
+              :dependency_scanning,
+              project: project,
+              pipeline: pipeline,
+              project_fingerprint: ds_finding.project_fingerprint,
+              vulnerability_data: ds_finding.raw_metadata,
+              finding_uuid: ds_finding.uuid
+            ),
+            create(
+              :vulnerability_feedback,
+              :dismissal,
+              :sast,
+              project: project,
+              pipeline: pipeline,
+              project_fingerprint: sast_finding.project_fingerprint,
+              vulnerability_data: sast_finding.raw_metadata,
+              finding_uuid: sast_finding.uuid
+            )
+          ]
+        end
 
-      context 'when unscoped' do
-        subject { described_class.new(pipeline: pipeline).execute }
+        before do
+          stub_feature_flags(vulnerability_finding_signatures: false)
+        end
+
+        context 'when unscoped' do
+          subject { described_class.new(pipeline: pipeline).execute }
 
-        it 'returns non-dismissed vulnerabilities' do
-          expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count - feedback.count)
-          expect(subject.findings.map(&:project_fingerprint)).not_to include(*feedback.map(&:project_fingerprint))
+          it 'returns non-dismissed vulnerabilities' do
+            expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count - feedback.count)
+            expect(subject.findings.map(&:project_fingerprint)).not_to include(*feedback.map(&:project_fingerprint))
+          end
         end
-      end
 
-      context 'when `dismissed`' do
-        subject { described_class.new(pipeline: pipeline, params: { report_type: %w[dependency_scanning], scope: 'dismissed' } ).execute }
+        context 'when `dismissed`' do
+          subject { described_class.new(pipeline: pipeline, params: { report_type: %w[dependency_scanning], scope: 'dismissed' } ).execute }
 
-        it 'returns non-dismissed vulnerabilities' do
-          expect(subject.findings.count).to eq(ds_count - 1)
-          expect(subject.findings.map(&:project_fingerprint)).not_to include(ds_finding.project_fingerprint)
+          it 'returns non-dismissed vulnerabilities' do
+            expect(subject.findings.count).to eq(ds_count - 1)
+            expect(subject.findings.map(&:project_fingerprint)).not_to include(ds_finding.project_fingerprint)
+          end
+        end
+
+        context 'when `all`' do
+          let(:params) { { report_type: %w[sast dast container_scanning dependency_scanning], scope: 'all' } }
+
+          it 'returns all vulnerabilities' do
+            expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count)
+          end
         end
       end
 
-      context 'when `all`' do
-        let(:params) { { report_type: %w[sast dast container_scanning dependency_scanning], scope: 'all' } }
+      context 'when vulnerability_finding_signatures feature flag is enabled' do
+        let!(:feedback) do
+          [
+            create(
+              :vulnerability_feedback,
+              :dismissal,
+              :sast,
+              project: project,
+              pipeline: pipeline,
+              project_fingerprint: sast_finding.project_fingerprint,
+              vulnerability_data: sast_finding.raw_metadata,
+              finding_uuid: sast_finding.uuid
+            )
+          ]
+        end
 
-        it 'returns all vulnerabilities' do
-          expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count)
+        before do
+          stub_feature_flags(vulnerability_finding_signatures: true)
+        end
+
+        context 'when unscoped' do
+          subject { described_class.new(pipeline: pipeline).execute }
+
+          it 'returns non-dismissed vulnerabilities' do
+            expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count - feedback.count)
+            expect(subject.findings.map(&:project_fingerprint)).not_to include(*feedback.map(&:project_fingerprint))
+          end
+        end
+
+        context 'when `dismissed`' do
+          subject { described_class.new(pipeline: pipeline, params: { report_type: %w[sast], scope: 'dismissed' } ).execute }
+
+          it 'returns non-dismissed vulnerabilities' do
+            expect(subject.findings.count).to eq(sast_count - 1)
+            expect(subject.findings.map(&:project_fingerprint)).not_to include(sast_finding.project_fingerprint)
+          end
+        end
+
+        context 'when `all`' do
+          let(:params) { { report_type: %w[sast dast container_scanning dependency_scanning], scope: 'all' } }
+
+          it 'returns all vulnerabilities' do
+            expect(subject.findings.count).to eq(cs_count + dast_count + ds_count + sast_count)
+          end
         end
       end
     end

ee/spec/lib/gitlab/ci/reports/security/finding_signature_spec.rb

@@ -18,15 +18,12 @@ RSpec.describe Gitlab::Ci::Reports::Security::FindingSignature do
         expect(subject.algorithm_type).to eq(params[:algorithm_type])
         expect(subject.signature_value).to eq(params[:signature_value])
       end
-    end
-  end
 
-  describe '#to_h' do
-    it 'returns a hash representation of the signature' do
-      expect(subject.to_h).to eq(
-        algorithm_type: params[:algorithm_type],
-        signature_sha: Digest::SHA1.digest(params[:signature_value])
-      )
+      describe '#valid?' do
+        it 'returns true' do
+          expect(subject.valid?).to eq(true)
+        end
+      end
     end
   end
 
@@ -50,4 +47,13 @@ RSpec.describe Gitlab::Ci::Reports::Security::FindingSignature do
       end
     end
   end
+
+  describe '#to_hash' do
+    it 'returns a hash representation of the signature' do
+      expect(subject.to_hash).to eq(
+        algorithm_type: params[:algorithm_type],
+        signature_sha: Digest::SHA1.digest(params[:signature_value])
+      )
+    end
+  end
 end

ee/spec/lib/gitlab/ci/reports/security/finding_spec.rb

@@ -137,7 +137,8 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
         scan: occurrence.scan,
         severity: occurrence.severity,
         uuid: occurrence.uuid,
-        details: occurrence.details
+        details: occurrence.details,
+        signatures: []
       })
     end
   end
@@ -197,87 +198,98 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
   end
 
   describe '#eql?' do
-    let(:identifier) { build(:ci_reports_security_identifier) }
-    let(:location) { build(:ci_reports_security_locations_sast) }
-    let(:finding) { build(:ci_reports_security_finding, severity: 'low', report_type: :sast, identifiers: [identifier], location: location) }
-
-    let(:report_type) { :secret_detection }
-    let(:identifier_external_id) { 'foo' }
-    let(:location_start_line) { 0 }
-    let(:other_identifier) { build(:ci_reports_security_identifier, external_id: identifier_external_id) }
-    let(:other_location) { build(:ci_reports_security_locations_sast, start_line: location_start_line) }
-    let(:other_finding) do
-      build(:ci_reports_security_finding,
-            severity: 'low',
-            report_type: report_type,
-            identifiers: [other_identifier],
-            location: other_location)
-    end
+    where(vulnerability_finding_signatures_enabled: [true, false])
+    with_them do
+      let(:identifier) { build(:ci_reports_security_identifier) }
+      let(:location) { build(:ci_reports_security_locations_sast) }
+      let(:finding) { build(:ci_reports_security_finding, severity: 'low', report_type: :sast, identifiers: [identifier], location: location, vulnerability_finding_signatures_enabled: vulnerability_finding_signatures_enabled) }
+
+      let(:report_type) { :secret_detection }
+      let(:identifier_external_id) { 'foo' }
+      let(:location_start_line) { 0 }
+      let(:other_identifier) { build(:ci_reports_security_identifier, external_id: identifier_external_id) }
+      let(:other_location) { build(:ci_reports_security_locations_sast, start_line: location_start_line) }
+      let(:other_finding) do
+        build(:ci_reports_security_finding,
+              severity: 'low',
+              report_type: report_type,
+              identifiers: [other_identifier],
+              location: other_location,
+              vulnerability_finding_signatures_enabled: vulnerability_finding_signatures_enabled)
+      end
 
-    subject { finding.eql?(other_finding) }
+      let(:signature) { ::Gitlab::Ci::Reports::Security::FindingSignature.new(algorithm_type: 'location', signature_value: 'value1') }
 
-    context 'when the primary_identifier is nil' do
-      let(:identifier) { nil }
+      subject { finding.eql?(other_finding) }
 
-      it 'does not raise an exception' do
-        expect { subject }.not_to raise_error
+      context 'when the primary_identifier is nil' do
+        let(:identifier) { nil }
+
+        it 'does not raise an exception' do
+          expect { subject }.not_to raise_error
+        end
       end
-    end
 
-    context 'when the other finding has same `report_type`' do
-      let(:report_type) { :sast }
+      context 'when the other finding has same `report_type`' do
+        let(:report_type) { :sast }
 
-      context 'when the other finding has same primary identifier fingerprint' do
-        let(:identifier_external_id) { identifier.external_id }
+        context 'when the other finding has same primary identifier fingerprint' do
+          let(:identifier_external_id) { identifier.external_id }
 
-        context 'when the other finding has same location fingerprint' do
-          let(:location_start_line) { location.start_line }
+          context 'when the other finding has same location signature' do
+            before do
+              finding.signatures << signature
+              other_finding.signatures << signature
+            end
 
-          it { is_expected.to be(true) }
-        end
+            let(:location_start_line) { location.start_line }
 
-        context 'when the other finding does not have same location fingerprint' do
-          it { is_expected.to be(false) }
+            it { is_expected.to be(true) }
+          end
+
+          context 'when the other finding does not have same location signature' do
+            it { is_expected.to be(false) }
+          end
         end
-      end
 
-      context 'when the other finding does not have same primary identifier fingerprint' do
-        context 'when the other finding has same location fingerprint' do
-          let(:location_start_line) { location.start_line }
+        context 'when the other finding does not have same primary identifier fingerprint' do
+          context 'when the other finding has same location signature' do
+            let(:location_start_line) { location.start_line }
 
-          it { is_expected.to be(false) }
-        end
+            it { is_expected.to be(false) }
+          end
 
-        context 'when the other finding does not have same location fingerprint' do
-          it { is_expected.to be(false) }
+          context 'when the other finding does not have same location signature' do
+            it { is_expected.to be(false) }
+          end
         end
       end
-    end
 
-    context 'when the other finding does not have same `report_type`' do
-      context 'when the other finding has same primary identifier fingerprint' do
-        let(:identifier_external_id) { identifier.external_id }
+      context 'when the other finding does not have same `report_type`' do
+        context 'when the other finding has same primary identifier fingerprint' do
+          let(:identifier_external_id) { identifier.external_id }
 
-        context 'when the other finding has same location fingerprint' do
-          let(:location_start_line) { location.start_line }
+          context 'when the other finding has same location signature' do
+            let(:location_start_line) { location.start_line }
 
-          it { is_expected.to be(false) }
-        end
+            it { is_expected.to be(false) }
+          end
 
-        context 'when the other finding does not have same location fingerprint' do
-          it { is_expected.to be(false) }
+          context 'when the other finding does not have same location signature' do
+            it { is_expected.to be(false) }
+          end
         end
-      end
 
-      context 'when the other finding does not have same primary identifier fingerprint' do
-        context 'when the other finding has same location fingerprint' do
-          let(:location_start_line) { location.start_line }
+        context 'when the other finding does not have same primary identifier fingerprint' do
+          context 'when the other finding has same location signature' do
+            let(:location_start_line) { location.start_line }
 
-          it { is_expected.to be(false) }
-        end
+            it { is_expected.to be(false) }
+          end
 
-        context 'when the other finding does not have same location fingerprint' do
-          it { is_expected.to be(false) }
+          context 'when the other finding does not have same location signature' do
+            it { is_expected.to be(false) }
+          end
         end
       end
     end
@@ -345,4 +357,55 @@ RSpec.describe Gitlab::Ci::Reports::Security::Finding do
 
     it { is_expected.to match_array(expected_keys) }
   end
+
+  describe '#hash' do
+    let(:scanner) { build(:ci_reports_security_scanner) }
+    let(:identifiers) { [build(:ci_reports_security_identifier)] }
+    let(:location) { build(:ci_reports_security_locations_sast) }
+    let(:uuid) { SecureRandom.uuid }
+
+    context 'with vulnerability_finding_signatures enabled' do
+      let(:finding) do
+        build(:ci_reports_security_finding,
+              scanner: scanner,
+              identifiers: identifiers,
+              location: location,
+              uuid: uuid,
+              compare_key: '',
+              vulnerability_finding_signatures_enabled: true)
+      end
+
+      let(:low_priority_signature) { ::Gitlab::Ci::Reports::Security::FindingSignature.new(algorithm_type: 'location', signature_value: 'value1') }
+      let(:high_priority_signature) { ::Gitlab::Ci::Reports::Security::FindingSignature.new(algorithm_type: 'scope_offset', signature_value: 'value2') }
+
+      it 'returns the expected hash with no signatures' do
+        expect(finding.signatures.length).to eq(0)
+        expect(finding.hash).to eq(finding.report_type.hash ^ finding.location.fingerprint.hash ^ finding.primary_identifier_fingerprint.hash)
+      end
+
+      it 'returns the expected hash with signatures' do
+        finding.signatures << low_priority_signature
+        finding.signatures << high_priority_signature
+
+        expect(finding.signatures.length).to eq(2)
+        expect(finding.hash).to eq(finding.report_type.hash ^ high_priority_signature.signature_hex.hash ^ finding.primary_identifier_fingerprint.hash)
+      end
+    end
+
+    context 'without vulnerability_finding_signatures enabled' do
+      let(:finding) do
+        build(:ci_reports_security_finding,
+              scanner: scanner,
+              identifiers: identifiers,
+              location: location,
+              uuid: uuid,
+              compare_key: '',
+              vulnerability_finding_signatures_enabled: false)
+      end
+
+      it 'returns the expected hash' do
+        expect(finding.hash).to eq(finding.report_type.hash ^ finding.location.fingerprint.hash ^ finding.primary_identifier_fingerprint.hash)
+      end
+    end
+  end
 end

ee/spec/models/concerns/vulnerability_finding_signature_helpers_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe VulnerabilityFindingSignatureHelpers do
+  let(:cls) do
+    Class.new do
+      include VulnerabilityFindingSignatureHelpers
+      attr_accessor :algorithm_type
+
+      def initialize(algorithm_type)
+        @algorithm_type = algorithm_type
+      end
+    end
+  end
+
+  describe '#priority' do
+    it 'returns numeric values of the priority string' do
+      expect(cls.new('scope_offset').priority).to eq(3)
+      expect(cls.new('location').priority).to eq(2)
+      expect(cls.new('hash').priority).to eq(1)
+    end
+  end
+
+  describe '#self.priority' do
+    it 'returns the numeric value of the provided string' do
+      expect(cls.priority('scope_offset')).to eq(3)
+      expect(cls.priority('location')).to eq(2)
+      expect(cls.priority('hash')).to eq(1)
+    end
+  end
+end

ee/spec/requests/api/vulnerability_findings_spec.rb

@@ -33,7 +33,8 @@ RSpec.describe API::VulnerabilityFindings do
              project: project,
              pipeline: pipeline,
              project_fingerprint: sast_report.findings.first.project_fingerprint,
-             vulnerability_data: sast_report.findings.first.raw_metadata
+             vulnerability_data: sast_report.findings.first.raw_metadata,
+             finding_uuid: sast_report.findings.first.uuid
       )
     end
 

ee/spec/serializers/vulnerabilities/feedback_entity_spec.rb

@@ -164,7 +164,7 @@ RSpec.describe Vulnerabilities::FeedbackEntity do
   end
 
   context 'when finding_uuid is not present' do
-    let(:feedback) { build_stubbed(:vulnerability_feedback, :issue, project: project) }
+    let(:feedback) { build_stubbed(:vulnerability_feedback, :issue, project: project, finding_uuid: nil) }
 
     it 'has a nil finding_uuid' do
       expect(subject[:finding_uuid]).to be_nil