Skip to content

Add gosec to linters#3224

Merged
tonistiigi merged 4 commits intomoby:masterfrom
jedevc:gosec
Oct 28, 2022
Merged

Add gosec to linters#3224
tonistiigi merged 4 commits intomoby:masterfrom
jedevc:gosec

Conversation

@jedevc
Copy link
Member

@jedevc jedevc commented Oct 26, 2022

Replaces #2449.

For the default excludes rules:

  • Potential hardcoded credentials and implicit memory aliasing for giving lots of false positives on correct behaviour (e.g. where the addressed value never leaves the loop, or we break immediately after).
  • TLS MinVersion warnings (we need to coordinate with other projects upstream if we want to do this).
  • net/http/cgi blocklist, since it errors for a vulnerability in a version of Go that we don't target (1.16, we now use generics so buildkit won't even compile with that version).

Signed-off-by: Sascha Schwarze <schwarzs@de.ibm.com>
Required for gosec

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
By default we exclude potential hardcoded credentials and implicit
memory aliasing for giving lots of false positives on correct behavior
(where the addressed value never leaves the loop, or we break
immediately after).

We additionally exclude TLS MinVersion warnings (we need to co-ordinate
with other projects upstream if we want to do this).

Finally, we exclude net/http/cgi blocklist, since it error for a
vulnerability in a version of Go that we don't target.

Signed-off-by: Justin Chadwell <me@jedevc.com>
gosec:
excludes:
- G101 # Potential hardcoded credentials (false positives)
- G402 # TLS MinVersion too low
Copy link
Member

Choose a reason for hiding this comment

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

We should track this somewhere (likely containerd)

- G101 # Potential hardcoded credentials (false positives)
- G402 # TLS MinVersion too low
- G601 # Implicit memory aliasing in for loop (false positives)
- G504 # Import blocklist: net/http/cgi
Copy link
Member

Choose a reason for hiding this comment

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

These comments could be improved. Eg. why do we have an import on "net/http/cgi" and what do we need to do to fix it.

Copy link
Member Author

@jedevc jedevc Oct 27, 2022

Choose a reason for hiding this comment

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

We only use it for testing in gitsource_test.go to use git http-backend - we could just remove the warning in that file, but the gosec warning itself is very weird.

The only reason it errors is that there's a vulnerability in an old version of go... that we don't support. I think it's reasonable to disable it globally, since we don't target that version.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Noticed the changes I commented about were already described in PR description. Still don't understand the "CGI" case though.

@jedevc jedevc added this to the v0.11.0 milestone Oct 27, 2022
@tonistiigi tonistiigi merged commit 838a9ae into moby:master Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants