Show SELinux label on failure by rhatdan · Pull Request #216 · opencontainers/selinux

rhatdan · 2024-07-31T20:50:27Z

We are seeing EINVAL errors with container engines setting SELinux labels. It would be helpful to see what Labels the engines are trying to set.

rhatdan · 2024-07-31T20:50:42Z

@kolyshkin @giuseppe @thaJeztah PTAL

thaJeztah · 2024-07-31T22:24:33Z

go-selinux/selinux_linux.go

 		}
 		if err != unix.EINTR {
-			return &os.PathError{Op: "lsetxattr", Path: fpath, Err: err}
+			return &os.PathError{Op: "lsetxattr", Path: fpath, Err: fmt.Errorf("label=%s: %v", label, err)}


Looks like a linter failed (need to change %v for %w to preserve the original error);

Error: go-selinux/selinux_linux.go:332:94: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint) return &os.PathError{Op: "lsetxattr", Path: fpath, Err: fmt.Errorf("label=%s: %v", label, err)} ^ Error: go-selinux/selinux_linux.go:351:93: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint) return &os.PathError{Op: "setxattr", Path: fpath, Err: fmt.Errorf("label=%s: %v", label, err)} ^

Yikes that is what I meant, good job linter,

Sometimes they're useful! 😂

thaJeztah · 2024-07-31T22:25:20Z

Change otherwise SGTM; error will look something like this when printed;

setxattr /some/path: label=system_u:object_r:bin_t:s0:c3,c4: invalid argument

thaJeztah

LGTM

but pinging @kolyshkin if he has any concerns on wrapping the underlying error

kolyshkin

My only concern is os.PathError.Err is probably expected to be a raw syscall.Errno. There's some old code which still does something like this:

if pathError, ok := err.(*os.PathError); ok && pathError.Err != unix.EDQUOT && pathError.Err != unix.ENOSPC {

and such code will obviously fail here.

So, maybe we can add extra context into Op instead. Something like

return &os.PathError{Op: "setxattr(label="+label+")", Path...

This might be more bullet-proof as I have yet to see code which checks PathError.Op.

rhatdan · 2024-08-06T14:02:27Z

@kolyshkin Ready to Merge?

kolyshkin

LGTM

thaJeztah · 2024-08-07T06:54:08Z

So, maybe we can add extra context into Op instead. Something like
return &os.PathError{Op: "setxattr(label="+label+")", Path...

Heh. I was originally considering posting exactly the same as an alternative, then didn't 😂. The checking for a raw syscall.Errno is a good point though (although it's generally not a contract for a non-stdlib package).

thaJeztah · 2024-08-07T07:10:11Z

go-selinux/selinux_linux.go

 		}
 		if err != unix.EINTR {
-			return &os.PathError{Op: "lsetxattr", Path: fpath, Err: err}
+			return &os.PathError{Op: fmt.Sprintf("lsetxattr(label=%q)", label), Path: fpath, Err: err}


Not a hard blocker (and maybe it's why @kolyshkin used string-concatenation in his suggestion); I tend to avoid using %q in formatting more nowadays; it's a great option, but the downside is that the (double) quotes can become a bit noisy if these messages pass some layers, they will be escaped, for example, when passing over an API, or when logging, it may become something like;

error="setxattr(label=\"system_u:object_r:bin_t:s0:c3,c4\") /some/path: invalid argument"

Assuming we don't expect any really horrible strings to be passed as label by someone, %s would probably still work as it would still be wrapped within the (label=<some value>);

error="setxattr(label=system_u:object_r:bin_t:s0:c3,c4) /some/path: invalid argument"

Probably less relevant for this module, but printing the raw value without quotes may also help in cases where (e.g.) a value including quotes was passed (I had some cases where the user expected quotes to be stripped by their shell, but made a mistake).

We are seeing EINVAL errors with container engines setting SELinux labels. It would be helpful to see what Labels the engines are trying to set. Signed-off-by: Daniel J Walsh <[email protected]>

thaJeztah

LGTM!

Sorry for the back-and-forth 🫶

thaJeztah · 2024-08-07T10:49:58Z

@kolyshkin PTAL; I think this one should be ready to go, but the push dismissed your LGTM

thaJeztah · 2024-08-07T11:31:04Z

I'll bring this one in; the current implementation is ~ matching @kolyshkin's suggestion (I merely asked him to re-review because we needed a second approval)

rhatdan requested review from kolyshkin, mrunalp, runcom and thaJeztah as code owners July 31, 2024 20:50

rhatdan mentioned this pull request Jul 31, 2024

Inside a container selinux.Chcon fails although chcon command succeeds in applying a SELinux context #215

Closed

thaJeztah reviewed Jul 31, 2024

View reviewed changes

rhatdan force-pushed the main branch from a45b490 to abee3c9 Compare August 1, 2024 15:34

thaJeztah previously approved these changes Aug 1, 2024

View reviewed changes

kolyshkin requested changes Aug 2, 2024

View reviewed changes

rhatdan dismissed thaJeztah’s stale review via 2f20e83 August 2, 2024 09:58

rhatdan force-pushed the main branch from abee3c9 to 2f20e83 Compare August 2, 2024 09:58

kolyshkin previously approved these changes Aug 7, 2024

View reviewed changes

thaJeztah reviewed Aug 7, 2024

View reviewed changes

Show SELinux label on failure

5bdefc7

We are seeing EINVAL errors with container engines setting SELinux labels. It would be helpful to see what Labels the engines are trying to set. Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan dismissed kolyshkin’s stale review via 5bdefc7 August 7, 2024 10:26

rhatdan force-pushed the main branch from 2f20e83 to 5bdefc7 Compare August 7, 2024 10:26

thaJeztah approved these changes Aug 7, 2024

View reviewed changes

giuseppe approved these changes Aug 7, 2024

View reviewed changes

thaJeztah merged commit 44b3337 into opencontainers:main Aug 7, 2024

bzsuni mentioned this pull request Nov 7, 2024

Dependences: update opencontainers/selinux to v1.11.1 kubernetes/kubernetes#128148

Merged

Conversation

rhatdan commented Jul 31, 2024

Uh oh!

rhatdan commented Jul 31, 2024

Uh oh!

thaJeztah Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

rhatdan Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 31, 2024

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Aug 6, 2024

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 7, 2024

Uh oh!

thaJeztah Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

rhatdan Aug 7, 2024

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 7, 2024

Uh oh!

thaJeztah commented Aug 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants