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

Meteor should check if is running as root and prevent it on Unix #7821

Merged
merged 5 commits into from Sep 28, 2016

Conversation

@mhidou
Copy link
Contributor

@mhidou mhidou commented Sep 26, 2016

Hi,

This is a solution for #7631

$ sudo meteor

It seems that you are running meteor as root, if you really know what you are
doing, add the unsafe-perm flag.

meteor --unsafe-perm <command>

$ sudo meteor --unsafe-perm

You are running meteor as root, don't forget to chown your .meteor directory
back to yourself.

chown -Rh <username> ~/.meteor

...

@mhidou mhidou force-pushed the mhidou:mhidou-prevent-sudo branch Sep 26, 2016
@mitar
Copy link
Collaborator

@mitar mitar commented Sep 27, 2016

I would output the second warning. Because it is not true that one wants to chown the directory back to your self. Or, hm, which .meteor does Meteor under sudo use? /home/user/.meteor or /root/.meteor?

Maybe you could display the second warning if the parent of .meteor directory is not the same as the .meteor directory (or as current running user).

So in Docker I run meteor as root, but not with sudo. And .meteor is inside /root/.meteor. There is no point for the second warning then.

tools/cli/main.js Outdated
Console.info("");
Console.info("meteor --unsafe-perm <command>");
Console.info("");
return;

This comment has been minimized.

@mitar

mitar Sep 27, 2016
Collaborator

Could you make sure that the process terminates with non-zero exit code?

This comment has been minimized.

@mhidou

mhidou Sep 27, 2016
Author Contributor

Fixed

tools/cli/main.js Outdated
var isRootSafe = _.has(rawOptions, '--unsafe-perm');

if(isRoot && !isRootSafe){
Console.info("");

This comment has been minimized.

@mitar

mitar Sep 27, 2016
Collaborator

And this should probably be an error, not info, then, if we are failing.

This comment has been minimized.

@mhidou

mhidou Sep 27, 2016
Author Contributor

Fixed

Prevent running sudo meteor run it on Unix

Fix for all commands

remove empty line

Revert "Prevent running sudo meteor run it on Unix"

This reverts commit d2867561bb6fb1b259a59556628b56a64c677a0b.

improve PR

improve PR

improve PR

fix PR
@mhidou mhidou force-pushed the mhidou:mhidou-prevent-sudo branch to 0eacb6d Sep 27, 2016
@mhidou
Copy link
Contributor Author

@mhidou mhidou commented Sep 27, 2016

@mitar I agree with you we have to display the second warning only when the command is run with sudo, it makes no sense if you are logged in as root.

When I run meteor with sudo I have process.env.SUDO_USER containing my username. Unfortunately I've found no documentation about this. Is it safe to check typeof process.env.SUDO_USER == 'string' instead of checking .meteor folder location? better performances?

Copy link
Member

@abernix abernix left a comment

My thoughts.

var platform = process.platform;

if (_.has(unixPlatforms, platform)){
var isRoot = process.env.USERNAME == 'root';

This comment has been minimized.

@abernix

abernix Sep 27, 2016
Member

Shouldn't this be process.env.USER? I'm pretty sure that USERNAME is something that sudo adds and would not exist if you were directly logged in as root (i.e. docker).

This comment has been minimized.

@benjamn

benjamn Sep 27, 2016
Member

It seems like the canonical way to do this check is process.getuid && process.getuid() === 0. The process.getuid function is only defined on POSIX platforms, and 0 is the UID of the root user.

var platform = process.platform;

if (_.has(unixPlatforms, platform)){
var isRoot = process.env.USERNAME == 'root';

This comment has been minimized.

@abernix

abernix Sep 27, 2016
Member

Also, === for comparison.


if(isRoot && !isRootSafe){
Console.error("");
Console.error("It seems that you are running meteor as root, if you really know what you are doing, add the unsafe-perm flag.");

This comment has been minimized.

@abernix

abernix Sep 27, 2016
Member

Saying "It seems" is wrong since they are definitely running it as root at this point. 😉.

As a suggestion for the text:

You are attempting to run Meteor as the "root" user.  If you are developing,
this is almost certainly *not* what you want to do and will likely result
in incorrect file permissions.  However, if you are running this in a build
process (CI, etc.) or you are absolutely sure you know what you are doing,
add the `--unsafe-perm` flag to this command to proceed.
Console.error("");
Console.error("It seems that you are running meteor as root, if you really know what you are doing, add the unsafe-perm flag.");
Console.error("");
Console.error("meteor --unsafe-perm <command>");

This comment has been minimized.

@abernix

abernix Sep 27, 2016
Member

With the above text, I think this can be removed.

This comment has been minimized.

@mitar

mitar Sep 27, 2016
Collaborator

I agree.

Console.info("");
Console.info("You are running meteor as root, don't forget to chown your .meteor directory back to yourself.");
Console.info("");
Console.info("chown -Rh <username> ~/.meteor");

This comment has been minimized.

@abernix

abernix Sep 27, 2016
Member

If run as root (with sudo or not) it would still use $HOME/.meteor (which in most cases would be /root) for meteor-tool-related stuff which seems like it should be fine and I don't believe is the source of the problem.

I think the issue arises with the root-owned files being written in <app-dir>/.meteor/local/ which would be in the user's home directory, no? (Though even that should be fine so long as they ALWAYS run every Meteor command with root, but they probably don't understand what they are doing if they're doing this.).

This comment has been minimized.

@abernix

abernix Sep 27, 2016
Member

This message could probably read:

You have run Meteor as root.  Your permissions in your app directory will be incorrect
if you ever attempt to perform any Meteor tasks as your non-root user.  You probably
didn't want this, but you can fix it by running the following from the root of your project:

    sudo chown -Rh ${`process.env.LOGNAME`} .meteor/local

This comment has been minimized.

@mitar

mitar Sep 27, 2016
Collaborator

I am not sure. I have seen issues with ~/.meteor as well. :-)

This comment has been minimized.

@mitar

mitar Sep 27, 2016
Collaborator

But I agree. This warning could be enough, and even if it is not, it will lead users to the right direction.

This comment has been minimized.

@mhidou

mhidou Sep 28, 2016
Author Contributor

When running with sudo process.env.LOGNAME contains 'root', process.env.SUDO_USER contains the user name but available only when run with sudo. This warning is also shown when logged in as root, better to keep <username>.

Console.error("");
process.exit(1);
}
if (isRoot && isRootSafe) {

This comment has been minimized.

@abernix

abernix Sep 27, 2016
Member

Maybe add && process.env.LOGNAME !== process.env.USER here to suppress this when it's irrelevant?

Copy link
Member

@benjamn benjamn left a comment

Let's use process.getuid, please.

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 27, 2016

I cannot upvote yout "Let's use process.getuid, please.". :-(

@abernix
Copy link
Member

@abernix abernix commented Sep 27, 2016

This PR is a step in the right direction (and likely still merge-able without this consideration), but this also raises some other concerns about the way Meteor runs its npm install commands internally since currently, if the meteor tool was run as root (again, not recommended), --unsafe-perm wouldn't be used

@mitar, I'm curious if you've experienced issues with this in your Docker builds already (and a bit surprised if you haven't!)

Perhaps once this is implemented, it would be ideal to let this argument through to the NPM installing functions accordingly.

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 27, 2016

NPM changed it so that it allows you to run root, but if you do so, then they complain that now you should probably use sudo: npm/npm@7288a13

So I do not think we have to pass anything to NPM.

@mitar
Copy link
Collaborator

@mitar mitar commented Sep 27, 2016

I have not had any problems with permissions on Docker or Travis CI, but I do have to run Travis CI with sudo: required. https://github.com/arunoda/travis-ci-meteor-packages/blob/master/.travis-sample.yml#L3

@mhidou
Copy link
Contributor Author

@mhidou mhidou commented Sep 28, 2016

What about changing gid and uid when meteor is running with sudo?

$ sudo meteor

You are attempting to run Meteor as the "root" user. If you are developing,
this is almost certainly *not* what you want to do and will likely result in
incorrect file permissions.

Meteor will now run as <username>

However, if you are running this in a build process (CI, etc.) or you are
absolutely sure you know what you are doing, add the `--unsafe-perm` flag to
this command to force meteor running as root.

=> Continues as username
$ sudo meteor --unsafe-perm

You have run Meteor as root. Your permissions in your app directory will be
incorrect if you ever attempt to perform any Meteor tasks as your non-root
user. You probably didn't want this, but you can fix it by running the
following from the root of your project:

sudo chown -Rh <username> .meteor/local

=> Continues as root
# meteor

You are attempting to run Meteor as the "root" user. If you are developing,
this is almost certainly *not* what you want to do and will likely result in
incorrect file permissions.

However, if you are running this in a build process (CI, etc.) or you are
absolutely sure you know what you are doing, add the `--unsafe-perm` flag to
this command to force meteor running as root.

=> Stop
# meteor --unsafe-perm

You have run Meteor as root. Your permissions in your app directory will be
incorrect if you ever attempt to perform any Meteor tasks as your non-root
user. You probably didn't want this, but you can fix it by running the
following from the root of your project:

sudo chown -Rh <username> .meteor/local

=> Continues as root
mhidou added 2 commits Sep 28, 2016
@mitar
Copy link
Collaborator

@mitar mitar commented Sep 28, 2016

I do not think we should be too smart here. Just display an error and instructions how to proceed. I think this is much better than trying to guess things. I think using uid/gid is better than relaying on environment variables.

@mhidou
Copy link
Contributor Author

@mhidou mhidou commented Sep 28, 2016

@mitar @benjamn @abernix PR updated

Copy link
Member

@abernix abernix left a comment

Almost there!

};
var platform = process.platform;

if (_.has(unixPlatforms, platform)){

This comment has been minimized.

@abernix

abernix Sep 28, 2016
Member

This entire unixPlatforms check (from here up) can go away and just be replaced with:

if (process.getuid) {

It will only be defined on POSIX systems.

This comment has been minimized.

@mhidou

mhidou Sep 28, 2016
Author Contributor

Fixed


if (_.has(unixPlatforms, platform)){
// On UNIX platforms.
if (process.getgid() === 0 && process.getuid() === 0){

This comment has been minimized.

@abernix

abernix Sep 28, 2016
Member

Please just check process.getuid() === 0. There is no need to check process.getgid() as well since uid/gid is always 0/0 for root.

This comment has been minimized.

@mhidou

mhidou Sep 28, 2016
Author Contributor

Fixed

Console.info("sudo chown -Rh <username> .meteor/local");
Console.info("");
// Fix issue when running `sudo meteor --unsafe-perm`, --unsafe-perm: unknown option.
delete rawOptions['--unsafe-perm'];

This comment has been minimized.

@abernix

abernix Sep 28, 2016
Member

Hmm, this is an interesting behavior. No idea why?

This comment has been minimized.

@mhidou

mhidou Sep 28, 2016
Author Contributor

If the --unsafe-perm flag is not deleted the command emit an error because this option is not in the command's available options.

@benjamn benjamn merged commit 2bc5b4b into meteor:devel Sep 28, 2016
3 checks passed
3 checks passed
CLA Author has signed the Meteor CLA.
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjamn
Copy link
Member

@benjamn benjamn commented Sep 28, 2016

Thanks @mhidou and everyone who provided feedback!

abernix added a commit to abernix/meteor that referenced this pull request Oct 13, 2016
meteor#7821 is a breaking change for anyone running the `meteor` tool as `root` right now.  This moves it higher and explain that it's not just a warning.
benjamn added a commit that referenced this pull request Oct 13, 2016
#7821 is a breaking change for anyone running the `meteor` tool as `root` right now.  This moves it higher and explain that it's not just a warning.
@abernix
Copy link
Member

@abernix abernix commented Oct 25, 2016

Just realized that this has a side effect of not allowing you to run meteor --version or meteor --help as root.

With meteor --version you can't even run it with --unsafe-perm, but with meteor --help you can. The --help isn't hard to quick-fix, but --version could potentially initiate a meteor-tool download which might cause file permission problems? Not sure? Maybe not?

It's possible that this will be purely an annoyance, but thought I'd put it out there/talk out loud.

@mitar
Copy link
Collaborator

@mitar mitar commented Oct 25, 2016

Thanks for reporting.

@rclai
Copy link

@rclai rclai commented Oct 25, 2016

Guys, why am I experiencing this PR when I haven't even upgraded Meteor? #7959

@benjamn
Copy link
Member

@benjamn benjamn commented Oct 25, 2016

@rclai The tools/cli/main.js code runs before the code that examines .meteor/release in order to switch to the app-specific Meteor version, so this logic applies any time you run meteor, regardless of .meteor/release.

Running Meteor as root has been problematic for a while (at least back to Meteor 1.3), so this warning is valid for other versions, if annoying. The fact that --unsafe-perm doesn't work is a bug, though.

I believe you can get around the warning by pointing the ~/.meteor/meteor symlink to a different version, e.g.:

cd ~/.meteor
rm meteor
ln -s packages/meteor-tool/1.4.1_3/mt-os.*/meteor ./
@skirunman
Copy link
Contributor

@skirunman skirunman commented Oct 25, 2016

Yes, this broke our CI script for Bitbucket Pipelines, which runs in a Docker image as root, and what we use to deploy to Galaxy. We currently run meteor --version as well as meteor npm --version, meteor node --version and meteor add some-package.

Will the meteor npm commands work with --unsafe-perm flag? I assume yes, but have not had time to test.

What about ability to run meteor add some-package?

@benjamn
Copy link
Member

@benjamn benjamn commented Oct 25, 2016

Yes, for what it's worth, meteor npm and meteor node will work as root, because that logic comes before tools/cli/main.js.

@skirunman
Copy link
Contributor

@skirunman skirunman commented Oct 25, 2016

@benjamn Thanks for the quick response.

I guess I'll change our script and wait for a fix to allow running other meteor commands with --unsafe-flag. Also need a fix for #7956

EDIT: I just tested and passing --unsafe-perm flag obviously does not depress the warning, but the commands worked for meteor --version and meteor add some-package in one case and not in another.

meteor --version --unsafe-perm

You have run Meteor as root. Your permissions in your app directory will be
incorrect if you ever attempt to perform any Meteor tasks as your non-root
user. You probably didn't want this, but you can fix it by running the
following from the root of your project:

sudo chown -Rh <username> .meteor/local

Meteor 1.4.2

and

meteor --version --unsafe-perm

You have run Meteor as root. Your permissions in your app directory will be
incorrect if you ever attempt to perform any Meteor tasks as your non-root
user. You probably didn't want this, but you can fix it by running the
following from the root of your project:

sudo chown -Rh <username> .meteor/local

  (  Downloading meteor-tool@1.4.1_3...        ... )
  (  Extracting meteor-tool@1.4.1_3...         ... )
  (  Loading meteor-tool@1.4.1_3...            ... )

Can't pass anything else along with --version.

I think the reason is we are deploying two apps and in the first case the app was updated to 1.4.2 and in the second app which is still at 1.4.1.3 the command failed as it was downloading 1.4.1.3 meteor tool and failed.

@mitar
Copy link
Collaborator

@mitar mitar commented Oct 26, 2016

I think we should just make sure that --unsafe-perm works for all and any command/argument.

@abernix
Copy link
Member

@abernix abernix commented Oct 26, 2016

@mitar Agree and actually, that might actually be the case already (--version and --help being the only exceptions as I outlined above) but I think the issue here has to do with running old projects since this unsafe-perm check happens before the springboard to the older version.

If you're using Meteor 1.4.2 tool on a Meteor 1.4.2 project, everything will work fine. If you're not running Meteor as root at all, this problem doesn't affect you. But I believe this is what is happening right now if you run the 1.4.2 meteor command on a project that isn't 1.4.2, as root.

  • The 1.4.2 meteor command runs the 1.4.2 meteor-tool's tools/cli/main.js. This is normal, as it does this before it springboards to the correct version (and downloads it if necessary).
  • tools/cli/main.js begins --unsafe-perm check...
    • If --unsafe-perm is passed, execution continues (unless you're using --version, per the bug I mentioned above)
      • BUT, this is a problem when we springboard to the older meteor-tool since it won't know how to deal with the --unsafe-perm flag and will fail for a another reason.
    • If --unsafe-perm is not passed, Meteor process.exit's to prevent the permission debacle.
      • This is a problem because, the older meteor-tool won't even be spring-boarded (since we're already exited).

As a work-around for now, if you're using Docker and building a pre-1.4.2 project, you should disable release checking and change your Meteor install to use the previous version of the Meteor tool (and ideally, whatever version of Meteor that you're using):

export METEOR_NO_RELEASE_CHECK=true # make sure this is set in your environment.
curl https://install.meteor.com/?release=1.4.1.3 | sh # You can also use the exact version you're deploying here.

This will have the added benefit of executing much more quickly and avoiding download of a tool that you're not going to use anyway, so you should probably be doing this in a Docker env anyway.

I will try to take a look at this today.

@abernix
Copy link
Member

@abernix abernix commented Oct 26, 2016

Upon thinking about this some more, it would seem the primary issue is the --unsafe-perm getting passed to the springboarded version of Meteor. So I might just propose the commit I just made here abernix@a38be16 if anyone has thoughts on it.

I'll drop that as a PR if nobody has any objections and I'm going to try to run it through some of my own CI builds right now.

benjamn added a commit that referenced this pull request Oct 27, 2016
This is important for `meteor npm`, since we don't parse or pass through
Meteor-specific command-line arguments when running `meteor npm`.

When $METEOR_UNSAFE_PERM is set, its value is now propagated to any npm
commands via the $NPM_CONFIG_UNSAFE_PERM variable.

Helps with #7959.
Follow-up to #7821.
benjamn added a commit that referenced this pull request Oct 27, 2016
This is important for `meteor npm`, since we don't parse or pass through
Meteor-specific command-line arguments when running `meteor npm`.

When METEOR_UNSAFE_PERM is set, its value is now propagated to any npm
commands via the NPM_CONFIG_UNSAFE_PERM variable.

Helps with #7959.
Follow-up to #7821.
benjamn added a commit that referenced this pull request Oct 27, 2016
This is important for `meteor npm`, since we don't parse or pass through
Meteor-specific command-line arguments when running `meteor npm`.

When METEOR_UNSAFE_PERM is set, its value is now propagated to any npm
commands via the NPM_CONFIG_UNSAFE_PERM variable.

Helps with #7959.
Follow-up to #7821.
benjamn added a commit that referenced this pull request Oct 27, 2016
This is important for `meteor npm`, since we don't parse or pass through
Meteor-specific command-line arguments when running `meteor npm`.

When METEOR_UNSAFE_PERM is set, its value is now propagated to any npm
commands via the NPM_CONFIG_UNSAFE_PERM variable.

Helps with #7959.
Follow-up to #7821.
benjamn added a commit that referenced this pull request Oct 28, 2016
This is important for `meteor npm`, since we don't parse or pass through
Meteor-specific command-line arguments when running `meteor npm`.

When METEOR_UNSAFE_PERM is set, its value is now propagated to any npm
commands via the NPM_CONFIG_UNSAFE_PERM variable.

Helps with #7959.
Follow-up to #7821.
benjamn added a commit that referenced this pull request Oct 31, 2016
By removing the `--allow-superuser` and `--unsafe-perm` arguments before springboarding, it prevents older versions of Meteor from objecting to its use.  Catching it at the highest level should be sufficient to discourage root usage as intended in #7821

Intended to fix...  #7959
@aldo235
Copy link

@aldo235 aldo235 commented Nov 9, 2016

its stil happend to me , some one have a sollution?

@abernix
Copy link
Member

@abernix abernix commented Nov 10, 2016

@aldo235 Let's discuss in the issue (#7959), not in the pull request. Thanks.

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.