[Freeipa-devel] [PATCH 0075] Change group ownership of CRL publish directory

[Martin Kosek]

On 06/27/2013 10:20 AM, Martin Kosek wrote:
> On 06/21/2013 02:18 PM, Tomas Babej wrote:
>> On 06/21/2013 02:15 PM, Martin Kosek wrote:
>>> On 06/21/2013 02:11 PM, Tomas Babej wrote:
>>>> On 06/20/2013 06:00 PM, Simo Sorce wrote:
>>>>> On Thu, 2013-06-20 at 17:47 +0200, Martin Kosek wrote:
>>>>>> On 06/20/2013 05:44 PM, Simo Sorce wrote:
>>>>>>> On Thu, 2013-06-20 at 17:33 +0200, Martin Kosek wrote:
>>>>>>>> On 06/20/2013 05:15 PM, Tomas Babej wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Spec file modified so that /var/lib/ipa/pki-ca/publish/ is owned
>>>>>>>>> by pkiuser group.
>>>>>>>>>
>>>>>>>>> https://fedorahosted.org/freeipa/ticket/3727
>>>>>>>>>
>>>>>>>>> Tomas
>>>>>>>>>
>>>>>>>> NACK. This won't fly. pkiuser is created by FreeIPA when server is
>>>>>>>> installed,
>>>>>>>> thus you cannot just simply change ownership in our spec file because in
>>>>>>>> the
>>>>>>>> time when package is installed or updated, pkiuser may not exist.
>>>>>>>>
>>>>>>>> I think you need to delete the %attr from spec file and set the correct
>>>>>>>> ownership during ipa-{server,ca}-install. When CA is configured, we should
>>>>>>>> also
>>>>>>>> probably let ipa-upgradeconfig check this directory and amend when
>>>>>>>> necessary
>>>>>>>> (to fix affected IPA CA instances).
>>>>>>> Probably even better to not create the directory via rpm at all, but
>>>>>>> make ipa-ca-install create it and remove it when --uninstall is run.
>>>>>>>
>>>>>>> Simo.
>>>>>> This could also work, sure. Could we then at least mark this directory in our
>>>>>> spec file as %ghost? So that "rpm -qf /var/lib/ipa/pki-ca/publish/" gives
>>>>>> some
>>>>>> information?
>>>>> I guess so.
>>>>>
>>>>> Simo.
>>>>>
>>>> Updated version attached.
>>>>
>>>> Tomas
>>> Looks good by reading (I did not test it), maybe just one nitpick:
>>>
>>> +                root_logger.warning("Error while removing CRL publish "
>>> +                                    "directory: %s" % str(e))
>>>
>>> This should read:
>>> +                root_logger.warning("Error while removing CRL publish "
>>> +                                    "directory: %s", e)
>>>
>>> We do not need to format the string before it is really logged and we also do
>>> not need to convert it to "str" as we already requested the conversion to
>>> string by "%s".
>>>
>>> Martin
>> Fixed.
>>
>> Tomas
> 
> The patch itself works fine, but there are still SELinux related questions and
> concerns which may also affect the patch (currently it does not work with
> enforced SELinux).
> 
> I posted them to the relevant Bugzilla:
> https://bugzilla.redhat.com/show_bug.cgi?id=976308
> 
> Martin
> 

I decided not to wait for SELinux bugs to be fixed, the patch will not change
when they are fixed anyway. I will deal with them in another patch.

So ACK, pushed to master, ipa-3-2.

Martin