[Freeipa-devel] [PATCH 0011] Move freeipa certmonger helpers to libexecdir.

David Kupka dkupka at redhat.com
Thu Feb 25 13:13:09 UTC 2016 

On 24/02/16 15:07, Rob Crittenden wrote:
> David Kupka wrote:
>> On 23/02/16 16:41, Rob Crittenden wrote:
>>> David Kupka wrote:
>>>> On 23/02/16 10:14, Martin Kosek wrote:
>>>>> On 02/23/2016 09:47 AM, David Kupka wrote:
>>>>>> On 22/02/16 16:15, Martin Kosek wrote:
>>>>>>> On 02/22/2016 04:04 PM, Jan Cholasta wrote:
>>>>>>>> On 22.2.2016 15:56, David Kupka wrote:
>>>>>>>>> On 22/02/16 07:28, Jan Cholasta wrote:
>>>>>>>>>> On 18.2.2016 10:10, David Kupka wrote:
>>>>>>>>>>> On 19/01/16 16:10, David Kupka wrote:
>>>>>>>>>>>> On 19/01/16 14:38, Jan Cholasta wrote:
>>>>>>>>>>>>> On 19.1.2016 14:26, Martin Kosek wrote:
>>>>>>>>>>>>>> On 01/19/2016 01:47 PM, David Kupka wrote:
>>>>>>>>>>>>>>> I've polished the patch attached to #5586 by Timo Aaltonen.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks for the patch. I've fixed the path in specfile and
>>>>>>>>>>>>>>> removed
>>>>>>>>>>>>>>> unused import
>>>>>>>>>>>>>>> but otherwise it works, ACK.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/5586
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Won't this break existing certmonger requests depending on
>>>>>>>>>>>>>> the old
>>>>>>>>>>>>>> path?
>>>>>>>>>>>>>
>>>>>>>>>>>>> It will, I don't see any upgrade code.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> # getcert list | grep '/usr/lib64/ipa/certmonger'
>>>>>>>>>>>>>>         pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/renew_ca_cert
>>>>>>>>>>>>>> "auditSigningCert
>>>>>>>>>>>>>> cert-pki-ca"
>>>>>>>>>>>>>>         pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/renew_ca_cert
>>>>>>>>>>>>>> "ocspSigningCert
>>>>>>>>>>>>>> cert-pki-ca"
>>>>>>>>>>>>>>         pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/renew_ca_cert
>>>>>>>>>>>>>> "subsystemCert
>>>>>>>>>>>>>> cert-pki-ca"
>>>>>>>>>>>>>>         pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/renew_ca_cert
>>>>>>>>>>>>>> "caSigningCert
>>>>>>>>>>>>>> cert-pki-ca"
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/renew_ra_cert
>>>>>>>>>>>>>>         pre-save command: /usr/lib64/ipa/certmonger/stop_pkicad
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/renew_ca_cert
>>>>>>>>>>>>>> "Server-Cert
>>>>>>>>>>>>>> cert-pki-ca"
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/restart_dirsrv
>>>>>>>>>>>>>> RHEL72
>>>>>>>>>>>>>>         post-save command:
>>>>>>>>>>>>>> /usr/lib64/ipa/certmonger/restart_httpd
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> You're right it will break the upgrade. I haven't noticed that
>>>>>>>>>>>> Server-Cert for DS and HTTPD are not handled by
>>>>>>>>>>>> certificate_renewal_update (ipaserver.install.server.upgrade)
>>>>>>>>>>>> where all
>>>>>>>>>>>> the other trackings are stopped and then configured again
>>>>>>>>>>>> with the
>>>>>>>>>>>> paths.CERTMONGER_COMMAND_TEMPLATE already updated.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks for the catch.
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I've updated Timo's patch little more and added
>>>>>>>>>>> start_tracking_certificates() for dsinstance and httpinstance.
>>>>>>>>>>> Now the
>>>>>>>>>>> upgrade works as expected.
>>>>>>>>>>
>>>>>>>>>> The way the patches are split is kind of weird and apparently
>>>>>>>>>> confusing
>>>>>>>>>> (see the other thread). IMO there should be 2 patches: the first
>>>>>>>>>> should
>>>>>>>>>> add the ability to change DS and HTTP certmonger config during
>>>>>>>>>> upgrade
>>>>>>>>>> (i.e. the start_tracking_certificates() methods and
>>>>>>>>>> certificate_renewal_update() changes), the second should move the
>>>>>>>>>> helpers (i.e. the actual move and certificate_renewal_update()
>>>>>>>>>> version
>>>>>>>>>> bump).
>>>>>>>>>>
>>>>>>>>> Honza, do I understand it correctly that the code is OK but I
>>>>>>>>> did not
>>>>>>>>> split it to the patches correctly?
>>>>>>>>
>>>>>>>> Yes.
>>>>>>>
>>>>>>> Before acking or pushing, can you please explain for me how the
>>>>>>> upgrade of
>>>>>>> certmonger tracking requests work? I want to make sure this is
>>>>>>> right, so please
>>>>>>> bear with me:
>>>>>>>
>>>>>>> 1) How does it edit existing tracking requests with the new helper
>>>>>>> paths?
>>>>>>>
>>>>>>> 2) Does it go and try to edit the requests on every upgrade? Or is
>>>>>>> there some
>>>>>>> check that requests were updated?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Martin
>>>>>>>
>>>>>>
>>>>>> Whole upgrade of renewal requests is done in
>>>>>> ipaserver/install/server/upgrade.py in certificate_renewal_upgrade().
>>>>>>
>>>>>> First there is version of requests and if it's the same as in state
>>>>>> file
>>>>>> upgrade is skipped.
>>>>>> Then every request is searched over certmonger's DBus interface and
>>>>>> if at least
>>>>>> one is not found it means that there was change in request
>>>>>> configuration. All
>>>>>> tracking requests are then stopped and started again with new
>>>>>> configuration.
>>>>>>
>>>>>> So to answer you questions:
>>>>>> 1) By stopping the old request with the old parameters (including
>>>>>> path) and
>>>>>> starting new with new parameters.
>>>>>>
>>>>>> 2) Only if version was bumped which happens only if some of the
>>>>>> requests changes.
>>>>>
>>>>> Ah, so IIUC, if you bump the version, requests should be properly
>>>>> updated. The
>>>>> change looks fine then.
>>>>>
>>>>
>>>> After discussion with Honza, we decided to drop the part comparing only
>>>> base names of pre- and post-save commands and use it as whole. I've also
>>>> split the patches so it's obvious what is going on.
>>>>
>>>> Patches should be applied in this order:
>>>>
>>>> freeipa-dkupka-0091.0
>>>
>>> A cert could silently fail to be tracked in
>>> start_tracking_certificates() if no serverid can be found.
>>
>> In that case it also wouldn't be stopped. The behavior is the same as in
>> existing stop_tracking_certificates(). Should we rather raise and stop
>> the upgrade? I guess not but warning would be probably useful. What
>> solution would you prefer, Rob?
>
> I don't know all the callers of this. It may be perfectly safe to assume
> that a serverid is always there, but the implication if it isn't is that
> some tracking cert won't be updated properly right? That potentially
> could mean no renewal.
>
> So the consequences could be severe, I just don't know the likelihood.
>
> In other words, a comment (# can never get here) might be perfectly
> adequate.
>
> rob
>
>>
>>>
>>>> freeipa-dkupka-0087.1
>>>> freeipa-dkupka-0088.1
>>>> freeipa-tjaalton-0011.2
>>>> freeipa-dkupka-0092.0
>>>>
>>>>
>>>>
>>
>>
>


Currently the function is called only from one place (also added in this 
patch) but to avoid problems in the future I made the serverid parameter 
mandatory.
Also I squashed the version bump into Timo's patch.

Updated patches attached. Apply in this order:

freeipa-dkupka-0091.1
freeipa-dkupka-0087.2
freeipa-dkupka-0088.2
freeipa-tjaalton-0011.3

-- 
David Kupka
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0087.2-dsinstance-add-start_tracking_certificates-method.patch
Type: text/x-patch
Size: 4097 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160225/b9e50e3c/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0088.2-httpinstance-add-start_tracking_certificates-method.patch
Type: text/x-patch
Size: 3767 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160225/b9e50e3c/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-dkupka-0091.1-upgrade-Match-whole-pre-post-command-not-just-basena.patch
Type: text/x-patch
Size: 4115 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160225/b9e50e3c/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tjaalton-0011.3-Move-freeipa-certmonger-helpers-to-libexecdir.patch
Type: text/x-patch
Size: 5709 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20160225/b9e50e3c/attachment-0003.bin>

More information about the Freeipa-devel mailing list