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

Add sbt-dependency-check #289

Closed
wants to merge 1 commit into from

Conversation

mdedetrich
Copy link
Contributor

@mdedetrich mdedetrich commented Apr 15, 2023

This PR adds sbt-dependency-check which scans the entire dependency tree of Pekko against OWASP to scan for any potential CVE's. This PR does a pretty basic addition of the plugin but there is future room for improvement, i.e. since this is a standard sbt plugin its possible to run dependencyCheckAggregate along with sourceDistGenerate when making a release. Another notable setting is dependencyCheckFailBuildOnCVSS which would cause the project to automatically fail if it finds a dependency with a certain security level.

The contents of both the generated report and the surpression file are located in the dependency folder (this can be renamed if needed). This also means that the report will be contained in the source dist distribution.

Note that the report has actually picked up CRITICAL CVE's from our dependency list which we should look into, i.e.
image

@mdedetrich
Copy link
Contributor Author

So I just checked the netty dependency which has the CRITICAL level but there isn't anything that can be done about it since there isn't a new proper release that solves it (see https://mvnrepository.com/artifact/io.netty/netty).

@pjfanning I can add this netty version into suppression.xml since there isn't anything we can do about it, just let me know if you want me to update the PR for this.

@pjfanning
Copy link
Contributor

do we really need to commit the html file?

@mdedetrich
Copy link
Contributor Author

mdedetrich commented Apr 15, 2023

do we really need to commit the html file?

I don't know? The primary point of the plugin is to create this file, aka report. One thing to note is that the generation of this report takes a looong amount of time. This is due to the fact that it downloads the entire OWASP database locally, does some processing on it and then scans all of your dependencies. You can think of it as similar to compiled game assets, i.e. theoretically speaking you shouldn't commit these into git but because they are expensive to generate and rarely change its done for pragmatic/practical reasons.

I would also presume that having this committed in git would make Apache review/release process of Pekko a lot easier (you just need to open the file) and since the report (at least currently) is in the source dist package its a nice QOL feature for Pekko users. As stated before we would make sure that the report is in sync when a release is made but this also can be automated.

Also incase you haven't opened the file, I would recommend do so because its actually interactive (i.e. its not a static text). i.e. it gives you the ability to direct to the original OWASP vulnerability and you can also use the generated html file to add suppressions, i.e.

image

Ultimately I have no problem with removing it from git but then we kind of need to ascertain what the goals are here. At least from my experience in working with companies for legal/license/CVE's, a report is always the best way to handle this and thats why these plugins generally make reports. This plugin is also just a wrapper for the well known https://github.com/jeremylong/DependencyCheck whos entire point is also just to generate reports.

Maybe we should get some opinions from people outside of the project as well?

@He-Pin
Copy link
Member

He-Pin commented Apr 15, 2023

Or a weekly action can do too?

@mdedetrich
Copy link
Contributor Author

Or a weekly action can do too?

We can do this, but the point is still where do we persist the file? I guess we can upload it as part of pekko docs and then rsync it/commit it to git. @pjfanning would you prefer this?

@pjfanning
Copy link
Contributor

We could rsync it to nightlies site.

@kw217
Copy link
Contributor

kw217 commented May 5, 2023

I agree, having the report pre-generated is really useful for consumers (esp. corporate). But we shouldn't have big generated artifacts in git (it slows down clones, for one thing). It should go on the nightlies site, or near the docs.

@mdedetrich
Copy link
Contributor Author

So I plan on changing this so it just gets included in the docs. I have similar work with sbt-license-report which I have yet to submit.

@pjfanning
Copy link
Contributor

If we remove the html file and possibly add the filename to .gitignore, I think we can merge this. We could do a 2nd PR that automates the publishing of the HTML.

@mdedetrich
Copy link
Contributor Author

In that case let me spend a few minutes today trying to automate this considering I already did it for sbt-license-report, I will ping when I update the PR.

@mdedetrich
Copy link
Contributor Author

So I did some work on this but due to the amount of time it takes to get the NVD database I have some concerns about how nicely it would work with github actions especially when integrated with paradox so I opened up an issue upstream at albuch/sbt-dependency-check#305

@mdedetrich
Copy link
Contributor Author

mdedetrich commented May 5, 2023

Good news is that after finally managing to get the NVD database to download onto my system I managed to integrate sbt-dependency-check directly into our paradox docs

image

The bad news is that as you can tell, since I am inlining the html the styling doesn't look the best. Unfortunately sbt-dependency-check doesn't have markdown as a supported output format which leaves us with 2 options

The latest changes have been pushed into the PR.

val sourceFile = (LocalRootProject / dependencyCheckOutputDirectory).value.get / "dependency-check-report.html"

(LocalRootProject / dependencyCheckAggregate).map { _ =>
val data = IO.readLines(sourceFile)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are probably better/more performant ways to do this but we can also look into this later

@mdedetrich
Copy link
Contributor Author

Closing this PR since #366 is already solving this problem in a more simple way.

@mdedetrich mdedetrich closed this Jul 30, 2023
@mdedetrich mdedetrich deleted the add-sbt-dependency-check branch July 30, 2023 13:32
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

4 participants