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

Improve permissions on crontabs created by rockstor #2559

Conversation

FroggyFlox
Copy link
Member

Fixes #2556
@phillxnet, @Hooverdan96, ready for review.

We currently do not enforce any specific permissions on the crontabs generated by Rockstor. This can lead to some instances where these are set with a mask of 644, for instance.
This pull request (PR) proposes to enforce a more restrictive mask of 600 at creation of these crontabs.

Note that this PR also proposes to slightly change the logic used for the creation of these files. Indeed, we currently edit/write the file "in-place", at its final destination (for instance, /etc/cron.d/rockstortab). In this PR, we use of potentially safer yet slightly slower (just a couple more calls) process akin to what is used more widely throughout the project:

  1. Create a tempfile
  2. Write the contents as needed
  3. Set permission on this file
  4. Copy to its final destination (/etc/cron.d/rockstortab for instance)

Demonstration

  1. Manually set one of the 2 crontabs in question with different permissions:
rockdev:/opt/rockstor # chmod 644 /etc/cron.d/rockstortab 
rockdev:/opt/rockstor # ls -lah /etc/cron.d/rockstortab
-rw-r--r-- 1 root root 193 May 25 14:56 /etc/cron.d/rockstortab
  1. Make a change to this crontab from the webUI; in this case, I deleted an existing scheduled task
  2. The crontab is re-written with proper persmissions:
rockdev:/opt/rockstor # ls -lah /etc/cron.d/rockstortab
-rw------- 1 root root 124 May 25 15:08 /etc/cron.d/rockstortab

Tests

On a Leap 15.4 VM:

rockdev:/opt/rockstor # cd src/rockstor/ ; poetry run django-admin test ; cd -
Creating test database for alias 'default'...
Creating test database for alias 'smart_manager'...
System check identified no issues (0 silenced).
.............................................................................................................................................................................................................................................................
----------------------------------------------------------------------
Ran 253 tests in 14.082s

OK

Our crontabs are currently created in-place with a 644 mask.
This commit instead writes the crontabs to a tempfile that is then
set with a 600 mask before being moved to its final destination.
@FroggyFlox FroggyFlox linked an issue May 25, 2023 that may be closed by this pull request
@FroggyFlox FroggyFlox changed the title 2556 suboptimal permission on crontabs created by rockstor Improve permissions on crontabs created by rockstor May 25, 2023
Copy link
Member

@phillxnet phillxnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FroggyFlox Thanks for creating and seeing to this issue so quickly. Much appreciated.

Simple test using an rpm build with suggested changes included:

Creating a new scheduled task results in:

ls -la /etc/cron.d/rockstortab
-rw------- 1 root root 192 May 27 16:19 /etc/cron.d/rockstortab

As intended.
Scheduled task concerned executed:

tasks-executed-successfully

May 27 16:20:01 rleap15-3 CRON[1987]: (root) CMD (/opt/rockstor/.venv/bin/st-snapshot 1 \*-*-*-*-*-*)
...
May 27 16:25:01 rleap15-3 CRON[3007]: (root) CMD (/opt/rockstor/.venv/bin/st-snapshot 1 \*-*-*-*-*-*)

Thanks for easing the review process by splitting out the black/string.format changes into a separate commit.

@phillxnet phillxnet merged commit 79ed934 into rockstor:testing May 27, 2023
@FroggyFlox FroggyFlox deleted the 2556_suboptimal_permission_on_crontabs_created_by_Rockstor branch May 27, 2023 15:37
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.

(t) Suboptimal permissions on crontabs created by Rockstor
2 participants