After adding .scan.scanner and .scan.type to the security reports as described in #202053 (closed), we should then remove .vulnerabilities[].scanner and .vulnerabilities[].category, for these are redundant.
remove Category and Scanner struct field from the Issue struct, in the common library, and update the Secure analyzers (SAST, Container Scanning, Secret Detection, and Dependency Scanning)
I'm setting this as blocked by #235390 since we might want to keep that object to reference the correct scanner if we allow for multiple ones in the same scan...
@gonzoyumo Actually I now disagree and believe we can move forward with the removal of .vulnerabilities[].scanner (and .vulnerabilities[].category). Even if we decide to have multiple scans in a single report, we'll need proper references to the scan objects (something like .vulnerabilities[].scan.id), and .vulnerabilities[].scanner won't be good enough. And in any case we'll have to update the schemas and make adjustments to allow for multiple scans. cc @cam_swords
@fcatteau, before we remove .vulnerabilities[].scanner from the analyzers/schema, Rails will need to be updated to parse scan.scanner instead of vulnerabilities[].scanner.
I agree that this is the direction we should head.
There is a counter-argument to this which is that a scanner could create vulnerabilities of any type. For example, maybe an IAST solution could create a DAST vulnerability and a SAST vulnerability? I think @d0c-s4vage was suggesting something along these lines.
@cam_swords This is indeed a real use case, for instance Anchore (3rd party) is capable of generating SAST, DS and CS results at the same time if I remember correctly.
Though, in such a case I would recommend to simply upload multiple artifacts from the same job as this is already supported by our architecture and I don't recall a use case where it would not be possible to split results into separate files.
NB: this also means we could later have a generic way to link results of different types, regardless of they are coming from one or multiple jobs.
I tried tracking the ramifications of this within the rails app but it's quite confusing since we refer to scan_type as scanner now . Best case: duplicate vulnerabilities_scanners entries, worst case: we break some data integrity between vulnerabilities and their scanners. We should probably loop groupthreat insights in prior to this change if they must modify their primary reference
It's worth verifying with groupthreat insights to see if they want to remove scan.type to enable "generic" security reports. If this is the case, then vulnerabilities[].category will not be able to be removed.
@gonzoyumo it sounds like the new generic template might be ready ? why don't we have a call to chat with @thiagocsf and @matt_wilson to chat though everything and see
The generic security report schema has been released as 13.0.0 but until #284062 (closed) is done (adds artifacts:reports:security_findings with a default to [gl-security-findings.json]), no reports will be ingested.
@NicoleSchwartz@matt_wilson I'm not really willing to proceed with this issue unless we have a clear idea of the future of our schema. As pointed in above threads this change might be undesirable depending on decisions we are not ready to make IMHO.
The immediate benefit would be some clarity and saved space in the report but we have to be confident on the direction we're taking, it's not an easy 2-way door change.
As we have higher priorities on better defined goals, I'd recommend pushing this into the backlog and revisit our need after we've rationalized the schemas.
It doesn't look like we can release schema 15.0.0, deprecate 14.*.* schemas, update all analyzers, and remove 14.*.* schemas by GitLab %15.0, so %Backlog is probably still a good choice.
@thiagocsf the plan is as above (copied below) which i believe that major of a change would need a version change as well? so 15.6 or 16.0 are our next chances?
remove Category and Scanner struct field from the Issue struct, in the common library, and update the Secure analyzers (SAST, Container Scanning, Secret Detection, and Dependency Scanning)
I think it makes sense to split the schema change from the analyzer updates.
Since removal of vulnerabilities[].scanner and vulnerabilities[].category is already captured in
#339812 (closed), I have added it as a blocker and made a note in the description.
i believe that major of a change would need a version change as well? so 15.6 or 16.0 are our next chances?
We could always release a patch version that makes the fields-to-be-removed optional. This way it unblocks the change on any integrations and reduces the coordination needed.