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 gosec issues and add gosec action #124

Merged
merged 22 commits into from
Jul 30, 2022
Merged

fix gosec issues and add gosec action #124

merged 22 commits into from
Jul 30, 2022

Conversation

asalzmann
Copy link
Contributor

@asalzmann asalzmann commented Jul 24, 2022

Summary

gosec fixes

There were two main types of changes raised by gosec

First, ints were being cast unsafely and could overflow. This was fixed by switching to using cast... https://github.com/spf13/cast

Second, non-deterministic map iteration was fixed

In Go, iterating over maps is intentionally non-deterministic as the runtime defines. Unfortunately for us, in the Cosmos-SDK, we encountered an issue with non-deterministic upgrades in Issue cosmos-sdk#10188 PR cosmos-sdk#10189 that resulted from exactly this non-deterministic iteration. To ensure determinism, we only permit an iteration to retrieve the map keys and then those keys can then be sorted, so instead of

for k, v := range m {
    // Do something with key and value.
    _, _ = k, v
}

the requested pattern is

keys := make([]string, 0, len(m))
for k := range m {
    keys = append(keys, k)
}
sort.Strings(keys)

for _, key := range keys {
    // Use the value.
    _ = m[key]
}

go doesn't support generics, so I added a number of files in utils to handle different map types

Test plan

Install gosec locally with go install github.com/securego/gosec/v2/cmd/gosec@latest
Then, make sure gosec action passes: make gosec

@github-actions github-actions bot added the T:CI label Jul 24, 2022
@asalzmann asalzmann changed the title add gosec fix gosec issues and add gosec action Jul 24, 2022
@asalzmann asalzmann added the A:automerge Automatically merge PR once checks pass label Jul 24, 2022
@asalzmann asalzmann requested a review from a team July 25, 2022 01:06
Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

Looks good and the tests passed for me! Added the gosec install instructions to the PR description and a few comments throughout. Two main Qs:

  • should we be using cast in more places than the changes shown in this PR?
  • nit: can we condense the utils.go by using interfaces (see the go playground example in my comment)

utils/utils.go Show resolved Hide resolved
x/epochs/client/cli/query.go Show resolved Hide resolved
x/interchainquery/keeper/abci.go Show resolved Hide resolved
x/stakeibc/module_simulation.go Show resolved Hide resolved
@riley-stride
Copy link
Contributor

Please check whether we need to sort app.go:849 before merging

@asalzmann
Copy link
Contributor Author

I don't think we need to sort in app.go:849 because it returns another map (the order of which is non-deterministic anyway)

Copy link
Contributor

@riley-stride riley-stride left a comment

Choose a reason for hiding this comment

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

lgtm, discussed in person!

@asalzmann asalzmann merged commit 772141b into main Jul 30, 2022
riley-stride pushed a commit that referenced this pull request Sep 5, 2022
sontrinh16 pushed a commit to notional-labs/stride that referenced this pull request Mar 27, 2023
fix gosec issues and add gosec action
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants