Show SELinux label on failure #216
Conversation
rhatdan
requested review from
kolyshkin,
mrunalp,
runcom and
thaJeztah
as code owners
go-selinux/selinux_linux.go
Outdated
| @@ -329,7 +329,7 @@ func lSetFileLabel(fpath string, label string) error { | |||
| break | |||
| } | |||
| 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)} | |||
There was a problem hiding this comment.
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)}
^
There was a problem hiding this comment.
Yikes that is what I meant, good job linter,
|
Change otherwise SGTM; error will look something like this when printed; |
There was a problem hiding this comment.
LGTM
but pinging @kolyshkin if he has any concerns on wrapping the underlying error
There was a problem hiding this comment.
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.
|
@kolyshkin Ready to Merge? |
Heh. I was originally considering posting exactly the same as an alternative, then didn't 😂. The checking for a raw |
go-selinux/selinux_linux.go
Outdated
| @@ -329,7 +329,7 @@ func lSetFileLabel(fpath string, label string) error { | |||
| break | |||
| } | |||
| 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} | |||
There was a problem hiding this comment.
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 <dwalsh@redhat.com>
|
@kolyshkin PTAL; I think this one should be ready to go, but the push dismissed your LGTM |
|
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) |
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.