Consider CI-friendly specification of nvd api key token

[lread]

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]

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]

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]

I'm thinking the order of things should be:

• dependencycheck.properties from DependencyCheck itself
• overridden by dependency-check.properties from inside Watson (e.g., overriding just the necessary difference)
• overridden by clj-watson.properties if present or if specified by -w (that file must be present)
• overridden by any CLJ_WATSON_<THE_KEY> environment variables (mapping to the.key properties)

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

[lread]

Yeah, that seems solid to me!

[seancorfield]

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

[lread]

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]

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]

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]

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]

This looks like it

[seancorfield]

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]

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

• resources/clj-watson-config.edn - which at this time only applies to the github-advisory strategy.
• suppressions XML file: Document how to suppress false positives #55 (comment) (well, other than point to it via suppression.file)

[seancorfield]

Correct.

[seancorfield]

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:

• figure out the delta between the underlying dependencycheck.properties (core) and dependency-check.properties (watson)
• reduce the latter to just the overrides
• ensure the core properties file is correctly picked up (not sure if core does this and watson overrides, or whether watson's properties replace the core one?)
• retain the behavior that specifying the dependency-check.properties file completely overrides both of those
• document the above clearly(!)

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]

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]

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

Either:

• dependencycheck.properties
• overridden by (the delta that is left in) dependency-check.properties

Or:

• the file specified by -d

Followed by:

• clj-watson.properties if present or specified via -w
• CLJ_WATSON_<THE_PROPERTY> env vars -- which will programmatically .setProperty("the.property",<value>) if the property isn't specified via -D to the JVM
• any JVM properties specified via -D to the JVM (handled by the core library automatically)

Does that sound right?

Said another way:

• JVM properties (java -Dthe.property=...) take precedence over
• environment variables CLJ_WATSON_<THE_PROPERTY>, which take precedence over
• clj-watson.properties file entries, if present on the classpath or specified via -w, which take precedence over
• dependency-check.properties (clj-watson defaults, or via the deprecated -d option), which take precedence over
• dependencycheck.properties in the core of DependencyCheck itself.

[lread]

Yes, I think that makes sense.

[seancorfield]

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]

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]

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

[lread]

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

[seancorfield]

Closed via #108