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

Bump Go version, CI deps, fix some linter issues... #218

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Oct 16, 2024

This started as a bump of some CI deps but quickly got out of hand 😬

See individual commits for details. High level overview:

  • ci: bump ubuntu to 22.04;
  • go.mod: bump go to 1.21, drop min/max;
  • ci: add go 1.23 to test matrix;
  • fix or silence new gosec warnings (these commits need to be reviewed carefully);
  • ci: bump golangci-lint to v1.61;
  • ci: fix golangci-lint jobs caching;
  • ci: add 32-bit tests;
  • ci: add all-done job.

@kolyshkin kolyshkin force-pushed the ci-bumps branch 2 times, most recently from fcc7c0e to e2f9e99 Compare October 17, 2024 00:06
@kolyshkin kolyshkin changed the title Modernize Bump Go version, CI deps, fix some linter issues Oct 17, 2024
@kolyshkin kolyshkin changed the title Bump Go version, CI deps, fix some linter issues Bump Go version, CI deps, fix some linter issues... Oct 17, 2024
@kolyshkin kolyshkin marked this pull request as ready for review October 17, 2024 00:11
@kolyshkin kolyshkin marked this pull request as draft October 17, 2024 00:13
@kolyshkin
Copy link
Collaborator Author

Oops, apparently most tests are skipped (because, of course, Ubuntu does not have selinux).

=== RUN   TestSetFileLabel
    selinux_linux_test.go:16: SELinux not enabled, skipping.
--- SKIP: TestSetFileLabel (0.00s)
=== RUN   TestKVMLabels
    selinux_linux_test.go:83: SELinux not enabled, skipping.
--- SKIP: TestKVMLabels (0.00s)
=== RUN   TestInitLabels
    selinux_linux_test.go:104: SELinux not enabled, skipping.
--- SKIP: TestInitLabels (0.00s)
=== RUN   TestSELinux
    selinux_linux_test.go:136: SELinux not enabled, skipping.
--- SKIP: TestSELinux (0.00s)
=== RUN   TestSetEnforceMode
    selinux_linux_test.go:183: SELinux not enabled, skipping.
--- SKIP: TestSetEnforceMode (0.00s)
=== RUN   TestCanonicalizeContext
    selinux_linux_test.go:206: SELinux not enabled, skipping.
--- SKIP: TestCanonicalizeContext (0.00s)
=== RUN   TestFindSELinuxfsInMountinfo
    selinux_linux_test.go:270: found "/sys/fs/selinux"
    selinux_linux_test.go:270: found "/root/chroot/selinux"
    selinux_linux_test.go:270: found ""
--- PASS: TestFindSELinuxfsInMountinfo (0.00s)
=== RUN   TestSecurityCheckContext
    selinux_linux_test.go:279: SELinux not enabled, skipping.
--- SKIP: TestSecurityCheckContext (0.00s)
=== RUN   TestClassIndex
    selinux_linux_test.go:304: SELinux not enabled, skipping.
--- SKIP: TestClassIndex (0.00s)
=== RUN   TestComputeCreateContext
    selinux_linux_test.go:324: SELinux not enabled, skipping.
....

Need to switch to a hosted Fedora or something like that. But that's a separate issue.

Let this be draft for now.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Fix the following gosec warnings in tests by using uint32 everywhere, so
we don't have to do a single cast:

pkg/pwalk/pwalk_test.go:29:20: G115: integer overflow conversion int -> uint32 (gosec)
        if count != uint32(total) {
                          ^
pkg/pwalk/pwalk_test.go:73:15: G115: integer overflow conversion int -> uint32 (gosec)
        max := uint32(total / 2)
                     ^
pkg/pwalk/pwalk_test.go:86:21: G115: integer overflow conversion int -> uint32 (gosec)
                if count != uint32(total) {
                                  ^
pkg/pwalkdir/pwalkdir_test.go:32:20: G115: integer overflow conversion int -> uint32 (gosec)
        if count != uint32(total) {
                          ^
pkg/pwalkdir/pwalkdir_test.go:76:15: G115: integer overflow conversion int -> uint32 (gosec)
        max := uint32(total / 2)
                     ^
pkg/pwalkdir/pwalkdir_test.go:89:21: G115: integer overflow conversion int -> uint32 (gosec)
                if count != uint32(total) {
                                  ^

While at it,
 - switch from atomic op (atomic.AddUint32) to atomic type
   (atomic.Int32) with methods, which is more error-prone;
 - rename max to maxFiles as the former is now a built-in function.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Currently supported go versions are 1.22 and 1.23.

Drop min and max functions now, as Go 1.21 has built-in ones.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Most of parseLevelItem users will cast its result to int. On a 32-bit
platform this means we may end up with a negative number.

So, let's limit bitSize to 31 in a call to ParseUint, and return int so
there are less typecasts in the code.

Also, change MLS level to use int, for the same reason (less
typecasts).

This fixes the following gosec warnings:

go-selinux/selinux_linux.go:505:30: G115: integer overflow conversion uint -> int (gosec)
				bitset.SetBit(bitset, int(i), 1)
				                         ^
go-selinux/selinux_linux.go:512:29: G115: integer overflow conversion uint -> int (gosec)
			bitset.SetBit(bitset, int(cat), 1)
			                         ^
go-selinux/selinux_linux.go:626:31: G115: integer overflow conversion uint -> int (gosec)
	low := "s" + strconv.Itoa(int(m.low.sens))
	                             ^
go-selinux/selinux_linux.go:635:32: G115: integer overflow conversion uint -> int (gosec)
	high := "s" + strconv.Itoa(int(m.high.sens))
	                              ^

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Gosec doesn't like this code:

go-selinux/selinux_linux.go:141:11: G115: integer overflow conversion int64 -> uint32 (gosec)
	if uint32(buf.Type) != uint32(unix.SELINUX_MAGIC) {
	         ^

But it is correct because
 - buf.Type is int64 or int32, depending on the platform;
 - unix.SELINUX_MAGIC is untyped int which overflows int32
   (i.e. it becomes negative).

So the best type to use here is uint32.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Gosec complains:

go-selinux/selinux_linux.go:587:14: G115: integer overflow conversion uint -> int (gosec)
	for i := int(c.TrailingZeroBits()); i < c.BitLen(); i++ {
	            ^

This is indeed a valid concern in case TrailingZeroBits returns a value
which uses a highest bit (i.e. more than MaxInt32 or MaxInt64, depending
on the platform).

But I think this is highly unlikely.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The new version produces the following warnings:

WARN [config_reader] The configuration option `linters.govet.check-shadowing` is deprecated. Please enable `shadow` instead, if you are not using `enable-all`.
WARN The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar.

so fix the configuration accordingly.

Note we do not enable copyloopvar since it requires Go 1.22 and we're
currently have it set to Go 1.21.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Since v5, golangci-lint-action relies on actions/setup-go for
caching, so remove "cache: false" from actions/setup-go to re-enable
caching.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The sole reason is to simplify branch protection rules,
requiring just this one to be passed.

I tried but could not find a way to list all other jobs, so had to add
all of them manually.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Collaborator Author

Oops, apparently most tests are skipped (because, of course, Ubuntu does not have selinux).

Of course we knew it for a long time, we just forgot :)

And since actions/runner-images#2307 is quite old, I filed a new one: actions/runner-images#10802

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant