Skip to content
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

Don't drop from root, causes error when changing file permission #17

Merged
merged 2 commits into from Oct 8, 2016
Merged

Don't drop from root, causes error when changing file permission #17

merged 2 commits into from Oct 8, 2016

Conversation

zkanda
Copy link
Contributor

@zkanda zkanda commented Oct 6, 2016

When running docker-compose with volumes, I always get this error from lounge.

Error: EPERM: operation not permitted, chmod '/home/lounge/data'
    at Error (native)
    at Object.fs.chmodSync (fs.js:997:18)
    at Object.<anonymous> (/usr/local/lib/node_modules/thelounge/src/command-line/index.js:20:5)
    at Module._compile (module.js:409:26)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Module.require (module.js:353:17)
    at require (internal/module.js:12:17)
    at Object.<anonymous> (/usr/local/lib/node_modules/thelounge/index.js:13:1)
fs.js:997
  return binding.chmod(pathModule._makeLong(path), modeNum(mode));

Which basically means it cannot change the permission of the folder data because it doesn't have the right permission.

It's either we don't drop the user to lounge or we don't do a chmod on https://github.com/thelounge/lounge/blob/master/src/command-line/index.js#L20

@astorije astorije added the bug label Oct 6, 2016
@williamboman
Copy link
Member

The container drops root for security reasons. The container runs as a non-privileged user lounge with 1000:1000 uid:gid. If you're trying to mount a data dir that already exists on the host system you need to change permissions on it before running the container, e.g.;

$ chown -R 1000:1000 ~/.lounge

@williamboman williamboman added invalid and removed bug labels Oct 6, 2016
@zkanda
Copy link
Contributor Author

zkanda commented Oct 6, 2016

I'm cautioned with that as well and thanks for the input. However that solution is not portable, I cannot just run docker-compose up -d and expect everything would be alright, here is a good discussion I found regarding this: https://forums.docker.com/t/root-user-or-non-root-user-inside-container/966

If it's indeed invalid, maybe we should document the extra steps here?

@williamboman
Copy link
Member

williamboman commented Oct 6, 2016

I agree with you, I tend to run my containers with root user. The Dockerfile ran as lounge user when we "inherited" it, so that's why it's there in the first place. After experimenting with doing fresh lounge installations with the docker image, I have to agree it's pretty cumbersome, so I wouldn't be against this change.

However, if this were to pass we'd need to remove the user part in the README, build args and some RUN directives in the Dockerfile.

We'd also probably want to update the LOUNGE_HOME path from /home/lounge/data to something else (and keep backward compatibility with deployments that still mount to /home/lounge/data). I'm thinking /etc/lounge, /var/opt/lounge, thoughts?

@williamboman
Copy link
Member

@zkanda Do you want to update the PR with the stuff I mentioned? Otherwise, if you unlock this PR I could take a look at it later today and update this PR.

@zkanda
Copy link
Contributor Author

zkanda commented Oct 7, 2016

@williamboman You can do it, this PR should already be unlock. As for the directory, it would make more sense if it's in /etc/, since that's for configuration files in the Filesystem Hierarchy

@williamboman
Copy link
Member

@zkanda Do you have any ideas how to change volume paths and keep backwards compatibility?

@zkanda
Copy link
Contributor Author

zkanda commented Oct 8, 2016

We need to detect if it's pre-2.0, how about we just tackle the paths later and just change the README and Dockerfile for now? That way we can attack that issue alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants