Add worker to trigger package metadata advisory sync

Why are we doing this work

Add a scheduled worker and a sync service to ingest advisories exported by License DB into the vulnerability_advisories DB table of the main Postgres DB.

See sync protocol discussed in #394723 (comment 1318712867)

Relevant links

• #394723 (comment 1318712867)

Non-functional requirements

• Documentation: n/a
• Feature flag: n/a
• Performance: n/a
• Testing: n/a

Version format 2

Version format 2 uses ndjson to store data: v2/<purl_type>/<timestamp>/<chunk>.ndjson.

Advisory data

Advisory data is stored one advisory per line. For example, { name: "rails", affected_range: ">=1.1.0 <1.1.6", identifier: "CVE-2006-4111", etc. }.

The fields will correspond with the yaml fields in the advisory-database repo: https://gitlab.com/gitlab-org/security-products/gemnasium-db/-/blob/master/conan/bison/CVE-2020-14150.yml

See research spike for more info and discussion: #394723 (closed)

Full specification here: https://gitlab.com/gitlab-org/security-products/gemnasium-db/-/tree/master#yaml-schema

Implementation plan

• Work to update the connector and data object for protocol v2 is covered in Support advisories and affected packages data s... (#406323 - closed)
• Work to update ingestion is covered in Ingest advisory and affected package data into ... (#406836 - closed)

Sync worker

Syncing advisories is done in parallel because of the different database access patterns (different tables and no foreign keys to existing data).

• add PackageMetadata::SyncAdvisoriesWorker and extract PackageMetadata::SyncWorker functionality into PackageMetadata::SyncLicensesWorker
  • add cron entry for this worker (similar to the one for existing)
  • add queue entry for this worker (similar to existing)

Sync service

• PackageMetadata::SyncService add parsing of data based on data_type.

Add feature flag

• Add feature flag for advisory sync.

See this MR for how data parsing will change: !120795 (diffs)

Some updates to sync_service may be required depending on the changes in the above MR.

added to epic &8025 (closed)

changed milestone to %Backlog

added Category:Container Scanning Category:Dependency Scanning [DEPRECATED] Category:GitLab Advisory Database WorkingGroupContinuousScanning backend devopsgovern featureaddition groupsecurity policies sectionsec typefeature workflowrefinement labels

changed the description

changed milestone to %15.4

set weight to 3

mentioned in issue #363545 (closed)

@bauerdominic, could you please refine this issue?

Bot policy.

assigned to @bauerdominic

assigned to @sashi_kumar and unassigned @bauerdominic

changed title from Create or update vulnerability_advisories when gemnasium advisories is updated to Add scheduled sidekiq job to update vulerability_advisories when advisories are updated

changed the description

set weight to 2

mentioned in issue #371063 (closed)

marked this issue as related to #363545 (closed)

Moving to workflowready for development and adding [SKIP] Seed the `vulnerability_advisories` tabl... (#363545 - closed) as a blocker

added workflowready for development label and removed workflowrefinement label

unassigned @sashi_kumar

added [deprecated] Accepting merge requests label

Setting label(s) Category:Security Policy Management based on groupsecurity policies.

added Category:Security Policy Management label

added Threat InsightsTangerine groupthreat insights labels and removed groupsecurity policies label

removed Category:Security Policy Management label

removed Category:GitLab Advisory Database label

added Category:Vulnerability Management label

mentioned in issue gitlab-org/quality/triage-reports#9127 (closed)

mentioned in issue gitlab-org/quality/triage-reports#9202 (closed)

added quad-planningcomplete-no-action label

changed milestone to %15.5

removed Threat InsightsTangerine label

added groupcomposition analysis label and removed groupthreat insights label

added devopssecure label and removed devopsgovern label

Moving to groupcomposition analysis per this discussion

removed WorkingGroupContinuousScanning label

removed Category:Vulnerability Management label

changed milestone to %Backlog

changed title from Add scheduled sidekiq job to update vulerability_advisories when advisories are updated to Add scheduled sync worker for vulnerability advisories

changed the description

@ifrenkel FYI I've removed details from the issue description since the exact implementation will depend on the outcome of Spike: How do we sync the backend with a source... (#394723 - closed); you're currently working on that spike issue.

Also, I've updated the title to align it with Add scheduled sync background worker for packag... (#383719 - closed).

Finally, I'm moving it back to workflowplanning breakdown since we might need multiple implementation issues to achieve this.

added workflowplanning breakdown label and removed workflowready for development label

changed title from Add scheduled sync worker for vulnerability advisories to Scheduled sync worker to ingest advisories into main DB

changed the description

marked this issue as related to #394723 (closed)

@ifrenkel @hacks4oats Does any of you want to refine this issue? You might split that up and create additional issues as part of the planning breakdown. New issues should be added to the same epic.

I can take this one.

@ifrenkel @fcatteau I've started filling out some of the implementation plan, but have some questions about the contents of the objects contained in the new lines. When reading the option selected, I got the impression that a line in the export could contain either a package license object or an advisory object. Here's an example of what I believe this could look like (note that I used a mock format for the advisory object).

{ "name": "packageA", "version": "1.0.0", license: "MIT" }
{ "cve": "CVE-2023-00001", "description": "..." }

If this is true, then I think this might create some complexity for the parsing and validation steps. I'd like to propose an approach where we export all advisories and licenses separately, and then reference them in the package metadata object via identifiers. This would allow us to simplify the validation logic, and it would also reduce the amount of times we try to insert a license. Here's an example of how this new structure would look like:

{ "name": "packageA", "version": "1.0.0", licenses: ["MIT", "AGPLv2"], advisories: [ "CVE-2023-00001", "CVE-2023-00002"] }

For licenses and advisories, we would change the bulk insertable task so that we only create relationships between the package/advisories and drop the attempt to create a new license/advisory record. WDYT?

When reading the option selected, I got the impression that a line in the export could contain either a package license object or an advisory object.

@hacks4oats License information and advisories would be exported separately, using separate Cloud Run services, and separate GCP Buckets for export.

For licenses and advisories, we would change the bulk insertable task so that we only create relationships between the package/advisories and drop the attempt to create a new license/advisory record. WDYT?

If we go for full database normalization then the backend should upsert into pm_packages when ingesting advisories, and also when ingesting licenses–like today.

However, we might denormalize the database schema to achieve better performance, and to better isolate Vulnerability Scanning from License Scanning.

It's related to two issues:

• Improve performance of package license query to... (#398679 - closed); see #398679 (comment 1327053461).
• Advisories stored in vulnerability_advisories a... (#375302 - closed); see #375302 (comment 1318686113) cc @ifrenkel

changed title from Scheduled sync worker to ingest advisories into main DB to Sync worker to ingest advisories into main DB

assigned to @hacks4oats

changed title from Sync worker to ingest advisories into main DB to Add worker to ingest new advisories into main DB

changed the description

mentioned in issue #394723 (closed)

changed the description

Updating the current classes vs creating new ones

In the current implementation plan I've added, the assumption has been to create new v2 classes that we would use when an FF is used (this would need to be created and could be something like package_metadata_synchronization_v2). The motivation for this is to provide an easy way to back out the changes in the case where there's an issue with the new version format or the sync implementation.

Pros

• FF toggles are faster than an MR revert
• Clear distinction between the v1 and v2 implementations
• We can improve on the classes with some of the lessons learned in the first implementation. For example, we could have a base class that's shared by both the GCP and Offline connectors.

Cons

• There is some duplication of code.

changed the description

Unfortunately, I wasn't able to complete the refinement for this before my PTO due to some higher priority work on the new license scanning feature. I'll be unassigning myself from this issue, but can help refine/work on this once I return.

Thanks for working on it @hacks4oats. I can take it over.

unassigned @hacks4oats

mentioned in issue #396762 (closed)

assigned to @ifrenkel

@fcatteau do we have an issue for updating the schema yet or is it still contingent on planning discussions/spikes?

@ifrenkel If you're talking about the schema of the main DB, then I didn't see the need for an issue; I considered that it would be sufficient to do Advisories stored in vulnerability_advisories a... (#375302 - closed).

That said, we probably need to alter the pm_checkpoints table to track import of advisories. Feel free to create an issue for this, or to simply cover it as a task of this issue.

Do you see anything else?

changed milestone to %15.11

added featureenhancement label and removed featureaddition label

mentioned in epic &8025 (closed)

Alternate implementation plan discussion

the implementation plan looks good but I have an alternate suggestion regarding the SyncWorker

@hacks4oats apologies about this. I didn't check over this comment before making it. I realize that we already have this entrypoint with the Evented worker.

There are a few points that are still useful to discuss though:

• we could push csv/json parsing out of DataObject into a dedicated parser which could handle either format
• this would allow us to still handle v1 format which would be useful in case we split licenses from CVS MVC
• Ingestion service may be a good candidate for splitting between advisories and licenses. On the one hand it might be nice to add an advisory step to the current ingestion as it would just work. OTH the advisories step wouldn't be related to the other data. WDYT?

• we could push csv/json parsing out of DataObject into a dedicated parser which could handle either format
• this would allow us to still handle v1 format which would be useful in case we split licenses from CVS MVC

I'm not sure how this would look like, and might need an example to better understand how this implementation would look. Would the parser parse each file instead of each line?

• Ingestion service may be a good candidate for splitting between advisories and licenses. On the one hand it might be nice to add an advisory step to the current ingestion as it would just work. OTH the advisories step wouldn't be related to the other data.

My choice would be to split the advisories so that we could keep the license scanning and dependency scanning concerns separate. The other benefit I see to this is that having a separate ingestion job for advisories makes it possible to speed up the initial sync with the external license db and can be a control/status in the admin panel described to be introduced by Show status of Package Metadata sync to instanc... (#394747).

Thanks for your feedback @hacks4oats. I have updated the implementation plan. Can you take a look at and let me know what you think?

I'm thinking that this needs to be broken up into at least 2 issues. WDYT?

PS: I removed the old implementation plan relying on the diff in the timeline to save the old version.

@ifrenkel Thanks for updating the implementation plan! I've reread it and have one minor question. Should the advisory worker be PackageMetadata::Worker::SyncAdvisoriesWorker or is it possible to use PackageMetadata::SyncLicensesWorker instead? The latter is a bit more concise, but I'm not sure if this abides to the Ruby way of doing things.

I'm thinking that this needs to be broken up into at least 2 issues.

That makes sense to me Do you think we can separate the issues so that the advisory work is separated from the license scanning work? My thinking behind this is that we can prioritize the advisory scanning issues higher than the license scanning ones (since the new license scanning has now launched). One possibility of the breakdown could look like this:

• Create AdvisorySyncWorker
• Create AdvisorySyncService
• Create DataFile and AdvisoryDataObject

Edit: Also, I wanted to say thank you for linking the relevant discussions about the format of the advisories. This helped clear a lot of the confusion I had around the structure of the data (I wasn't aware that we would use the same format used in GLAD).

PackageMetadata::Worker::SyncAdvisoriesWorker

Thanks. That was a typo on my part.

separate the issues so that the advisory work is separated from the license scanning work

@hacks4oats are you talking about not doing the renames in this case? Like don't rename the sync service to PackageMetadata::LicenseSyncService but instead have PackageMetadata::SyncService and PackageMetadata::AdvisorySyncService?

@ifrenkel I was thinking of doing them, but after introducing the advisory specific classes. Maybe I'm being too cautious here, but to me keeping the code where helps with stability of the license scanning feature while we work on ingesting advisories. I don't have a strong opinion on this and can see the clarity brought by renaming PackageMetadata::SyncService to PackageMetadata::LicenseSyncService when creating PackageMetadata::AdvisorySyncService .

@hacks4oats these are good points. One thing to note is that the 2 sync services would share a lot of common functionality, to the point where one of the points in the plan is to extract it up into a base class. If we do this, we touching license stuff anyways. WDYT?

marked this issue as blocked by #375302 (closed)

I'm marking this issue as blocked by Advisories stored in vulnerability_advisories a... (#375302 - closed) because the vulnerability_advisories table will change as part of this issue.

@ifrenkel Is this where we'll share the specs of the new export format for advisories?

@fcatteau good call. I will update the description with the format.

Great! Thank you!

changed the description

changed the description

Validation

@hacks4oats the previous implementation plan had a point about adding a validator. I temporarily excluded from the current implementation plan because I want to discuss it.

The validators referenced have to do with public schemas whereas our sync protocol is not for 3rd parties to publish or consume but rather it's between the external db and the monolith. So I was thinking that's not really necessary. WDYT?

@ifrenkel I added the validator to the implementation plan to act as a safeguard during the parsing of the exported artifacts. I vaguely remember that some entries in the license exports were missing a field which caused the synchronization to error, and thought that validation could help guard against something like this.

Pros

• We can track entries that don't pass validation either through a counter, logs, or both.

Cons

• There's a performance impact for validating each line (small or large).

@hacks4oats that's an interesting point. What would you say to me creating another issue for validation as a stretch goal?

That sounds good to me!

changed the description

I added a weight of 5 to this issue and believe it can be broken up into several issues. It will definitely be multiple MRs. As I don't have time left this week, I believe we can proceed on the 1st two points of the plan. The exception is ingestion (in point 2) which may be changed depending on Spike: Should we import CVEs and affected packa... (#404996 - closed)

Let me know your thoughts @hacks4oats about this being ready. I can break this issue up early next week to reduce the weight.

@ifrenkel the issue looks ready to me Thank you for picking this up and refining it

added workflowready for development label and removed workflowplanning breakdown label

set weight to 5

unassigned @ifrenkel

marked this issue as related to #406323 (closed)

mentioned in issue #406323 (closed)

changed the description

mentioned in issue #371046 (closed)

changed the description

mentioned in issue #406836 (closed)

changed title from Add worker to ingest new advisories into main DB to Add package metadata sync worker to ingest advisories

changed the description

set weight to 3

added Stretch label

changed milestone to %16.0

removed Stretch label

added Deliverable label

Setting health status to on track as the milestone has just begun.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to on track

added Category:Software Composition Analysis SCA:Dependency Scanning labels

changed the description