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

[BUG] Unable to manipulate users crontab as a root using cron.absent #62940

Closed
lufik opened this issue Oct 24, 2022 · 7 comments
Closed

[BUG] Unable to manipulate users crontab as a root using cron.absent #62940

lufik opened this issue Oct 24, 2022 · 7 comments
Labels
Bug broken, incorrect, or confusing behavior Execution-Module needs-triage Regression The issue is a bug that breaks functionality known to work in previous releases.

Comments

@lufik
Copy link
Contributor

lufik commented Oct 24, 2022

Description
We use this state:

cron_absent_identifier:
  cron.absent:
    - name: some_name
    - user: our_user
    - minute: 0
    - hour: *
    - daymonth: *
    - month: *
    - dayweek: *
    - comment: some_name
    - commented: False
    - identifier: identifier

After this commit we started getting error:

Cron cron_script.sh for user our_user failed to commit with error You (our_user) are not allowed to use this program (crontab)
See crontab(1) for more information

IMHO salt is running under root so it should edit users the crontab.

Setup
Doesn't matter. The logic problem is in salt/modules/cron.py (function def _write_cron_lines).
The only important part is that the system is not AIX or Solaris.

Steps to Reproduce the behavior
Remove /etc/cron.allow and /etc/cron.deny (do not allow user to modify its crontab).
Use state from description (be sure user is not root). It doesn't matter if cron line exists or not (it's rewritten every time).
Run salt as root with non-root user in cron.absent definition (do not use test mode).

Expected behavior
No error that, run the crontab command as root with parameter -u user.

Versions Report
All versions containing the commit 305f5cf (at least 3000 and later).
It worked as expected in version: 2018.3.4.

Additional context
There is difference in using crontab between write functions in salt/modules/cron.py:

def raw_cron(user):
    '''
    Return the contents of the user's crontab

    CLI Example:

    .. code-block:: bash

        salt '*' cron.raw_cron root
    '''
    # Some OS' do not support specifying user via the `crontab` command
    if __grains__.get('os_family') in ('Solaris', 'AIX'):
        cmd = 'crontab -l'
        # Preserve line endings
        lines = salt.utils.data.decode(__salt__['cmd.run_stdout'](cmd,
                                           runas=user,
                                           ignore_retcode=True,
                                           rstrip=False,
                                           python_shell=False)).splitlines(True)
    # If Salt is running from same user as requested in cron module we don't need any user switch
    elif _check_instance_uid_match(user):
        cmd = 'crontab -l'
        # Preserve line endings
        lines = salt.utils.data.decode(__salt__['cmd.run_stdout'](cmd,
                                           ignore_retcode=True,
                                           rstrip=False,
                                           python_shell=False)).splitlines(True)
    # If Salt is running from root user it could modify any user's crontab
    elif _check_instance_uid_match('root'):
        cmd = 'crontab -u {0} -l'.format(user)
        # Preserve line endings
        lines = salt.utils.data.decode(__salt__['cmd.run_stdout'](cmd,
                                           ignore_retcode=True,
                                           rstrip=False,
                                           python_shell=False)).splitlines(True)
    # Edge cases here, let's try do a runas
    else:
        cmd = 'crontab -l'
        # Preserve line endings
        lines = salt.utils.data.decode(__salt__['cmd.run_stdout'](cmd,
                                           runas=user,
                                           ignore_retcode=True,
                                           rstrip=False,
                                           python_shell=False)).splitlines(True)

    if len(lines) != 0 and lines[0].startswith('# DO NOT EDIT THIS FILE - edit the master and reinstall.'):
        del lines[0:3]
    return ''.join(lines)

versus problematic:

def _write_cron_lines(user, lines):
    '''
    Takes a list of lines to be committed to a user's crontab and writes it
    '''
    lines = [salt.utils.stringutils.to_str(_l) for _l in lines]
    path = salt.utils.files.mkstemp()
    if _check_instance_uid_match('root') or __grains__.get('os_family') in ('Solaris', 'AIX'):
        # In some cases crontab command should be executed as user rather than root
        with salt.utils.files.fpopen(path, 'w+', uid=__salt__['file.user_to_uid'](user), mode=0o600) as fp_:
            fp_.writelines(lines)
        sys.stderr.write("cron module: runas with user = {}\n".format(user))
        ret = __salt__['cmd.run_all'](_get_cron_cmdstr(path),
                                      runas=user,
                                      python_shell=False)
    else:
        with salt.utils.files.fpopen(path, 'w+', mode=0o600) as fp_:
            fp_.writelines(lines)
        sys.stderr.write("cron module: under root with user = {}\n".format(user))
        ret = __salt__['cmd.run_all'](_get_cron_cmdstr(path, user),
                                      python_shell=False)
    sys.stderr.write("cron module: done lines.\n")
    os.remove(path)
    return ret
@lufik lufik added Bug broken, incorrect, or confusing behavior needs-triage labels Oct 24, 2022
@welcome
Copy link

welcome bot commented Oct 24, 2022

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

@OrangeDog
Copy link
Contributor

Pinging @Oloremo as the author.

@OrangeDog OrangeDog added Regression The issue is a bug that breaks functionality known to work in previous releases. Execution-Module labels Oct 24, 2022
@lufik
Copy link
Contributor Author

lufik commented Oct 24, 2022

The pull request just tries to show what I'm talking about.

@Oloremo
Copy link
Contributor

Oloremo commented Oct 24, 2022

It's been a while.

PR above LGTM from a quick glance.

@lufik
Copy link
Contributor Author

lufik commented Oct 27, 2022

Is it possible to backport the patch to older versions (e.g. 3000)? What is the process - What should I do for it?

@lufik
Copy link
Contributor Author

lufik commented Nov 4, 2022

Hi guys, just a gentle reminder. The patch is already merged.
I would like backport of this patch also to salt version 3000. Am I suppose to do something for it?
pinging @OrangeDog

@OrangeDog
Copy link
Contributor

3000 is long out of support. If you want to patch your local installation you will have to do it yourself.

@OrangeDog OrangeDog added this to the Sulphur v3006.0 milestone Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Execution-Module needs-triage Regression The issue is a bug that breaks functionality known to work in previous releases.
Projects
None yet
Development

No branches or pull requests

3 participants