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

Wed Feb 24 14:07:53 UTC 2016

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
>>>
>>>
>>>
> 
> 