[Vulnerabilities]Adding new Job for updating v3 Vulnerabilities files.

[ryuyu]

This is the first proposal for a new job to automatically update the v3 Vulnerability files.
This should implement the design indicated at https://github.com/NuGet/Engineering/pull/4940

Much of the mechanics of pulling from GitHub are tested in the GitHubVulnerabilities2Db.Facts project.

The primary work is done in the FlushAsync method starting at line 53 of src/GitHubVulnerabilities2v3/Extensions/BlobStorageVulnerabilityWriter.cs

Summary of intended flow:

1. Read the date in the cursor and determine if the run is an update run or a full regeneration run
2. Query GitHub and collect vulnerabilities starting at the date in the cursor and running up to now.
3. For regeneration run, update index.json, base.json, and push an empty update.json. Update cursor to now.
4. For update run, Check the special case
5. The special case is when we see the same vulnerability URL in the update that already exists in base. For this case, we skip this update and regenerate on the next run.
6. Otherwise, update the update.json and index.json. Cursor should NOT be changed.

[erdembayar]

Quick clarification question before diving into: Should this new job be called NuGet.Jobs? What is the criteria for determining where a new job should go?
Also, I noticed that there is a checked-in binary asset at src/GitHubVulnerabilities2v3/Scripts/nssm.exe. Is this binary asset signed?

[ryuyu]

Quick clarification question before diving into: Should this new job be called NuGet.Jobs? What is the criteria for determining where a new job should go? Also, I noticed that there is a checked-in binary asset at src/GitHubVulnerabilities2v3/Scripts/nssm.exe. Is this binary asset signed?

Great questions! The general split for jobs being here vs NuGet.Jobs is a little bit arbitrary. Jobs that have harder gallery integration/depend on NuGetGallery.Core or Entities TEND TO lean towards being in NuGetGallery. This job is here becuase it relies on NuGet.Services.GitHub, which one COULD argue should actually be moved to another repo, but as it was extracted from GitHubVulnerabilities2Db (which is in here because of dependence on gallery DB interaction mechanics), it has not been moved as it would likely involve some additional changes to the build.

The checked in binary asset is a copy of the one at https://github.com/NuGet/NuGetGallery/blob/main/src/GitHubVulnerabilities2Db/Scripts/nssm.exe
I believe this is used to install the job as a service, and I'm not 100% sure about the necessity of as I can't quite remember the exact mechanics of how the job installation as a service works.

[erdembayar]

The checked in binary asset is a copy of the one at https://github.com/NuGet/NuGetGallery/blob/main/src/GitHubVulnerabilities2Db/Scripts/nssm.exe I believe this is used to install the job as a service, and I'm not 100% sure about the necessity of as I can't quite remember the exact mechanics of how the job installation as a service works.

It doesn't look signed binary, and it's 3rd party. I prefer not to add 3rd party binary directly into code. At least could we add as nupkg dependency?

[zhhyu]

wasWithdrawn

This is one of main concerns. Should this piece of information be included in "_vulnerabilityDict" and handled properly when generating/regenerating the document?

[ryuyu]

Hmm. That's a great question. I've added a section that removes vulnerabilities if they are withdrawn, but that won't solve the case of it existing in base and being removed down the line. I also don't think client would know how to handle that case (advisory present in base, removed in an update) @zivkan

Worst case, it will be fixed on regeneration 30 days later, and today, is effectively in the same state since we only update manually. @zhhyu

[zivkan]

I also don't think client would know how to handle that case (advisory present in base, removed in an update)

That's correct. Client simply concatenates all advisories across all files (and across all sources), deduplicates (both advisory URL + version range must match), and uses the resulting list. There's no syntax in the file format to "remove advisory previously declared"

[zhhyu]

Should the base document get regenerated if one advisory is withdrawn? I don't know how 2DB handles this case, but there is a need to keep the consistency between the two endpoints as much as possible, and also as a customer, I'd like not to see a withdrawn warning and spend time looking into how to resolve the dependency graph when unnecessary.

[erdembayar]

Sorry for late review. My comments are not blocking.

All comments addressed. Requested review.

[erdembayar]

Last questions before approving the PR:

1. Where are gzip compression and cache setting set? Does it happen at blob storage or CDN? I don't see any changes here.
2. What happens if someone just change just change the version range for existing advisory? How do you detect it?

[ryuyu]

Last questions before approving the PR:

1. Where are gzip compression and cache setting set? Does it happen at blob storage or CDN? I don't see any changes here.
2. What happens if someone just change just change the version range for existing advisory? How do you detect it?

1. The header settings are set in the Configuration now. Currently, they have default values, but we can modify them at will.
2. If an advisory range is changed, that will be picked up on the next run with a collision to an existing advisory URL and would trigger a regeneration (as intended for now).

[erdembayar]

🚢
Thank you for your patience.

[ryuyu]

Great catch by @erdembayar . Gzipping was apparently not enabled. Pushed some new changes that should enable compression of the stream.

[erdembayar]