golangci-lint: update to latest version & related tweaks #5737
Conversation
.golangci.yml
Outdated
| @@ -32,3 +36,6 @@ linters-settings: | |||
| rules: | |||
| - name: unused-parameter | |||
| disabled: true | |||
| gosec: | |||
| excludes: | |||
| - G115 # "Potential integer overflow when converting between integer types"; TODO re-enable eventually | |||
There was a problem hiding this comment.
I personally think that it would be better to either exclude this individually in each line that it's observed today and we know that it's safe, or find a solution for the current instances.
I'm worried that we can keep this forever and hide real issues in the future.
There was a problem hiding this comment.
That's fair. I can look into using https://github.com/ccoVeille/go-safecast to properly address these issues (i.e. to add the required checks), if adding that dependency seems acceptable? If not, we could implement equivalent functionality in some simpler helpers ourselves.
7a8d88b to
c244717
Compare
|
Hm, I'm puzzled by the Windows-only integration test failure. The relevant part of the error is: I got this error previously, but fixed it. The relevant source code line, spire/pkg/agent/plugin/keymanager/v1.go Line 122 in c244717 This should not cause the error. Of note, the Linux version of that integration test runs successfully. Also of note is that the relevant code is already covered by unit tests, which succeed correctly on both Linux and Windows. Before the last couple of force-pushes, I did have a different version of the above line, which indeed caused the error (I noticed it when running tests locally): spire/pkg/agent/plugin/keymanager/v1.go Line 122 in 41da0f7 But I inspected the claimed git commit that the Windows integration test is running against and it has the new, fixed version of the code. And this happened twice now, with both of the most recent force-pushes. So it feels like some kind of caching is going on that is not seeing the latest version of the code. Is there any kind of state in the CI runner that could be cleared? Could the cache-deps CI step be causing this somehow? |
|
Anyway, aside from this strange CI failure, this PR now addresses all the integer conversion issues found by |
|
Ok, rebasing against |
|
Having looked at the https://github.com/ccoVeille/go-safecast package a bit more closely, I actually think we're better off implementing the functionality we need from it ourselves, at least for now. Specifically, the issue is that I'll update this PR accordingly (though eventually maybe the go-safecast package can also be improved). |
The latest golangci-lint version comes with two new detected issues, both of which we choose to silence here: 1) staticcheck: Deprecation of (crypto/x509).CertPool.Subjects(): This is still useful to us in the tests and there seems to be no good alternative (we use it merely to count the number of certs). 2) gosec: Potential integer overflow: This is a potentially useful check, but I didn't want to address this as part of this PR. https://github.com/ccoVeille/go-safecast might be useful for this (in fact it looks like it was inspired by the recent addition of this gosec rule). Additionally, change the Makefile to tell golangci-lint to emit all lints, as opposed to imposing its default limits. Those limits could actually be misleading, as it could e.g. lead to someone disabling lints without having seen all the reported issues. Signed-off-by: Carlo Teubner <carlo@cteubner.net>
Signed-off-by: Carlo Teubner <carlo@cteubner.net>
Signed-off-by: Carlo Teubner <carlo@cteubner.net>
Silence these complaints only in test code. Signed-off-by: Carlo Teubner <carlo@cteubner.net>
...with our own implementation that is more typesafe and more convenient. Signed-off-by: Carlo Teubner <carlo@cteubner.net>
|
I've now updated this PR as indicated above, with our owned checked-int-cast helper function. |
The latest golangci-lint version comes with two new detected issues:
staticcheck: Deprecation of
(crypto/x509).CertPool.Subjects():This is still useful to us in the tests and there seems to be no good alternative (we use it merely to count the number of certs).
gosec: Potential integer overflow:
I'm addressing this using some new helpers I'm introducing for checked casting between int types. Inspired by https://github.com/ccoVeille/go-safecast but simpler and more suitable for our purposes.
Additionally, change the Makefile to tell golangci-lint to emit all lints, as opposed to imposing its default limits. Those limits could actually be misleading, as it could e.g. lead to someone disabling lints without having seen all the reported issues.
For completeness – full list of staticcheck CertPool.Subjects() reports:
And full list of gosec integer overflow reports:
cmd/spire-server/cli/federation/create.go:104:14: G115: integer overflow conversion int32 -> uint32 (gosec) codes.Code(r.Status.Code), ^ cmd/spire-server/cli/federation/update.go:100:14: G115: integer overflow conversion int32 -> uint32 (gosec) codes.Code(r.Status.Code), ^ pkg/server/datastore/sqlstore/migration.go:394:76: G115: integer overflow conversion uint64 -> int64 (gosec) if dbCodeVersion.Major != thisCodeVersion.Major || (math.Abs(float64(int64(dbCodeVersion.Minor)-int64(thisCodeVersion.Minor))) > 1) { ^ pkg/server/datastore/sqlstore/sqlstore.go:1310:14: G115: integer overflow conversion int -> int32 (gosec) return int32(count), nil ^ pkg/server/datastore/sqlstore/sqlstore.go:1596:14: G115: integer overflow conversion int -> int32 (gosec) return int32(count), nil ^ pkg/server/datastore/sqlstore/sqlstore.go:1695:15: G115: integer overflow conversion int -> int32 (gosec) val += int32(len(resp.Nodes)) ^ pkg/server/datastore/sqlstore/sqlstore.go:3334:15: G115: integer overflow conversion int -> int32 (gosec) val += int32(len(resp.Entries)) ^ pkg/server/datastore/sqlstore/sqlstore.go:3859:28: G115: integer overflow conversion int64 -> int32 (gosec) entry.X509SvidTtl = int32(r.RegTTL.Int64) ^ pkg/server/datastore/sqlstore/sqlstore.go:3862:27: G115: integer overflow conversion int64 -> int32 (gosec) entry.JwtSvidTtl = int32(r.RegJwtSvidTTL.Int64) ^ pkg/server/datastore/sqlstore/sqlstore_test.go:1578:30: G115: integer overflow conversion int -> uint (gosec) greaterThanEventID: uint(len(expectedEvents) / 2), ^ pkg/server/datastore/sqlstore/sqlstore_test.go:1579:30: G115: integer overflow conversion int -> uint (gosec) expectedFirstEventID: uint(len(expectedEvents)/2) + 1, ^ pkg/server/datastore/sqlstore/sqlstore_test.go:1585:30: G115: integer overflow conversion int -> uint (gosec) lessThanEventID: uint(len(expectedEvents) / 2), ^ pkg/server/datastore/sqlstore/sqlstore_test.go:1587:30: G115: integer overflow conversion int -> uint (gosec) expectedLastEventID: uint(len(expectedEvents)/2) - 1, ^ pkg/server/datastore/sqlstore/sqlstore_test.go:4063:30: G115: integer overflow conversion int -> uint (gosec) greaterThanEventID: uint(len(expectedEvents) / 2), ^ pkg/server/datastore/sqlstore/sqlstore_test.go:4064:30: G115: integer overflow conversion int -> uint (gosec) expectedFirstEventID: uint(len(expectedEvents)/2) + 1, ^ pkg/server/datastore/sqlstore/sqlstore_test.go:4070:30: G115: integer overflow conversion int -> uint (gosec) lessThanEventID: uint(len(expectedEvents) / 2), ^ pkg/server/datastore/sqlstore/sqlstore_test.go:4072:30: G115: integer overflow conversion int -> uint (gosec) expectedLastEventID: uint(len(expectedEvents)/2) - 1, ^ pkg/server/datastore/sqlstore/sqlstore_test.go:5286:17: G115: integer overflow conversion int -> uint (gosec) EventID: uint(len(expectedEvents) + 1), ^ pkg/server/plugin/upstreamauthority/v1.go:38:22: G115: integer overflow conversion int64 -> int32 (gosec) PreferredTtl: int32(preferredTTL / time.Second), ^ cmd/spire-server/cli/agent/agent_test.go:837:15: G115: integer overflow conversion int -> int32 (gosec) Count: int32(len(s.agents)), ^ cmd/spire-server/cli/token/generate.go:51:17: G115: integer overflow conversion int -> int32 (gosec) Ttl: int32(g.TTL), ^ test/clock/clock.go:93:36: G115: integer overflow conversion int -> int32 (gosec) if m.tickerCount.Load() >= int32(count) { ^ test/clock/clock.go:94:33: G115: integer overflow conversion int -> int32 (gosec) m.tickerCount.Add(-1 * int32(count)) ^ pkg/common/peertracker/peertracker_test.go:77:24: G115: integer overflow conversion int -> int32 (gosec) require.Equal(t, int32(os.Getpid()), conn.Info.Caller.PID) ^ pkg/common/peertracker/peertracker_test.go:81:24: G115: integer overflow conversion int -> int32 (gosec) require.Equal(t, int32(os.Getpid()), conn.Info.Watcher.PID()) ^ pkg/agent/endpoints/peertracker_test.go:66:48: G115: integer overflow conversion int -> int32 (gosec) func (w FakeWatcher) PID() int32 { return int32(os.Getpid()) } ^ pkg/server/plugin/keymanager/v1.go:121:35: G115: integer overflow conversion uint -> int32 (gosec) return keymanagerv1.HashAlgorithm(h) ^ pkg/server/plugin/keymanager/v1.go:158:25: G115: integer overflow conversion int -> int32 (gosec) SaltLength: int32(opts.SaltLength), ^ cmd/spire-server/cli/x509/mint.go:171:14: G115: integer overflow conversion int64 -> int32 (gosec) return int32((ttl + time.Second - 1) / time.Second) ^ cmd/spire-server/cli/x509/mint_test.go:336:27: G115: integer overflow conversion int64 -> int32 (gosec) assert.Equal(t, int32(tt.ttl/time.Second), req.Ttl) ^ pkg/server/api/svid/v1/service_test.go:551:15: G115: integer overflow conversion int64 -> int32 (gosec) Ttl: int32(tt.ttl / time.Second), ^ pkg/server/api/svid/v1/service_test.go:802:20: G115: integer overflow conversion int64 -> int32 (gosec) Ttl: int32(tt.ttl / time.Second), ^ pkg/server/api/status.go:16:17: G115: integer overflow conversion uint32 -> int32 (gosec) Code: int32(code), ^ pkg/server/api/status.go:24:17: G115: integer overflow conversion uint32 -> int32 (gosec) Code: int32(code), ^ cmd/spire-server/cli/bundle/common_test.go:283:15: G115: integer overflow conversion int -> int32 (gosec) Count: int32(len(f.bundles)), ^ pkg/common/containerinfo/extract.go:126:42: G115: integer overflow conversion int -> int32 (gosec) cgroups, err := cgroups.GetCgroups(int32(pid), dirFS(e.RootDir)) ^ cmd/spire-server/cli/jwt/mint.go:136:14: G115: integer overflow conversion int64 -> int32 (gosec) return int32((ttl + time.Second - 1) / time.Second) ^ cmd/spire-server/cli/jwt/mint_test.go:357:27: G115: integer overflow conversion int64 -> int32 (gosec) assert.Equal(t, int32(tt.ttl/time.Second), req.Ttl) ^ pkg/server/plugin/keymanager/base/keymanagerbase.go:173:27: G115: integer overflow conversion int32 -> uint (gosec) signerOpts = crypto.Hash(opts.HashAlgorithm) ^ pkg/server/plugin/keymanager/base/keymanagerbase.go:183:27: G115: integer overflow conversion int32 -> uint (gosec) Hash: crypto.Hash(opts.PssOptions.HashAlgorithm), ^ pkg/agent/plugin/keymanager/v1.go:121:35: G115: integer overflow conversion uint -> int32 (gosec) return keymanagerv1.HashAlgorithm(h) ^ pkg/agent/plugin/keymanager/v1.go:158:25: G115: integer overflow conversion int -> int32 (gosec) SaltLength: int32(opts.SaltLength), ^ pkg/server/api/bundle/v1/service_test.go:2482:23: G115: integer overflow conversion int32 -> uint32 (gosec) switch codes.Code(result.Status.Code) { ^ pkg/server/api/audit/audit.go:64:32: G115: integer overflow conversion int32 -> uint32 (gosec) err := status.Error(codes.Code(s.Code), s.Message) ^ pkg/common/selector/set_utils.go:72:44: G115: integer overflow conversion int -> uint64 (gosec) binaryString := strconv.FormatUint(uint64(i), 2) ^ test/tpmsimulator/simulator.go:274:9: G115: integer overflow conversion int -> uint16 (gosec) uint16(len(ekCert))) ^ pkg/agent/plugin/workloadattestor/v1.go:18:13: G115: integer overflow conversion int -> int32 (gosec) Pid: int32(pid), ^ cmd/spire-server/cli/entry/create.go:186:21: G115: integer overflow conversion int -> int32 (gosec) X509SvidTtl: int32(c.x509SVIDTTL), ^ cmd/spire-server/cli/entry/create.go:187:21: G115: integer overflow conversion int -> int32 (gosec) JwtSvidTtl: int32(c.jwtSVIDTTL), ^ cmd/spire-server/cli/entry/create.go:257:14: G115: integer overflow conversion int32 -> uint32 (gosec) codes.Code(r.Status.Code), ^ cmd/spire-server/cli/entry/delete.go:138:14: G115: integer overflow conversion int32 -> uint32 (gosec) codes.Code(result.Status.Code), ^ cmd/spire-server/cli/entry/update.go:179:21: G115: integer overflow conversion int -> int32 (gosec) X509SvidTtl: int32(c.x509SvidTTL), ^ cmd/spire-server/cli/entry/update.go:180:21: G115: integer overflow conversion int -> int32 (gosec) JwtSvidTtl: int32(c.jwtSvidTTL), ^ cmd/spire-server/cli/entry/update.go:243:14: G115: integer overflow conversion int32 -> uint32 (gosec) codes.Code(r.Status.Code), ^ pkg/agent/plugin/keymanager/base/keymanagerbase.go:173:27: G115: integer overflow conversion int32 -> uint (gosec) signerOpts = crypto.Hash(opts.HashAlgorithm) ^ pkg/agent/plugin/keymanager/base/keymanagerbase.go:183:27: G115: integer overflow conversion int32 -> uint (gosec) Hash: crypto.Hash(opts.PssOptions.HashAlgorithm), ^ pkg/server/endpoints/authorized_entryfetcher_attested_nodes.go:242:31: G115: integer overflow conversion uint -> int (gosec) if a.skippedNodeEvents != int(a.eventTracker.EventCount()) { ^ pkg/server/endpoints/authorized_entryfetcher_attested_nodes.go:243:28: G115: integer overflow conversion uint -> int (gosec) a.skippedNodeEvents = int(a.eventTracker.EventCount()) ^ pkg/server/endpoints/authorized_entryfetcher_registration_entries.go:254:32: G115: integer overflow conversion uint -> int (gosec) if a.skippedEntryEvents != int(a.eventTracker.EventCount()) { ^ pkg/server/endpoints/authorized_entryfetcher_registration_entries.go:255:29: G115: integer overflow conversion uint -> int (gosec) a.skippedEntryEvents = int(a.eventTracker.EventCount()) ^ pkg/server/endpoints/eventTracker.go:23:13: G115: integer overflow conversion int64 -> uint (gosec) return uint(1 + (trackTime-1)/pollTime) ^ pkg/agent/plugin/workloadattestor/systemd/systemd_posix.go:58:42: G115: integer overflow conversion int32 -> uint (gosec) uInfo, err := p.getUnitInfo(ctx, p, uint(req.Pid)) ^ pkg/agent/api/debug/v1/service.go:100:40: G115: integer overflow conversion int -> int32 (gosec) SvidsCount: int32(s.m.CountX509SVIDs()), ^ pkg/agent/api/debug/v1/service.go:101:40: G115: integer overflow conversion int -> int32 (gosec) CachedX509SvidsCount: int32(s.m.CountX509SVIDs()), ^ pkg/agent/api/debug/v1/service.go:102:40: G115: integer overflow conversion int -> int32 (gosec) CachedJwtSvidsCount: int32(s.m.CountJWTSVIDs()), ^ pkg/agent/api/debug/v1/service.go:103:40: G115: integer overflow conversion int -> int32 (gosec) CachedSvidstoreX509SvidsCount: int32(s.m.CountSVIDStoreX509SVIDs()), ^