-
Notifications
You must be signed in to change notification settings - Fork 152
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
build: run govulncheck with go version from build image #2931
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the Go version in vulncheck should be the same as used. It's also possible to define a Go version explicitly in the yaml file, so that they a synchronous
So instead |
Actually recent Go releases automatically also specify the patch version in |
When I run
"Used" version is whatever we have in the build image https://github.com/kanisterio/kanister/blob/master/docker/build/Dockerfile#L1 Alternatively we might consider using github workflow tools instead of the build image, maybe. |
Wouldn't it be easier at this point to install and run
This way it's guaranteed that the check against the Go standard library version is always the same as in the build image. That's especially useful for minor version upgrades that need to be postponed. Another advantage is that this would allow for version pinning. This would create fully reproducible results and is also a security recommendation. [1] (The [1] OpenSSF Scorecard / Check Documentation / Pinned-Dependencies
Ah ok, what would be the difference for that option? |
(Arguably checking a commit from a year ago with So maybe
might provide the best trade-offs) |
That also makes sense. Then we will need to bump the go version in the build image if we have vulnerability report. That streamlines the process a bit. |
.github/workflows/govulncheck.yaml
Outdated
@@ -23,3 +23,4 @@ jobs: | |||
repo-checkout: false | |||
cache: false | |||
go-version-file: go.mod | |||
check-latest: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@psilva-veeam IIUC, the line above (25) makes it so the Go version used to run govulncheck
matches the one used to build the code up to the minor version, but maybe not the patch version since it is not (currently) specified in the go.mod
file.
Is this your understanding as well? Is that sufficient or is there a need to run with the specific patch
version?
Also, the go.mod
file specifies the toolchain
with a patch version, perhaps that needs to be updated?
The go version used by govulncheck
is whatever the actions/setup-go
action downloads. It is not clear whether or not it uses the toolchain
entry from the go.mod
file. I didn't see it described in the documentation, but that is easy to test.
@hairyhum How does check-latest:
interact or change the behavior of go-version-file: go.mod
?
https://github.com/actions/setup-go?tab=readme-ov-file#check-latest-version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julio-lopez I think so, this is also my understanding. So I've been running quite a few tests with govulncheck
when it was added. What I noticed is that the difference in patch versions produces occasional warnings. Especially in net
and net/http
, e.g. the HTTP/2 bug that was fixed in go1.21.9 and 1.22.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. I think I have a better understanding now. These warning are specific to vulnerabilities detected in the standard library.
Does this comment describe the risk?
#2931 (comment)
Yes, updating the toolchain setting to But I'm not 100% sure that's the right way to go, because if we build the binaries with So I feel like we better either keep go version in |
My understanding, (and please verify because I could be wrong) is that the Go toolchain will attempt to use the newer toolchain when the version of the currently running toolchain does not match. From the "Go Toolchains" documentation:
Having said that, if this ends up downloading the new toolchain every time, we might as well upgrade the local version of Go. It appears that there is a risk in not upgrading Go. |
IIUC, the risk would be to incorrectly deem a build as non-vulnerable when there is an actual vulnerability. Is that the concern? The scenario would be:
|
@@ -2,7 +2,7 @@ module github.com/kanisterio/kanister | |||
|
|||
go 1.22 | |||
|
|||
toolchain go1.22.2 | |||
toolchain go1.22.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would address the most recent vulnerabilities flagged by govulncheck
.
Does it does address the risk described in this comment?
Not unless there's a Unfortunately it doesn't fail or warn us the the toolchain is set to later version than the local and just builds with whatever it currently has. If we set |
098d765
to
e231620
Compare
Go version from the code might be resolved differently, we build packages using build image local toolchain. Make workflow resolve latest build image version for govulncheck
e231620
to
3a56e79
Compare
REWORK The PR now gets go version from the build image. |
@@ -15,11 +15,16 @@ jobs: | |||
steps: | |||
- name: 'Checkout Repository' | |||
uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332 # v4.1.7 | |||
- name: 'Get go version from build image' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: add a comment that makes it explicit that this is used in the next step to specify the Go version to use. It is easy to miss the connection (via ${{ steps.go_version.outputs.go_version }}
) when going through the workflow.
Change Overview
Patch Go version from cache might contain vulnerabilities, but we specify minor version only in go.mod file.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
Run the workflow from this branch