search

clj-holmes / clj-watson

clojure deps SCA
Eclipse Public License 2.0
84 stars 9 forks source link

Consider CI-friendly specification of nvd api key token #66

Closed lread closed 3 months ago

lread commented 3 months ago

Currently...

The nvd data feed api key can be specified only in a --clj-watson-properties file.

But...

This makes things a bit awkward for CI usage, where the token would typically be stored as a secret.

Options

Option 1: Do nothing

Tough cookies, spit a clj-watson.properties file out on CI and, and use that.

Option 2: Allow token to be specified via env var.

CI secrets are often conveyed to a step via an environment variable.

Introduce CLJ_WATSON_NVD_API_KEY environment variable. If set, it would take precedence over a token specified in the --clj-watson-properties file.

Proposal

I like option 2.

Next Steps

I am happy to explore further. If you agree we should take action, I volunteer to create a PR.

seancorfield commented 3 months ago

Would it be worth having a general mechanism where CLJ_WATSON_<THE_KEY> was mapped to the.key and arbitrary properties could be overridden that way?

lread commented 3 months ago

That's is an interesting idea. So, CLJ_WATSON_NVD_API_KEY would be a general way to override (and an alternative to) the clj-watson.properties file nvd.api.key.

I don't currently have a need for this generality, but it is interesting. Waddya think?

seancorfield commented 3 months ago

I'm thinking the order of things should be:

That should be backward-compatible and CI-friendly and general enough for the future.

lread commented 3 months ago

Yeah, that seems solid to me!

seancorfield commented 3 months ago

See also discussion in #70 around --dependency-check-properties

lread commented 3 months ago

Oh, what about Java system properties? Should they come into play? I.e. if a user passes in -J-Dsomeproperty=somevalue on the command line.

lread commented 3 months ago

Just checked: Dependency Check is affected by system properties passed in on the cmd line. But maybe this is automatic when dealing with properties files in Java? Can't remember.

seancorfield commented 3 months ago

Can you link me to how/where DC uses JVM system properties? I had a quick search of the docs and code and couldn't see that...

lread commented 3 months ago

I did not look in their code, but tried it like so from clj-watson main branch:

$ clojure -J-Dodc.autoupdate=false -M:clj-watson scan -p deps.edn

Read 119 dependency-check properties.
No additional properties found.

Downloading/Updating database.
2024-08-04 17:41:34,849 INFO Engine - Checking for updates
2024-08-04 17:41:34,871 INFO Engine - Check for updates complete (21 ms)
Download/Update completed.
2024-08-04 17:41:35,483 INFO Engine - 

Dependency-Check is an open source tool performing a best effort analysis of 3rd party dependencies; false positives and false negatives may exist in the analysis performed by the tool. Use of the tool and the reporting provided constitutes acceptance for use in an AS IS condition, and there are NO warranties, implied or otherwise, with regard to the analysis or its use. Any use of the tool and the reporting provided is at the user's risk. In no event shall the copyright holder or OWASP be held liable for any damages whatsoever arising out of or in connection with the use of this tool, the analysis performed, or the resulting report.

Notice 1 update of db: 2024-08-04 17:41:34,849 INFO Engine - Checking for updates

And now if I repeat:

$ clojure -J-Dodc.autoupdate=true -M:clj-watson scan -p deps.edn

Read 119 dependency-check properties.
No additional properties found.

Downloading/Updating database.
2024-08-04 17:44:16,202 INFO Engine - Checking for updates
2024-08-04 17:44:16,212 INFO NvdApiDataSource - Skipping the NVD API Update as it was completed within the last 720 minutes
2024-08-04 17:44:16,870 INFO KnownExploitedDataSource - Skipping Known Exploited Vulnerabilities update check since last check was within 24 hours.
2024-08-04 17:44:16,876 INFO Engine - Check for updates complete (672 ms)
Download/Update completed.
2024-08-04 17:44:17,410 INFO Engine - Checking for updates
2024-08-04 17:44:17,412 INFO NvdApiDataSource - Skipping the NVD API Update as it was completed within the last 720 minutes
2024-08-04 17:44:17,610 INFO KnownExploitedDataSource - Skipping Known Exploited Vulnerabilities update check since last check was within 24 hours.
2024-08-04 17:44:17,618 INFO Engine - Check for updates complete (207 ms)
2024-08-04 17:44:17,724 INFO Engine - 

Dependency-Check is an open source tool performing a best effort analysis of 3rd party dependencies; false positives and false negatives may exist in the analysis performed by the tool. Use of the tool and the reporting provided constitutes acceptance for use in an AS IS condition, and there are NO warranties, implied or otherwise, with regard to the analysis or its use. Any use of the tool and the reporting provided is at the user's risk. In no event shall the copyright holder or OWASP be held liable for any damages whatsoever arising out of or in connection with the use of this tool, the analysis performed, or the resulting report.

Notice 2 upates:

2024-08-04 17:44:16,202 INFO Engine - Checking for updates
...
2024-08-04 17:44:17,410 INFO Engine - Checking for updates

This is pre-merge of #90. I can poke around to learn how this comes to be.

lread commented 3 months ago

This looks like it

seancorfield commented 3 months ago

Ah, well spotted! I was searching for getProperty but they've added that to several classes and then a bunch of getString* functions... ugh! Crazy OO Java code 🙂

lread commented 3 months ago

Since we are talking about config, I guess it is probably a good idea to state these changes do not apply to:

seancorfield commented 3 months ago

Correct.

seancorfield commented 3 months ago

PR #99 handled the -Dnvd.api.key=<your key here> approach, but we probably need to expand that documentation for this issue since any property in the config files can be overridden by JVM properties on the command-line (or :jvm-opts).

So, the main work left here is:

Then I think split the env var stuff into a separate ticket (or at least PR) and determine whether env vars or JVM properties should take precedence? I think since core handles the JVM properties, the only way to have env vars override is to programmatically call setProperty() to map them from env vars to properties (and let core handle them)?

Are JVM properties sufficiently CI-friendly, or does it really need to be env vars?

lread commented 3 months ago

determine whether env vars or JVM properties should take precedence?

I think system properties typically override env vars? For example user.timezone system property will override env var TZ.

$ clojure -M -e "(-> (java.util.TimeZone/getDefault) .getDisplayName)" 
"Eastern Standard Time"
$ TZ="US/Hawaii" clojure -M -e "(-> (java.util.TimeZone/getDefault) .getDisplayName)"
"Hawaii-Aleutian Standard Time"
$ TZ="US/Hawaii" clojure -J-Duser.timezone="UTC" -M -e "(-> (java.util.TimeZone/getDefault) .getDisplayName)"
"Coordinated Universal Time"

Because an option can be configured in multiple ways, it would be nice to summarize from where a user-specified option is sourced. But maybe this is more #79.

Are JVM properties sufficiently CI-friendly, or does it really need to be env vars?

An environment variable for a secret is nice because it avoids the question, "Is CI going to expose my secret?". CI systems will mask secrets, but as a dev, I'd rather not wonder if I should trust a CI system to do so.

seancorfield commented 3 months ago

OK, so the order of property handling is going to be:

Either:

Or:

Followed by:

Does that sound right?

Said another way:

lread commented 3 months ago

Yes, I think that makes sense.

seancorfield commented 3 months ago

It feels like this is still quite a chunk of work, so maybe we want to split it into two (or three?) separate issues to address in PRs.

I think that once this issue and associated documentation issues are addressed, I'd like to cut a 6.0.0-beta release for folks to test (a lot has changed since 5.1.3 but I don't think any of it is really "breaking" but the API key workflow could be a big change for some users).

Once 6.0.0 is tested and goes "gold", we can look at some of the other issues on the board for 6.1.0 release.

lread commented 3 months ago

Great idea to cut a release! It's nice to get changes out there to exercise them and hopefully get some feedback.

And yes, I agree: I raised this issue with a very specific problem, but we're discussing a broader revamp of how users can convey options to clj-watson. So I think this issue should remain open until it is addressed, but creating separate issues/PRs makes sense to me too.

seancorfield commented 3 months ago

LMK if I missed anything in #103 and/or #104 and whether that covers all of this ticket?

lread commented 3 months ago

Yeah I think that covers it @seancorfield.
The precedence you described above will remain a good reference.

seancorfield commented 3 months ago

Closed via #108