selinux: package to query SELinux status and verify/restore file contexts #6285

bboozzoo · 2018-12-11T08:21:31Z

Adding a new package for working with SELinux.

There is basic functionality to query SELinux state, enabled and
permissive/enforcing. The code founds out where the selinux filesystem is
mounted and uses that path to query the status, rather than hardcoding
/sys/fs/selinux.

The package has additional helpers to verify whether a path is labeled accroding
it its default context and possibly restore the context. Trying to avoid calling
to libselinux directly, the command line tools matchpathcon and restorecon
are used.

…exts Adding a new package for working with SELinux. There is basic functionality to query SELinux state, enabled and permissive/enforcing. The code founds out where the selinux filesystem is mounted and uses that path to query the status, rather than hardcoding /sys/fs/selinux. The package has additional helpers to verify whether a path is labeled accroding it its default context and possibly restore the context. Trying to avoid calling to libselinux directly, the command line tools `matchpathcon` and `restorecon` are used. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…pport-package

…arwin Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga · 2018-12-12T09:17:20Z

selinux/label_darwin.go

+
+// Verifypathcon checks whether a given path is labeled according to its default
+// SELinux context
+func Verifypathcon(aPath string) (bool, error) {


Suggested change

func Verifypathcon(aPath string) (bool, error) {

func VerifyPathContext(aPath string) (bool, error) {

zyga · 2018-12-12T09:17:31Z

selinux/label_darwin.go

+ */
+package selinux
+
+// Verifypathcon checks whether a given path is labeled according to its default


Suggested change

// Verifypathcon checks whether a given path is labeled according to its default

// VerifyPathContext checks whether a given path is labeled according to its default

zyga · 2018-12-12T09:17:45Z

selinux/label_darwin.go

+}
+
+// Restorecon restores the default SELinux context of given path
+func Restorecon(aPath string, recursive bool) error {


Suggested change

func Restorecon(aPath string, recursive bool) error {

func RestoreContext(aPath string, recursive bool) error {

zyga · 2018-12-12T09:18:12Z

selinux/label_linux.go

+	"github.com/snapcore/snapd/osutil"
+)
+
+var (


Nitpick: drop the var ( ... ) and use just one-liner var ...

zyga · 2018-12-12T09:18:22Z

selinux/label_linux.go

+
+// Verifypathcon checks whether a given path is labeled according to its default
+// SELinux context
+func Verifypathcon(aPath string) (bool, error) {


Suggested change

func Verifypathcon(aPath string) (bool, error) {

func VerifyPathContext(aPath string) (bool, error) {

zyga · 2018-12-12T09:18:33Z

selinux/label_linux.go

+	matchIncorrectLabel = regexp.MustCompile("^.* has context .* should be .*\n$")
+)
+
+// Verifypathcon checks whether a given path is labeled according to its default


Suggested change

// Verifypathcon checks whether a given path is labeled according to its default

// VerifyPathContext checks whether a given path is labeled according to its default

zyga · 2018-12-12T09:19:27Z

selinux/label_linux.go

+	return false, err
+}
+
+// Restorecon restores the default SELinux context of given path


Suggested change

// Restorecon restores the default SELinux context of given path

// RestoreContext restores the default SELinux context of given path

zyga · 2018-12-12T09:19:39Z

selinux/label_linux.go

+}
+
+// Restorecon restores the default SELinux context of given path
+func Restorecon(aPath string, recursive bool) error {


Suggested change

func Restorecon(aPath string, recursive bool) error {

func RestoreContext(aPath string, recursive bool) error {

zyga · 2018-12-12T09:20:24Z

selinux/label_linux.go

+		return err
+	}
+	// if restorecon is found we may restore SELinux context
+	restoreconPath, err := exec.LookPath("restorecon")


Is there a reason you are going through the hook of calling LookPath yourself? exec.Command will do that for you unless you provide an absolute path.

It was left behind after some earlier refactoring. I'll drop it in the next push.

zyga · 2018-12-12T09:20:39Z

selinux/label_linux_test.go

+	cmd := testutil.MockCommand(c, "matchpathcon", "")
+	defer cmd.Restore()
+
+	ok, err := selinux.Verifypathcon(l.path)


Suggested change

ok, err := selinux.Verifypathcon(l.path)

ok, err := selinux.VerifyPathContext(l.path)

zyga · 2018-12-12T09:20:51Z

selinux/label_linux_test.go

+	cmd := testutil.MockCommand(c, "matchpathcon", "echo gibberish; exit 1 ")
+	defer cmd.Restore()
+
+	ok, err := selinux.Verifypathcon(l.path)


Suggested change

ok, err := selinux.Verifypathcon(l.path)

ok, err := selinux.VerifyPathContext(l.path)

zyga · 2018-12-12T09:21:03Z

selinux/label_linux_test.go

+	cmd := testutil.MockCommand(c, "matchpathcon", "echo gibberish; exit 5 ")
+	defer cmd.Restore()
+
+	ok, err := selinux.Verifypathcon(l.path)


Suggested change

ok, err := selinux.Verifypathcon(l.path)

ok, err := selinux.VerifyPathContext(l.path)

zyga · 2018-12-12T09:21:15Z

selinux/label_linux_test.go

+	cmd := testutil.MockCommand(c, "matchpathcon", ``)
+	defer cmd.Restore()
+
+	ok, err := selinux.Verifypathcon("does-not-exist")


Suggested change

ok, err := selinux.Verifypathcon("does-not-exist")

ok, err := selinux.VerifyPathContext("does-not-exist")

zyga · 2018-12-12T09:21:26Z

selinux/label_linux_test.go

+}
+
+func (l *labelSuite) TestVerifyFailNoTool(c *check.C) {
+	ok, err := selinux.Verifypathcon(l.path)


Suggested change

ok, err := selinux.Verifypathcon(l.path)

ok, err := selinux.VerifyPathContext(l.path)

zyga · 2018-12-12T09:21:38Z

selinux/label_linux_test.go

+exit 1`, l.path))
+	defer cmd.Restore()
+
+	ok, err := selinux.Verifypathcon(l.path)


Suggested change

ok, err := selinux.Verifypathcon(l.path)

ok, err := selinux.VerifyPathContext(l.path)

zyga · 2018-12-12T09:21:50Z

selinux/label_linux_test.go

+	cmd := testutil.MockCommand(c, "restorecon", "")
+	defer cmd.Restore()
+
+	err := selinux.Restorecon(l.path, false)


Suggested change

err := selinux.Restorecon(l.path, false)

err := selinux.RestoreContext(l.path, false)

zyga · 2018-12-12T09:22:01Z

selinux/label_linux_test.go

+
+	cmd.ForgetCalls()
+
+	err = selinux.Restorecon(l.path, true)


Suggested change

err = selinux.Restorecon(l.path, true)

err = selinux.RestoreContext(l.path, true)

zyga · 2018-12-12T09:22:11Z

selinux/label_linux_test.go

+}
+
+func (l *labelSuite) TestRestoreFail(c *check.C) {
+	err := selinux.Restorecon(l.path, false)


Suggested change

err := selinux.Restorecon(l.path, false)

err := selinux.RestoreContext(l.path, false)

zyga · 2018-12-12T09:22:24Z

selinux/label_linux_test.go

+	cmd := testutil.MockCommand(c, "restorecon", "exit 1")
+	defer cmd.Restore()
+
+	err = selinux.Restorecon(l.path, false)


Suggested change

err = selinux.Restorecon(l.path, false)

err = selinux.RestoreContext(l.path, false)

zyga · 2018-12-12T09:22:34Z

selinux/label_linux_test.go

+	err = selinux.Restorecon(l.path, false)
+	c.Assert(err, check.ErrorMatches, "exit status 1")
+
+	err = selinux.Restorecon("does-not-exist", false)


Suggested change

err = selinux.Restorecon("does-not-exist", false)

err = selinux.RestoreContext("does-not-exist", false)

zyga

Looks very nice overall.

I made a small swarm of suggestions for names that are more natural and not match the underlaying commands.

LGTM

@zyga

Renames suggested during the review (thanks @zyga!) Extend the comment on matchpathcon. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis

looks good overall but left some comments

pedronis · 2018-12-12T13:45:38Z

selinux/label_linux.go

+}
+
+// RestoreContext restores the default SELinux context of given path
+func RestoreContext(aPath string, recursive bool) error {


should we have a on element option struct to avoid the unclear bool only argument?

pedronis · 2018-12-12T13:47:39Z

selinux/label_linux.go

+	//   <the-path> has context <some-context>, should be <some-other-context>
+	// match the message so that we can distinguish a failed verification
+	// case from other errors
+	if exit == 1 && matchIncorrectLabel.Match(out) {


seems we need a test that calls the real tool if available to test this bit, otherwise we are just checking the expected output against again our own expectations, if the tool output changes nothing breaks here

Calling the actual tool we'd have to have the policy module installed in the host system. The only way to verify this reasonably is a spread test that does the same thing I'm doing locally: mkdir ~/snap/foo && chcon -t user_home_t -R ~/snap && snap run <snap> && ls -RZ ~/snap.

We definitely want some kind of test (here or in the one using this) that tests this bit as directly/with as little layers as possible. Otherwise the other approach would be to write our own tool in C using libselinux, then we control the format completely.

I've pushed a spread test to #6265 where this API is actually exercised. That's probably as much as can be done now until #6270 lands.

pedronis · 2018-12-12T13:48:43Z

selinux/label_darwin.go

@@ -0,0 +1,30 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-


we need the per-plaform impls because we will use selinux in the cmd/snap code, right?

pedronis · 2018-12-12T13:54:02Z

selinux/label_linux_test.go

+	c.Assert(ok, check.Equals, false)
+}
+
+func (l *labelSuite) TestVerifyFailNoTool(c *check.C) {


this will fail on fedora like systems, no? aren't we running unit tests there?

Good point. I'll need to skip this iff the tool is already present. AFAIK we run unit tests only Ubuntu and Debian

…pport-package

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

Replace a naked bool passed to RestoreContext with a struct to make the code easier to read/follow. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis

lgtm

pedronis · 2018-12-13T13:21:18Z

selinux/label_linux.go

+}
+
+// RestoreContext restores the default SELinux context of given path
+func RestoreContext(aPath string, mode RestoreMode) error {


we tend to use pointer for option structs so that nil can be passed and mean the default as well but no strong opinion for this particular case

bboozzoo added the SELinux label Dec 11, 2018

bboozzoo force-pushed the bboozzoo/selinux-support-package branch from fee58a3 to 81dd13c Compare December 11, 2018 08:22

bboozzoo mentioned this pull request Dec 11, 2018

cmd/snap: attempt to restore SELinux context of snap user directories #6265

Merged

Merge remote-tracking branch 'origin/master' into bboozzoo/selinux-su…

7f9ed6b

…pport-package

bboozzoo mentioned this pull request Dec 11, 2018

release: support probing SELinux state #6286

Merged

selinux: tag implementation files are linux specific, add stubs for d…

e29faaf

…arwin Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

zyga reviewed Dec 12, 2018

View reviewed changes

zyga approved these changes Dec 12, 2018

View reviewed changes

selinux: renames and tweaks

2bcbe94

Renames suggested during the review (thanks @zyga!) Extend the comment on matchpathcon. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis reviewed Dec 12, 2018

View reviewed changes

bboozzoo added 3 commits December 12, 2018 15:34

Merge remote-tracking branch 'origin/master' into bboozzoo/selinux-su…

9372938

…pport-package

selinux: skip relevant tests when the tools are present

08632d8

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

selinux: drop naked bool in RestoreContext

8cd8b25

Replace a naked bool passed to RestoreContext with a struct to make the code easier to read/follow. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

pedronis approved these changes Dec 13, 2018

View reviewed changes

bboozzoo merged commit 6a2be91 into snapcore:master Dec 13, 2018

	func Verifypathcon(aPath string) (bool, error) {
	func VerifyPathContext(aPath string) (bool, error) {

	// Verifypathcon checks whether a given path is labeled according to its default
	// VerifyPathContext checks whether a given path is labeled according to its default

	func Restorecon(aPath string, recursive bool) error {
	func RestoreContext(aPath string, recursive bool) error {

	// Restorecon restores the default SELinux context of given path
	// RestoreContext restores the default SELinux context of given path

	ok, err := selinux.Verifypathcon(l.path)
	ok, err := selinux.VerifyPathContext(l.path)

	ok, err := selinux.Verifypathcon("does-not-exist")
	ok, err := selinux.VerifyPathContext("does-not-exist")

	err := selinux.Restorecon(l.path, false)
	err := selinux.RestoreContext(l.path, false)

	err = selinux.Restorecon(l.path, true)
	err = selinux.RestoreContext(l.path, true)

	err = selinux.Restorecon(l.path, false)
	err = selinux.RestoreContext(l.path, false)

	err = selinux.Restorecon("does-not-exist", false)
	err = selinux.RestoreContext("does-not-exist", false)

selinux: package to query SELinux status and verify/restore file contexts #6285

selinux: package to query SELinux status and verify/restore file contexts #6285

Conversation

bboozzoo commented Dec 11, 2018

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

bboozzoo Dec 12, 2018 • edited

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

zyga left a comment

Choose a reason for hiding this comment

pedronis left a 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

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboozzoo Dec 12, 2018 •

edited