[Freeipa-devel] [PATCH] 0057 Only allow root to run update plugins

[Rob Crittenden]

Petr Viktorin wrote:
> On 06/05/2012 06:53 PM, Petr Viktorin wrote:
>> On 06/05/2012 04:18 PM, Rob Crittenden wrote:
>>> Petr Viktorin wrote:
>>>> On 06/05/2012 03:00 PM, Rob Crittenden wrote:
>>>>> Petr Viktorin wrote:
>>>>>> On 06/05/2012 10:06 AM, Martin Kosek wrote:
>>>>>>> On Mon, 2012-06-04 at 11:51 -0400, Simo Sorce wrote:
>>>>>>>> On Mon, 2012-06-04 at 17:22 +0200, Petr Viktorin wrote:
>>>>>>>>> An update plugin needed root privileges, and aborted the update
>>>>>>>>> if an
>>>>>>>>> ordinary user user ran it.
>>>>>>>>> With this patch the plugin is skipped with a warning in that case.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/2621
>>>>>>>>
>>>>>>>> Hi Petr,
>>>>>>>> I am not sure I like the proposed solution.
>>>>>>>>
>>>>>>>> If there is a legitimate reason to run this plugin as non-root (eg
>>>>>>>> admin
>>>>>>>> user) then you should change the connection part to try to use
>>>>>>>> GSSAPI
>>>>>>>> auth over ldap when non-root, not just throw a warning.
>>>>>>>>
>>>>>>>> If there is no reason for anyone but root to run this script
>>>>>>>> then we
>>>>>>>> should just abort if not root IMO.
>>>>>>>>
>>>>>>>> Simo.
>>>>>>>>
>>>>>>>
>>>>>>> I would keep this script runable for root users only. Regularly,
>>>>>>> this
>>>>>>> should not be run manually but as a part of RPM update which is
>>>>>>> done by
>>>>>>> root. It is being run manually only when something is broken anyway
>>>>>>> and
>>>>>>> I am not convinced that non-root users should be involved in such
>>>>>>> recovery.
>>>>>>>
>>>>>>> Martin
>>>>>>>
>>>>>>
>>>>>> Thanks for the advice. The attached patch only allows root to run
>>>>>> ipa-ldap-updater.
>>>>>
>>>>> NACK. It is very handy for developers to be able to run
>>>>> ipa-ldap-updater
>>>>> to test update files.
>>>>>
>>>>> rob
>>>>
>>>> Developers can run it as root, I don't see a problem here.
>>>
>>> I'd really rather not. This does nothing requiring root permissions,
>>> it's all done over LDAP. I'd rather trade not running some plugins than
>>> always requiring root.
>>>
>>> rob
>>>
>>
>> Thanks for info on how the tool is used. I looked into it deeper.
>> The proper fix would be to use the ldap2 backend here, instead of the
>> IPAdmin. That's ticket 2660, and it'll be quite a lot of work to get
>> ReplicationManager and tools that depend on that ported.
>>
>>
>> But, I think it makes sense to require root if (and only if) plugins are
>> run. Justification below. Would that work for your use case?
>>
>>
>> There are currently three modes ipa-ldap-updater can run in:
>> 1) --upgrade (needs root, runs plugins)
>> 2) no --upgrade, either no files specified or --plugins (doesn't need
>> root, runs plugins)
>> 3) no --upgrade, specific files specified without --plugins (doesn't
>> need root, doesn't run plugins)
>>
>> I propose to make mode 2 require root.
>>
>> There are two major uses of the script: install/upgrade (first two
>> modes), and a developer testing update files (third or possibly second
>> mode). Install/upgrade is always run as root, and the developer usually
>> doesn't need to run the plugins (if they do, they should run as root
>> anyway, so that some (parts of) plugins aren't skipped).
>>
>> Some of the plugins ask to restart the DS. Without root privileges, the
>> restart (but not the rest of the plugin) is skipped. I think this is
>> just asking for trouble.
>> Some plugins (or parts of plugins) don't need root, but I don't think
>> singling these out and testing both cases is worth the effort.
>>
>>
>
> The attached patch that implements the above. I re-ordered the code a
> bit to put the checks before the DM password prompt, so you don't enter
> the password only to find out you had to use sudo or different options.

I'll try to live with not being able to run plugins as non-root. If it 
turns out to be very painful to develop w/o it I'll open a new ticket.

ACK, pushed to master.

rob