Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: pull updates from mirror with --use-mirror flag #2811

Merged
merged 8 commits into from
Mar 22, 2023
Merged

feat: pull updates from mirror with --use-mirror flag #2811

merged 8 commits into from
Mar 22, 2023

Conversation

b31ngd3v
Copy link
Contributor

part of #2577

image

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2023

Codecov Report

Merging #2811 (a6e2851) into main (6af5701) will decrease coverage by 0.30%.
The diff coverage is 74.35%.

@@            Coverage Diff             @@
##             main    #2811      +/-   ##
==========================================
- Coverage   82.29%   82.00%   -0.30%     
==========================================
  Files         651      655       +4     
  Lines       10254    10420     +166     
  Branches     1382     1409      +27     
==========================================
+ Hits         8439     8545     +106     
- Misses       1446     1505      +59     
- Partials      369      370       +1     
Flag Coverage Δ
longtests 82.00% <74.35%> (+0.19%) ⬆️
win-longtests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
cve_bin_tool/cvedb.py 75.38% <20.00%> (+0.60%) ⬆️
cve_bin_tool/cli.py 67.46% <25.00%> (-1.42%) ⬇️
cve_bin_tool/fetch_json_db.py 70.23% <70.23%> (ø)
cve_bin_tool/error_handler.py 91.54% <100.00%> (+0.12%) ⬆️
test/test_fetch_json_db.py 100.00% <100.00%> (ø)

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@anthonyharrison
Copy link
Contributor

@b31ngd3v @terriko I think #2356 may also be relevent here.

@anthonyharrison
Copy link
Contributor

@b31ngd3v @terriko What is the reason for pulling data from mirror if more than 7 days old? Should we not make this an evironment variable (the default would be 7 days) so if we want to change the period (shorter or longer), we can esily doso. How does this change work in offline mode as the mirror won't be available?

@terriko
Copy link
Contributor

terriko commented Mar 15, 2023

I'm not sure why 7 days either.

What I was envisioning was more like this:

  • mirror gets data
  • cve-bin-tool defaults to using the mirror (so no one needs to get a nvd key on first run of the product)
  • cve-bin-tool has options to configure the mirror in use (presumably to choose a more "local" one, but also allowing people to use an internal company one or share a cache across machines in an air-gapped network)
  • cve-bin-tool provides an option skip the mirror and go directly to nvd (i.e. the current default behaviour)
  • in future: we figure out how to also deal with mirroring of gad/redhat/etc. and maybe how to configure mirroring of each of those separately/together

Maybe we could convert this PR to adding support for a flag for now? --use-mirror $url or something?

@b31ngd3v
Copy link
Contributor Author

@terriko @anthonyharrison after fetching the data from mirror, should it incrementally update the database too? that should be the normal behavior when --use-mirror $url flag is used right?

@b31ngd3v
Copy link
Contributor Author

hi @terriko looks like the mirror-sandbox repo is private, can you make the repo public so that i can setup an example where you can fetch the data using the mirror? thanks.

@terriko
Copy link
Contributor

terriko commented Mar 15, 2023

hi @terriko looks like the mirror-sandbox repo is private, can you make the repo public so that i can setup an example where you can fetch the data using the mirror? thanks.

should be fixed now, sorry!

@b31ngd3v
Copy link
Contributor Author

b31ngd3v commented Mar 15, 2023

@terriko i was trying to push the ci changes, then this happened

image

also from the github ui, there is no merge button

image

@terriko
Copy link
Contributor

terriko commented Mar 16, 2023

Sorry, I forgot that I hadn't set you as an admin on that repo. you should be able to do everything there now. You'll need to set up a new NVD API key for it and set it into secrets; I didn't want to re-use the one from the main repo.

@b31ngd3v
Copy link
Contributor Author

@terriko can you please enable this option? i'm unable to change it! thanks!!

image

image

@b31ngd3v b31ngd3v changed the title feat: pull updates from mirror if database is more than 7 days old feat: pull updates from mirror with --use-mirror flag Mar 17, 2023
@terriko
Copy link
Contributor

terriko commented Mar 20, 2023

Wow, github permissions are weird. I had to change it in the org (the repo wouldn't let me change it either, just gave a no-entry icon🚫 ) but I think it may work now?

@b31ngd3v
Copy link
Contributor Author

@terriko @anthonyharrison I think this PR is ready! you can test it with cve-bin-tool --use-mirror 'https://raw.githubusercontent.com/sec-data/mirror-sandbox/main/exported_data' --nvd-api-key YOUR_API_KEY_FOR_INCREMENTAL_UPDATE

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit about return codes, but this is looking really good!

For this PR to get merged I'd like to see:

  • documentation of the new flag
  • a test (We could use the sandbox mirror but it's probably better to use mock and have it get a fake updated cve for testing that can immediately be deleted)

For either this PR or a future one, I think we need to add and validate a jsonschema to make sure that the mirror data isn't garbage. I think we'll also be doing some fancier stuff with storing mirrors in the config file too, but that's definitely future PR material and not needed yet.

But yeah, I'm really excited to see this coming together. Let's get docs and tests at minimum and get it merged so people can try it out!

@b31ngd3v
Copy link
Contributor Author

@terriko done! re-requesting review! thanks!

Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good enough to merge! I think we'll be iterating and refactoring a bit to add some validation but this should be good enough to get started!

@terriko terriko merged commit 57fba59 into intel:main Mar 22, 2023
@b31ngd3v b31ngd3v deleted the feat_update_from_mirror branch March 22, 2023 19:44
terriko pushed a commit to anthonyharrison/cve-bin-tool that referenced this pull request Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants