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

Display vulnerabilities for returned dependencies #249

Closed
wants to merge 36 commits into from
Closed

Display vulnerabilities for returned dependencies #249

wants to merge 36 commits into from

Conversation

shaikhu
Copy link
Contributor

@shaikhu shaikhu commented Aug 24, 2023

Firstly wanted to say thank you for developing such an amazing tool!

I started to look into issue #22 and this draft PR demonstrates a possible implementation. Any thoughts/ideas/feedback would be most welcome.

The OSS Index Rest API provides two endpoints for accessing a component report (listing known vulnerabilities). Sadly both endpoints involve rate limiting (I don't know the figures off the top of my head) but the authorized endpoint provides a higher limit. You can register and use an email and api token to authenticate each request. See instructions.

  1. The rest endpoint uses a Package Url to identify a dependency. This is the specification and this is Java implementation I used.
  2. This PR uses the unauthorized endpoint (and quickly ran into issues with rate limiting). I like the discussion of having a default configuration where oss index credentials could be stored.
  3. Example displaying cve ids (didn't make sense to show the "description" as I believe this is intended for html output)
    image
  4. Example displaying reference links (unfortunately the links are quite long)
    image
  5. Example when searching for dependency leads to a direct match
    image

Do you already have a solution in mind? Any preference for how vulnerabilities should be displayed?

@mthmulders
Copy link
Owner

Thanks for your great initiative and your kind words, @shaikhu. I'm going to have a proper look at your code soon(ish). In the meantime, I'll think of whether the 'default configuration' idea (#187) should be implemented first, so this feature could be built on top of it. I think making it possible to specify API credentials will greatly enhance the usability of this feature!

@mthmulders
Copy link
Owner

As far as the output goes, may I ask for the following?

  1. I would prefer if MCS would not print all CVE's in a list of search results. I would prefer if the "Vulnerabilities" column was summarised, for example, "3 high, 1 medium, 5 low". Something like this (fake data):
  Coordinates                                                    Last updated                 Vulnerabilities
  ===========                                                    ============                 ===============
  com.luhuiguo.netty:netty-handler:4.1.63.GM                     24 Feb 2022 at 15:26 (CET)   -
  io.netty:netty-handler:5.0.0.Alpha2                            03 Mar 2015 at 17:10 (CET)   1 high
  org.jboss.errai.io.netty:netty-handler:4.0.0.Alpha1.errai.r1   23 Feb 2012 at 06:53 (CET)   3 medium, 1 low
  io.netty.contrib:netty-handler-proxy:5.0.0.Alpha2              30 Sep 2022 at 08:28 (CEST)  5 medium
  io.netty:netty5-handler:5.0.0.Alpha5                           28 Sep 2022 at 16:09 (CEST)  - 
  io.netty:netty-handler-proxy:5.0.0.Alpha2                      03 Mar 2015 at 17:15 (CET)   2 high, 4 low
  io.netty:netty-handler-ssl-ocsp:4.1.97.Final                   23 Aug 2023 at 11:04 (CEST)  -
  1. For the detailed output (only one artifact result, printed as XML/Groovy/...), I would prefer to see all the vulnerabilities, each on one line. It would be great if they were sorted by priority, then by age, and could include a link to a detail page. Something like this (again, with fake data):
  <dependency>
      <groupId>io.netty</groupId>
      <artifactId>netty-handler</artifactId>
      <version>5.0.0.Alpha2</version>
  </dependency>

  Vulnerabilities:
  CVE-2023-xxxx (High) - https://ossindex.sonatype.org/component/.........
  CVE-2023-xxxx (Medium) - https://ossindex.sonatype.org/component/.........
  CVE-2023-xxxx (Medium) - https://ossindex.sonatype.org/component/.........
  CVE-2023-xxxx (Low) - https://ossindex.sonatype.org/component/.........

@shaikhu
Copy link
Contributor Author

shaikhu commented Sep 1, 2023

As far as the output goes, may I ask for the following?

Great suggestions @mthmulders! For the severity text I used the following reference on the NVD website. I think this is consistent with with OSS index reports vuls.

  1. I would prefer if MCS would not print all CVE's in a list of search results. I would prefer if the "Vulnerabilities" column was summarised, for example, "3 high, 1 medium, 5 low". Something like this (fake data):

See f11e909 and screen grab below
image

  1. For the detailed output (only one artifact result, printed as XML/Groovy/...), I would prefer to see all the vulnerabilities, each on one line. It would be great if they were sorted by priority, then by age, and could include a link to a detail page. Something like this (again, with fake data):

See 35e5ffb and screen grab below
image

@mthmulders
Copy link
Owner

Thanks @shaikhu for the screen grabs and all your efforts so far! Really appreciate it!

I'm finalising a mechanism to let users pass JVM system properties through a configuration file (#255), would that be a suitable way to pass the credentials for that Sonatype API you're using?

@shaikhu
Copy link
Contributor Author

shaikhu commented Sep 10, 2023

Thanks @shaikhu for the screen grabs and all your efforts so far! Really appreciate it!

I'm finalising a mechanism to let users pass JVM system properties through a configuration file (#255), would that be a suitable way to pass the credentials for that Sonatype API you're using?

Look great @mthmulders! See 7dbcf1b and ed9001c. Also worth mentioning

  1. Because we are using JVM system props, to test I had pass the properties using -D e.g. -Dossindex.uername=xxx -Dossindex.password=xxx
  2. There is a bug which prevented me using PasswordAuthenticator. Instead I had to manually add the http auhtorization header as a work-a-round.

@mthmulders
Copy link
Owner

I'd love to have this contribution in MCS, @shaikhu. If you're OK with that, let's remove the 'draft' status and I'll go through the code.

A word of warning: I might be picky. This is absolutely not to disappoint you or mock you, it's because I'd love the code to flourish and be maintainable by myself (and possibly others).

@shaikhu shaikhu marked this pull request as ready for review September 12, 2023 06:27
@shaikhu shaikhu changed the title POC - Display vulnerabilities for returned dependencies Display vulnerabilities for returned dependencies Sep 12, 2023
shaikhu and others added 3 commits September 14, 2023 06:29
…ortResponseBodyHandlerTest.java

Co-authored-by: Maarten Mulders <[email protected]>
…ortResponseBodyHandlerTest.java

Co-authored-by: Maarten Mulders <[email protected]>
@shaikhu shaikhu requested a review from mthmulders September 15, 2023 05:20
Copy link
Owner

@mthmulders mthmulders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run some manual tests and found a few issues. Could you please look into them?

@shaikhu shaikhu requested a review from mthmulders September 17, 2023 19:04
@mthmulders
Copy link
Owner

I had an email with a question about artifacts without reported vulnerabilities, but I can't find it back here. Anyway,

  • for the tabular output, I'd say "empty is just fine" (as it currently is).
  • for the single output, maybe it's better to print "none reported" to make it explicit that we did query, but found none.

@shaikhu
Copy link
Contributor Author

shaikhu commented Sep 18, 2023

I had an email with a question about artifacts without reported vulnerabilities, but I can't find it back here. Anyway,

That was me sorry!

  • for the tabular output, I'd say "empty is just fine" (as it currently is).

I deleted the question/comment and updated to print "-" with 639b16f. Happy to change back to empty space if you prefer.

  • for the single output, maybe it's better to print "none reported" to make it explicit that we did query, but found none.

agreed. a20c616

@shaikhu shaikhu requested a review from mthmulders September 18, 2023 12:23
Copy link
Owner

@mthmulders mthmulders left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all is fine now!

@mthmulders
Copy link
Owner

@all-contributors please add @shaikhu for code.

Copy link
Contributor

@mthmulders

I've put up a pull request to add @shaikhu! 🎉

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