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: add NVD API key #1529

Merged
merged 7 commits into from
Jan 24, 2022
Merged

feat: add NVD API key #1529

merged 7 commits into from
Jan 24, 2022

Conversation

terriko
Copy link
Contributor

@terriko terriko commented Jan 13, 2022

This PR will add support for NVD API keys

This includes the following:

  • adding the NVD notice as required by the NVD API docs
  • adding --nvd-api-key to the command line
  • modifying CVEDB to accept the api key and pass it through to NVD_API
  • modifying NVD_API to accept and use the api key by adding it to the parameters sent with the request.
  • documentation on how to get and use an api key
  • accepting an $nvd_api_key environment variable as alternative to using the command-line flag. This allows users of GitHub Actions (including us!) to use their built-in secrets options to keep the key private in the CI logs.
  • enabled the nvd_api_key in our GitHub Actions setup -- this allows the long tests to pass without being rate limited

@codecov-commenter
Copy link

Codecov Report

Merging #1529 (b08f084) into main (9934aed) will increase coverage by 3.74%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1529      +/-   ##
==========================================
+ Coverage   79.27%   83.01%   +3.74%     
==========================================
  Files         281      281              
  Lines        5548     5553       +5     
  Branches      905      906       +1     
==========================================
+ Hits         4398     4610     +212     
+ Misses        966      754     -212     
- Partials      184      189       +5     
Flag Coverage Δ
longtests 83.01% <71.42%> (+3.74%) ⬆️

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

Impacted Files Coverage Δ
cve_bin_tool/nvd_api.py 81.00% <0.00%> (-6.76%) ⬇️
cve_bin_tool/cli.py 74.42% <100.00%> (+2.66%) ⬆️
cve_bin_tool/cvedb.py 85.83% <100.00%> (+13.88%) ⬆️
cve_bin_tool/version_scanner.py 57.42% <0.00%> (+0.99%) ⬆️
cve_bin_tool/extractor.py 65.21% <0.00%> (+1.24%) ⬆️
cve_bin_tool/checkers/glibc.py 100.00% <0.00%> (+4.16%) ⬆️
test/test_cli.py 93.60% <0.00%> (+10.95%) ⬆️
test/test_scanner.py 74.61% <0.00%> (+12.30%) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9934aed...b08f084. Read the comment docs.

@terriko
Copy link
Contributor Author

terriko commented Jan 14, 2022

Note to self: since I don't want to put a real API key directly into the tests, we'll probably just want to test what happens when a bad one is specified. As you might expect, this causes the NVD API to return an error.

(I am going to have to plumb a real API key through github actions' secrets and make it work in pytest eventually, but that might not happen in this PR.)

@terriko terriko marked this pull request as ready for review January 14, 2022 01:54
@terriko
Copy link
Contributor Author

terriko commented Jan 14, 2022

Okay, this should be working and ready for review. I realized it doesn't really need separate tests yet because it's changing the main code path and I don't have any good way to put a real API key in there at the moment.

I am going to add code to handle bad API keys and tests for that, but I think that's going to happen in a separate PR.

@terriko
Copy link
Contributor Author

terriko commented Jan 14, 2022

Pinging @BreadGenie, @anthonyharrison and @imsahil007 for review if you have time. (since github won't let me request reviews from any of you directly)

doc/MANUAL.md Outdated Show resolved Hide resolved
@imsahil007
Copy link
Contributor

I think it would be great if we can add a message as a suggestion with the requesting NVD API key link in case the user is not setting this value. But that can be added later in a separate issue.
Other than that, this looks good to me.

@terriko
Copy link
Contributor Author

terriko commented Jan 19, 2022

Looks like I should probably set up the NVD key in github actions so that the long tests pass here. I need to do some backend wrangling to set it up as a github secret, but I'll go start the request process for that now.

@terriko
Copy link
Contributor Author

terriko commented Jan 19, 2022

I think it would be great if we can add a message as a suggestion with the requesting NVD API key link in case the user is not setting this value. But that can be added later in a separate issue. Other than that, this looks good to me.

Yeah, that's a good plan. It would be ideal if we put it any time someone gets a 403 from NVD (because that's usually the indication that they're rate limited and really need it) but let me at least add a reminder to go under the boilerplate text from NVD in this PR and we can figure out some improved reminders in a future one.

@terriko
Copy link
Contributor Author

terriko commented Jan 19, 2022

Fixed the key case issue (thanks @BreadGenie ) and added a "how to get a key" link (thanks @imsahil007 )

I expect to be getting my NVD API key set up in github actions .. probably tomorrow? (I have to go through our ops team to set secret variables on the main repo.) So hopefully I'll be able to resolve our failing long test when that's integrated.

@terriko
Copy link
Contributor Author

terriko commented Jan 20, 2022

I've added support for an $nvd_api_key environment variable, then set that up to work as expected in GitHub actions. (Thanks awesome and fast ops folk!)

This should fix the issue we've been having with rate limiting in GitHub Actions where the long tests have been failing. At least, it fixed it for this PR, but as the issue has been a bit sporadic it's still possible that once is a coincidence.

@terriko
Copy link
Contributor Author

terriko commented Jan 24, 2022

I was hoping to get a code review form @antoniogi but he's been a bit too busy and it's been over a week. Since this should fix most of our CI issues at the moment I don't want to wait any longer, so I'm going to go ahead and merge. Reviews always welcome later if anyone has anything else to say about the API code.

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.

Inclusion of NVD API key in nvd_api.py
4 participants