[9096]: Add govulncheck step to CI

[jaehnri]

What this PR does / why we need it:

This PR adds an additional CI step that runs govulncheck against the go executable. govulncheck reports known vulnerabilities that affect Go code. It uses static analysis of source code or a binary's symbol table to narrow down reports to only those that could affect the application.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• CVE Report (Scanner found CVE and adding report)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation only

Which issue/s this PR fixes

fixes #9096

How Has This Been Tested?

I have created a similar PR in my ingress-nginx fork. The result of this CI check can be found here. For now, this step does not block the CI in any way.

To implement this new step, a new binary "scannable-nginx-ingress-controller" is compiled in build/build.sh. This new binary is needed because govulncheck requires the Go symbol table to run, which is currently being disabled by the -ldflags="-s" flag.

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added unit and/or e2e tests to cover my changes.
• All new and existing tests passed.
• Added Release Notes.

Does my pull request need a release note?

Any user-visible or operator-visible change qualifies for a release note. This could be a:

• CLI change
• API change
• UI change
• configuration schema change
• behavioral change
• change in non-functional attributes such as efficiency or availability, availability of a new platform
• a warning about a deprecation
• fix of a previous Known Issue
• fix of a vulnerability (CVE)

No release notes are required for changes to the following:

• Tests
• Build infrastructure
• Fixes for unreleased bugs

For more tips on writing good release notes, check out the Release Notes Handbook

NONE

[k8s-ci-robot]

Hi @jaehnri. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[longwuyuan]

@jaehnri the production binary that is shipped can not have its name changed to prefix "scannable"

[jaehnri]

@jaehnri the production binary that is shipped can not have its name changed to prefix "scannable"

Sure. The "scannable" binary comes from a new build in build/build.sh that compiles with the symbol table. In the end, I decided to not change anything in the productive binary

[longwuyuan]

(1) Should govulncheck report this CVE #9109 ?

(2) There was talk on binary size increasing to 40+ MB (increase of 3-4MB) so can ou confirm that that is not a concern anymore as binary with symbols will only be used (and deleted) in CI, to run govulncheck ?

[jaehnri]

Thanks for the review, @longwuyuan!

(1) As per the documentation:

For analyzing source code, that configuration is the Go version specified by the “go” command found on the PATH. For binaries, the build configuration is the one used to build the binary.

When the binary is scanned, like we are doing in the pipeline, no vulnerability is reported. However, when the source code is scanned, the vulnerability indeed is reported:

$govulncheck $PWD/cmd/nginx
...
Found 1 known vulnerability.

Vulnerability #1: GO-2022-0969
  HTTP/2 server connections can hang forever waiting for a clean
  shutdown that was preempted by a fatal error. This condition can
  be exploited by a malicious client to cause a denial of service.
...

As we recently updated ingress to Go 1.19.1, I guess ingress-nginx wasn't actually being affected by the vulnerability.

(2) The sizes of both image and binary aren't being affected. Before this change:

$ du -sh nginx-ingress-controller 
38M    nginx-ingress-controller

$ docker image ls gcr.io/k8s-staging-ingress-nginx/controller:v1.4.0q
REPOSITORY                                    TAG       IMAGE ID       CREATED          SIZE
gcr.io/k8s-staging-ingress-nginx/controller   v1.4.0q   f8276d255d24   52 seconds ago   264MB

After this change:

$ du -sh nginx-ingress-controller                                    
38M    nginx-ingress-controller

$ docker image ls gcr.io/k8s-staging-ingress-nginx/controller:v1.4.0 
REPOSITORY                                    TAG       IMAGE ID       CREATED         SIZE
gcr.io/k8s-staging-ingress-nginx/controller   v1.4.0    6083d2e0186e   9 seconds ago   264MB

[longwuyuan]

@jaehnri,

Are you going to scan both src bits as well as compiled bits in the CI ? Basically want to know if that existing vulnerability got reported in your fork's CI run.

[jaehnri]

@longwuyuan Initially I was planning to scan only the binary. Do you think we should scan source too? It seems like the binary is more precise than source. Source may lead to some false positives

[longwuyuan]

Goal is to know about a CVE asap.
Followup action is to fix the CVE.
Just stating the obvious for clarity.

Wait for other comments as I am not a developer.

[jaehnri]

Hi, all!
So, I decided to investigate if scanning the binary did not report the HTTP/2 hanging connection (GO-2022-0969) vulnerability because it was already fixed or because it wasn't recognized by govulncheck.

I checkout to a previous commit regarding the release 1.3.1 (db3cdc04e40c5fab51c010902eac2522572e9a3c) and ran the tool. As expected, the vulnerability was indeed reported:

$ go version
go version go1.19 darwin/amd64

$ git checkout db3cdc04e40c5fab51c010902eac2522572e9a3c
Note: switching to 'db3cdc04e40c5fab51c010902eac2522572e9a3c'.
...

$ make build
...

$ govulncheck rootfs/bin/amd64/nginx-ingress-controller 
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Scanning for dependencies with known vulnerabilities...
Found 12 known vulnerabilities.

...
Vulnerability #10: GO-2022-0969
  HTTP/2 server connections can hang forever waiting for a clean
  shutdown that was preempted by a fatal error. This condition can
  be exploited by a malicious client to cause a denial of service.
  Found in: net/[email protected]
  Fixed in: net/[email protected]
  More info: https://pkg.go.dev/vuln/GO-2022-0969

...

Then, I checkout to commit regarding release 1.4.0, after Long's commit updating go to version 1.19.1 and before James's commit updating x/net's version:

$ git checkout cc79e1474512b43351c7d33500b14eb8dc4d0a9a
Previous HEAD position was db3cdc04e release 1.3.1 (#9014)
HEAD is now at cc79e1474 Merge pull request #9108 from strongjz/release-1.4.0

$ make build
...

$ govulncheck rootfs/bin/amd64/nginx-ingress-controller
govulncheck is an experimental tool. Share feedback at https://go.dev/s/govulncheck-feedback.

Scanning for dependencies with known vulnerabilities...
Found 2 known vulnerabilities.

Vulnerability #1: GO-2022-1037
  Reader.Read does not set a limit on the maximum size of file
  headers. A maliciously crafted archive could cause Read to
  allocate unbounded amounts of memory, potentially causing
  resource exhaustion or panics. After fix, Reader.Read limits the
  maximum size of header blocks to 1 MiB.
  Found in: archive/[email protected]
  Fixed in: archive/[email protected]
  More info: https://pkg.go.dev/vuln/GO-2022-1037

Vulnerability #2: GO-2022-1039
  Programs which compile regular expressions from untrusted
  sources may be vulnerable to memory exhaustion or denial of
  service. The parsed regexp representation is linear in the size
  of the input, but in some cases the constant factor can be as
  high as 40,000, making relatively small regexps consume much
  larger amounts of memory. After fix, each regexp being parsed is
  limited to a 256 MB memory footprint. Regular expressions whose
  representation would use more space than that are rejected.
  Normal use of regular expressions is unaffected.
  Found in: regexp/[email protected]
  Fixed in: regexp/[email protected]
  More info: https://pkg.go.dev/vuln/GO-2022-1039

As expected, the vulnerability wasn't reported because it had already been fixed by the go version update. I'll be happy to investigate further if needed :)

[longwuyuan]

@jaehnri , this helps.
Please update on how to report this on slack or email if there is vulnerability found. maybe can use anonymous sendmail (mail command etc) right after the check, in the same CI step.

[strongjz]

/kind feature
/triage accepted
/priority backlog

[strongjz]

/ok-to-test

[k8s-ci-robot]

@jaehnri: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-ingress-nginx-codegen	b9d4a50	link	true	/test pull-ingress-nginx-codegen

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[k8s-ci-robot]

Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages.

The list of commits with invalid commit messages:

• 785f5b2 Bump helm/chart-releaser-action from 1.4.0 to 1.4.1 (Bump helm/chart-releaser-action from 1.4.0 to 1.4.1 #9136)
• d5c0355 Bump github/codeql-action from 2.1.25 to 2.1.27 (Bump github/codeql-action from 2.1.25 to 2.1.27 #9137)
• d31c802 Bump ossf/scorecard-action from 2.0.3 to 2.0.4 (Bump ossf/scorecard-action from 2.0.3 to 2.0.4 #9138)
• a03b08f Bump google.golang.org/grpc from 1.49.0 to 1.50.0 (Bump google.golang.org/grpc from 1.49.0 to 1.50.0 #9134)
• b316320 Bump actions/checkout from 3.0.2 to 3.1.0 (Bump actions/checkout from 3.0.2 to 3.1.0 #9135)
• 304e64c Bump dorny/paths-filter from 2.10.2 to 2.11.1 (Bump dorny/paths-filter from 2.10.2 to 2.11.1 #9183)
• 2ae47ab Bump github.com/spf13/cobra from 1.5.0 to 1.6.0 (Bump github.com/spf13/cobra from 1.5.0 to 1.6.0 #9173)
• d859bef Bump google.golang.org/grpc from 1.50.0 to 1.50.1 (Bump google.golang.org/grpc from 1.50.0 to 1.50.1 #9174)
• acd4300 Bump k8s.io/component-base from 0.25.2 to 0.25.3 (Bump k8s.io/component-base from 0.25.2 to 0.25.3 #9175)
• 024da09 Bump github.com/fsnotify/fsnotify from 1.5.4 to 1.6.0 (Bump github.com/fsnotify/fsnotify from 1.5.4 to 1.6.0 #9176)
• b76faf7 Bump github.com/onsi/ginkgo/v2 from 2.2.0 to 2.3.1 (Bump github.com/onsi/ginkgo/v2 from 2.2.0 to 2.3.1 #9177)
• 69395e5 Bump actions/dependency-review-action from 2.4.0 to 2.5.0 (Bump actions/dependency-review-action from 2.4.0 to 2.5.0 #9179)
• c1ba412 Bump github.com/onsi/ginkgo/v2 from 2.3.1 to 2.4.0 (Bump github.com/onsi/ginkgo/v2 from 2.3.1 to 2.4.0 #9201)
• cecde86 Bump goreleaser/goreleaser-action from 3.1.0 to 3.2.0 (Bump goreleaser/goreleaser-action from 3.1.0 to 3.2.0 #9208)
• ffa9260 Bump github.com/stretchr/testify from 1.8.0 to 1.8.1 (Bump github.com/stretchr/testify from 1.8.0 to 1.8.1 #9202)
• ab590c8 Bump ossf/scorecard-action from 2.0.4 to 2.0.6 (Bump ossf/scorecard-action from 2.0.4 to 2.0.6 #9203)
• a2e9d1a Bump actions/setup-go from 3.3.0 to 3.3.1 (Bump actions/setup-go from 3.3.0 to 3.3.1 #9205)
• 3d547ff Bump github/codeql-action from 2.1.27 to 2.1.28 (Bump github/codeql-action from 2.1.27 to 2.1.28 #9206)
• 411eb59 Bump github.com/prometheus/client_model from 0.2.0 to 0.3.0 (Bump github.com/prometheus/client_model from 0.2.0 to 0.3.0 #9200)
• d575194 Bump actions/dependency-review-action from 2.5.0 to 2.5.1 (Bump actions/dependency-review-action from 2.5.0 to 2.5.1 #9237)
• fb6099d Bump github/codeql-action from 2.1.28 to 2.1.29 (Bump github/codeql-action from 2.1.28 to 2.1.29 #9236)
• 732a15a Bump github.com/spf13/cobra from 1.6.0 to 1.6.1 (Bump github.com/spf13/cobra from 1.6.0 to 1.6.1 #9233)
• 14e213d Bump actions/upload-artifact from 3.1.0 to 3.1.1 (Bump actions/upload-artifact from 3.1.0 to 3.1.1 #9234)
• 4bf022f Bump github.com/prometheus/client_golang from 1.13.0 to 1.13.1 (Bump github.com/prometheus/client_golang from 1.13.0 to 1.13.1 #9262)
• 10164c2 Bump github/codeql-action from 2.1.29 to 2.1.31 (Bump github/codeql-action from 2.1.29 to 2.1.31 #9263)
• 323dafe Bump aquasecurity/trivy-action from 0.7.1 to 0.8.0 (Bump aquasecurity/trivy-action from 0.7.1 to 0.8.0 #9264)
• 77be92b Bump k8s.io/component-base from 0.25.3 to 0.25.4 (Bump k8s.io/component-base from 0.25.3 to 0.25.4 #9300)
• 9918ffe Bump actions/dependency-review-action from 2.5.1 to 3.0.0 (Bump actions/dependency-review-action from 2.5.1 to 3.0.0 #9301)
• 5372ba6 Bump github.com/onsi/ginkgo/v2 from 2.4.0 to 2.5.1 (Bump github.com/onsi/ginkgo/v2 from 2.4.0 to 2.5.1 #9317)
• 54391a3 Bump golang.org/x/crypto from 0.1.0 to 0.3.0 (Bump golang.org/x/crypto from 0.1.0 to 0.3.0 #9318)
• 100896e Bump actions/dependency-review-action from 3.0.0 to 3.0.1 (Bump actions/dependency-review-action from 3.0.0 to 3.0.1 #9319)
• 9b84899 Bump github.com/prometheus/client_golang from 1.13.1 to 1.14.0 (Bump github.com/prometheus/client_golang from 1.13.1 to 1.14.0 #9298)
• 3e88921 Bump google.golang.org/grpc from 1.50.1 to 1.51.0 (Bump google.golang.org/grpc from 1.50.1 to 1.51.0 #9316)
• 2c5a9f8 Bump github/codeql-action from 2.1.31 to 2.1.35 (Bump github/codeql-action from 2.1.31 to 2.1.35 #9369)
• 00f8b51 Bump actions/setup-go from 3.3.1 to 3.4.0 (Bump actions/setup-go from 3.3.1 to 3.4.0 #9370)
• 5f53697 Bump github/codeql-action from 2.1.35 to 2.1.36 (Bump github/codeql-action from 2.1.35 to 2.1.36 #9400)
• 4f3bab6 Bump github.com/onsi/ginkgo/v2 from 2.5.1 to 2.6.0 (Bump github.com/onsi/ginkgo/v2 from 2.5.1 to 2.6.0 #9398)
• 71603dd Bump github.com/prometheus/common from 0.37.0 to 0.39.0 (Bump github.com/prometheus/common from 0.37.0 to 0.39.0 #9416)
• 8f05393 Bump ossf/scorecard-action from 2.0.6 to 2.1.0 (Bump ossf/scorecard-action from 2.0.6 to 2.1.0 #9422)
• bb6ec69 Bump actions/dependency-review-action from 3.0.1 to 3.0.2 (Bump actions/dependency-review-action from 3.0.1 to 3.0.2 #9424)
• 75e78e6 Bump goreleaser/goreleaser-action from 3.2.0 to 4.1.0 (Bump goreleaser/goreleaser-action from 3.2.0 to 4.1.0 #9426)
• b42e2c2 Bump actions/checkout from 3.1.0 to 3.2.0 (Bump actions/checkout from 3.1.0 to 3.2.0 #9425)
• f3890a5 Bump github/codeql-action from 2.1.36 to 2.1.37 (Bump github/codeql-action from 2.1.36 to 2.1.37 #9423)
• e06d78a Bump github.com/onsi/ginkgo/v2 from 2.6.0 to 2.6.1 (Bump github.com/onsi/ginkgo/v2 from 2.6.0 to 2.6.1 #9421)
• ab51ce9 Bump golang.org/x/crypto from 0.3.0 to 0.4.0 (Bump golang.org/x/crypto from 0.3.0 to 0.4.0 #9397)

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. I understand the commands that are listed here.

[jaehnri]

Well, looks like the rebase didn't quite work. I'll see what can I do. I think that maybe opening another clean PR would be better. There aren't many changes anyway.. 🙃

[jaehnri]

As the rebase went wrong, I created another PR (see #9474).