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

Fix NVD API token bug #174

Closed
wants to merge 3 commits into from

Conversation

kelvinqian00
Copy link

@kelvinqian00 kelvinqian00 commented Jan 31, 2024

Address the issue #173, in which the NVD_API_TOKEN variable does not work when the :nvd-api :key value is present but nil (which is the case with the default generated config file). I also verified that passing in the API key via the config file and as an env var both work by doing a local make install followed by a scan via clojure tools.

Side note: For some reason when I run the integration tests locally they fail on the dogfooding config steps, even before I made any changes. It may be something funky with my local environment.

@kelvinqian00 kelvinqian00 marked this pull request as draft January 31, 2024 22:18
@kelvinqian00 kelvinqian00 marked this pull request as ready for review January 31, 2024 22:26
@vemv
Copy link
Collaborator

vemv commented Feb 1, 2024

Hi!

Thanks for the PR. I disagree on the change - if a nil key is present, it should be honored, which here means failing fast and clearly.

So, users either set it to a valid value, or remove it, setting the NVD_API_TOKEN env var.

I'd suggest undoing the design change and fixing the bug(s) only.

I enabled GH actions now.

Cheers - V

@kelvinqian00
Copy link
Author

kelvinqian00 commented Feb 1, 2024

Hi @vemv,

Thanks for the clarification on your intent. Unfortunately I think your goal of failing fast on nil values of :key is problematic, since the scan automatically generates an nvd-clojure.edn file with {:key nil}. Thus, the user has a few choices:

  • Manually replace that nil value with an API key; all well and good, but not straightforward in a context like automated CI
  • Use the NVD_API_TOKEN env var and manually delete the config file, which is also problematic in the above contexts
  • Use the NVD_API_TOKEN env var and leave the config file alone, which currently fails "silently" (as highlighted in the issue) or, under your intent, fails fast

The point is, it is problematic in contexts where users cannot easily modify or remove the nvd-clojure.edn config file.

@vemv
Copy link
Collaborator

vemv commented Feb 20, 2024

Hi @kelvinqian00, sorry for the delay, I've had limited bandwith in the last few weeks.

I would not want to try forcing a specific direction - it's not a good use of our time.

I'll fix this directly in another branch asap.

(Bear in mind that there are workarounds that can be used as of today)

My sincere thanks for giving this a shot and trying to improve things!

@vemv vemv closed this Feb 20, 2024
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.

2 participants