Add LFileLabel and LSetFileLabel by rhatdan · Pull Request #169 · opencontainers/selinux

rhatdan · 2021-11-16T16:25:28Z

SELinux C library has two functions for dealing with file labels, one
which follows symlinks and one that does not.

Golang bindings should work the same way. The lack of this function is
resulting in containers/buildah#3630 which has
to hack around the problem.

Signed-off-by: Daniel J Walsh [email protected]

rhatdan · 2021-11-16T16:26:02Z

This is a breaking change, but I think it is worth it, and probably affects almost nobody.

rhatdan · 2021-11-16T16:26:23Z

@nalind PTAL

rhatdan · 2021-11-16T19:51:51Z

@thaJeztah @kolyshkin @mrunalp PTAL

kolyshkin · 2021-11-16T20:09:01Z

go-selinux/selinux.go

 // SetFileLabel sets the SELinux label for this path or returns an error.
+// follow symlinks


This have to be a complete sentence.

kolyshkin · 2021-11-16T20:09:33Z

go-selinux/selinux.go

 	return setFileLabel(fpath, label)
 }

+// SetFileLabel sets the SELinux label for this path or returns an error.


s/SetFileLabel/LSetFileLabel/g

kolyshkin · 2021-11-16T20:12:05Z

go-selinux/selinux.go

 }

+// SetFileLabel sets the SELinux label for this path or returns an error.
+// does not follow symlinks


If the argument is a symbolic link, the label is set on the link itself. This function makes no attempt to follow the symlink.

Or somesuch.

kolyshkin · 2021-11-16T20:12:53Z

go-selinux/selinux.go

+// LFileLabel returns the SELinux label for this path does not follow symlinks
+// or returns an error.


Does not look like a valid English sentence.

kolyshkin · 2021-11-16T20:13:16Z

go-selinux/selinux_linux.go

@@ -317,7 +317,7 @@ func classIndex(class string) (int, error) {
 }

 // setFileLabel sets the SELinux label for this path or returns an error.


s/setFileLabel/lsetFileLabel/

kolyshkin · 2021-11-16T20:16:33Z

go-selinux/selinux.go


+// SetFileLabel sets the SELinux label for this path or returns an error.
+// does not follow symlinks
+func LSetFileLabel(fpath string, label string) error {


Maybe it should be named LsetFileLabel as other similar functions have lowercase letter after L (os.Lstat, unix.Lsetxattr).

kolyshkin · 2021-11-16T20:17:17Z

go-selinux/selinux_linux.go

+	return string(label), nil
+}
+
+// fileLabel returns the SELinux label for this path or returns an error.


function name is wrong

kolyshkin · 2021-11-16T20:17:40Z

go-selinux/selinux_linux.go

+func lfileLabel(fpath string) (string, error) {
+	if fpath == "" {
+		return "", ErrEmptyPath
+	}


I would use lFileLabel.

kolyshkin · 2021-11-16T20:18:08Z

go-selinux/selinux_linux.go


+	label, err := getxattr(fpath, xattrNameSelinux)
+	if err != nil {
+		return "", &os.PathError{Op: "lgetxattr", Path: fpath, Err: err}


typo: /slgetxattr/getxattr/

kolyshkin · 2021-11-16T20:19:16Z

go-selinux/xattrs_linux.go

+		}
+
+		dest = make([]byte, sz)
+		sz, errno = doLgetxattr(path, attr, dest)


Typo? s/doLgetxattr/dogetxattr/

@rhatdan ^^^

rhatdan · 2021-11-18T12:44:58Z

@kolyshkin @thaJeztah I think i have fixed all my cut and paste errors. PTAL

thaJeztah

code LGTM

the remaining nits are for non-exported functions, not a show-stopper per-se

thaJeztah · 2021-11-18T15:23:39Z

go-selinux/selinux_linux.go


-// setFileLabel sets the SELinux label for this path or returns an error.
-func setFileLabel(fpath string, label string) error {
+// lSetFileLabel sets the SELinux label for this path or returns an error.


nit: I guess this one also could use the "not following symlinks"

thaJeztah · 2021-11-18T15:24:02Z

go-selinux/selinux_linux.go

 	return nil
 }

+// setFileLabel sets the SELinux label for this path or returns an error.


thaJeztah · 2021-11-18T15:24:22Z

go-selinux/selinux_linux.go

+	return string(label), nil
+}
+
+// lFileLabel returns the SELinux label for this path or returns an error.


thaJeztah · 2021-11-18T15:25:44Z

The only thing I'm wondering (w.r.t. breaking change); could there be cases where labels are set on /var/run/.... (which usually is a symlink to /run/... ?

rhatdan · 2021-11-18T20:40:33Z

I think this change would not effect a path with a symlink in it, it only effects if the basename is a symlink.
So if you were trying to label /var/run, you would now label /run, and leave the /var/run symlink alone.

But if you were labeling /var/run/foobar, and foobar was a file, it would label the file correctly.

SELinux C library has two functions for dealing with file labels, one which follows symlinks and one that does not. Golang bindings should work the same way. The lack of this function is resulting in containers/buildah#3630 which has to hack around the problem. Signed-off-by: Daniel J Walsh <[email protected]>

kolyshkin

LGTM

thaJeztah · 2021-11-19T17:10:49Z

I think this change would not effect a path with a symlink in it, it only effects if the basename is a symlink. So if you were trying to label /var/run, you would now label /run, and leave the /var/run symlink alone.

But if you were labeling /var/run/foobar, and foobar was a file, it would label the file correctly.

Thanks! Makes sense (should probably have looked closer); agreed I don't expect that to be too problematic.

(post-merge LGTM - saw you addressed those last nits; thanks!)

rhatdan requested review from kolyshkin, mrunalp, runcom and thaJeztah as code owners November 16, 2021 16:25

rhatdan force-pushed the lgetfile branch from 7cc96fd to 8434dd6 Compare November 16, 2021 16:27

kolyshkin reviewed Nov 16, 2021

View reviewed changes

rhatdan force-pushed the lgetfile branch 4 times, most recently from 7902b0b to 71d5187 Compare November 18, 2021 12:42

thaJeztah previously approved these changes Nov 18, 2021

View reviewed changes

rhatdan dismissed thaJeztah’s stale review via 1b18907 November 18, 2021 20:43

rhatdan force-pushed the lgetfile branch from 71d5187 to 1b18907 Compare November 18, 2021 20:43

kolyshkin approved these changes Nov 18, 2021

View reviewed changes

kolyshkin requested a review from thaJeztah November 19, 2021 03:22

rhatdan merged commit 66c8503 into opencontainers:main Nov 19, 2021

thaJeztah mentioned this pull request Nov 2, 2022

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

Merged

		// SetFileLabel sets the SELinux label for this path or returns an error.
		// follow symlinks

		// LFileLabel returns the SELinux label for this path does not follow symlinks
		// or returns an error.

		@@ -317,7 +317,7 @@ func classIndex(class string) (int, error) {
		}

		// setFileLabel sets the SELinux label for this path or returns an error.

Conversation

rhatdan commented Nov 16, 2021

Uh oh!

rhatdan commented Nov 16, 2021

Uh oh!

rhatdan commented Nov 16, 2021

Uh oh!

rhatdan commented Nov 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Nov 18, 2021

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 18, 2021

Uh oh!

rhatdan commented Nov 18, 2021

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants