Make an entry in /etc/group when we modify /etc/passwd #7541
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
Still needs tests |
mheon
force-pushed
the
modify_group
branch
3 times, most recently
from
56a4a50
to
9870b5d
Compare
|
We need e2e and/or system tests for this as well, to verify that we handle the situations correctly. Group Name added to container (--keep-id and --user=1234) |
|
Tests added, and converted to fix #7499 as well (generate a |
| } | ||
|
|
||
| // Check if the group already exists | ||
| _, err = lookup.GetGroup(c.state.Mountpoint, group) |
There was a problem hiding this comment.
Nit can you do this on one line.
if _, err := lookup.GetGroup(c.state.Mountpoint, group); err !=nil {
...
}
mheon
force-pushed
the
modify_group
branch
4 times, most recently
from
d457cba
to
099feb5
Compare
|
Ah, silly me, grepping for |
To ensure that the user running in the container ahs a valid entry in /etc/passwd so lookup functions for the current user will not error, Podman previously began adding entries to the passwd file. We did not, however, add entries to the group file, and this created problems - our passwd entries included the group the user is in, but said group might not exist. The solution is to mirror our logic for /etc/passwd modifications to also edit /etc/group in the container. Unfortunately, this is not a catch-all solution. Our logic here is only advanced enough to *add* to the group file - so if the group already exists but we add a user not a part of it, we will not modify that existing entry, and things remain inconsistent. We can look into adding this later if we absolutely need to, but it would involve adding significant complexity to this already massively complicated function. While we're here, address an edge case where Podman could add a user or group whose UID overlapped with an existing user or group. Also, let's make users able to log into users we added. Instead of generating user entries with an 'x' in the password field, indicating they have an entry in /etc/shadow, generate a '*' indicating the user has no password but can be logged into by other means e.g. ssh key, su. Fixes containers#7503 Fixes containers#7389 Fixes containers#7499 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
|
This is starting to go green. |
|
LGTM |
1 similar comment
|
LGTM |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
To ensure that the user running in the container ahs a valid entry in /etc/passwd so lookup functions for the current user will not error, Podman previously began adding entries to the passwd file. We did not, however, add entries to the group file, and this created problems - our passwd entries included the group the user is in, but said group might not exist. The solution is to mirror our logic for /etc/passwd modifications to also edit /etc/group in the container.
Unfortunately, this is not a catch-all solution. Our logic here is only advanced enough to add to the group file - so if the group already exists but we add a user not a part of it, we will not modify that existing entry, and things remain inconsistent. We can look into adding this later if we absolutely need to, but it would involve adding significant complexity to this already massively complicated function.
While we're here, address an edge case where Podman could add a user or group whose UID overlapped with an existing user or group.
Fixes #7503
Fixes #7389