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

Only first user can run provision -h local. Other lack lock file permissions. #2877

Open
lukaszachy opened this issue Apr 19, 2024 · 9 comments · May be fixed by #3252
Open

Only first user can run provision -h local. Other lack lock file permissions. #2877

lukaszachy opened this issue Apr 19, 2024 · 9 comments · May be fixed by #3252
Assignees
Labels
step | provision Stuff related to the provision step
Milestone

Comments

@lukaszachy
Copy link
Collaborator

As a root create two users and execute tmt run for each of them in sequence:

# useradd A
# su -l A
$ tmt init -t mini
$ tmt run -a provision -h local
....
total: 1 test passed

$ logout

# useradd B
# su -l B
$ tmt init -t mini
$ tmt run -a provision -h local
...
warn: Test failed to manage its pidfile.
...
total: 1 error

Debug output shows

            errr /script-00 (pidfile locking)
                output.txt: /var/tmp/tmt/run-007/plans/example/execute/data/guest/default-0/script-00-1/output.txt
        Workdir '/var/tmp/tmt/run-007/plans/example/report/default-0' created.
        Read file '/var/tmp/tmt/run-007/plans/example/execute/data/guest/default-0/script-00-1/output.txt'.
                content: flock: cannot open lock file /var/tmp/tmt-test.pid.lock: Permission denied
@janhavlin janhavlin added this to the 1.34 milestone Apr 22, 2024
@happz happz modified the milestones: 1.34, 1.35 Jun 11, 2024
@psss
Copy link
Collaborator

psss commented Jun 25, 2024

Summary from the discussion: We could make the pidfile writeable by anyone. Looks like a first good issue. Let's see!

@psss psss added good first issue Good for newcomers step | provision Stuff related to the provision step labels Jun 25, 2024
@martinhoyer martinhoyer removed this from the 1.35 milestone Jul 30, 2024
@mcasquer
Copy link
Collaborator

Summary from the discussion: We could make the pidfile writeable by anyone. Looks like a first good issue. Let's see!

@psss I've been thinking about two possible solutions

  1. Make the pidfile creation dependent on the current user, this way permission errors are directly avoided, i.e. creating the file at some place under the user's home directory
  2. Changing the owner of the pidfile (777 permissions don't work 😕) to the current one?, but this would need root permissions so it could be a problem as well

@psss
Copy link
Collaborator

psss commented Sep 24, 2024

Make the pidfile creation dependent on the current user, this way permission errors are directly avoided, i.e. creating the file at some place under the user's home directory

I'm afraid that would break tmt-reboot when called from outside of the test session as we need to have a standard location where to look for the lock:

# Two-level reboot: `tmt-reboot` extracts command line arguments, and
# calls `tmt-reboot-core` *while holding the tmt test pidfile lock!*
# That should assure the pidfile would exist and contain valid info.
[ -n "$TMT_DEBUG" ] && set -x
TMT_TEST_PIDFILE_LOCK="${TMT_TEST_PIDFILE_LOCK:-/var/tmp/tmt-test.pid}"

Changing the owner of the pidfile (777 permissions don't work 😕) to the current one?, but this would need root permissions so it could be a problem as well

Hm, could it be caused by the sticky bit of /var/tmp? So that only owner of the file can get the lock? I quickly tried with a world-writable directory:

mkdir /var/tmp/pid
chmod ugo+rwx /var/tmp/pid
TMT_TEST_PIDFILE_ROOT=/var/tmp/pid tmt run -av provision -h local

This seems to be working. Not sure how safe this approach would be though. @happz, any thoughts on this?

@happz
Copy link
Collaborator

happz commented Sep 25, 2024

The only hard rule is probably the location, pid file must be possible to find from multiple tmt invocations so one can reboot the machine while the other is still running. So any directory on which they can agree should be fine. This most likely rules out $HOME, one instance might be running under root, other under test with sudo, so home directory will be different and therefore permissions will kill it anyway. /var/tmp/pid should work, but I think we had a problem when the pid file was located under /var/tmp/tmt, let me try find it in history. Might be relevant for /var/tmp/pid.

@happz
Copy link
Collaborator

happz commented Sep 25, 2024

"Change the default test pidfile directory to /var/tmp": 7552550

@mcasquer
Copy link
Collaborator

@psss @happz I'll try to be more specific, likely I am missing something obvious

Including the following code at internal.py creates once the desired path and gives it full permissions:

pid_directory = '/var/tmp/pid'
Path(pid_directory).mkdir(parents=True, exist_ok=True)
os.chmod(pid_directory, 0o777)
TEST_PIDFILE_ROOT = Path(pid_directory)  # noqa: S108 insecure usage of temporary dir

After tmt run ... the pidfile is successfully created

total: 1 test passed
$ ls -lrsta /var/tmp/pid
total 4
4 drwxrwxrwt. 12 root root 4096 Sep 26 04:43 ..
0 -rw-r--r--   1 root root    0 Sep 26 04:43 tmt-test.pid.lock
0 drwxrwxrwx   2 root root   31 Sep 26 04:43 .

But if It try with a different user

$ tmt run -a provision -h local
Error: failed while importing tmt package: [Errno 1] Operation not permitted: '/var/tmp/pid'

For sure, I can access the directory, so that's why I thought about the owner issue

@happz
Copy link
Collaborator

happz commented Sep 26, 2024

@psss @happz I'll try to be more specific, likely I am missing something obvious

Including the following code at internal.py creates once the desired path and gives it full permissions:

pid_directory = '/var/tmp/pid'
Path(pid_directory).mkdir(parents=True, exist_ok=True)
os.chmod(pid_directory, 0o777)
TEST_PIDFILE_ROOT = Path(pid_directory)  # noqa: S108 insecure usage of temporary dir

After tmt run ... the pidfile is successfully created

total: 1 test passed
$ ls -lrsta /var/tmp/pid
total 4
4 drwxrwxrwt. 12 root root 4096 Sep 26 04:43 ..
0 -rw-r--r--   1 root root    0 Sep 26 04:43 tmt-test.pid.lock
0 drwxrwxrwx   2 root root   31 Sep 26 04:43 .

But if It try with a different user

$ tmt run -a provision -h local
Error: failed while importing tmt package: [Errno 1] Operation not permitted: '/var/tmp/pid'

For sure, I can access the directory, so that's why I thought about the owner issue

Can you run it with TMT_SHOW_TRACEBACK=1 envvar set? My guess: directory exists, but it's owned by root, your user is not root and tries to chmod of a directory owned by root. OS will not let that happen.

@mcasquer
Copy link
Collaborator

Can you run it with TMT_SHOW_TRACEBACK=1 envvar set? My guess: directory exists, but it's owned by root, your user is not root and tries to chmod of a directory owned by root. OS will not let that happen.

@happz thanks ! Indeed, if only the chmod is done when the user matches to the file owner the issue is solved, I will send a PR for this issue :)

@psss psss linked a pull request Oct 31, 2024 that will close this issue
6 tasks
@psss psss added this to the 1.39 milestone Oct 31, 2024
@mcasquer mcasquer linked a pull request Oct 31, 2024 that will close this issue
6 tasks
@psss psss modified the milestones: 1.39, 1.40 Nov 21, 2024
@psss psss removed the good first issue Good for newcomers label Nov 21, 2024
@psss psss modified the milestones: 1.40, 1.41 Dec 10, 2024
@happz happz modified the milestones: 1.41, 1.42 Jan 13, 2025
@thrix thrix modified the milestones: 1.42, 1.43 Jan 28, 2025
@thrix
Copy link
Collaborator

thrix commented Jan 28, 2025

Moved to 1.43 as the underlying PRs still need more work, and they were already moved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
step | provision Stuff related to the provision step
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants