Make sast and secret_detection methods available to CE
The frontend work for the epic: &4388 has been blocked as we do not have the SAST and Secret Detection vulnerabilities comparison endpoints available to CE. In this issue, we will move those endpoints to CE and investigate the side-effect for these changes. This will unblock frontend work. Additionally, we need to move some more methods from EE to CE. Here is an attempt: !43716 (closed). We will try to investigate this draft MR and try to provide a solution.
Where we are at (as of March 5th, 2021)
Merged and Reverted
Draft MR (57 files changed)
Draft MR (87 files changed)
Merged - several changes were made, but underlying classes are still in EE, so feature will not work in CE yet
Merged - behind Feature Flag, cannot be enabled until this Issue is complete
Merged
Known remaining work
Files changed/moved in Saikat's draft MRs Draft: Move security reports comparer to CE and Draft: Move models related to vulnerabilities to CE
Either one or both of these MRs will need to be finished, or a new MR will need to be started based on the work that they've done.
- app/finders/security/pipeline_vulnerabilities_finder.rb
- app/models/ci/pipeline.rb
- app/models/project.rb
- app/models/vulnerabilities/export.rb
- app/models/vulnerabilities/external_issue_link.rb
- app/models/vulnerabilities/feedback.rb
- app/models/vulnerabilities/finding.rb
- app/models/vulnerabilities/finding_identifier.rb
- app/models/vulnerabilities/finding_link.rb
- app/models/vulnerabilities/finding_pipeline.rb
- app/models/vulnerabilities/finding_remediation.rb
- app/models/vulnerabilities/historical_statistic.rb
- app/models/vulnerabilities/identifier.rb
- app/models/vulnerabilities/issue_link.rb
- app/models/vulnerabilities/projects_grade.rb
- app/models/vulnerabilities/remediation.rb
- app/models/vulnerabilities/scanner.rb
- app/models/vulnerabilities/stat_diff.rb
- app/models/vulnerabilities/statistic.rb
- app/policies/base_policy.rb
- app/policies/project_policy.rb
- app/policies/vulnerabilities/export_policy.rb
- app/policies/vulnerabilities/feedback_policy.rb
- app/policies/vulnerabilities/issue_link_policy.rb
- app/policies/vulnerabilities/scanner_policy.rb
- app/serializers/vulnerabilities/feedback_entity.rb
- app/serializers/vulnerabilities/feedback_serializer.rb
- app/serializers/vulnerabilities/finding_diff_serializer.rb
- app/serializers/vulnerabilities/finding_entity.rb
- app/serializers/vulnerabilities/finding_reports_comparer_entity.rb
- app/serializers/vulnerabilities/finding_serializer.rb
- app/serializers/vulnerabilities/identifier_entity.rb
- app/serializers/vulnerabilities/request_entity.rb
- app/serializers/vulnerabilities/response_entity.rb
- app/serializers/vulnerabilities/scanner_entity.rb
- app/services/ci/compare_security_reports_service.rb
- ee/app/models/ee/ci/pipeline.rb
- ee/app/models/ee/project.rb
- ee/app/policies/ee/base_policy.rb
- ee/app/policies/ee/project_policy.rb
- ee/changelogs/unreleased/move_security_reports.yml
- ee/changelogs/unreleased/move_vulnerability_models.yml
- lib/gitlab/ci/reports/security/aggregated_report.rb
- lib/gitlab/ci/reports/security/report.rb
- lib/gitlab/ci/reports/security/reports.rb
- lib/gitlab/ci/reports/security/vulnerability_reports_comparer.rb
- spec/factories/ci/reports/security/aggregated_reports.rb
- spec/factories/ci/reports/security/reports.rb
- spec/factories/users.rb
- spec/factories/vulnerabilities/exports.rb
- spec/factories/vulnerabilities/external_issue_links.rb
- spec/factories/vulnerabilities/feedback.rb
- spec/factories/vulnerabilities/finding_identifiers.rb
- spec/factories/vulnerabilities/finding_links.rb
- spec/factories/vulnerabilities/finding_pipelines.rb
- spec/factories/vulnerabilities/findings.rb
- spec/factories/vulnerabilities/historical_statistics.rb
- spec/factories/vulnerabilities/identifiers.rb
- spec/factories/vulnerabilities/issue_links.rb
- spec/factories/vulnerabilities/scanners.rb
- spec/factories/vulnerabilities/statistics.rb
- spec/finders/security/pipeline_vulnerabilities_finder_spec.rb
- spec/lib/gitlab/ci/reports/security/aggregated_report_spec.rb
- spec/lib/gitlab/ci/reports/security/vulnerability_reports_comparer_spec.rb
- spec/models/vulnerabilities/export_spec.rb
- spec/models/vulnerabilities/external_issue_link_spec.rb
- spec/models/vulnerabilities/feedback_spec.rb
- spec/models/vulnerabilities/finding_identifier_spec.rb
- spec/models/vulnerabilities/finding_link_spec.rb
- spec/models/vulnerabilities/finding_pipeline_spec.rb
- spec/models/vulnerabilities/finding_remediation_spec.rb
- spec/models/vulnerabilities/finding_spec.rb
- spec/models/vulnerabilities/historical_statistic_spec.rb
- spec/models/vulnerabilities/identifier_spec.rb
- spec/models/vulnerabilities/issue_link_spec.rb
- spec/models/vulnerabilities/projects_grade_spec.rb
- spec/models/vulnerabilities/remediation_spec.rb
- spec/models/vulnerabilities/scanner_spec.rb
- spec/models/vulnerabilities/stat_diff_spec.rb
- spec/models/vulnerabilities/statistic_spec.rb
- spec/policies/vulnerabilities/export_policy_spec.rb
- spec/policies/vulnerabilities/feedback_policy_spec.rb
- spec/policies/vulnerabilities/issue_link_policy_spec.rb
- spec/policies/vulnerabilities/scanner_policy_spec.rb
- spec/serializers/vulnerabilities/feedback_entity_spec.rb
- spec/serializers/vulnerabilities/finding_entity_spec.rb
- spec/serializers/vulnerabilities/finding_reports_comparer_entity_spec.rb
- spec/serializers/vulnerabilities/finding_serializer_spec.rb
- spec/serializers/vulnerabilities/identifier_entity_spec.rb
- spec/serializers/vulnerabilities/request_entity_spec.rb
- spec/serializers/vulnerabilities/response_entity_spec.rb
- spec/serializers/vulnerabilities/scanner_entity_spec.rb
- spec/services/ci/compare_security_reports_service_spec.rb
Additional files to change/move based on Mehmet's suggestion
These files will also need to be changed/moved.
- ee/app/services/security/merge_reports_service.rb
- ee/lib/gitlab/ci/parsers/security/common.rb
- ee/spec/lib/gitlab/ci/parsers/security/common_spec.rb
- ee/spec/services/security/merge_reports_service_spec.rb
Difficulties
- It may not be possible to break down into separate chunks
- Scope of the changes may not be fully known
- File locations/names, etc. will change over time, potentially making some of the info in this Issue outdated
Storing reports high level flow (default branch only)
- CI job scans project, generates, and uploads
gl-sast-report.jsonjob artifact - When pipeline finishes, a background job is started. This worker merges all of the pipeline's jobs security reports together to deduplicate findings.
graph LR
A[CI SAST job runs scan] -->|Upload gl-sast-report.json| B(Deserialize to POROs)
B --> C{Security::StoreReportService saves to DB}
C -->D[INSERT INTO vulnerability_occurrences]
C -->E[INSERT INTO vulnerabilities]
C -->F[INSERT INTO vulnerability_scanners]
C -->G[INSERT INTO vulnerability_finding_pipelines]
C -->H[INSERT INTO vulnerability_identifiers]
C -->I[INSERT INTO ...]callstack
Ci::Pipeline- ActiveRecord model calls lifecycle hook when pipeline status changes to completeSecurity::StoreReportsService#store_reports- CallsStoreReportServiceonce per report type to persist findings to DBCi::Pipeline#security_reports- iterate over builds and merge each build job's artifacts togetherCi::Build#collect_security_reports!Ci::Build#parse_security_artifact_blobGitLab::Ci::Parsers.fabricate!Gitlab::Ci::Parsers::Security::Sast.newinherits fromCommonGitlab::Ci::Parsers::Security::Sast#create_vulnerability- iterates over JSON, instantiates findings, and appends them to::Gitlab::Ci::Reports::Security::Reports.new(that was passed fromCi::Pipeline) with POROs
StoreReportService- Persists all findings to postgresql DBSecurity::MergeReportsService- Merges multiple JSON reports into one (to dedupe findings)Vulnerabilities::UpdateServiceVulnerabilities::CreateService
notes
ci_buildis the internal name for a CI Job, the terms are used interchangeably- There is a lot of passthrough here but the general idea is:
- a "pipeline" is composed of "CI builds"
- a build has JSON artifacts
- a "security report" is composed of many findings
- to "collect security reports" a build will call a "CI parser" to deserialize JSON into POROs
- to merge multiple JSON reports together, an empty PORO "CI report" is instantiated at the pipeline level and passed down and each JSON report is appended onto the PORO report
- CI parsers exist in both CE and EE. If you examine the CE parsers file and the EE parsers file](https://gitlab.com/gitlab-org/gitlab/-/blob/8b3a22a68b9d3338775dc455231cb2dd87895423/ee/lib/ee/gitlab/ci/parsers.rb#L10) you can see which are only available in EE (which up until now has meant all security parsers are under EE).
Displaying MR reports
graph LR
A[MergeRequestController#sast_reports fetches added/fixed findings]
A --> C{Security::CompareSecurityReportsService}
C -->D[Security::PipelineVulnerabilitiesFinder deserializes JSON to POROs]
C -->E[VulnerabilityReportsComparer diffs source and target reports]callstack
Projects::MergeRequestsController#sast_reports# Controller endpointMergeRequests# ActiveRecord modelCi::CompareSecurityReportsService# Returns hash of "added" and "fixed" findings to diff in the frontendSecurity::PipelineVulnerabilitiesFinder# Finder service for normalizing JSON into POROsGitlab::Ci::Reports::Security::VulnerabilityReportsComparer# Performant comparison between reports
Migration path
Phase 1 (starts June 7th)
- Move POROs into CE (
ee/lib/gitlab/ci/**)- Everything under
ee/lib/gitlab/ci/reports/securitytolib/gitlab/ci/reports/security(except non SASTlocations) Gitlab::Ci::Parsers::Security::CommonGitlab::Ci::Parsers::Security::SastandGitlab::Ci::Parsersreference
- Everything under
- Move
Security::MergeReportsServiceto theapp/services/security
Phase 2 (starts July 12th)
- Move the
Gitlab::Ci::Reports::Security::VulnerabilityReportsComparerto the CE directory level
Phase 3 (starts August 2nd)
- Move the
Ci::CompareSecurityReportsServiceto theapp/services/ci/directory - (AS NEEDED) Move the bulk of these classes into CE with some EE-only features remaining in EE.
Vulnerabilities::FindingDiffSerializer&&FindingEntitywith associate modelsSecurity::PipelineVulnerabilitiesFinder
Vulnerabilityshould remain under EEFindingshould remain under CE