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

fix(cli): secret scanning perf link fix #2607

Merged
merged 7 commits into from
Aug 15, 2022
Merged

fix(cli): secret scanning perf link fix #2607

merged 7 commits into from
Aug 15, 2022

Conversation

Moulick
Copy link
Contributor

@Moulick Moulick commented Jul 27, 2022

Description

Fixes the URL in the logs of the scanner

Related issues

Before

2022-07-27T18:57:49.498+0530	INFO	Vulnerability scanning is enabled
2022-07-27T18:57:49.498+0530	INFO	Secret scanning is enabled
2022-07-27T18:57:49.498+0530	INFO	If your scanning is slow, please try '--security-checks vuln' to disable secret scanning
2022-07-27T18:57:49.498+0530	INFO	Please see also https://aquasecurity.github.io/trivy/0.30.4/docs/secret/scanning/#recommendation for faster secret detection

After

2022-07-27T18:57:49.498+0530	INFO	Vulnerability scanning is enabled
2022-07-27T18:57:49.498+0530	INFO	Secret scanning is enabled
2022-07-27T18:57:49.498+0530	INFO	If your scanning is slow, please try '--security-checks vuln' to disable secret scanning
2022-07-27T18:57:49.498+0530	INFO	Please see also https://aquasecurity.github.io/trivy/v0.30.4/docs/secret/scanning/#recommendation for faster secret detection

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

@CLAassistant
Copy link

CLAassistant commented Jul 27, 2022

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Moulick Moulick changed the title Fix(cli): secret scanning perf link fix fix(cli): secret scanning perf link fix Jul 27, 2022
@Moulick Moulick marked this pull request as ready for review July 27, 2022 13:42
@Moulick Moulick requested a review from knqyf263 as a code owner July 27, 2022 13:42
@knqyf263
Copy link
Collaborator

Hmm. We fixed it before.
#2422

@DmitriyLewen Could you look into what causes?

@Moulick
Copy link
Contributor Author

Moulick commented Jul 27, 2022

My first thought was to add an if-else like

ver := fmt.Sprintf("v%s", opt.AppVersion)
if opt.AppVersion == "dev" {
  ver = opt.AppVersion
  }

which is like essentially reverting #2422

There are three sources of the version I see

  1. If you simply do go build ./cmd/trivy, It defaults to dev. This produces a correct URL for dev.
  2. If you do make build, make take the version from VERSION := $(shell git describe --tags --always), but this also creates an incorrect URL as this returns eg v0.30.4-3-g4be15455
  3. If the build is produced by goreleaser, it strips the v and hence URL is broken

Currently, I have fixed it for goreleaser, I can also fix it for makefile

@DmitriyLewen
Copy link
Contributor

DmitriyLewen commented Jul 28, 2022

Hello @knqyf263
we have 2 cases:

  1. binaries built with goreleaser don't have v prefix:
➜  29.1 ./trivy -v
Version: 0.29.1
  1. binaries built with make build have v prefix(looks like in #2422 used this case):
➜  git checkout v0.29.1
Note: switching to 'v0.29.1'.
...
➜  make build
go build -ldflags "-s -w -X=main.version=v0.29.1" ./cmd/trivy
➜  ./trivy -v
Version: v0.29.1

I think we need to create same logic for both cases:
add v prefix to goreleaser (as @Moulick did) or remove v prefix from Makefile and return this logic:

ver := fmt.Sprintf("v%s", opt.AppVersion)
if opt.AppVersion == "dev" {
  ver = opt.AppVersion
  }

I think logic of this PR better.

UPD.
I built image with goreleaser. Version number has v prefix.

➜  ./trivy -v
Version: v0.30.4-SNAPSHOT-4be15455

@knqyf263
Copy link
Collaborator

knqyf263 commented Jul 28, 2022

Version: v0.29.1

It's no big deal, but it looks weird to me. v means version,so it is like Version: version 0.29.1.

What if we remove v here?

VERSION := $(shell git describe --tags --always)

@knqyf263
Copy link
Collaborator

And add v of course.

add v prefix to goreleaser (as @Moulick did) or remove v prefix from Makefile and return this logic:

@Moulick
Copy link
Contributor Author

Moulick commented Jul 29, 2022

Version: v0.29.1

It's no big deal, but it looks weird to me. v means version,so it is like Version: version 0.29.1.

What if we remove v here?

VERSION := $(shell git describe --tags --always)

Yes, doable. The documentation links will need to be updated to remove the v as well.

@knqyf263
Copy link
Collaborator

@Moulick Could you fix it as below?

  1. Remove v here.

    VERSION := $(shell git describe --tags --always)

  2. Add v here as Dmitriy suggested, considering dev.

    log.Logger.Infof("Please see also https://aquasecurity.github.io/trivy/%s/docs/secret/scanning/#recommendation for faster secret detection", opts.AppVersion)

@Moulick
Copy link
Contributor Author

Moulick commented Jul 31, 2022

Should work as expected now.

Makefile Outdated Show resolved Hide resolved
Moulick and others added 7 commits August 13, 2022 16:20
Signed-off-by: Moulick Aggarwal <moulickaggarwal@gmail.com>
Signed-off-by: Moulick Aggarwal <moulickaggarwal@gmail.com>
Signed-off-by: Moulick Aggarwal <moulickaggarwal@gmail.com>
Signed-off-by: Moulick Aggarwal <moulickaggarwal@gmail.com>
Signed-off-by: Moulick Aggarwal <moulickaggarwal@gmail.com>
@knqyf263 knqyf263 merged commit ddffb1b into aquasecurity:main Aug 15, 2022
@knqyf263
Copy link
Collaborator

@Moulick Thanks for your great contribution and patience!

@Moulick
Copy link
Contributor Author

Moulick commented Aug 15, 2022

@knqyf263 I'd be curious about the comment I added above 🤔.

@Moulick Moulick deleted the log-typo-fix branch August 15, 2022 13:24
@knqyf263
Copy link
Collaborator

Which comment?

@Moulick
Copy link
Contributor Author

Moulick commented Aug 15, 2022

I had added a review comment, I suppose GitHub did not send a notification. Attached a screenshot in case it's not visible for some reason 😅
image

@knqyf263
Copy link
Collaborator

aquasecurity/go-version is already used directly, while hashicorp/go-version is an indirect dependency. Also, we can easily modify that library as it is maintained by us.

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.

Incorrect URL in log for recommendation for faster secret detection
4 participants