golangci-lint icon indicating copy to clipboard operation
golangci-lint copied to clipboard

Published 20 hours ago • verified publisher icon golangci

add vulncheck as a linter

Open luxifer opened this issue 3 years ago • 4 comments
trafficstars

Related to #3094

luxifer avatar Sep 07 '22 13:09 luxifer

Hey, thank you for opening your first Pull Request !

boring-cyborg[bot] avatar Sep 07 '22 13:09 boring-cyborg[bot]

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 07 '22 13:09 CLAassistant

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?

paskal avatar Sep 12 '22 17:09 paskal

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

mvrahden avatar Sep 14 '22 10:09 mvrahden

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

luxifer avatar Sep 26 '22 13:09 luxifer

It would be nice to have:

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

Dentrax avatar Sep 26 '22 13:09 Dentrax

[ ] enable official linter in golangci-lint itself

Don't do that, please.

ldez avatar Sep 26 '22 14:09 ldez

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

luxifer avatar Sep 26 '22 14:09 luxifer

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 avatar Sep 26 '22 14:09 ldez

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

  • [x] It must not be a duplicate of another linter or a rule of a linter. (the team will help to verify that)
  • [x] It must have a valid license and the file must contain the required information by the license, ex: author, year, etc.
  • [x] The linter repository must have a CI and tests.
  • [x] 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)
  • [x] The linter repository should have a readme and linting.
  • [x] 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.

ldez avatar Sep 26 '22 14:09 ldez

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

luxifer avatar Sep 26 '22 14:09 luxifer

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

rodrigc avatar Oct 12 '22 16:10 rodrigc

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

I will take the time this weekend

luxifer avatar Oct 13 '22 07:10 luxifer

@ldez @luxifer

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 how do you recommend making a test file with a vulnerability? This blog shows an example, https://mariocarrion.com/2022/09/23/golang-software-architecture-govulncheck-vulnerability-management-security.html#detecting-vulnerabilities-in-compiled-binaries

but the vulnerability GO-2022-0988 is only in go1.19, and fixed in go1.19.1

rodrigc avatar Oct 31 '22 19:10 rodrigc

@rodrigc Here is a list of all reports: https://pkg.go.dev/vuln/list. I think that https://pkg.go.dev/vuln/GO-2022-1059 is a good candidate. You can have a "success" testdata with the latest golang.org/x/text/language and a "failing" with golang.org/x/text/language < 0.3.8.

pellared avatar Oct 31 '22 20:10 pellared

Kind ping here. 🤞

Dentrax avatar Nov 21 '22 08:11 Dentrax

Sorry, I had a lot of work these days. I'm on it

luxifer avatar Dec 10 '22 10:12 luxifer

I'm trying to fix the test cases, adapting from loggercheck but I can't make it work :/

luxifer avatar Dec 10 '22 11:12 luxifer

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.

luxifer avatar Jan 04 '23 17:01 luxifer

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

Dentrax avatar Jan 20 '23 13:01 Dentrax

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.

luxifer avatar Jan 20 '23 14:01 luxifer

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

Dentrax avatar Feb 11 '23 11:02 Dentrax

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.

ldez avatar Feb 11 '23 13:02 ldez

Any progress here? I gave it another shot — still clueless as to why it's not working properly. See https://github.com/golangci/golangci-lint/pull/3199#pullrequestreview-1278203806

leonklingele avatar Apr 14 '23 11:04 leonklingele

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 avatar Apr 26 '23 14:04 luxifer

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/[email protected]/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/[email protected]/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/[email protected]/vulncheck/utils.go:36 +0xbe
                                golang.org/x/vuln/vulncheck.buildSSA({0xc00012a278, 0x1, 0xc000fa8fb8?}, 0xf44a74?)
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/[email protected]/vulncheck/utils.go:44 +0x14b
                                golang.org/x/vuln/vulncheck.Source.func1()
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/[email protected]/vulncheck/source.go:78 +0xa5
                                created by golang.org/x/vuln/vulncheck.Source
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/[email protected]/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/[email protected]/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/[email protected]/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/[email protected]/vulncheck/utils.go:36 +0xbe
                                golang.org/x/vuln/vulncheck.buildSSA({0xc00012a278, 0x1, 0xc000fa8fb8?}, 0xf44a74?)
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/[email protected]/vulncheck/utils.go:44 +0x14b
                                golang.org/x/vuln/vulncheck.Source.func1()
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/[email protected]/vulncheck/source.go:78 +0xa5
                                created by golang.org/x/vuln/vulncheck.Source
                                        /home/fviel/apps/go/pkg/mod/golang.org/x/[email protected]/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

luxifer avatar Apr 26 '23 14:04 luxifer

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

Dentrax avatar Sep 08 '23 09:09 Dentrax

https://github.com/golangci/golangci-lint/issues/3094#issuecomment-1636093709

ldez avatar Sep 08 '23 09:09 ldez

Based on the reaction on https://github.com/golangci/golangci-lint/issues/3094#issuecomment-1636093709 I will close this PR.

ldez avatar Sep 08 '23 09:09 ldez

@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 https://github.com/golangci/golangci-lint/issues/3094#issuecomment-1636093709 , 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: https://github.com/golangci/golangci-lint/pull/4074 which is a work in progress. Please reconsider your decision about it.

rodrigc avatar Sep 08 '23 14:09 rodrigc