search

grpc-ecosystem / grpc-health-probe

A command-line tool to perform health-checks for gRPC applications in Kubernetes and elsewhere
Apache License 2.0
1.4k stars 187 forks source link

Security scanning in PR actions #178

Open jon-whit opened 4 months ago

jon-whit commented 4 months ago

👋 We use grpc-health-probe in the OpenFGA project, and we actually embed the binary in our built images. We do this so that we can ship a single image but reference different target entrypoints. This is particularly handy for tooling such as Helm charts and Docker Compose, because it allows us to reduce our dependency to just our image which includes the tools needed for health checks and to run the server.

We've quite regularly noticed that dependencies of grpc-health-probe have unresolved vulnerabilities reported.

Screenshot 2024-02-20 at 11 24 35 AM

Security vulnerability scanning is much more common in cloud-native software delivery today, and since a broader gRPC community may depend on this tool, it would be nice if there was security scanning implemented on top of the artifacts produced by this project.

Would you be open to a PR that adds a vulnerability scan to the PR Github action workflow?

The most common scanners used include:

I would recommend at least adding scans using govulncheck and trivy as these are widely used across the CNCF ecosystem.

ahmetb commented 4 months ago

No because the issues flagged by scanners don't come up during PRs. The project is largely in maintenance mode, yet so many useless scanners alerts are reported frequently due to vulns found in pkgs we import, albeit none of these are really applicable or exploitable in the conditions this tool is used in.

jon-whit commented 4 months ago

@ahmetb

The project is largely in maintenance mode

What is "maintenance mode" considered to be to your maintainer team? In my experience maintenance mode refers to patches and vulnerability fixes, but no new major features etc.. But I understand that may differ project to project.

No because the issues flagged by scanners don't come up during PRs.

How do you figure? If you upgrade a package in the scope of a PR, and if that package introduces a vulnerability that may be exploitable in your software artifacts, then is it not your responsibility as maintainers to make sure you do your dilligence to patch that by upgrading that dependency to the appropriate version including the fix? Likewise, is it not the projects responsibility to release patch fixes when new vulnerabilities are discovered that impact the software artifacts that have previously been released? At minimum it should be a maintainer team's responsibility to at least review vulnerability reports and ascertain whether that the vulnerability is or is not impacting your software. Otherwise it's pure negligence on the maintainers part.

ahmetb commented 4 months ago

maintenance mode refers to patches and vulnerability fixes, but no new major features etc.

Correct. That's the stage the proejct is in.

How do you figure? If you upgrade a package in the scope of a PR, and if that package introduces a vulnerability that may be exploitable in your software artifacts

We have @dependabot that actively sends PRs for vulnerabilities. On top of that, GitHub has security checkers enabled in the repo and if we were merging something with vulns, it does alert.

But that doesn't happen because most all PRs we do nowadays is updating things to the very last version. So, as I said, vuln reports come in much later after the code is merged and the vuln is discovered; not "at the time of merging".

jon-whit commented 4 months ago

@ahmetb gotcha. So it sounds like the majority of PRs are strictly for dependency updates and for patch fixes etc. As you said:

But that doesn't happen because most all PRs we do nowadays is updating things to the very last version. So, as I said, vuln reports come in much later after the code is merged and the vuln is discovered; not "at the time of merging".

Doesn't this highlight even more why it would be a good idea to accept a community contribution to setup a vulnerability scanning step in the Github actions workflow to prevent vulnerabilities from being introduced in these PRs? If these are the only kinds of PRs that are really getting merged today, then it seems better to block the PR before the code gets into the released artifact in the first place. Why would you not want to do that?