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

bug(gobinary): incorrect ldflags parsing when version part has prefix #6702

Closed
DmitriyLewen opened this issue May 16, 2024 Discussed in #6669 · 8 comments · Fixed by #6705 or #6745
Closed

bug(gobinary): incorrect ldflags parsing when version part has prefix #6702

DmitriyLewen opened this issue May 16, 2024 Discussed in #6669 · 8 comments · Fixed by #6705 or #6745
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@DmitriyLewen
Copy link
Contributor

DmitriyLewen commented May 16, 2024

Description

Trivy detects different versions of Gobinary modules during multiple scans.
This happens when gobinari uses 2 ldflags with the suffix versions.
e.g.:

	build	-ldflags="-X github.com/argoproj/argo-cd/v2/common.version=2.11.0 
 -X github.com/argoproj/argo-cd/v2/common.kubectlVersion=v0.26.11 
-X \"github.com/argoproj/argo-cd/v2/common.extraBuildInfo=\" -extldflags \"-static\""

Discussed in #6669

@DmitriyLewen DmitriyLewen added the kind/bug Categorizes issue or PR as related to a bug. label May 16, 2024
@DmitriyLewen DmitriyLewen self-assigned this May 16, 2024
@knqyf263
Copy link
Collaborator

#6705 addresses the case of ArgoCD, but a bigger problem is the inconsistent result. In the following case, we will face the same problem.

-X github.com/argoproj/argo-cd/v2/common.version=2.11.0
-X github.com/argoproj/argo-cd/v2/kubectl.version=v0.26.11 

What if we collect all versions and select one of them deterministically? Or return an error, like several versions found?

@DmitriyLewen
Copy link
Contributor Author

Or return an error, like several versions found?

I think this is bad idea. I think it's not their own binaries that users are scanning, so they can't update ldflags.
And then users will not be able to scan binaries with multiple *.version=x.x.x flags.

What if we collect all versions and select one of them deterministically?

I didn't find many binaries using ldflags.
I saw main, version, common.
I didn't notice this, but I guess we can also check module name (something like that -X https://github.com/aquasecurity/trivy/trivy.version=v0.51.0

I suggest the following order <module_name> => common => main =>version.

@knqyf263
Copy link
Collaborator

And then users will not be able to scan binaries with multiple *.version=x.x.x flags.

First of all, the scan does not fail because analyzer errors are always converted into warnings. Next, when I said return error, I meant leave the version empty, rather than select a version that could be wrong on our end.

I didn't find many binaries using ldflags.

How do they embed versions without using ldflags?

@DmitriyLewen
Copy link
Contributor Author

How do they embed versions without using ldflags?

Sorry, my brain isn't working well today. I checked installed in my PC binaries which were installed using go install command.
Obviously these binaries do not use ldflags.

First of all, the scan does not fail because analyzer errors are always converted into warnings. Next, when I said return error, I meant leave the version empty, rather than select a version that could be wrong on our end.

Got it. Then this solution looks good.
I tried to discover some logic that we can use to detect core packages, but ldglag is so custom that all users use their own logic...
e.g. go-containerregistry uses github.com/google/go-containerregistry/cmd/crane/cmd.Version + github.com/google/go-containerregistry/pkg/v1/remote/transport.Version (path not included "transport").
Goreleaser has a default value - https://goreleaser.com/cookbooks/using-main.version/, but it is easy to configure.

It also looks like we need to parse URL.
e.g. user might use version.Version=x.x.x for two modules in their repository:

  • github.com/owner/cmd/module/version.Version=2.0.0
  • github.com/owner/pkg/submodule/submodule/version.Version=1.0.0

It looks like you are right and best solution is to skip version for binaries with more than one version in ldflags.

@knqyf263
Copy link
Collaborator

I didn't notice this, but I guess we can also check module name (something like that -X https://github.com/aquasecurity/trivy/trivy.version=v0.51.0

As you suggested, it may be a good idea to increase the priority if -X starts with the module name and /cmd/, like github.com/google/go-containerregistry/cmd. If we still find more than one applicable version, it should leave it empty.

@DmitriyLewen
Copy link
Contributor Author

Just to confirm that I understood you correctly:

  1. github.com/<owner>/<repo>/cmd/*/*.version=x.x.x format

    • format only:
      • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 => 1.0.0.
      • (cmd from non-root dir): github.com/google/go-containerregistry/v1/cmd/crane/any.Version=1.0.0 => step 2
    • format + hardcoded prefix:
      • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 + github.com/google/go-containerregistry/pkg/v1/remote/main.2.0.0 => 1.0.0 or empty?
    • format + another prefix:
      • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 + github.com/google/go-containerregistry/pkg/v1/remote/transport.2.0.0 => 1.0.0 or empty?
  2. hardcoded prefixes (main, common, version, cmd):

    • one version with hardcoded prefix:
      • common.Version=1.0.0 => 1.0.0
    • hardcoded + another prefixes:
      • common.Version=1.0.0 + transport.Version=2.0.0 => 1.0.0
    • two hardcoded prefixes:
      • common.Version=1.0.0 + main.Version=2.0.0 => empty version.
  3. other prefixes

    • one version:
      • kubectl.Version=1.0.0 => 1.0.0
    • two version:
      • terraform.Version=1.0.0 + kubectl.Version=2.0.0 => empty version.

@knqyf263
Copy link
Collaborator

knqyf263 commented May 21, 2024

In any case, it is a heuristic, so it is simply how we define it. If there is more than one matching version in the same category (1, 2, 3 as you described), make the version empty. And in order of priority from the top.

  1. github.com/<owner>/<repo>/cmd/*/*.version=x.x.x format
  2. hardcoded prefixes (main, common, version, cmd):
  3. other prefixes
graph TD
   A{Number of versions matching 1} -->|0| B{Number of versions matching 2}
   A -->|1| C[Adopt that version]
   A -->|2+| D[Empty]
   
   B -->|0| E{Number of versions matching 3}
   B -->|1| F[Adopt that version]
   B -->|2+| G[Empty]
   
   E -->|0| H[Empty]
   E -->|1| I[Adopt that version]
   E -->|2+| J[Empty]
Loading
  • format + hardcoded prefix:
    • github.com/google/go-containerregistry/cmd/crane/any.Version=1.0.0 + github.com/google/go-containerregistry/pkg/v1/remote/main.2.0.0 => 1.0.0 or empty?

Since there is only one version matching the category 1, we take 1.0.0.

  • format + another prefix:

same as above.

@knqyf263
Copy link
Collaborator

Actually, all categories must start with the module name and end with version or ver. So, I think we can simplify them.

  1. Include /cmd/ path after the module name
  2. hardcoded prefixes (main, common, version, cmd):
  3. other prefixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
2 participants