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

Integrate govulncheck #9091

Closed
sbueringer opened this issue Jul 31, 2023 · 9 comments · Fixed by #9144
Closed

Integrate govulncheck #9091

sbueringer opened this issue Jul 31, 2023 · 9 comments · Fixed by #9144
Assignees
Labels
needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.

Comments

@sbueringer
Copy link
Member

sbueringer commented Jul 31, 2023

govulncheck is a tool provided by the Go team: https://go.dev/blog/vuln

It allows scanning Go code for vulnerabilities and then reports if the vulnerable code is actually used (it could be that we have a Go Dependency with a CVE, but the affected package in the dependency is not actually used). The huge benefit here is that it can help to assess the impact of CVE's (i.e. no impact if we don't use the code).

It should be pretty easy to integrate as it can be ~ run via go run golang.org/x/vuln/cmd/govulncheck ./.... But let's integrate it with go install like other tools.

Limitations:

  • Trivy could find CVE's that are either outside of the scope of govulncheck (e.g. base image) or that govulncheck is not aware of yet

Notes:

  • The idea is to run it in our scan GitHub action after Trivy
  • We can decide if we action should also fail is govulncheck fails (could be that it finds issues that are unknown to Trivy). I would say yes, we can always iterate
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 31, 2023
@chrischdi
Copy link
Member

chrischdi commented Aug 3, 2023

Example how govulncheck could help:

When taking a look at today's main branch commit 1567f68f1ae50134991497a5b1602c152a314619:

Importing vulnerable package but not affected:

Example vulnerability/CVE: https://pkg.go.dev/vuln/GO-2023-1988

govulncheck is able to detect that we are not affected. output from govulncheck ./... regarding this CVE is only in the Informational section:

$ govulncheck ./...
Using go1.20.4 and govulncheck@v1.0.0 with vulnerability data from https://vuln.go.dev (last modified 2023-08-02 20:33:39 +0000 UTC).

[REDACTED]

=== Informational ===

Found 1 vulnerability in packages that you import, but there are no call
stacks leading to the use of this vulnerability. You may not need to
take any action. See https://pkg.go.dev/golang.org/x/vuln/cmd/govulncheck
for details.

Vulnerability #1: GO-2023-1988
    Improper rendering of text nodes in golang.org/x/net/html
  More info: https://pkg.go.dev/vuln/GO-2023-1988
  Module: golang.org/x/net
    Found in: golang.org/x/net@v0.12.0
    Fixed in: golang.org/x/net@v0.13.0

Vulnerability in used go sdk version

Note: If I am right, this is something which does not get detected by trivy (cc @sbueringer)

Note2: To detect go sdk dependent CVE's,govulncheck uses the version of go which is in the current PATH, which in my test was go 1.20.4.

Example output for a CVE where we are affected and bumping the used go sdk to a newer version would fix the issue:

$ govulncheck ./...
Using go1.20.4 and govulncheck@v1.0.0 with vulnerability data from https://vuln.go.dev (last modified 2023-08-02 20:33:39 +0000 UTC).

[REDACTED]

Vulnerability #2: GO-2023-1878
    Insufficient sanitization of Host header in net/http
  More info: https://pkg.go.dev/vuln/GO-2023-1878
  Standard library
    Found in: net/http@go1.20.4
    Fixed in: net/http@go1.20.6
    Example traces found:
      #1: cmd/clusterctl/client/repository/repository_gitlab.go:158:34: repository.gitLabRepository.GetFile calls http.Client.Do
      #2: internal/test/envtest/environment.go:231:24: envtest.newEnvironment calls envtest.Environment.Start, which eventually calls http.Client.Get
      #3: controlplane/kubeadm/internal/etcd/etcd.go:148:33: etcd.NewClient calls client.New, which eventually calls http.Request.Write
      
[REDACTED]

In this case main is affected by the vulnerability (note: in reality not, because we bumped go in our CI, just on my local test). Bumping go to >= 1.20.6 would fix the CVE.

@furkatgofurov7
Copy link
Member

Overall I like the idea, and +1 to:

The idea is to run it in our scan GitHub action after Trivy

We have to make sure to document if it needs any update the same way as Trivy as part of release tasks (https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/release/release-tasks.md#setup-jobs-and-dashboards-for-a-new-release-branch)

@sbueringer
Copy link
Member Author

Thx for testing this and reporting

Note: If I am right, this is something which does not get detected by trivy (cc @sbueringer)

I think you're right. Just tested it by changing the GO_VERSION in the Makefile to 1.20.0 and running ./hack/verify-container-images.sh. No results

I would be in favor to add this to ./hack/verify-container-images.sh and also fail if govulncheck finds something. We can figure out over time how we handle the results (in case we end up having false positives)

@chrischdi
Copy link
Member

As follow-up step of integrating this we could automate to not fail if there are only trivy false-positives which are identified via govulncheck.

@sbueringer
Copy link
Member Author

sbueringer commented Aug 3, 2023

As follow-up step of integrating this we could automate to not fail if there are only trivy false-positives which are identified via govulncheck.

I would prefer not doing this for the foreseeable future. Up until now whenever we found a CVE we could fix it by bumping a dependency (and now also the Go SDK). So I don't want to start ignoring Trivy findings until we have to.

(I mostly see this as finding more CVEs (e.g. in the Go SDK) & additional info to assess the impact of a CVE, I would like to bump independent of that)

@chrischdi
Copy link
Member

xref: kubernetes/sig-security#95

@chrischdi
Copy link
Member

/assign

@chrischdi
Copy link
Member

chrischdi commented Aug 14, 2023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one.
Projects
None yet
4 participants