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

nvd: recommend a pattern less prone to classpath interference. #31

Closed
wants to merge 1 commit into from

Conversation

vemv
Copy link
Contributor

@vemv vemv commented Dec 19, 2021

Dependencies from nvd shouldn't affect the analysed corpus, and vice versa.

Dependencies from nvd shouldn't affect the analysed corpus, and vice versa.
@practicalli-johnny
Copy link
Contributor

The user level aliases in this configuration file should work across any project.

Unless I have missed something, this proposed change does not work unless a value for YOUR_PROJECT is defined each time. Adding the alias to the project deps.edn file doesnt improve the situation either and is out of scope of this project anyway.

Changing out of the project to be inspected for security issues without setting the project directory to include each time does not actually check the project, making the alias of little use.

Changing out of the project directory, cd has no value here, as the user level alias will add the clojure-nvd jar and its dependencies to the classpath.

It would seem the options are

  1. Add a warning in the documentation that some security warnings may come from the Java libraries used by clojure-nvd
  2. Use clojure-nvd as a tool (still not ideal according to the slack discussion)
  3. remove the alias from this project (and only include clojure-nvd and examples in a section on security within the Practicalli Clojure book

@vemv
Copy link
Contributor Author

vemv commented Dec 20, 2021

this proposed change does not work unless a value for YOUR_PROJECT is defined each time

I meant it as something that can be filled in however the user desires. I didn't particularly invite people to rely on such an env var.

Some possible solutions include a bash function, and a Makefile. Both would do the cding for you.

I particularly like the bash alias:

nvd(){
  local here=$PWD
  cd ~
  clojure -M:security/nvd "" "$(cd $here; clojure -Spath)"
  cd $here
}

seems a handy thing to invoke over a terminal session. It will work fairly universally, and does make effective use of the :security/nvd alias i.e. the project is contributing something.

And It does make correct use of classpath in both directions (we're not analysing nvd itself + the given project's dep tree doesn't affect nvd's).

So it's an approach I can fully recommend.

@practicalli-johnny
Copy link
Contributor

Whilst the suggestion is interesting, I dont believe it fits into the scope of this project. All aliases should be self-contained and not rely on external factors.

I am afraid I dont see an obvious way to make this tool fit into the purposed of the project, so I am inclined to remove it.

I'll do some more testing to see how much of an issue I get when scanning with clojure-nvd included the class path. If it were obvious in the output of the tool where the dependency were, then I think thats acceptable.

It would also be useful to get some input from @joshrotenberg , the author of the original pull request to add this alias.

@vemv
Copy link
Contributor Author

vemv commented Dec 20, 2021

As a maintainer of nvd-clojure who has fixed important years-long issues to it and keeps attending user issues that boil down to classpath interference, my recommendation is clear: there are relatively few ways of running this tool correctly, and only those are supported.

In a close future we'll deprecate all unsafe patterns / apis, and we will go as far to detecting when such misuse happens rm-hull/nvd-clojure#117

@practicalli-johnny
Copy link
Contributor

Your work on the clojure-nvd project is much appreciated by the community.

It is unfortunate that the scope of the practicalli/clojure-deps-edn project doesnt fit within the constrains for correct operation of the clojure-nvd tool.

I'll plan to remove the alias and include the tools as part of a security section in the Practicalli Clojure book, where there is more scope to run the tool correctly.

Thank you.

@vemv
Copy link
Contributor Author

vemv commented Dec 20, 2021

Cheers no issue, that's a reasonable plan 🍻

@vemv vemv closed this Dec 20, 2021
@joshrotenberg
Copy link
Contributor

I can remove it since I added it in the first place.

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.

None yet

3 participants