Skip to content

If $HOME is not set, return homedir from /etc/passwd#11287

Merged
tiborvass merged 1 commit intomoby:masterfrom
rhatdan:homedir
Mar 10, 2015
Merged

If $HOME is not set, return homedir from /etc/passwd#11287
tiborvass merged 1 commit intomoby:masterfrom
rhatdan:homedir

Conversation

@rhatdan
Copy link
Contributor

@rhatdan rhatdan commented Mar 10, 2015

Running docker within a systemd unit file, the $HOME environment variable is not set. This causes docker to attempt to write the .docker directory to /. In a Atomic platform the / is immutable, so docker fails.

This patch will look up the users homedir if the $HOME environment variable is not set.

Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)
@duglin
Copy link
Contributor

duglin commented Mar 10, 2015

Not sure if this is a valid concern, but what if someone is calling this method in order to determine if $HOME is set or not - I think this will return a false positive.

And, is it ok that this pulls in libcontainer into the 'homedir' package?

@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 10, 2015

If you want to check if HOME is set you should call os.GetEnv(HOME). As far as the second question. I don't know. But if the goal of the homedir package is to get the homedir, I think my solution is more correct then what we currently have.

@duglin
Copy link
Contributor

duglin commented Mar 10, 2015

Perhaps we need a GetEffectiveHome() type of func - then people can decide if they want the current HOME/USERPROFILE env var value or if they want to find the user's home dir (meaning do what Dan added if necessary).

@estesp
Copy link
Contributor

estesp commented Mar 10, 2015

The pkg/ packages are standalone--they can't include anything from docker or libcontainer proper. This definitely sounds like it requires a solution (for the unwritable /), but I think the solution will have to be on the client side and not in the generic package for returning the $HOME setting.

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 10, 2015

I dunno, libcontainer/user is standalone too :) Maybe we should move that function there btw.

@estesp
Copy link
Contributor

estesp commented Mar 10, 2015

Ha, yes, @LK4D4 is correct. So, I thought there was a general rule that pkg/ content tried to not include non-Go-standard packages so that they could be used by any piece and not drag in the "rest" of Docker, but I see there is some inclusion of vendor libs, and the funky archive/tar replacement, which is sort of a known hack. If we are OK with libcontainer/user because it is currently standalone, I'm fine with that.

@rhatdan
Copy link
Contributor Author

rhatdan commented Mar 10, 2015

I was kind of shocked standard golang user does not support this on X86_64 machines.

@tianon
Copy link
Member

tianon commented Mar 10, 2015 via email

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 10, 2015

Yeah, it calls glibc functions.

@LK4D4
Copy link
Contributor

LK4D4 commented Mar 10, 2015

LGTM

@tiborvass
Copy link
Contributor

We can revisit standaloneness in pkg/ another time, it's not perfect.

LGTM

tiborvass added a commit that referenced this pull request Mar 10, 2015
If $HOME is not set, return homedir from /etc/passwd
@tiborvass tiborvass merged commit d26e3cf into moby:master Mar 10, 2015
tiborvass pushed a commit to tiborvass/docker that referenced this pull request Sep 24, 2019
About github.com/opencontainers/runc/libcontainer/user:

According to opencontainers/runc@195d8d5
this package has two functions:
- Have a static implementation of user lookup, which is now supported in the
  os/user stdlib package with the osusergo build tag, but wasn't at the time.
- Have extra functions that os/user doesn't have, but none of those are used
  in homedir.

Since moby#11287, homedir depended directly on
libcontainer's user package for CurrentUser().
This is being replaced with os/user.Current(), because all of our static
binaries are compiled with the osusergo tag, and for dynamic libraries it
is more correct to use libc's implementation than parsing /etc/passwd.

About github.com/docker/docker/pkg/idtools:

Only dependency was from GetStatic() which uses idtools.LookupUID(uid).
The implementation of idtools.LookupUID just calls to
github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks
to exec-ing to getent (since moby#27599).

This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out
of supporting nss lookups in static binaries via exec-ing to getent for
the homedir package.

If homedir package users need to support nss lookups, they are advised
to compile dynamically instead.

Signed-off-by: Tibor Vass <tibor@docker.com>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Sep 26, 2019
About github.com/opencontainers/runc/libcontainer/user:

According to opencontainers/runc@195d8d5
this package has two functions:
- Have a static implementation of user lookup, which is now supported in the
  os/user stdlib package with the osusergo build tag, but wasn't at the time.
- Have extra functions that os/user doesn't have, but none of those are used
  in homedir.

Since moby/moby#11287, homedir depended directly on
libcontainer's user package for CurrentUser().
This is being replaced with os/user.Current(), because all of our static
binaries are compiled with the osusergo tag, and for dynamic libraries it
is more correct to use libc's implementation than parsing /etc/passwd.

About github.com/docker/docker/pkg/idtools:

Only dependency was from GetStatic() which uses idtools.LookupUID(uid).
The implementation of idtools.LookupUID just calls to
github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks
to exec-ing to getent (since moby/moby#27599).

This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out
of supporting nss lookups in static binaries via exec-ing to getent for
the homedir package.

If homedir package users need to support nss lookups, they are advised
to compile dynamically instead.

Signed-off-by: Tibor Vass <tibor@docker.com>
Upstream-commit: a8608b5b67c77169276863cf31c6bc89a9ab3d8c
Component: engine
burnMyDread pushed a commit to burnMyDread/moby that referenced this pull request Oct 21, 2019
About github.com/opencontainers/runc/libcontainer/user:

According to opencontainers/runc@195d8d5
this package has two functions:
- Have a static implementation of user lookup, which is now supported in the
  os/user stdlib package with the osusergo build tag, but wasn't at the time.
- Have extra functions that os/user doesn't have, but none of those are used
  in homedir.

Since moby#11287, homedir depended directly on
libcontainer's user package for CurrentUser().
This is being replaced with os/user.Current(), because all of our static
binaries are compiled with the osusergo tag, and for dynamic libraries it
is more correct to use libc's implementation than parsing /etc/passwd.

About github.com/docker/docker/pkg/idtools:

Only dependency was from GetStatic() which uses idtools.LookupUID(uid).
The implementation of idtools.LookupUID just calls to
github.com/opencontainers/runc/libcontainer/user.LookupUid or fallbacks
to exec-ing to getent (since moby#27599).

This patch replaces calls to homedir.GetStatic by homedir.Get(), opting out
of supporting nss lookups in static binaries via exec-ing to getent for
the homedir package.

If homedir package users need to support nss lookups, they are advised
to compile dynamically instead.

Signed-off-by: Tibor Vass <tibor@docker.com>
Signed-off-by: zach <Zachary.Joyner@linux.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants