-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove /etc/passwd and /etc/group parsing on runc run/exec #3999
base: main
Are you sure you want to change the base?
Conversation
28aadc5
to
4866cd2
Compare
95e76a7
to
ab0c62a
Compare
This is still a draft pending #4325 merge -- once that one is merged, I will rework setUser here. |
bb70a3b
to
e87bc0e
Compare
214d77a
to
348c250
Compare
This seems to be ready now. @AkihiroSuda @cyphar @lifubang @rata PTAL. Overall I think it's a big step forward:
Also, I know that the second commit is a bit too big for review. I tried to split it into two but that seems unfeasible, as the two features (changing user string to uid:gid and moving the setting of HOME to prepareEnv) are interdependent. |
Historically, when HOME is not explicitly set in process.Env, and UID to run as doesn't have a corresponding entry in container's /etc/passwd, runc sets HOME=/ as a fallback. Add the corresponding check, for the sake of backward compatibility preservation. Signed-off-by: Kir Kolyshkin <[email protected]>
This addresses the following TODO in the code (added back in 2015 by commit 845fc65): > // TODO: fix libcontainer's API to better support uid/gid in a typesafe way. Historically, libcontainer internally uses strings for user, group, and additional (aka supplementary) groups. Yet, runc receives those credentials as part of runtime-spec's process, which uses integers for all of them (see [1], [2]). What happens next is: 1. runc start/run/exec converts those credentials to strings (a User string containing "UID:GID", and a []string for additional GIDs) and passes those onto runc init. 2. runc init converts them back to int, in the most complicated way possible (parsing container's /etc/passwd and /etc/group). All this conversion and, especially, parsing is totally unnecessary, but is performed on every container exec (and start). The only benefit of all this is, a libcontainer user could use user and group names instead of numeric IDs (but runc itself is not using this feature, and we don't know if there are any other users of this). Let's remove this back and forth translation, hopefully increasing runc exec performance. The only remaining need to parse /etc/passwd is to set HOME environment variable for a specified UID, in case $HOME is not explicitly set in process.Env. This can now be done right in prepareEnv, which simplifies the code flow a lot. Alas, we can not use standard os/user.LookupId, as it could cache host's /etc/passwd or the current user (even with the osusergo tag). PS Note that the structures being changed (initConfig and Process) are never saved to disk as JSON by runc, so there is no compatibility issue for runc users. Still, this is a breaking change in libcontainer, but we never promised that libcontainer API will be stable (and there's a special package that can handle it -- github.com/moby/sys/user). Reflect this in CHANGELOG. For 3998. [1]: https://github.com/opencontainers/runtime-spec/blob/v1.0.2/config.md#posix-platform-user [2]: https://github.com/opencontainers/runtime-spec/blob/v1.0.2/specs-go/config.go#L86 Signed-off-by: Kir Kolyshkin <[email protected]>
Since the UID/GID/AdditonalGroups fields are now numeric, we can address the following TODO item in the code (added by commit d2f4969 back in 2016): > TODO: We currently can't do > this check earlier, but if libcontainer.Process.User was typesafe > this might work. Move the check to much earlier phase, when we're preparing to start a process in a container. Signed-off-by: Kir Kolyshkin <[email protected]>
Switch from github.com/moby/sys/user to Go stdlib os/user (which has both libc-backed and pure Go implementations). Signed-off-by: Kir Kolyshkin <[email protected]>
This package is marked as deprecated in v1.2.0, and have no internal users, so let's remove it (for v1.3.0). Signed-off-by: Kir Kolyshkin <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mostly LGTM, left some small comments
home, err := getUserHome(uid) | ||
if err != nil { | ||
// For backward compatibility, don't return an error, but merely log it. | ||
logrus.WithError(err).Debugf("HOME not set in process.env, and getting UID %d homedir failed", uid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to log we set it to /
?
@@ -1,81 +0,0 @@ | |||
package user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes complete sense, but if the window between deprecation and removal is very small, we might slow down the adoption of new runc releases. I have mixed feelings with this, given that I don't think it will cause a huge pain to keep it for one more release, but it should be very simple to change for users too.
It seems there are more than 1.3k projects importing this: https://pkg.go.dev/github.com/opencontainers/runc/libcontainer/user?tab=importedby
I guess we are fine with this, but a little more elaboration on why it seems safe for the rest would be reassuring for me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well,
- the number (1.3K) seems incorrect -- at the very least we should exclude those ancient repos with
Godeps
and multiple github forks of Docker etc. which are not district pieces of software, but merely a way to work with Github; - github.com/moby/sys/user is a direct replacement, so the migration should be easy and straightforward.
Having said that, we can definitely wait for runc v1.4 to remove this. @thaJeztah WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, I haven't tried to see which of those 1.3k are not those forks.
I'm fine with both, given that the migration is so easy. Just wanted to make sure we were all on the same page regarding potential users and the small window. Given that is indeed very easy to migrate, I think this is just fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
I'm fine if we merge now dropping the deprecated package or if we want to keep it a little longer.
libct: switch to numeric UID/GID/groups
This addresses the following TODO in the code (added back in 2015
by commit 845fc65):
Historically, libcontainer internally uses strings for user, group, and
additional (aka supplementary) groups.
Yet, runc receives those credentials as part of runtime-spec's process,
which uses integers for all of them (see 1, 2).
What happens next is:
string containing "UID:GID", and a []string for additional GIDs) and
passes those onto runc init.
possible (parsing container's /etc/passwd and /etc/group).
All this conversion and, especially, parsing is totally unnecessary,
but is performed on every container exec (and start).
The only benefit of all this is, a libcontainer user could use user and
group names instead of numeric IDs (but runc itself is not using this
feature, and we don't know if there are any other users of this).
Let's remove this back and forth translation, hopefully increasing
runc exec performance.
The only remaining need to parse /etc/passwd is to set HOME environment
variable for a specified UID, in case $HOME is not explicitly set in
process.Env. This can now be done right in prepareEnv, which simplifies
the code flow a lot. Alas, we can not use standard os/user.LookupId, as
it could cache host's /etc/passwd or the current user (even with the
osusergo tag).
PS Note that the structures being changed (initConfig and Process) are
never saved to disk as JSON by runc, so there is no compatibility issue
for runc users.
Still, this is a breaking change in libcontainer, but we never promised
that libcontainer API will be stable (and there's a special package
that can handle it -- github.com/moby/sys/user). Reflect this in
CHANGELOG.
For 3998.
libct: earlier Rootless vs AdditionalGroups check
Since the UID/GID/AdditonalGroups fields are now numeric,
we can address the following TODO item in the code (added
by commit d2f4969 back in 2016):
Move the check to much earlier phase, when we're preparing
to start a process in a container.
runc list: use standard os/user
Switch from github.com/moby/sys/user to Go stdlib os/user
(which has both libc-backed and pure Go implementations).
libct/user: remove
This package is marked as deprecated in v1.2.0, and have no internal
users now, so let's remove it (for v1.3.0).
Fixes: #3998