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): parse main mod version from build info settings #6564

Merged

Conversation

oatovar
Copy link
Contributor

@oatovar oatovar commented Apr 26, 2024

Description

  • Parse the build info for -ldflags. If included, the flags will be further parsed, and searched for -X sub-flags. These sub-flags are commonly used to set the binary version during build time. A common pattern is to run go build -ldflags='-X main.version=<semver>' so that the main package's version variable is replaced with the latest tag. It's not guaranteed that this flag will propogate into the binary. See cmd/go: using --trimpath in go build removes -ldflags from go version -m output golang/go#63432 for more info.

  • Flag parsing reuses the pflags library that's used by the main Trivy binary. This keeps the implementation concise, and a bit more robust than creating a custom flag parser for -X flag commonly passed to -ldflags.

  • Add simple binary to test for validity of ldflags parsing.

  • The documentation has been updated to reflect the improved accuracy of the Go binary parser.

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).

Output in CycloneDX

    {
      "bom-ref": "pkg:golang/github.com/aquasecurity/[email protected]",
      "type": "library",
      "name": "github.com/aquasecurity/trivy",
      "version": "0.50.1-65-ge80e222e9",
      "purl": "pkg:golang/github.com/aquasecurity/[email protected]",
      "properties": [
        {
          "name": "aquasecurity:trivy:LayerDiffID",
          "value": "sha256:41f456757360f81ad1905937cc923e2d07edb9f590e1de739e18a56543b905c1"
        },
        {
          "name": "aquasecurity:trivy:PkgType",
          "value": "gobinary"
        }
      ]
    }

@oatovar oatovar marked this pull request as ready for review April 26, 2024 16:11
@oatovar
Copy link
Contributor Author

oatovar commented Apr 26, 2024

I used a test project to test the new parsing ability with test data. I wasn't sure how to generate the test data otherwise. Please let me know if there's something that I've missed about this.

@oatovar oatovar marked this pull request as draft April 26, 2024 16:58
@oatovar oatovar marked this pull request as ready for review April 26, 2024 20:58
@oatovar oatovar marked this pull request as draft April 26, 2024 20:58
@oatovar oatovar force-pushed the oatovar/parse-main.version-in-go-binaries branch from 9bb94c4 to 812f898 Compare April 26, 2024 20:59
@oatovar oatovar marked this pull request as ready for review April 26, 2024 21:00
* Parse the build info for -ldflags. If included, the flags will be
  further parsed, and searched for -X sub-flags. These sub-flags are
  commonly used to set the binary version during build time. A common
  pattern is to run `go build -ldflags='-X main.version=<semver>'` so
  that the main package's version variable is replaced with the latest
  tag. It's not guaranteed that this flag will propogate into the
  binary. See golang/go#63432 for more info.

* Flag parsing reuses the pflags library that's used by the main Trivy
  binary. This keeps the implementation concise, and a bit more robust
  than creating a custom flag parser for -X flag commonly passed to
  -ldflags.

* Add simple binary to test for validity of ldflags parsing. The flag

* The documentation has been updated to reflect the improved accuracy of
  the Go binary parser.
@oatovar oatovar force-pushed the oatovar/parse-main.version-in-go-binaries branch from 812f898 to 98370b3 Compare April 28, 2024 04:07
@oatovar
Copy link
Contributor Author

oatovar commented Apr 29, 2024

Fixed the unit test that changed after the merge conflict. 😢 I forgot to run mage test:unit before pushing that resolution, sorry.

Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Thanks for your great contribution! Looks almost good. I left some comments.

We'd love to include it in v0.51.0, which will be released this week. Please let me know if you don't have time. We'll add small tweaks and wrap it up.


key = strings.ToLower(key)
// The check for a 'ver' prefix enables the parser to pick up Trivy's own version value that's set.
if strings.HasSuffix(key, "version") || strings.HasSuffix(key, "ver") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if validating semver?

Suggested change
if strings.HasSuffix(key, "version") || strings.HasSuffix(key, "ver") {
if (strings.HasSuffix(key, "version") || strings.HasSuffix(key, "ver")) && semver.IsValid(value) {

// The flag can also be set multiple times, so a string slice is needed
// to handle that edge case.
var x []string
fset.StringSliceVarP(&x, "", "X", nil, `Set the value of the string variable in importpath named name to value.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need the original usage here.

Comment on lines 130 to 156
var x []string
fset.StringSliceVarP(&x, "", "X", nil, `Set the value of the string variable in importpath named name to value.
This is only effective if the variable is declared in the source code either uninitialized
or initialized to a constant string expression. -X will not work if the initializer makes
a function call or refers to other variables.
Note that before Go 1.5 this option took two separate arguments.`)
if err := fset.Parse(flags); err != nil {
p.logger.Error("Could not parse -ldflags found in build info", "err", err)
return ""
}

for _, xx := range x {
// It's valid to set the -X flags with quotes so we trim any that might
// have been provided: Ex:
//
// -X main.version=1.0.0
// -X=main.version=1.0.0
// -X 'main.version=1.0.0'
// -X='main.version=1.0.0'
// -X="main.version=1.0.0"
// -X "main.version=1.0.0"
xx = strings.Trim(xx, `'"`)
key, val, found := strings.Cut(xx, "=")
if !found {
p.logger.Debug("Unable to parse an -ldflags -X flag value", "got", xx)
continue
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

StringToStringVarP possibly simplifies parsing.

Suggested change
var x []string
fset.StringSliceVarP(&x, "", "X", nil, `Set the value of the string variable in importpath named name to value.
This is only effective if the variable is declared in the source code either uninitialized
or initialized to a constant string expression. -X will not work if the initializer makes
a function call or refers to other variables.
Note that before Go 1.5 this option took two separate arguments.`)
if err := fset.Parse(flags); err != nil {
p.logger.Error("Could not parse -ldflags found in build info", "err", err)
return ""
}
for _, xx := range x {
// It's valid to set the -X flags with quotes so we trim any that might
// have been provided: Ex:
//
// -X main.version=1.0.0
// -X=main.version=1.0.0
// -X 'main.version=1.0.0'
// -X='main.version=1.0.0'
// -X="main.version=1.0.0"
// -X "main.version=1.0.0"
xx = strings.Trim(xx, `'"`)
key, val, found := strings.Cut(xx, "=")
if !found {
p.logger.Debug("Unable to parse an -ldflags -X flag value", "got", xx)
continue
}
var x map[string]string
fset.StringToStringVarP(&x, "", "X", nil, "")
if err := fset.Parse(flags); err != nil {
p.logger.Error("Could not parse -ldflags found in build info", "err", err)
return ""
}
for key, value := range x {
key = strings.ToLower(key)
// The check for a 'ver' prefix enables the parser to pick up Trivy's own version value that's set.
if (strings.HasSuffix(key, "version") || strings.HasSuffix(key, "ver")) && semver.IsValid(value) {
return value
}
}```

}

// parseLDFlags attempts to parse the binary's version from any `-ldflags` passed to `go build` at build time.
func (p *Parser) parseLDFlags(name string, flags []string) string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The input is so varied that it is worth exporting this method and writing tests for it.

Suggested change
func (p *Parser) parseLDFlags(name string, flags []string) string {
func (p *Parser) ParseLDFlags(name string, flags []string) string {
func TestParser_ParseLDFlags(t *testing.T) {
	type args struct {
		name  string
		flags []string
	}
	tests := []struct {
		name string
		args args
		want string
	}{
		{
			name: "normal",
			args: args{
				name: "github.com/aquasecurity/trivy",
				flags: []string{
					"-s",
					"-w",
					"-X=foo=bar",
					"-X='github.com/aquasecurity/trivy/pkg/version.ver=0.50.1'",
				},
			},
			want: "0.50.1",
		},
		{
			name: "empty",
			args: args{
				name:  "github.com/aquasecurity/test",
				flags: []string{},
			},
			want: "",
		},
                ... (more tests) ...
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			p := binary.NewParser().(*binary.Parser)
			assert.Equal(t, tt.want, p.ParseLDFlags(tt.args.name, tt.args.flags))
		})
	}
}


// parseLDFlags attempts to parse the binary's version from any `-ldflags` passed to `go build` at build time.
func (p *Parser) parseLDFlags(name string, flags []string) string {
log.Debug("Parsing dependency's build info settings", "dependency", name, "-ldflags", flags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
log.Debug("Parsing dependency's build info settings", "dependency", name, "-ldflags", flags)
p.logger.Debug("Parsing dependency's build info settings", "dependency", name, "-ldflags", flags)

oatovar and others added 4 commits April 30, 2024 23:38
Reuse the parsing done by StringToStringVarP to parse the -ldflags.
Additionally, a check for valid semver strings is added, and the check
for a valid sub-flag name is extracted to increase readability. Test
cases are also added for the exported ParseLDFlags function.

Co-authored-by: Teppei Fukuda <[email protected]>
@oatovar
Copy link
Contributor Author

oatovar commented May 1, 2024

@knqyf263 Thank you for the review. Having use the pflag library in the past, I had overlooked the StringToStringVarP functionality - it's very powerful! I've applied your suggestions, and I think the PR is ready for another review.

@knqyf263
Copy link
Collaborator

knqyf263 commented May 1, 2024

Great! I'll take another look today.

Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Looks good! I left very small comments.

pkg/dependency/parser/golang/binary/parse.go Outdated Show resolved Hide resolved
pkg/dependency/parser/golang/binary/parse.go Outdated Show resolved Hide resolved
pkg/dependency/parser/golang/binary/parse.go Outdated Show resolved Hide resolved
pkg/dependency/parser/golang/binary/parse_test.go Outdated Show resolved Hide resolved
@oatovar
Copy link
Contributor Author

oatovar commented May 1, 2024

Thank you for catching those errors 😄 I've applied them all.

Copy link
Collaborator

@knqyf263 knqyf263 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@knqyf263 knqyf263 enabled auto-merge May 2, 2024 05:11
@knqyf263 knqyf263 added this pull request to the merge queue May 2, 2024
Merged via the queue into aquasecurity:main with commit 419e3d2 May 2, 2024
17 checks passed
knqyf263 added a commit to knqyf263/trivy that referenced this pull request May 2, 2024
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
…b 0.0.1

Trivy recently started inferring the version of the binary as of 0.51.0, see
aquasecurity/trivy#6564

The version used generated by go releaser is 0.0.1-next, and trivy detects
that as version 0.0.1 of SpiceDB, and flags that as having CVEs, even though
it's not really version 0.0.1.
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
…b 0.0.1

Trivy recently started inferring the version of the binary as of 0.51.0, see
aquasecurity/trivy#6564

The version used generated by go releaser is 0.0.1-next, and trivy detects
that as version 0.0.1 of SpiceDB, and flags that as having CVEs, even though
it's not really version 0.0.1.
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
…b 0.0.1

Trivy recently started inferring the version of the binary as of 0.51.0, see
aquasecurity/trivy#6564

The version used generated by go releaser is 0.0.1-next, and trivy detects
that as version 0.0.1 of SpiceDB, and flags that as having CVEs, even though
it's not really version 0.0.1.
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
…b 0.0.1

Trivy recently started inferring the version of the binary as of 0.51.0, see
aquasecurity/trivy#6564

The version used generated by go releaser is 0.0.1-next, and trivy detects
that as version 0.0.1 of SpiceDB, and flags that as having CVEs, even though
it's not really version 0.0.1.
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
this fixes an issue with trivy where it flags SpiceDB as
vulnerable, possibly as of aquasecurity/trivy#6564
include in version 0.51.0. It's flagged because it parses it as version
0.0.1-next
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
this fixes an issue with trivy where it flags SpiceDB as
vulnerable, possibly as of aquasecurity/trivy#6564
include in version 0.51.0. It's flagged because it parses it as version
0.0.1-next
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
this fixes an issue with trivy where it flags SpiceDB as
vulnerable, possibly as of aquasecurity/trivy#6564
include in version 0.51.0. It's flagged because it parses it as version
0.0.1-next
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
this fixes an issue with trivy where it flags SpiceDB as
vulnerable, possibly as of aquasecurity/trivy#6564
include in version 0.51.0. It's flagged because it parses it as version
0.0.1-next
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
this fixes an issue with trivy where it flags SpiceDB as
vulnerable, possibly as of aquasecurity/trivy#6564
include in version 0.51.0. It's flagged because it parses it as version
0.0.1-next as generated by goreleaser, because it does not have the tags
available
vroldanbet added a commit to authzed/spicedb that referenced this pull request May 3, 2024
this fixes an issue with trivy where it flags SpiceDB as
vulnerable, possibly as of aquasecurity/trivy#6564
include in version 0.51.0. It's flagged because it parses it as version
0.0.1-next as generated by goreleaser, because it does not have the tags
available
fl0pp5 pushed a commit to altlinux/trivy that referenced this pull request May 6, 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.

2 participants