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

feat(go): use toolchain as stdlib version for go.mod files #7163

Merged

Conversation

DmitriyLewen
Copy link
Contributor

Description

Use toolchain as stdlib version.

➜ cat ./go.mod
module github.com/aquasecurity/trivy

go 1.22.0

toolchain go1.22.4

➜ trivy -q fs ./go.mod 

go.mod (gomod)

Total: 1 (UNKNOWN: 0, LOW: 0, MEDIUM: 1, HIGH: 0, CRITICAL: 0)

┌─────────┬────────────────┬──────────┬────────┬───────────────────┬─────────────────┬──────────────────────────────────────────────────────────┐
│ Library │ Vulnerability  │ Severity │ Status │ Installed Version │  Fixed Version  │                          Title                           │
├─────────┼────────────────┼──────────┼────────┼───────────────────┼─────────────────┼──────────────────────────────────────────────────────────┤
│ stdlib  │ CVE-2024-24791 │ MEDIUM   │ fixed  │ 1.22.4            │ 1.21.12, 1.22.5 │ net/http: Denial of service due to improper 100-continue │
│         │                │          │        │                   │                 │ handling in net/http                                     │
│         │                │          │        │                   │                 │ https://avd.aquasec.com/nvd/cve-2024-24791               │
└─────────┴────────────────┴──────────┴────────┴───────────────────┴─────────────────┴──────────────────────────────────────────────────────────┘

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@DmitriyLewen DmitriyLewen self-assigned this Jul 15, 2024
@DmitriyLewen DmitriyLewen changed the title feat(golang): use toolchain as stdlib version for go.mod files feat(go): use toolchain as stdlib version for go.mod files Jul 15, 2024
@DmitriyLewen DmitriyLewen marked this pull request as ready for review July 15, 2024 05:23
@knqyf263
Copy link
Collaborator

knqyf263 commented Jul 15, 2024

There is one discussion. Technically, toolchain is not always used for compilation. If the local Go version is newer than the toolchain version, Go uses the local version.

$ go mod init example.com/test
$ go get [email protected]
$ go get [email protected]
$ cat go.mod
module example.com/test

go 1.22.0

toolchain go1.22.2
$ cat main.go
package main

import "fmt"

func main(){
        fmt.Println("hello world")
}
$ go version
go version go1.22.3 darwin/arm64
$ go build ./main.go
$ go version -m ./main | head -n 1
./main: go1.22.3 // <= toolchain go1.22.2 is not used

However, if the local version is older, the toolchain version is used for forward compatibility.

$ go get [email protected]
go: upgraded toolchain go1.22.2 => go1.22.4
$ cat go.mod
module example.com/test

go 1.22.0

toolchain go1.22.4

$ go build ./main.go
go version -m ./main | head -n 1
./main: go1.22.4

They can be configured through GOTOOLCHAIN, but I think it's too much. We can focus on the default behavior.

, +auto, or +path. GOTOOLCHAIN=auto is shorthand for=local+auto

@DmitriyLewen Do you think we should work in the same way?

@DmitriyLewen
Copy link
Contributor Author

There is one discussion. Technically, toolchain is not always used for compilation. If the local Go version is newer than the toolchain version, Go uses the local version.

Hmm... I didn't look at that.
In this case, I don't know how we can correctly determine stdlib version. We need to define the local version of Go, but GOROOT is not a required env.

This seems to be the same case as for the SDK in Dark.
We can only show a minimal version.

Do you think we should work in the same way?

Let's start from default logic.
We will handle GOTOOLCHAIN ​​later if users need it.

@knqyf263
Copy link
Collaborator

This is how govulncheck works.
https://github.com/golang/vuln/blob/d9ad5223849a23e412d4a6ac1f61ecfc94cab0d8/internal/scan/run.go#L89-L93

go version actually respects go.mod.

$ go get [email protected]
go go: downgraded toolchain go1.22.4 => go1.22.3
$ cat go.mod
module example.com/test

go 1.22.0

toolchain go1.22.3

$ go version
go version go1.22.3 darwin/arm64
$ go get [email protected]
go: upgraded toolchain go1.22.3 => go1.22.4
$ cat go.mod
module example.com/test

go 1.22.0

toolchain go1.22.4
$ go version
go version go1.22.4 darwin/arm64

But I'm still not sure this is a good approach. If the environment in which Trivy is run is different from the environment in which the project is built, the vulnerability may be overlooked. Also, people can be confused as a vulnerability may be detected or not detected even though they scan the same project.

@DmitriyLewen
Copy link
Contributor Author

https://github.com/golang/vuln/blob/d9ad5223849a23e412d4a6ac1f61ecfc94cab0d8/internal/scan/run.go#L89-L93

I thought that we don't want to run other apps in Trivy, even if it's go.

But I'm still not sure this is a good approach. If the environment in which Trivy is run is different from the environment in which the project is built, the vulnerability may be overlooked. Also, people can be confused as a vulnerability may be detected or not detected even though they scan the same project.

in this case, users will need to check (or just be mindful) of many things when scanning go.mod file.
e.g. version of my local Go is not equal to version of Go from our CI/CD.
When i will scan go.mod => i can get a vulnerable binary (or I will need to scan binary again, but then why would I scan the go.mod)

@knqyf263
Copy link
Collaborator

OK, it is better to use the toolchain version at the moment for reproducibility. Since it's technically different from how the Go version is selected for compiling a binary, we should document it.

@DmitriyLewen
Copy link
Contributor Author

Yeah. I will update docs.

@DmitriyLewen
Copy link
Contributor Author

@knqyf263 i updated docs.
Please take a look.
You might want to add something.

@@ -83,6 +83,19 @@ func (p *Parser) Parse(r xio.ReadSeekerAt) ([]ftypes.Package, []ftypes.Dependenc
skipIndirect = lessThan117(modFileParsed.Go.Version)
}

// Stdlib
if toolchain := modFileParsed.Toolchain; toolchain != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since Go uses go line if toolchain is omitted, we probably need to check the go line as well.

If the toolchain line is omitted, the module or workspace is considered to have an implicit toolchain goV line, where V is the Go version from the go line.

https://go.dev/doc/toolchain

But we need to consider how to treat a go line omitting a patch version, like go 1.22. I think we can skip stdlib in this case.

Copy link
Contributor Author

@DmitriyLewen DmitriyLewen Jul 18, 2024

Choose a reason for hiding this comment

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

I think we can skip stdlib in this case.

If module uses version without patch (and child modules don't use patch and toolchain) - go doesn't add patch/toolchain:

➜ cat ../greetings/go.mod 
module github.com/greetings

go 1.22
➜ cat go.mod
module example.com/hello

go 1.22

replace example.com/greetings => ../greetings

require example.com/greetings v0.0.0-00010101000000-000000000000
➜ go version
go version go1.22.0 darwin/arm64

since we say we use minimum required version for stdlib - we can say that v1.x.0 (v1.21.0 for this example) is the minimum required version, no?
I think I'm missing something, but I can't figure out what 😄

Copy link
Contributor Author

@DmitriyLewen DmitriyLewen Jul 18, 2024

Choose a reason for hiding this comment

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

or do you mean that if go version doesn't contain patch - that means it is not a situation where toolchan is omitted?
and we don't need to check for cases where toolchain is not used (or omitted).

But it doesn't work for v1.19 or early: The standard Go toolchains are named goV where V is a Go version denoting a beta release, release candidate, or release. For example, go1.21rc1 and go1.21.0 are toolchain names; go1.21 and go1.22 are not (the initial releases are go1.21.0 and go1.22.0), but go1.20 and go1.19 are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can say that v1.x.0 (v1.21.0 for this example) is the minimum required version, no?

I found answer - 1.21 != 1.21.0:
For example, 1.21 < 1.21rc1 < 1.21rc2 < 1.21.0 < 1.21.1 < 1.21.2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knqyf263 I updated this PR:

  • if toolchain is omitted - check go line
    • check go version (take only >= 1.21)
    • check patch
    • check rc releases

Copy link
Collaborator

Choose a reason for hiding this comment

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

Detection using the minimum version may be better enabled when this flag is used. For example, django>=3.0.0 in requirements.txt, we can take 3.0.0 as the version even if the project may use newer than 3.0.0. The toolchain version is the same. From toolchain go1.21.4 in go.mod, we consider it Go 1.21.4 even if the project may actually use Go 1.21.5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detection using the minimum version may be better enabled when this flag is used.

Agree with you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I've converted this PR to draft. Let's finalize this proposal first and come back here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

django>=3.0.0 in requirements.txt, we can take 3.0.0 as the version even if the project may use newer than 3.0.0

this is good idea 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knqyf263 I updated this PR using --detection-priority flag.
Take a look, when you have time, please.

@knqyf263 knqyf263 marked this pull request as draft July 22, 2024 08:16
@DmitriyLewen DmitriyLewen marked this pull request as ready for review August 30, 2024 08:08
pkg/dependency/parser/golang/mod/parse.go Outdated Show resolved Hide resolved

// Use minimal required go version from `toolchain` line (or from `go` line if `toolchain` is omitted) as `stdlib`.
// Show `stdlib` only with `useMinVersion` flag.
if toolchainVer := toolchainVersion(modFileParsed.Toolchain, modFileParsed.Go); p.useMinVersion && toolchainVer != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: It's no big deal, but if useMinVersion == false, we don't need to get the toolchain version. We may want to evaluate useMinVersion first before getting the toolchain version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It make sense.
Updated in e6411c0

@knqyf263 knqyf263 added this pull request to the merge queue Sep 3, 2024
Merged via the queue into aquasecurity:main with commit 2d80769 Sep 3, 2024
17 checks passed
@aqua-bot aqua-bot mentioned this pull request Sep 3, 2024
@knqyf263 knqyf263 mentioned this pull request Sep 5, 2024
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.

feat(golang): support toolchain directive
2 participants