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

add vulncheck as a linter #3199

Closed
wants to merge 8 commits into from
Closed

add vulncheck as a linter #3199

wants to merge 8 commits into from

Conversation

luxifer
Copy link

@luxifer luxifer commented Sep 7, 2022

Related to #3094

@boring-cyborg
Copy link

boring-cyborg bot commented Sep 7, 2022

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Sep 7, 2022

CLA assistant check
All committers have signed the CLA.

@ldez ldez marked this pull request as draft September 7, 2022 13:26
@ldez ldez changed the title WIP init vuncheck linter init vuncheck linter Sep 7, 2022
@ldez ldez added the blocked Need's direct action from maintainer label Sep 7, 2022
@luxifer luxifer marked this pull request as ready for review September 12, 2022 12:16
@paskal
Copy link

paskal commented Sep 12, 2022

I'm feeling silly to ask, but could you please fix the typo in the subject of the PR so that it would be easier to search?

go.mod Outdated Show resolved Hide resolved
@ldez ldez changed the title init vuncheck linter init vulncheck linter Sep 12, 2022
@ldez ldez self-assigned this Sep 12, 2022
@ldez ldez changed the title init vulncheck linter add vulncheck as a linter Sep 13, 2022
pkg/golinters/vulncheck.go Outdated Show resolved Hide resolved
@mvrahden
Copy link

@luxifer Thank you for having made this an integrated linter 💯 🚀

Copy link

@mvrahden mvrahden left a comment

Choose a reason for hiding this comment

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

LGTM besides the two mentioned issues :)

@ldez ldez self-requested a review September 14, 2022 12:16
@luxifer
Copy link
Author

luxifer commented Sep 26, 2022

I made some changes on the MR if you want to check 😉

@Dentrax
Copy link

Dentrax commented Sep 26, 2022

It would be nice to have:

P.S: I'm not an official maintainer, those are just a nice-to-have list. Otherwise, LGTM!

@ldez
Copy link
Member

ldez commented Sep 26, 2022

[ ] enable official linter in golangci-lint itself

Don't do that, please.

@luxifer
Copy link
Author

luxifer commented Sep 26, 2022

I will see how I can fake the vuln database to have test cases

@ldez
Copy link
Member

ldez commented Sep 26, 2022

I will see how I can fake the vuln database to have test cases

you don't need to fake, you just have to create a file with a vulnerability

@ldez
Copy link
Member

ldez commented Sep 26, 2022

In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.

Pull Request Description

  • It must have a link to the linter repository.
  • It must provide a short description about the linter.

Linter

  • It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
  • It must have a valid license and the file must contain the required information by the license, ex: author, year, etc.
  • The linter repository must have a CI and tests.
  • It must use go/analysis.
  • It must have a valid tag, ex: v1.0.0, v0.1.0.
  • It must not contain init().
  • It must not contain panic(), log.fatal(), os.exit(), or similar.
  • It must not have false positives/negatives. (the team will help to verify that)
  • It must have tests inside golangci-lint.

The Linter Tests Inside Golangci-lint

  • They must have at least one std lib import.
  • They must work with T=<name of the linter test file>.go make test_linters. (the team will help to verify that)

.golangci.reference.yml

  • The linter must be added to the list of available linters (alphabetical case-insensitive order).
    • enable and disable options
  • If the linter has a configuration, the exhaustive configuration of the linter must be added (alphabetical case-insensitive order)
    • The values must be different from the default ones.
    • The default values must be defined in a comment.
    • The option must have a short description.

Others Requirements

  • The files (tests and linter) inside golangci-lint must have the same name as the linter.
  • The .golangci.yml of golangci-lint itself must not be edited and the linter must not be added to this file.
  • The linters must be sorted in the alphabetical order (case-insensitive) in the Manager.GetAllSupportedLinterConfigs(...) and .golangci.reference.yml.
  • The load mode (WithLoadMode(...)):
    • if the linter doesn't use types: goanalysis.LoadModeSyntax
    • goanalysis.LoadModeTypesInfo required WithLoadForGoAnalysis() in the Manager.GetAllSupportedLinterConfigs(...)
  • The version in WithSince(...) must be the next minor version (v1.X.0) of golangci-lint.

Recommendations

  • The linter should not use SSA. (currently, SSA does not support generics)
  • The linter repository should have a readme and linting.
  • The linter should be published as a binary. (useful to diagnose bug origins)

The golangci-lint team will edit this comment to check the boxes before and during the review.

This checklist does not imply that we will accept the linter.

@luxifer
Copy link
Author

luxifer commented Sep 26, 2022

I will take the time to review that checklist and make the proper changes

@rodrigc
Copy link

rodrigc commented Oct 12, 2022

@luxifer Do you have time to push this vulncheck linter forward? This is very handy

mihaitodor added a commit to mihaitodor/benthos that referenced this pull request Oct 12, 2022
This is temporary while golangci/golangci-lint#3094
is being worked on here: golangci/golangci-lint#3199
mihaitodor added a commit to mihaitodor/benthos that referenced this pull request Oct 12, 2022
This is temporary while golangci/golangci-lint#3094
is being worked on here: golangci/golangci-lint#3199
@luxifer
Copy link
Author

luxifer commented Jan 4, 2023

Hi! I wish you all a happy new year!

If anyone can take a look at my PR an why I can't run my testcases. I'm a bit stuck here.

@Dentrax
Copy link

Dentrax commented Jan 20, 2023

Any progress here, @luxifer? Is it ready to release on the next golangci-lint cycle?

@luxifer
Copy link
Author

luxifer commented Jan 20, 2023

Any progress here, @luxifer? Is it ready to release on the next golangci-lint cycle?

I will rebase my PR but I still have issues running test cases for my new linter. I'll try to work on it this weekend.

Copy link
Member

@leonklingele leonklingele left a comment

Choose a reason for hiding this comment

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

After a bit of investigation, I made the test run.

Although found by the official govulncheck command, no vulnerability is detected using the linter. I'm clueless as to why.

diff --git a/pkg/golinters/vulncheck.go b/pkg/golinters/vulncheck.go
index 62c852d1..ed3df51b 100644
--- a/pkg/golinters/vulncheck.go
+++ b/pkg/golinters/vulncheck.go
@@ -51,7 +51,7 @@ func NewVulncheck(settings *config.VulncheckSettings) *goanalysis.Linter {
 		}
 	}).WithIssuesReporter(func(*linter.Context) []goanalysis.Issue {
 		return resIssues
-	})
+	}).WithLoadMode(goanalysis.LoadModeSyntax)
 }
 
 func vulncheckRun(lintCtx *linter.Context, pass *analysis.Pass, settings *config.VulncheckSettings) ([]goanalysis.Issue, error) {
diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go
index 0042628a..94e92e36 100644
--- a/pkg/lint/lintersdb/manager.go
+++ b/pkg/lint/lintersdb/manager.go
@@ -883,6 +883,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
 		linter.NewConfig(golinters.NewVulncheck(vulncheckCfg)).
 			WithSince("v1.50.2").
 			WithPresets(linter.PresetModule).
+			WithLoadForGoAnalysis().
 			WithURL("https://vuln.go.dev/"),
 	}
 

pkg/config/linters_settings.go Show resolved Hide resolved
.golangci.reference.yml Outdated Show resolved Hide resolved
@Dentrax
Copy link

Dentrax commented Feb 11, 2023

Do you need any help on this? @luxifer I can give a hand on this if you add me as collaborator to your fork.

@ldez
Copy link
Member

ldez commented Feb 11, 2023

I can give a hand on this if you add me as collaborator to your fork.

you don't need to be a collaborator, you can just open PR on the fork.

@leonklingele
Copy link
Member

Any progress here? I gave it another shot — still clueless as to why it's not working properly. See #3199 (review)

@luxifer
Copy link
Author

luxifer commented Apr 26, 2023

I see that in the v0.1.0 release of golang.org/x/vuln they moved the client the internal/ so it won't be possible to integrate it for now. I'm still trying to do it with the current used ref

@luxifer
Copy link
Author

luxifer commented Apr 26, 2023

With the current implem I'm facing the following issue running the tests:

❯ T=vulncheck make test_linters_sub                                                                                                                                                  
GL_TEST_RUN=1 go test -v ./test -count 1 -run TestSourcesFromTestdataSubDir/vulncheck
=== RUN   TestSourcesFromTestdataSubDir
=== RUN   TestSourcesFromTestdataSubDir/vulncheck
    linters_test.go:40: testdata/vulncheck/*.go
=== RUN   TestSourcesFromTestdataSubDir/vulncheck/vulncheck.go
=== PAUSE TestSourcesFromTestdataSubDir/vulncheck/vulncheck.go
=== CONT  TestSourcesFromTestdataSubDir/vulncheck/vulncheck.go
level=info msg="[test] ran [/home/fviel/workspace/golangci-lint/golangci-lint run --internal-cmd-test --no-config --allow-parallel-runners --disable-all --out-format=json --max-same-issues=100 -Evulncheck vulncheck.go] in 258.861853ms"
    linters_test.go:72: 
                Error Trace:    /home/fviel/workspace/golangci-lint/test/linters_test.go:122
                                                        /home/fviel/workspace/golangci-lint/test/linters_test.go:72
                Error:          Not equal: 
                                expected: 1
                                actual  : 2
                Test:           TestSourcesFromTestdataSubDir/vulncheck/vulncheck.go
                Messages:       Unexpected exit code: panic: unexpected builtin object: builtin append
                            
                                goroutine 113 [running]:
                                golang.org/x/tools/go/ssa.memberFromObject(0xc000f8a000, {0x1b32398, 0xc0001ad400?}, {0x0, 0x0})
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/tools@v0.8.1-0.20230421161920-b9619ee54b47/go/ssa/create.go:57 +0xa05
                                golang.org/x/tools/go/ssa.(*Program).CreatePackage(0xc000236600, 0x0, {0x0, 0x0, 0x0}, 0x0, 0x1)
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/tools@v0.8.1-0.20230421161920-b9619ee54b47/go/ssa/create.go:232 +0x409
                                golang.org/x/vuln/vulncheck.buildSSA.func1({0xc00034cf80, 0x2, 0xc0014e55d0?})
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/utils.go:36 +0xbe
                                golang.org/x/vuln/vulncheck.buildSSA({0xc00012a278, 0x1, 0xc000fa8fb8?}, 0xf44a74?)
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/utils.go:44 +0x14b
                                golang.org/x/vuln/vulncheck.Source.func1()
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/source.go:78 +0xa5
                                created by golang.org/x/vuln/vulncheck.Source
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/source.go:76 +0x3e5
    analysis.go:49: 
                Error Trace:    /home/fviel/workspace/golangci-lint/test/testshared/analysis.go:49
                                                        /home/fviel/workspace/golangci-lint/test/linters_test.go:124
                                                        /home/fviel/workspace/golangci-lint/test/linters_test.go:72
                Error:          Received unexpected error:
                                invalid character 'p' looking for beginning of value
                Test:           TestSourcesFromTestdataSubDir/vulncheck/vulncheck.go
                Messages:       panic: unexpected builtin object: builtin append
                            
                                goroutine 113 [running]:
                                golang.org/x/tools/go/ssa.memberFromObject(0xc000f8a000, {0x1b32398, 0xc0001ad400?}, {0x0, 0x0})
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/tools@v0.8.1-0.20230421161920-b9619ee54b47/go/ssa/create.go:57 +0xa05
                                golang.org/x/tools/go/ssa.(*Program).CreatePackage(0xc000236600, 0x0, {0x0, 0x0, 0x0}, 0x0, 0x1)
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/tools@v0.8.1-0.20230421161920-b9619ee54b47/go/ssa/create.go:232 +0x409
                                golang.org/x/vuln/vulncheck.buildSSA.func1({0xc00034cf80, 0x2, 0xc0014e55d0?})
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/utils.go:36 +0xbe
                                golang.org/x/vuln/vulncheck.buildSSA({0xc00012a278, 0x1, 0xc000fa8fb8?}, 0xf44a74?)
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/utils.go:44 +0x14b
                                golang.org/x/vuln/vulncheck.Source.func1()
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/source.go:78 +0xa5
                                created by golang.org/x/vuln/vulncheck.Source
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/vuln@v0.0.0-20230309043308-bbc736fc3bc1/vulncheck/source.go:76 +0x3e5
--- FAIL: TestSourcesFromTestdataSubDir (0.46s)
    --- FAIL: TestSourcesFromTestdataSubDir/vulncheck (0.20s)
        --- FAIL: TestSourcesFromTestdataSubDir/vulncheck/vulncheck.go (0.26s)
FAIL
FAIL    github.com/golangci/golangci-lint/test  0.521s
FAIL
make: *** [Makefile:52: test_linters_sub] Error 1

@Dentrax
Copy link

Dentrax commented Sep 8, 2023

How should we move forward on this? Almost one year has passed...

@ldez
Copy link
Member

ldez commented Sep 8, 2023

#3094 (comment)

@ldez
Copy link
Member

ldez commented Sep 8, 2023

Based on the reaction on #3094 (comment) I will close this PR.

@ldez ldez closed this Sep 8, 2023
@ldez ldez added declined and removed blocked Need's direct action from maintainer labels Sep 8, 2023
@rodrigc
Copy link

rodrigc commented Sep 8, 2023

@ldez I understand some of the reasoning behind your decision and respect it,
but I would like to offer my disagreement for your thoughts in #3094 (comment) , and explain my thoughts.

Although govulncheck detects vulnerabilities, as an end-user, I view this as an alternate form of "linting", or "pre-compilation checks".

For this reason, I view it as appropriate to add to golangci-lint, as an optional linter that the end-user would have to enable.

Although the vulnerability database is outside, it would be nice to have govulncheck available for use inside golangci-lint.
I believe that this would allow more go projects to incorporate govulncheck in their CI, and improve the overall security of many go projects.

I took the work that @luxifer started, and made another PR: #4074
which is a work in progress.
Please reconsider your decision about it.

@golangci golangci locked as resolved and limited conversation to collaborators Sep 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
declined linter: new Support new linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants