Use the root user for app installation #188

Psycojoker · 2016-11-24T19:08:37Z

Hello,

I'm opening this PR to avoid forgetting it but I'm not calling for a decision yet.

It's the implementation of "don't use admin user for installing apps because it doesn't brings anything and that forgetting sudo is the first bug ever while developping apps", also will greatly reduce broken situation du to missing admin user (well ... hide them).

We'll probably need to talk again about this choice.

Psycojoker · 2016-12-02T22:06:35Z

I'm putting this as a medium decision because this is going to affect application installation (but exception for permissions stuff I don't see what it can really change, but I'm expecting you to find other situations)

Psycojoker · 2016-12-11T11:36:30Z

Up. I'd least I'd like to have your opinion on the principle of moving to root for app installation.

@maniackcrudelis @scith I'd like your opinion on the principle of this change since you are in the apps team.

scith · 2016-12-11T16:03:34Z

Hi, how may it affect app installation? Root has more privileges than admin, and we already use sudo most of the times.
Would there be an alternative to using root, while still taking into account the fact that many apps require sudo rights at least for install? If not, I agree with this

Psycojoker · 2016-12-11T19:25:20Z

Hi, how may it affect app installation? Root has more privileges than admin, and we already use sudo most of the times.

You won't have to put sudo everywhere from now on, which was (for the people I've talked to) the most common error they got when making applications.

Would there be an alternative to using root, while still taking into account the fact that many apps require sudo rights at least for install? If not, I agree with this

You can still uses su $user -c when needed :)

Since admin has sudo right without a password it was basically having root access while having to put sudo everywhere, it's was complicating things more than anything else (and other system wide package managers uses root too).

maniackcrudelis · 2016-12-11T20:41:13Z

It's ok for me, I think isn't a problem for packages, because the permissions are set when it's necessary.

julienmalik · 2016-12-12T11:15:12Z

src/yunohost/hook.py

@@ -330,7 +331,11 @@ def hook_exec(path, args=None, raise_on_error=False, no_trace=False,
        cmd_script = path

    # Construct command to execute
-    command = ['sudo', '-n', '-u', 'admin', '-H', 'sh', '-c']
+    if user == "root":
+        command = ['sh', '-c']


This means we always consider this python code is executed as root. Shouldn't we add a sudo here just in case ?

YunoHost can't be runned as another user than root.

See https://github.com/YunoHost/yunohost/blob/unstable/bin/yunohost#L188

julienmalik · 2016-12-12T11:19:08Z

Is it intentionnal that you handle install/upgrade/remove, and not backup/restore ?

Psycojoker · 2016-12-12T11:21:03Z

No, that's a mistake.

mbugeia · 2016-12-12T11:22:44Z

While I'm not really against this (after all apt use root for install), I have to disagree with the first statement of this PR:
"don't use admin user for installing apps because it doesn't brings anything" -> It does bring something: consciousness that you're doing an action that can break things and. And it respect the principle of least privilege.
"forgetting sudo is the first bug ever while developping apps" -> I disagree, this is the easiest problem to solve when developping apps.
"will greatly reduce broken situation du to missing admin user (well ... hide them)." -> IMO hiding a bug just makes things worst.

alexAubin · 2016-12-12T13:12:51Z

Same as @mbugeia, though not against it either (c.f. apt).

M5oul

Works fine.
I installed an app without sudo prefixs.
I removed it with sudo prefixs.

zamentur · 2017-01-07T08:37:22Z

I think hooks are missing in this PR.

There is pro and cons with this idea to run hook script with user root.

About backup, it may be a problem if there is no backup because the user admin (so the nscd, nslcd or slapd) is not available.

At the same time, I'm agree too with the args from mbugeia about hiding a bug. But, an error during the app setup is not the good moment for warning the administrator about a missing admin user... About this bug, we should check that very earlier.

May be this PR could break some app package. Some program need to be run as a non root user so currently (without this pr) if you run in a script a program like this it works, but with this pr it could raise an error like "This programm need to be run as a non root user".

Psycojoker · 2017-02-03T13:58:28Z

I think hooks are missing in this PR.

Going to add it then.

There is pro and cons with this idea to run hook script with user root.

I have no opinion on this one, I can easily add it if you want.

About backup, it may be a problem if there is no backup because the user admin (so the nscd, nslcd or slapd) is not available.

Therefor it would be better to add it then? Making all app scripts running with root seems more coherent.

polytan02 · 2017-02-27T00:46:46Z

Je voulais dire que je suis d'accord avec le root pour une raison toute simple : on finit par mettre sudo partout.
Pour moi sudo est utile quand on est en console et qu'on a besoin de lancer une ou deux commandes et qu'on ne peut/veut pas se connecter en root. Là on fait des scripts où tout doit être exécuté en root sinon ça ne marche pas.

Par contre, je suis d'accord avec @mbugeia sur un point inquiétant : certains qui font des apps ne se rendent pas compte des commandes qu'ils lancent ni du pouvoir qu'elles ont sur le systèmes (on parlait de rm -rf malheureux par exemple dans un script remove).

C'est aussi pour ça que je souhaitais pouvoir afficher le readme.md de l'app avant de l'installer.

Autre question : comment faire pour cocher "changes approved" ?

… user

Psycojoker · 2017-03-05T15:44:55Z

Work is ready to be rewied. Hooks run using root now and I've fixed the backup.py where root wasn't specified.

Autre question : comment faire pour cocher "changes approved" ?

You need to be a member of the core dev team for that.

Psycojoker · 2017-03-19T23:58:54Z

We need one or two more approval to merge this PR (I always wonder why so few people have reviewed it :/)

alexAubin · 2017-03-20T00:13:20Z

From my point of view, I don't know what to think about the purpose of PR (not trying to say it's bad, I genuinely don't know what to think). On one hand, we really do use sudo most of the time in app scripts and it's annoying to have to loose time because of forgetting to put "sudo" in install script ;). On the other hand, I agree with @mbugeia on the principle of least privilege. But if apt does the same thing, well, okay.

However what I'm worried about at this point is wether or not this breaks some apps. As @zamentur said, some apps or specific program need to be run as a non-root user. What's the plan to test apps ? Imho we can rely on the App CI if it runs on testing/unstable. But we should be careful and not release this too quickly.

Psycojoker · 2017-03-20T01:05:22Z

01:58  <Aleks__> bah après sur le principe ok, apt installe en root
01:58  <Aleks__> la question imho c'est surtout est-ce que le gain "plus avoir à faire sudo" est supérieur à tous les 
                 risques qu'on encoure en changeant un truc critique comme ça :/
01:58  <Aleks__> et notamment casser les apps
01:59  <Aleks__> mais tant qu'on teste bien, why not
01:59 @<Bram> niveau casser les apps le risque est quand même fort bas ici
01:59  <Aleks__> ah, oké
01:59 @<Bram> et niveau avantage ça rend le dev d'app bien moins pénible
02:00 @<Bram> et je pense que faciliter le boulot des développeurs d'apps est quand même un très gros +
02:00 @<Bram> après oui, on va pas merger/déployer ça en mode swag du slip
02:00  <Aleks__> après j'suis biaisé par la dernière app que j'ai voulu packager (wekan) parce que meteor voulait pas etre 
                 root /o\
02:00 @<Bram> sinon
02:00 @<Bram> pour rigoler https://github.com/search?q=org%3AYunoHost-Apps+sudo&type=Code
02:01  <Aleks__> 655
02:01  <Aleks__> xD
02:01 @<Bram> et c'est 655 *fichiers*, pas 655 sudo :p
02:01 @<Bram> this is madness ™
02:01  <Aleks__> ogod
02:01 @<Bram> https://github.com/search?q=org%3AYunoHost-Apps+sudo&type=Commits
02:01 @<Bram> celui là aussi est super marrant comme résultat

And to insist, just look at any random script file of a YunoHost app, sudo is used for 80% of the lines and the % goes higher if you remove the variable declarations lines.

At this point it's just making the life of every package developper harder for nothing.

Psycojoker · 2017-03-20T01:19:27Z

After having read a lot of random app scripts (and having made apps myself), I'm now really convinced that keeping sudo and not switching to root is ever a terrible bad design decision or just voluntary making the life of app developers painful.

polytan02

I don't have the knowledge to check the python code, but I agree with the fact we run root and stop using sudo.

Psycojoker · 2017-03-20T17:47:21Z

@M5oul @maniackcrudelis already agreed on that PR apparently:

It's ok for me, I think isn't a problem for packages, because the permissions are set when it's necessary.

M5oul changed the title ~~Use the root user for installation~~ Use the root user for app installation Nov 24, 2016

Psycojoker force-pushed the install_as_root branch from 0d0214c to ab75319 Compare November 25, 2016 13:53

M5oul mentioned this pull request Nov 25, 2016

Remove 'sudo' check YunoHost/package_linter#5

Closed

Psycojoker added the medium/standard decision label Dec 2, 2016

M5oul added this to the 2.5.x milestone Dec 11, 2016

julienmalik reviewed Dec 12, 2016

View reviewed changes

M5oul approved these changes Dec 14, 2016

View reviewed changes

Psycojoker modified the milestones: 2.6.x, 2.5.x Dec 15, 2016

Psycojoker force-pushed the install_as_root branch from a1f70a9 to 7ddb35e Compare March 5, 2017 14:56

Psycojoker added 6 commits March 5, 2017 16:41

[enh] defaulting running hook_exec as root

b0730d8

[mod] change behavior, admin by default, as to explicitly set root as…

33e101b

… user

[enh] use root for app related hook_exec

8794178

[mod] remove unused import

0122cbd

[fix] run missing backup scripts as root

e1a5074

[enh] run hooks as root

76a7a36

Psycojoker force-pushed the install_as_root branch from 8daa084 to 76a7a36 Compare March 5, 2017 15:42

M5oul added the opinion needed label Mar 7, 2017

polytan02 approved these changes Mar 20, 2017

View reviewed changes

M5oul requested a review from maniackcrudelis March 20, 2017 07:39

maniackcrudelis approved these changes Mar 20, 2017

View reviewed changes

JimboJoe approved these changes Mar 20, 2017

View reviewed changes

M5oul removed the opinion needed label Mar 20, 2017

M5oul merged commit d00ed6c into unstable Mar 20, 2017

M5oul deleted the install_as_root branch March 20, 2017 20:17

yunohost-bot mentioned this pull request May 24, 2018

run app scripts and others (hooks) as root instead of admin YunoHost/issues#763

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the root user for app installation #188

Use the root user for app installation #188

Psycojoker commented Nov 24, 2016

Psycojoker commented Dec 2, 2016 •

edited by M5oul

Psycojoker commented Dec 11, 2016

scith commented Dec 11, 2016

Psycojoker commented Dec 11, 2016

maniackcrudelis commented Dec 11, 2016

julienmalik Dec 12, 2016

Psycojoker Dec 12, 2016

Psycojoker Dec 12, 2016

julienmalik commented Dec 12, 2016

Psycojoker commented Dec 12, 2016

mbugeia commented Dec 12, 2016

alexAubin commented Dec 12, 2016

M5oul left a comment

zamentur commented Jan 7, 2017

Psycojoker commented Feb 3, 2017 •

edited

polytan02 commented Feb 27, 2017

Psycojoker commented Mar 5, 2017 •

edited

Psycojoker commented Mar 19, 2017

alexAubin commented Mar 20, 2017

Psycojoker commented Mar 20, 2017 •

edited

Psycojoker commented Mar 20, 2017

polytan02 left a comment

Psycojoker commented Mar 20, 2017

Use the root user for app installation #188

Use the root user for app installation #188

Conversation

Psycojoker commented Nov 24, 2016

Psycojoker commented Dec 2, 2016 • edited by M5oul

Psycojoker commented Dec 11, 2016

scith commented Dec 11, 2016

Psycojoker commented Dec 11, 2016

maniackcrudelis commented Dec 11, 2016

julienmalik Dec 12, 2016

Choose a reason for hiding this comment

Psycojoker Dec 12, 2016

Choose a reason for hiding this comment

Psycojoker Dec 12, 2016

Choose a reason for hiding this comment

julienmalik commented Dec 12, 2016

Psycojoker commented Dec 12, 2016

mbugeia commented Dec 12, 2016

alexAubin commented Dec 12, 2016

M5oul left a comment

Choose a reason for hiding this comment

zamentur commented Jan 7, 2017

Psycojoker commented Feb 3, 2017 • edited

polytan02 commented Feb 27, 2017

Psycojoker commented Mar 5, 2017 • edited

Psycojoker commented Mar 19, 2017

alexAubin commented Mar 20, 2017

Psycojoker commented Mar 20, 2017 • edited

Psycojoker commented Mar 20, 2017

polytan02 left a comment

Choose a reason for hiding this comment

Psycojoker commented Mar 20, 2017

Psycojoker commented Dec 2, 2016 •

edited by M5oul

Psycojoker commented Feb 3, 2017 •

edited

Psycojoker commented Mar 5, 2017 •

edited

Psycojoker commented Mar 20, 2017 •

edited