add GetSeUserByName, fallback to failsafe context in GetDefaultContextWithLevel #232
Conversation
capnspacehook
requested review from
kolyshkin,
mrunalp,
rhatdan,
runcom and
thaJeztah
as code owners
|
Missed this, could you sign your commit? |
|
LGTM |
capnspacehook
force-pushed
the
getseuserbyname
branch
from
b0b1674 to
34ac566
Compare
|
@rhatdan no prob, signed commit |
capnspacehook
force-pushed
the
getseuserbyname
branch
from
34ac566 to
845d1a4
Compare
|
@capnspacehook can you split this into two commits, by functionality. Same PR is fine. |
capnspacehook
force-pushed
the
getseuserbyname
branch
from
845d1a4 to
cb77b1c
Compare
Signed-off-by: Andrew LeFevre <[email protected]>
Signed-off-by: Andrew LeFevre <[email protected]>
capnspacehook
force-pushed
the
getseuserbyname
branch
from
cb77b1c to
3a28444
Compare
|
@kolyshkin split into two commits |
| usr, err := user.Lookup(username) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("failed to lookup user %q", username) | ||
| } | ||
| gids, err := usr.GroupIds() | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("failed to find user %q's groups", username) | ||
| } |
There was a problem hiding this comment.
It looks like both user.Lookup and (*user.User).GroupIds already return adequate errors, such as:
user: unknown user xxx
user: lookup username xxx: <os-error>
user: list groups for xxx: invalid gid yyy
user: xxx is a member of more than <maxGroups> groups
user: list groups for xxx failed
Why are you choosing to not pass it onto the caller? I would just do return err here.
| func getSeUserByName(username string) (seUser string, level string, err error) { | ||
| seUsersConf := filepath.Join(policyRoot(), "seusers") | ||
| confFile, err := os.Open(seUsersConf) |
There was a problem hiding this comment.
It looks like we have two err variables here: first comes from a named return value, and the second you instantiate here. Not a bug per se but makes the code slightly more complicated.
| var groupSeUser, groupLevel string | ||
|
|
||
| lineNum := -1 | ||
| scanner := bufio.NewScanner(r) |
There was a problem hiding this comment.
I would use bufio.NewReader and ReadBytes('\n') here.
| // remove any trailing comments, then extra whitespace | ||
| parts := strings.SplitN(line, "#", 2) |
There was a problem hiding this comment.
Are trailing comments allowed? In C code they ignore empty lines, or lines consisting entirely of whitespace, or lines that start with (optional) whitespace and then #. I don't see any trailing comments handling.
| continue | ||
| } | ||
|
|
||
| parts = strings.SplitN(line, ":", 3) |
There was a problem hiding this comment.
I'd use strings.Cut here as otherwise you're allocating slices for the garbage collector to free.
There was a problem hiding this comment.
Regarding the second patch -- I see we're opening two files in getDefaultContextWithLevel, while we might not even read the second one. Your patch adds a third file to open.
Maybe we can rework the code first to open the file only when needed (the most changes will be in the tests I guess), then add the failsafe context?
Ported getseuserbyname (https://github.com/SELinuxProject/selinux/blob/main/libselinux/src/seusers.c#L188) to Go, and added
failsafe_contextparsing toGetDefaultContextWithLevelas that's what libselinux's get_default_context_with_level does (https://github.com/SELinuxProject/selinux/blob/main/libselinux/src/get_context_list.c#L488).getseuserbynamereads theseusersfile (https://www.man7.org/linux/man-pages/man5/seusers.5.html) to find the SELinux user and the MLS level for a given Linux user.get_default_context_with_leveltakes a SELinux user, MLS level, and an SELinux context (combination of SELinux user, role, domain, and MLS level, in many cases this context is the context of the caller) and returns the SELinux context that should be used when creating processes as a certain Linux user.get_default_context_with_levelattempts to find a suitable context for the specific SELinux user that was passed first, then searches in the list of global context mappings if no match was found, and finally just returns the failsafe context if no match was found anywhere else.GetDefaultContextWithLevelpreviously returned an error if no context could be found for the SELinux user or globally.Man page for failsafe_context: https://www.man7.org/linux//man-pages/man5/failsafe_context.5.html