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

kolyshkin · 2024-10-16T23:59:05Z

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 24.04 (see The Ubuntu 20.04 Actions runner image will begin deprecation on 2025-02-01 and will be fully unsupported by 2025-04-15 actions/runner-images#11101);
~~go.mod: bump go to 1.21, drop min/max functions;~~
ci: add go 1.19, 1.23, 1.24, drop go 1.21, 1.22 from test matrix;
fix or silence new gosec warnings (these commits need to be reviewed carefully);
ci: bump golangci-lint to v1.64, fix golangci-lint jobs caching;
ci: add 32-bit tests;
ci: add all-done job;
go.mod: bump to golang.org/x/sys v0.1.0;
ci: show avc denials after tests.

kolyshkin · 2024-10-17T00:16:44Z

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.

kolyshkin · 2024-10-17T00:58:51Z

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

rhatdan · 2024-10-22T10:32:25Z

LGTM, Are the SELinux tests running on Ubuntu now?

kolyshkin · 2025-02-11T02:23:03Z

LGTM, Are the SELinux tests running on Ubuntu now?

No :(

We need to switch to Fedora, I filed #220 for that.

This PR still makes sense but with or without it we actually don't test much :(

thaJeztah · 2025-02-11T08:44:27Z

@kolyshkin wondering, would this be something that could be provided through those "actuated.dev" runners?

CI: add actuated-arm64 runc#4142

(perhaps the "actuated.com" runners could provide a configuration with SELinux installed?)

alexellis · 2025-02-11T10:49:55Z

Current generations of arm64 servers available via cloud do not have working nested virtualisation. It may come in the future, where a standard runner could pivot into a RPM-based distro.

So whilst that's not available, and GitHub may not have it on their radar to support SELinux, we could provide SELinux based runners with fedora or a fedora-like distro like Alma for x86_64 and arm64.

Feel free to reach out to me to discuss options if you know of an end-user company willing to sponsor it.

kolyshkin · 2025-03-03T22:14:52Z

@rhatdan @thaJeztah PTAL (the lack of selinux testing is now addressed by #221).

rhatdan · 2025-03-09T04:42:22Z

It would be helpful if the audit.log is collected on failures, to see if there is an AVC reported.

kolyshkin · 2025-03-12T03:10:13Z

Flakes are being fixed by #227; I'd rather merge this PR first though (before #227).

thaJeztah · 2025-03-12T05:42:42Z

go-selinux/selinux_linux.go

@@ -639,21 +639,6 @@ func (m mlsRange) String() string {
 	return low + "-" + high
 }

-// TODO: remove min and max once Go < 1.21 is not supported.
-func max(a, b uint) uint {


Was there a strong reason not to continue supporting older Go versions? If not, I'd prefer continuing compatibility; we can either move this to a //go:build ... file, or rename the func to not shadow the builtin

No strong reasons; the weak ones are quite usual:

by switching to a newer version we can use new language and stdlib features, some of those I really like;

Go < 1.22 is not supported and might have some unpatched issues.

Sure, if there is a need, we can try maintaining backward compatibility, especially since this repo is not too bif or complex.

we can either move this to a //go:build ... file

It's not possible. Now I remember I actually tried that before, only to get something like this:

$ make GOOS=linux GOARCH=amd64 go build ./... # github.com/opencontainers/selinux/go-selinux go-selinux/selinux_linux.go:666:22: built-in max requires go1.21 or later (-lang was set to go1.19; check go.mod) go-selinux/selinux_linux.go:669:23: built-in min requires go1.21 or later (-lang was set to go1.19; check go.mod)

Done (renamed to minInt/maxInt).

Would like to hear your reasons for supporting Go 1.19.

thaJeztah · 2025-03-12T05:43:22Z

go.mod

@@ -1,5 +1,5 @@
 module github.com/opencontainers/selinux

-go 1.19
+go 1.21


Same here, unless we really cannot

thaJeztah · 2025-03-12T05:49:22Z

go-selinux/selinux_linux.go

@@ -138,7 +138,7 @@ func verifySELinuxfsMount(mnt string) bool {
 		return false
 	}

-	if uint32(buf.Type) != uint32(unix.SELINUX_MAGIC) {
+	if uint32(buf.Type) != uint32(unix.SELINUX_MAGIC) { //nolint:gosec


Can you use the #nosec G115 style? That way we can supported only G115 without any other gosec issue that may be found

Alas this is not working for me. Perhaps you only used standalone gosec?

$ golangci-lint run ./... go-selinux/selinux_linux.go:141:11: G115: integer overflow conversion int64 -> uint32 (gosec) if uint32(buf.Type) != uint32(unix.SELINUX_MAGIC) { // #nosec G115 -- this code is right. ^

This is weird. The same annotation works just fine in a different place:

i0 := int(c.TrailingZeroBits()) //#nosec G115 -- don't expect TralingZeroBits to return values with highest bit set.

bit not here. I am lost 😿

OK, it doesn't work because it's after {. It works if I put in on a previous line:

//#nosec G115 -- there is no overflow here. if uint32(buf.Type) != uint32(unix.SELINUX_MAGIC) {

But, in this case it might want for the whole block, rather than just this line. I'm lost again 😕

Ah, the "whole block" is return false so I guess it's fine.

Yeah, they are kinda horrible, the #nosec comments! I think it's good to use them though (to not accidentally mask other security-related discoveries).

Thanks!

thaJeztah · 2025-03-12T05:51:15Z

go-selinux/selinux_linux.go

@@ -583,7 +583,8 @@ func bitsetToStr(c *big.Int) string {
 	var str string

 	length := 0
-	for i := int(c.TrailingZeroBits()); i < c.BitLen(); i++ {
+	i0 := int(c.TrailingZeroBits()) //nolint:gosec


Same here, and possible add a comment after the // #nosec G115 -- <short commend here >

Ubuntu 20.04 is being deprecated. Signed-off-by: Kir Kolyshkin <[email protected]>

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 a built-in function in Go >= 1.21. Signed-off-by: Kir Kolyshkin <[email protected]>

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 <[email protected]>

This fixes the following linter warnings: go-selinux/selinux_linux.go:645:6: function max has same name as predeclared identifier (predeclared) func max(a, b int) int { ^ go-selinux/selinux_linux.go:652:6: function min has same name as predeclared identifier (predeclared) func min(a, b int) int { ^ Signed-off-by: Kir Kolyshkin <[email protected]>

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 <[email protected]>

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 <[email protected]>

In both of these cases we're writing to a file in /sys/fs/selinux virtual file system, the file is pre-existing (and if it isn't, it could not be created), so the third WriteFile argument is never used and can as well be 0. Signed-off-by: Kir Kolyshkin <[email protected]>

While at it, fix naked returns. Signed-off-by: Kir Kolyshkin <[email protected]>

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. WARN The linter 'tenv' is deprecated (since v1.64.0) due to: Duplicate feature another linter. Replaced by usetesting. 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 in go.mod. Also, remove "cache: false" from actions/setup-go@v5 since golangci-lint-action@v6 now relies on setup-go caching. Also, rename "deadline" to "timeout" as the former is no longer supported. Signed-off-by: Kir Kolyshkin <[email protected]>

Also, replace Go 1.21 with Go 1.19 as this is the minimally supported version for now. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

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 <[email protected]>

Use the oldest tagged version here, because all the functionality this package needs is in there already. Signed-off-by: Kir Kolyshkin <[email protected]>

Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2025-03-12T06:55:22Z

@thaJeztah please take another look, I think I've addressed all your comments.

Aside from changes that you've requested, I added Go 1.19 to CI matrix, and added a new commit which removes a couple of unneeded nolint:gosec annotations.

thaJeztah · 2025-03-12T07:04:09Z

Thanks! Yeah, users of older go versions should become rare, but as long as it's not adding a ton of burden (generally, this repository won't require too many updates I expect), I think it's good to keep <oldest supported>, current stable, old stable as our test-matrix.

But we sure can update <oldest supported> once things become complicated / we significantly benefit from new go features.

thaJeztah

LGTM, thanks!

kolyshkin · 2025-03-12T07:16:41Z

Thanks! Yeah, users of older go versions should become rare, but as long as it's not adding a ton of burden (generally, this repository won't require too many updates I expect), I think it's good to keep <oldest supported>, current stable, old stable as our test-matrix.

Yes to that!

But we sure can update <oldest supported> once things become complicated / we significantly benefit from new go features.

In this particular case this was min/max functions, which I had to adopt in two regards:

change from uint to int (as their user changed the type);
change their names (or add linter annotations).

I played a bit and decided the best way forward is to just bump min Go version and drop them.

But it's not a big deal to keep compatibility if needed. At least for now.

rhatdan

LGTM

rhatdan · 2025-03-12T15:51:33Z

Might be worth adding renovate to this repo to keep the repo more up 2 date.

thaJeztah · 2025-03-12T17:12:56Z

Might be worth adding renovate to this repo to keep the repo more up 2 date.

For github actions used, and go version, probably. For go.mod, I think we can stick to go's MVS recommendations (specify the minimum version required)

kolyshkin force-pushed the ci-bumps branch 2 times, most recently from fcc7c0e to e2f9e99 Compare October 17, 2024 00:06

kolyshkin changed the title ~~Modernize~~ Bump Go version, CI deps, fix some linter issues Oct 17, 2024

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 marked this pull request as ready for review October 17, 2024 00:11

kolyshkin requested review from mrunalp, rhatdan, runcom and thaJeztah as code owners October 17, 2024 00:11

kolyshkin force-pushed the ci-bumps branch from ba47548 to 8a21e7b Compare October 17, 2024 00:13

kolyshkin marked this pull request as draft October 17, 2024 00:13

kolyshkin force-pushed the ci-bumps branch from 8a21e7b to 5b20191 Compare October 17, 2024 00:20

This was referenced Oct 17, 2024

[test] test in actuated runners #219

Closed

Set up Cirrus CI to test on Fedora #220

Closed

kolyshkin force-pushed the ci-bumps branch 3 times, most recently from 0594972 to 48e0c04 Compare March 3, 2025 22:02

kolyshkin marked this pull request as ready for review March 3, 2025 22:11

kolyshkin force-pushed the ci-bumps branch from 7c62bec to 43624cb Compare March 7, 2025 21:56

kolyshkin force-pushed the ci-bumps branch 3 times, most recently from ad0d740 to 255f274 Compare March 10, 2025 01:50

kolyshkin requested a review from rhatdan March 12, 2025 04:28

kolyshkin mentioned this pull request Mar 12, 2025

Improve SetKeyCreate error reporting, fix test flakes #227

Merged

thaJeztah reviewed Mar 12, 2025

View reviewed changes

kolyshkin added 9 commits March 11, 2025 23:09

ci/gha: bump ubuntu to 24.04

f0722f8

Ubuntu 20.04 is being deprecated. Signed-off-by: Kir Kolyshkin <[email protected]>

pkg/pwalk*: use t.TempDir in tests

1cdb812

While at it, fix naked returns. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the ci-bumps branch from 1c06cce to 802b87e Compare March 12, 2025 06:52

kolyshkin added 5 commits March 11, 2025 23:53

ci: add Go 1.23, 1.24 to test matrix

f5339ba

Also, replace Go 1.21 with Go 1.19 as this is the minimally supported version for now. Signed-off-by: Kir Kolyshkin <[email protected]>

ci: add 32-bit test

336f2dd

Signed-off-by: Kir Kolyshkin <[email protected]>

ci: add all-done job

9e7caf6

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 <[email protected]>

deps: bump golang.org/x/sys to v0.1.0

6ef149a

Use the oldest tagged version here, because all the functionality this package needs is in there already. Signed-off-by: Kir Kolyshkin <[email protected]>

ci: show avc denials after tests

c826d9e

Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the ci-bumps branch from 802b87e to c826d9e Compare March 12, 2025 06:54

kolyshkin requested a review from thaJeztah March 12, 2025 06:56

thaJeztah approved these changes Mar 12, 2025

View reviewed changes

rhatdan approved these changes Mar 12, 2025

View reviewed changes

rhatdan merged commit 21fd359 into opencontainers:main Mar 12, 2025
16 checks passed

kolyshkin added this to the v1.12 milestone Mar 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kolyshkin commented Oct 16, 2024 •

edited

Loading

kolyshkin commented Oct 17, 2024

kolyshkin commented Oct 17, 2024

rhatdan commented Oct 22, 2024

kolyshkin commented Feb 11, 2025

thaJeztah commented Feb 11, 2025 •

edited

Loading

alexellis commented Feb 11, 2025

kolyshkin commented Mar 3, 2025

rhatdan commented Mar 9, 2025

kolyshkin commented Mar 12, 2025

thaJeztah Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

thaJeztah Mar 12, 2025

kolyshkin Mar 12, 2025

thaJeztah Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin Mar 12, 2025

thaJeztah Mar 12, 2025

thaJeztah Mar 12, 2025

kolyshkin Mar 12, 2025

kolyshkin commented Mar 12, 2025

thaJeztah commented Mar 12, 2025

thaJeztah left a comment

kolyshkin commented Mar 12, 2025

rhatdan left a comment

rhatdan commented Mar 12, 2025

thaJeztah commented Mar 12, 2025

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

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

Conversation

kolyshkin commented Oct 16, 2024 • edited Loading

kolyshkin commented Oct 17, 2024

kolyshkin commented Oct 17, 2024

rhatdan commented Oct 22, 2024

kolyshkin commented Feb 11, 2025

thaJeztah commented Feb 11, 2025 • edited Loading

alexellis commented Feb 11, 2025

kolyshkin commented Mar 3, 2025

rhatdan commented Mar 9, 2025

kolyshkin commented Mar 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Mar 12, 2025

thaJeztah commented Mar 12, 2025

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin commented Mar 12, 2025

rhatdan left a comment

Choose a reason for hiding this comment

rhatdan commented Mar 12, 2025

thaJeztah commented Mar 12, 2025

kolyshkin commented Oct 16, 2024 •

edited

Loading

thaJeztah commented Feb 11, 2025 •

edited

Loading