netz98 / n98-magerun

The swiss army knife for Magento developers, sysadmins and devops. The tool provides a huge set of well tested command line commands which save hours of work time. All commands are extendable by a module API.
http://magerun.net/
Other
1.44k stars 400 forks source link

Show warning "It's not recommended to run n98-magerun as root user" only once in script mode #619

Open amenk opened 9 years ago

amenk commented 9 years ago

Currently the warning is displayed for each line

tkn98 commented 9 years ago

IMHO we could als let it just hard fail and refuse to run at all. Only with an explicit (new) option given and set to "true", it should process. The warning should still be given unless the no-warning option is used (which already exists),.

Background: I've been seeing too many misconfigurations the last week that are caused by scripts run as root (not specifically magerun,), so I'd like to use this report to remind on the principle of least privilege ;).

cmuench commented 9 years ago

This is a feature not a bug. Any user should see the warning for each command if it is called as root.

ktomk commented 9 years ago

shouldn't we be more strict and not only warn but actually prevent to execute the command? when you see the warning and still execute, it might be already too late.

if it's wanted to to run as root, it should be explicitly flagged via an additional parameter.

amenk commented 9 years ago

Not such a bad idea.

cmuench commented 9 years ago

This would break many automatic deployments. If you know what you are doing it's not a problem. Most problems are related two generated cache files with wrong owner or permissions. This is not a problem in every case. The deploy script can change the permissions or the user uses i.e. redis as cache backend. I don't like that we control to much on the users server.

ktomk commented 9 years ago

If you know what you are doing it's not a problem.

In that case it wouldn't hinder you to add the flag to explicitly add a flag to allow root usage. this won't break automatic deployments then.

Most problems are related two generated cache files with wrong owner or permissions.

Jup, and the warning is normally ignored.

Problem is less the automation, problem I see is that users aren't aware until it's too late. So failing early and requiring to either allow root -or- to execute the command under a different user looks like a good solution to that problem.

I don't like that we control to much on the users server.

I don't understand what you want to say by that in the context of warning / accident protection.

The user should not be hindered, e.g. she should be able to continue to use magerun as root if she pleases. I just suggest to make this more explicit and less implicit.

amenk commented 9 years ago

By the way, those caching problems are not only created if a user is root. Also the user might be a different one than the website's user.

In imi-conrun I added a check in cache:flush today to see if the cache really could be cleared. In magerun there is already a check that checks for existence of /tmp/magento which also helps a bit

All are not 100% .. just heuristics.

A bigger warning on the magerun website might also fit.

cmuench commented 9 years ago

Yes, it's a common problem in web environments.

Look at http://symfony.com/doc/current/book/installation.html "Setting up Permissions"

ktomk commented 9 years ago

I suggest to not focus so much on those commons with web environments. I think we're all professional enough to know - as amenk also wrote - that it's impossible make heuristics for all cases.

However I'm pretty sure that when you execute magerun as root there are only two use cases:

For the first case, I'd say the warning was a good first step, but it's not enough as it does not prevent the accident.

For the second case, the warning most likely is superfluous.

That's why I suggest to not execute a command by default if root but to allow all commands if you're root and you signalled to magerun that it's your intention to run magerun as root. In that case, the warning could be removed by default and instead an informal message could be displayed, that the program runs as root. In that case it would be enough to display this once per session (IMHO). A deployment script for example would then automatically document what has been run as root.

If you see more weight that extra-adding a commandline option for root allowance would cripple standard usage, one would need to discuss that.

cmuench commented 9 years ago

Yes, i propose to close this "issue". If you deploy as root you should know what you do. Then disable the root warning by config on the target server. If you run the tool as root user during development it's a good hint if a problem occures.

ktomk commented 9 years ago

Hmm, there still remains the problem that the warning is displayed when it's too late.

And it's not always that the person using magerun has actually set it up on that host. As I wrote earlier, I see little ground to discuss on automated deployments, it's more about the accidents.

n3o77 commented 9 years ago

If root execution would be prevented that could make problems with the self-update when installed in /usr/local/bin . Only time when I see this message.

ktomk commented 9 years ago

@n3o77;: It would not be prevented.

ktomk commented 8 years ago
pbogut commented 6 years ago

Couldn't that be at least sent to stderr instead of stdout? --format=json is a bit usless if there is additional information printed.