fractal-analytics-platform / fractal-server

Fractal backend
https://fractal-analytics-platform.github.io/fractal-server/
BSD 3-Clause "New" or "Revised" License
10 stars 3 forks source link

Should we include `sudo --set-home` when impersonating users? #1761

Closed tcompa closed 4 days ago

tcompa commented 5 days ago

(thanks @lorenzocerrone for reporting this, in the context of setting PLANTSEG_HOME)

Within the sudo-slurm runner, we impersonate users as in sudo -u someone /some/python /some/task.py --arguments .... This does not preserve the user $HOME, as visible in

root@somewhere:~# sudo -u someone python3 -c 'import pathlib; print(pathlib.Path.home())'
/root

A possible fix is to use the sudo --set-home flag:

$ man sudo
...
     -H, --set-home
                 Request that the security policy set the HOME environment variable to the home directory specified by the target user's password database entry.  Depending on the policy, this may be the default behavior.
...

which, in my simple test, did work:

root@somewhere:~# sudo -H -u someone python3 -c 'import pathlib; print(pathlib.Path.home())'
/home/someone
tcompa commented 5 days ago

Note: if this works with no drawbacks, then it could also immediately fix (or at least mitigate) #1755

tcompa commented 5 days ago

Side comment: the behavior above was verified with Ubuntu18, but was changed starting from Ubuntu19 (see https://bugs.launchpad.net/ubuntu/+source/sudo/+bug/1556302 or https://askubuntu.com/questions/1186999/how-does-sudo-handle-home-differently-since-19-10).

I verified on Ubuntu22 (in fractal-containers):

root@slurm:/# sudo -u test01 python3 -c 'import pathlib; print(pathlib.Path.home())'
/home/test01

root@slurm:/# sudo --set-home -u test01 python3 -c 'import pathlib; print(pathlib.Path.home())'
/home/test01

root@slurm:/# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 22.04.4 LTS
Release:    22.04
Codename:   jammy

This changes nothing for the existing instance, but it's good to be aware of it in a broader context (e.g. on non-ubuntu machines).

tcompa commented 4 days ago

There is actually no drawback in adding this flag, since we never want to use the fractal-user home folder. I'm merging #1762, and will include it in v2.4.2.