Skip to content

Fix user checking#61

Merged
aalexfvk merged 2 commits intomainfrom
MDB-25061_fix_user_checking
Sep 4, 2023
Merged

Fix user checking#61
aalexfvk merged 2 commits intomainfrom
MDB-25061_fix_user_checking

Conversation

@aalexfvk
Copy link
Contributor

@aalexfvk aalexfvk commented Sep 1, 2023

After running highstate on CH host via pssh-sh state running /usr/local/yandex/ch_wait_started.py hangs up to timeout.
Because such a command does not work

sudo salt-call cmd.run "timeout 5 sudo -u monitor /usr/bin/ch-monitoring ping"
[ERROR   ] Command 'timeout' failed with return code: 1
[ERROR   ] stdout: Wrong current user: root
[ERROR   ] retcode: 1
[ERROR   ] Command 'timeout' failed with return code: 1
[ERROR   ] output: Wrong current user: root

Or without salt-call

sudo timeout 5 sudo -u monitor /usr/bin/ch-monitoring ping
Wrong current user: root

The point is that getpass.getuser() function relies on environment variables (LOGNAME, USER, etc) but nested sudo does not update it:

$ sudo -u monitor printenv USER
monitor
$ sudo sudo -u monitor printenv USER
root
$ sudo -u alexfvk sudo -u monitor printenv USER
alexfvk

I propose to extract name for effective uid and not to rely on environment variables.
After fix:

$ timeout 5 sudo /usr/bin/ch-monitoring ping
0;OK
$ timeout 5 sudo -u monitor /usr/bin/ch-monitoring ping
0;OK
$ sudo timeout 5 sudo -u monitor /usr/bin/ch-monitoring ping
0;OK
$ sudo -u alexfvk timeout 5 sudo -u monitor /usr/bin/ch-monitoring ping
0;OK
$ sudo -u alexfvk timeout 5 sudo /usr/bin/ch-monitoring ping
0;OK
$ sudo -u alexfvk timeout 5 /usr/bin/ch-monitoring ping
Wrong current user: alexfvk
$ timeout 5 /usr/bin/ch-monitoring ping
Wrong current user: alexfvk

@aalexfvk aalexfvk requested a review from ianton-ru September 1, 2023 16:08
@ianton-ru
Copy link
Contributor

If I remember correct the reason was that when ch-monitoring starter from root, it creates some artifacts like log files with owner root:root. And later when it starts from user monitor it fails with access denied error. So sudo /usr/bin/ch-monitoring ping must stops with Wrong current user: root.

@ianton-ru
Copy link
Contributor

PS. May be behavior was changed since that time and now it works fine from any user, need to check, and if answer is yes, remove user check.

@aalexfvk
Copy link
Contributor Author

aalexfvk commented Sep 4, 2023

If I remember correct the reason was that when ch-monitoring starter from root, it creates some artifacts like log files with owner root:root. And later when it starts from user monitor it fails with access denied error. So sudo /usr/bin/ch-monitoring ping must stops with Wrong current user: root.

sudo /usr/bin/ch-monitoring ping should work because in this case an effective user is set here

pw = pwd.getpwnam(DEFAULT_USER)
if os.path.isfile(LOG_FILE):
os.chown(LOG_FILE, pw.pw_uid, pw.pw_gid)
groups = os.getgrouplist(DEFAULT_USER, pw.pw_gid)
os.setgroups(groups)
os.setgid(pw.pw_gid)
os.setegid(pw.pw_gid)
os.setuid(pw.pw_uid)
os.seteuid(pw.pw_uid)

We shouldn't see Wrong current user: root at all.

Moreover in the case I have fixed we see Wrong current user: root when effective user is monitor actually as I showed in the examples.

@aalexfvk
Copy link
Contributor Author

aalexfvk commented Sep 4, 2023

PS. May be behavior was changed since that time and now it works fine from any user, need to check, and if answer is yes, remove user check.

The behaviour hasn't been changed. If we eliminate the code for user checking it still raises an exception after starting under root previosly.
I propose to fix the current approach because it can lead to hanging to a couple of hours highstate now.

@aalexfvk aalexfvk requested a review from Alex-Burmak September 4, 2023 10:14
@aalexfvk aalexfvk merged commit 2c1d593 into main Sep 4, 2023
@aalexfvk aalexfvk deleted the MDB-25061_fix_user_checking branch September 4, 2023 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants