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

[Martin Kosek]

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