go-selinux: add coverage for LfileLabel, LsetFileLabel #194

thaJeztah · 2022-11-02T13:03:11Z

updates: 1b18907 (#169)

rhatdan · 2022-11-02T13:11:54Z

A few changes.

rhatdan · 2022-11-02T13:14:43Z

go-selinux/selinux_linux_test.go

+	if err != nil {
+		t.Fatalf("LfileLabel failed: %s", err)
+	}
+	if linkLabel != "" {


This should be linkLabel == con

My thinking here was that no label should be set at all on the symlink, correct?

All files have labels. Getting a file without a label on an SELinux system is difficult.

rhatdan · 2022-11-02T13:16:31Z

go-selinux/selinux_stub_test.go

+	if _, err := LfileLabel("/etc"); err != nil {
+		t.Error(err)
+	}
+	if err := LsetFileLabel("/etc", testLabel); err != nil {


This could be dangerous if run as root.

Yes, I was looking for a good path to use (this is for the stubs, so should not do anything, but the existing tests also used /etc)

Updated to use a temp-file to test with 👍

If we are using system directories elsewhere we probably should not.

thaJeztah · 2022-11-02T13:38:30Z

(oh! forgot to post); Hmm... right, so this won't help a lot; while looking, of course I stumbled upon this ticket from @kolyshkin 😂 actions/runner-images#2307

Also had a look if we could install SELinux on Ubuntu, but it requires a reboot, so not an option 😞

=== RUN   TestSetFileLabel
    selinux_linux_test.go:16: SELinux not enabled, skipping.
--- SKIP: TestSetFileLabel (0.00s)
=== RUN   TestKVMLabels
    selinux_linux_test.go:78: SELinux not enabled, skipping.
--- SKIP: TestKVMLabels (0.00s)
=== RUN   TestInitLabels
    selinux_linux_test.go:99: SELinux not enabled, skipping.
--- SKIP: TestInitLabels (0.00s)
=== RUN   TestSELinux
    selinux_linux_test.go:131: SELinux not enabled, skipping.
--- SKIP: TestSELinux (0.00s)
...

rhatdan · 2022-11-02T19:15:39Z

We really should be testing on Centos or Fedora.

rhatdan · 2022-11-02T19:17:59Z

$ sudo make test
Place your right index finger on the fingerprint reader
Failed to match fingerprint
Place your right index finger on the fingerprint reader
Failed to match fingerprint
Place your right index finger on the fingerprint reader
Failed to match fingerprint
[sudo] password for dwalsh: 
go test -timeout 3m  -v ./...
go: downloading golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8
=== RUN   TestSetFileLabel
    selinux_linux_test.go:33: symlink selinux_test selinux_test_link: file exists
--- FAIL: TestSetFileLabel (0.00s)
=== RUN   TestKVMLabels
    selinux_linux_test.go:85: system_u:system_r:container_kvm_t:s0:c31,c770
    selinux_linux_test.go:86: system_u:object_r:container_file_t:s0:c31,c770
--- PASS: TestKVMLabels (0.00s)
=== RUN   TestInitLabels
    selinux_linux_test.go:106: system_u:system_r:container_init_t:s0:c171,c397
    selinux_linux_test.go:107: system_u:object_r:container_file_t:s0:c171,c397
--- PASS: TestInitLabels (0.00s)
=== RUN   TestSELinux
    selinux_linux_test.go:140: system_u:system_r:container_t:s0:c349,c767
    selinux_linux_test.go:141: system_u:object_r:container_file_t:s0:c349,c767
    selinux_linux_test.go:143: system_u:system_r:container_t:s0:c442,c780
    selinux_linux_test.go:144: system_u:object_r:container_file_t:s0:c442,c780
    selinux_linux_test.go:148: system_u:system_r:container_t:s0:c637,c891
    selinux_linux_test.go:149: system_u:object_r:container_file_t:s0:c637,c891
    selinux_linux_test.go:151: ClearLabels
    selinux_linux_test.go:153: system_u:system_r:container_t:s0:c23,c373
    selinux_linux_test.go:154: system_u:object_r:container_file_t:s0:c23,c373
    selinux_linux_test.go:158: PID:20794 MCS:s0:c20,c544
    selinux_linux_test.go:161: unconfined_u:unconfined_r:unconfined_t:s0 <nil>
    selinux_linux_test.go:168:  <nil>
    selinux_linux_test.go:173: system_u:system_r:init_t:s0 <nil>
--- PASS: TestSELinux (0.00s)
=== RUN   TestSetEnforceMode
    selinux_linux_test.go:184: Enforcing Mode: 1
    selinux_linux_test.go:186: Default Enforce Mode: 1
--- PASS: TestSetEnforceMode (0.03s)
=== RUN   TestCanonicalizeContext
--- PASS: TestCanonicalizeContext (0.00s)
=== RUN   TestFindSELinuxfsInMountinfo
    selinux_linux_test.go:265: found "/sys/fs/selinux"
    selinux_linux_test.go:265: found "/root/chroot/selinux"
    selinux_linux_test.go:265: found ""
--- PASS: TestFindSELinuxfsInMountinfo (0.00s)
=== RUN   TestSecurityCheckContext
    selinux_linux_test.go:283: SecurityCheckContext("unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023")
--- PASS: TestSecurityCheckContext (0.00s)
=== RUN   TestClassIndex
--- PASS: TestClassIndex (0.00s)
=== RUN   TestComputeCreateContext
    selinux_linux_test.go:326: ComputeCreateContext(system_u:system_r:init_t:s0, system_u:object_r:tmp_t:s0, file)
    selinux_linux_test.go:338: ComputeCreateContext(badcon, system_u:object_r:tmp_t:s0, process)
--- PASS: TestComputeCreateContext (0.00s)
=== RUN   TestGlbLub
--- PASS: TestGlbLub (0.00s)
=== RUN   TestContextWithLevel
=== RUN   TestContextWithLevel/match_exists_in_user_context_file
=== RUN   TestContextWithLevel/match_exists_in_default_context_file,_but_not_in_user_file
=== RUN   TestContextWithLevel/no_match_in_user_or_default_context_files
--- PASS: TestContextWithLevel (0.00s)
    --- PASS: TestContextWithLevel/match_exists_in_user_context_file (0.00s)
    --- PASS: TestContextWithLevel/match_exists_in_default_context_file,_but_not_in_user_file (0.00s)
    --- PASS: TestContextWithLevel/no_match_in_user_or_default_context_files (0.00s)
FAIL
FAIL	github.com/opencontainers/selinux/go-selinux	0.031s
=== RUN   TestInit
--- PASS: TestInit (0.00s)
=== RUN   TestDuplicateLabel
--- PASS: TestDuplicateLabel (0.00s)
=== RUN   TestRelabel
--- PASS: TestRelabel (0.00s)
=== RUN   TestValidate
--- PASS: TestValidate (0.00s)
=== RUN   TestIsShared
--- PASS: TestIsShared (0.00s)
=== RUN   TestSELinuxNoLevel
--- PASS: TestSELinuxNoLevel (0.00s)
=== RUN   TestSocketLabel
--- PASS: TestSocketLabel (0.00s)
=== RUN   TestKeyLabel
--- PASS: TestKeyLabel (0.00s)
=== RUN   TestFileLabel
--- PASS: TestFileLabel (0.00s)
=== RUN   TestFormatMountLabel
--- PASS: TestFormatMountLabel (0.00s)
PASS
ok  	github.com/opencontainers/selinux/go-selinux/label	0.003s
=== RUN   TestWalk
    pwalk_test.go:38: concurrency: 24, files found: 61
--- PASS: TestWalk (0.00s)
=== RUN   TestWalkManyErrors
    pwalk_test.go:59: found 185 of 361 files
--- PASS: TestWalkManyErrors (0.01s)
PASS
ok  	github.com/opencontainers/selinux/pkg/pwalk	0.014s
=== RUN   TestWalkDir
    pwalkdir_test.go:42: concurrency: 24, files found: 61
--- PASS: TestWalkDir (0.00s)
=== RUN   TestWalkDirManyErrors
    pwalkdir_test.go:63: found 190 of 361 files
--- PASS: TestWalkDirManyErrors (0.01s)
PASS
ok  	github.com/opencontainers/selinux/pkg/pwalkdir	0.013s
FAIL
make: *** [Makefile:27: test] Error 1

rhatdan · 2022-11-02T19:24:25Z

Apply this patch to yours and the tests passes.
diff.patch

kolyshkin · 2022-11-02T21:52:30Z

Also had a look if we could install SELinux on Ubuntu, but it requires a reboot, so not an option

I remember @avagin implemented a complicated trick with upgrading the kernel and rebooting a travis machine while keeping the process that connects to travis infra intact (using CRIU), which helped us to test newer kernels.

Today, we can use something like cirrus-ci which you can use to install centos (7, 8, or 9) and also run fedora via KVM. See runc's .cirrus.yml for example.

We can also use github ci to run fedora in a KVM, but this is more error prone since the only way is to use Mac OS X and there's a 5 to 10% chance that it will fail, based on what I saw.

kolyshkin

A nits to simplify code by using t.TempDir more directly

kolyshkin · 2022-11-02T21:56:52Z

go-selinux/label/label_stub_test.go

+	tmpFile := filepath.Join(t.TempDir(), "test_file")
+	out, err := os.OpenFile(tmpFile, os.O_WRONLY|os.O_CREATE, 0)
+	if err != nil {
+		t.Fatal(err)
+	}


Why not merely use t.TempDir(). It returns a directory name which you can use directly as an argument to FileLabel and SetFileLabel.

kolyshkin · 2022-11-02T21:57:03Z

go-selinux/selinux_stub_test.go

@@ -13,12 +15,18 @@ func TestSELinuxStubs(t *testing.T) {
 	if GetEnabled() {
 		t.Error("SELinux enabled on non-linux.")
 	}
+	tmpFile := filepath.Join(t.TempDir(), "test_file")


rhatdan · 2022-11-14T18:45:36Z

@thaJeztah Still working on this?

thaJeztah · 2022-11-14T20:11:29Z

Oh! Looks like I somehow missed the comments here; updated 👍

go-selinux/label/label_stub_test.go

While these stubs shouldn't make actual changes, it's better to not use `/etc` in the tests, as they may be run as `root`. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

updates: 1b18907 Co-authored-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

rhatdan · 2022-11-15T13:16:51Z

LGTM
@kolyshkin @giuseppe @vrothberg PTAL

rhatdan · 2022-11-15T13:17:00Z

@lsm5 PTAL

thaJeztah · 2022-11-15T13:34:44Z

@lsm5 PTAL

Oh! Voice from the past. It's been a while since we interacted (hope you're doing well!)

rhatdan · 2022-11-15T14:44:43Z

@lsm5 has shown interest in SELinux so I figured I would ping him.

lsm5 · 2022-11-15T15:21:17Z

@lsm5 PTAL

Oh! Voice from the past. It's been a while since we interacted (hope you're doing well!)

@thaJeztah 👋 I'm good, hope you are too! 😄

@lsm5 has shown interest in SELinux so I figured I would ping him.

@rhatdan shown interest yes, but this is outta my reviewing capacity atm, LGTM from a cursory look though. Also sudo make test passes for this PR on my F37 env.

Also, RE: fedora/centos testing, do you want that to be a CI task here?

rhatdan · 2022-11-15T22:21:32Z

It would be awesome if we could test Fedora or Centos here.

lsm5 · 2022-11-16T11:13:14Z

@rhatdan IIUC, github actions is ubuntu only. So, we'll likely need some other setup.

@cevich wdyt? will it be a lot of pain to enable cirrus outside the containers org?

thaJeztah · 2022-11-16T13:01:02Z

Yeah, it's complicated. It's possible to attach custom runners to GitHub actions, but those would need to be maintained somewhere (possibly the CNCF has infra for this?). Some caution is needed for these as well to prevent those runners from also be running on forks (and to be sure they're only running on "good" pull requests, to prevent crypto miners from using them).

Of course the ideal would be for GitHub to provide at least "one" variant of an rpm/yum/dnf flavoured distro.

cevich · 2022-11-21T18:27:34Z

@cevich wdyt? will it be a lot of pain to enable cirrus outside the containers org?

I have an open issue to make the CI VM images public, given there's no real secret sauce in them. That would probably go a long way toward making this easier. The second piece is getting a dedicated GCE or AWS project setup and paid for by someone (potentially forever). That's more likely the most difficult part of the two.

Minor note: I would not recommend trying to re-use an existing project, unless you absolutely don't care about some nasty code escaping confinement and wrecking anything else running there. Trying to lock-down CI-jobs and damage-control an escape with security policy, is a fools-errand IMHO. Easier to just keep the "wild-west" completely separate from civilization.

rhatdan reviewed Nov 2, 2022

View reviewed changes

thaJeztah force-pushed the lfilelabel_coverage branch from c6b1f77 to 5440774 Compare November 2, 2022 13:51

thaJeztah marked this pull request as ready for review November 2, 2022 14:24

thaJeztah requested review from kolyshkin, mrunalp and runcom as code owners November 2, 2022 14:24

kolyshkin requested changes Nov 2, 2022

View reviewed changes

thaJeztah force-pushed the lfilelabel_coverage branch from 5440774 to eaeb0ab Compare November 14, 2022 20:11

thaJeztah commented Nov 14, 2022

View reviewed changes

go-selinux/label/label_stub_test.go Outdated Show resolved Hide resolved

thaJeztah and others added 2 commits November 14, 2022 21:16

use safer path for stub tests

bc94efc

While these stubs shouldn't make actual changes, it's better to not use `/etc` in the tests, as they may be run as `root`. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

go-selinux: add coverage for LfileLabel, LsetFileLabel

d6656c6

updates: 1b18907 Co-authored-by: Daniel J Walsh <dwalsh@redhat.com> Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the lfilelabel_coverage branch from eaeb0ab to d6656c6 Compare November 14, 2022 20:17

rhatdan approved these changes Nov 15, 2022

View reviewed changes

rhatdan merged commit 9538649 into opencontainers:main Nov 15, 2022

thaJeztah deleted the lfilelabel_coverage branch November 16, 2022 12:57

This was referenced Oct 17, 2024

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

Draft

Set up Cirrus CI to test on Fedora #220

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go-selinux: add coverage for LfileLabel, LsetFileLabel #194

go-selinux: add coverage for LfileLabel, LsetFileLabel #194

thaJeztah commented Nov 2, 2022

rhatdan commented Nov 2, 2022 •

edited

Loading

rhatdan Nov 2, 2022

thaJeztah Nov 2, 2022

rhatdan Nov 2, 2022

rhatdan Nov 2, 2022

thaJeztah Nov 2, 2022

thaJeztah Nov 2, 2022

rhatdan Nov 2, 2022

thaJeztah commented Nov 2, 2022

rhatdan commented Nov 2, 2022

rhatdan commented Nov 2, 2022

rhatdan commented Nov 2, 2022

kolyshkin commented Nov 2, 2022

kolyshkin left a comment

kolyshkin Nov 2, 2022

kolyshkin Nov 2, 2022

rhatdan commented Nov 14, 2022

thaJeztah commented Nov 14, 2022

rhatdan commented Nov 15, 2022

rhatdan commented Nov 15, 2022

thaJeztah commented Nov 15, 2022

rhatdan commented Nov 15, 2022

lsm5 commented Nov 15, 2022

rhatdan commented Nov 15, 2022

lsm5 commented Nov 16, 2022

thaJeztah commented Nov 16, 2022

cevich commented Nov 21, 2022

go-selinux: add coverage for LfileLabel, LsetFileLabel #194

go-selinux: add coverage for LfileLabel, LsetFileLabel #194

Conversation

thaJeztah commented Nov 2, 2022

rhatdan commented Nov 2, 2022 • edited Loading

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

thaJeztah commented Nov 2, 2022

rhatdan commented Nov 2, 2022

rhatdan commented Nov 2, 2022

rhatdan commented Nov 2, 2022

kolyshkin commented Nov 2, 2022

kolyshkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Nov 14, 2022

thaJeztah commented Nov 14, 2022

rhatdan commented Nov 15, 2022

rhatdan commented Nov 15, 2022

thaJeztah commented Nov 15, 2022

rhatdan commented Nov 15, 2022

lsm5 commented Nov 15, 2022

rhatdan commented Nov 15, 2022

lsm5 commented Nov 16, 2022

thaJeztah commented Nov 16, 2022

cevich commented Nov 21, 2022

rhatdan commented Nov 2, 2022 •

edited

Loading