qa: add informalsystems/gosec static analysis passes to be a part of the build process. #10572
Comments
robert-zaremba
changed the title
|
The issue with the informal gosec fork right now is that (AFAIU) it panics on certain map iteration syntax patterns, due to incomplete syntax matching patterns being applied |
@ValarDragon in the past week we've worked on that, and @kirbyquerby from Orijtech is working on ensuring it is ready for prime time. |
This lets us determine types much more reliably without having to worry about adding more cases to switch statements and pulling out the debugger when we panic on a new AST structure we haven't handled yet. It's always nice to half the number of lines in a file as well :) And of course this change causes the rule to notice more map statements that it previously mixed, so I've also fixed those. I also discovered a new case that this rule incorrectly flags -- map copying is safe to do directly. I've filed #24 and suppressed the rule for the map copy in analyzer.go. With this change, I'm able to run gosec on cosmos/cosmos-sdk without it crashing. Updates cosmos/cosmos-sdk#10572
|
update on this? |
@marbar3778 we need Informal Systems to add some Docker image building permissions but we haven't yet gotten that help. @kirbyquerby, could you please send a PR with our Orijtech fork that has gosec as a Docker image and Github action with a comment that says to switch to informalsystems/gosec once it is ready? |
|
should we fork this into the cosmos org? we can get this done in a day |
@marbar3778, I chatted with @ebuchman and we have the blessing: please go ahead and let's fork it to cosmos/gosec and please help give admin permissions to @kirbyquerby @elias-orijtech and myself for starters so that we can get it in shape and working as a Github action. Thank you for the follow up and for the patience. |
|
should we fork straight from goes to not be a fork of a fork? |
@marbar3778 could you please rephrase this? I don't understand what you mean. If you mean transfer from informalsystems/ to cosmos/ then please ask @ebuchman, but I think we should kick off with the fork directly from informalsystems and then get the action working and then later figured out the ergonomics. |
|
its here now https://github.com/cosmos/gosec you have perms @odeke-em |
|
Awesome, thank you @marbar3778! I've pushed out the first PR to migrate to the new URL cosmos/gosec#33. @kirbyquerby time for action. |
|
@kirbyquerby could you please help send an actions PR so that we can close out this issue? |
This workflow is based on evmos's workflow: https://github.com/evmos/evmos/blob/main/.github/workflows/security.yml Fixes cosmos#10572
|
I've put together #13311 but gosec raises 5000+ linting errors: https://github.com/cosmos/cosmos-sdk/pull/13311/checks?check_run_id=8382756626 @odeke-em How do we handle this? |
|
Thank you @kirbyquerby! Let's mark that PR as ready for review and then we shall work on a way to fix up that laundry list during Q4 2022. We shall also need to ensure that gosec skips over code in *_test.go as well as generated code in for example *.pb.go and *.gw.go files |
Do you want to open an issue for that? Or leave this one open? |
A while ago we got bit by non-deterministic map iterations which caused issues in upgrades. Later on an issue with
time.Now()usage in state machine was reported. @ebuchman took a stand that we need a proper static analyzers to disallow non-deterministic code. Another chain got finessed by non-deterministic map iteration and we made the announcement of our collaboration per https://twitter.com/informalinc/status/1460265932455124994?s=20Also a while ago there was a bug that I reported where using strconv.ParseUint with the wrong bit size then cast to an integer could cause an overflow that is hard to detect so we've added such a pass that'll flag such code
This issue is to add informalsystems/gosec to the QA process so that we don't accept code with such insidious bugs. I shall be sending out the static analyzer for time.Now() soon.
Kindly cc-ing the QA team @robert-zaremba @anilcse @kaustubhkapatral @marbar3778
Ref: #10329
For Admin Use
The text was updated successfully, but these errors were encountered: