[Freeipa-devel] [PATCHES] 319, 324-335 CA management and renewal fixes

Tue Sep 30 08:19:06 UTC 2014

Dne 30.9.2014 v 10:09 Martin Kosek napsal(a):
> On 09/30/2014 09:59 AM, Jan Cholasta wrote:
>> Dne 29.9.2014 v 18:09 Rob Crittenden napsal(a):
>>> Jan Cholasta wrote:
>>>> Dne 29.9.2014 v 15:05 Rob Crittenden napsal(a):
>>>>> Jan Cholasta wrote:
>>>>>> Dne 26.9.2014 v 17:13 Rob Crittenden napsal(a):
>>>>>>> Jan Cholasta wrote:
>>>>>>>> Dne 23.9.2014 v 20:39 Rob Crittenden napsal(a):
>>>>>>>>> Jan Cholasta wrote:
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> the attached patches fix various bugs and shortcomings in the CA
>>>>>>>>>> management and renewal code. Related tickets:
>>>>>>>>>> <https://fedorahosted.org/freeipa/ticket/4416>,
>>>>>>>>>> <https://fedorahosted.org/freeipa/ticket/4460>.
>>>>>>>>>>
>>>>>>>>>> (Patch 319 was originally posted at
>>>>>>>>>> <http://www.redhat.com/archives/freeipa-devel/2014-September/msg00132.html>.)
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Only two of the patches includes what ticket(s) they address. Most
>>>>>>>>> have
>>>>>>>>> the tersest of commit messages. If got more and more difficult to see
>>>>>>>>> why the changes were needed at all, as you'll see.
>>>>>>>>
>>>>>>>> Sorry, fixed (hopefully).
>>>>>>>>
>>>>>>>> Note that most of these patches fix stuff I didn't have time to fix
>>>>>>>> before I posted the original CA management patches, hence the missing
>>>>>>>> tickets.
>>>>>>>
>>>>>>> Well, the policy is that every commit should have a ticket. So I guess
>>>>>>> re-open the old tickets or open new ones. This will help someone in the
>>>>>>> future know the "why" of a patch. I've certainly been guilty
>>>>>>
>>>>>> OK, I will reopen the related tickets.
>>>>>>
>>>>>>>
>>>>>>> Here is a new set of reviews as trying to intermingle was making my
>>>>>>> eyes
>>>>>>> cross:
>>>>>>>
>>>>>>> 319:
>>>>>>>
>>>>>>> I guess I still don't understand why you can't pull the certs out of
>>>>>>> LDAP when creating this database. When this code runs, we know that the
>>>>>>> client is configured, so we have access to authentication. Why can't
>>>>>>> create_ipa_nssdb pull the certs directly? It seems more robust to me,
>>>>>>> and the code is already written in ipa-client-install to do this.
>>>>>>
>>>>>> Well, I don't understand why do you want them to be updated so much, as
>>>>>> nothing will break if they are not. Also try to imagine what would
>>>>>> happen if, say, 10k clients were updated at the same moment...
>>>>>
>>>>> What's the point of a database missing certificates?
>>>>
>>>> It won't be missing any certificates if /etc/pki/nssdb was not missing
>>>> any certificates before the update.
>>>>
>>>> As I said, the update will not break anything. It will not fix anything
>>>> either, but I don't think this kind of fixing should be done during
>>>> client RPM upgrade. It is not consistent with anything else we do during
>>>> client RPM upgrade, it does not scale well and it just does not feel
>>>> right to me in overall.
>>>
>>> Ok, I'll concede the point that there is no difference post-update, but
>>> it doesn't do what ipa-certupdate does. You can potentially end up with
>>> a completely diffferent set of certificates. So why the difference?
>>
>> If you end up with a *completely* different set of certificates, it's because
>> the admins screwed up, so it's theirs responsibility to fix it, not ours IMHO.
>>
>>>
>>> Post install of new client bits:
>>>
>>> # certutil -L -d /etc/ipa/nssdb/
>>>
>>> IPA CA                                                       CT,C,C
>>>
>>> After running ipa-certupdate:
>>>
>>> # certutil -L -d /etc/ipa/nssdb/
>>>
>>> CN=Primary CA,O=example.com,C=US                             ,,
>>> CN=Secondary CA,O=example.com,C=US                           ,,
>>> GREYOAK.COM IPA CA                                           CT,C,C
>>>
>>> Quite the difference.
>>
>> Not much of a difference when you realize that "IPA CA" and "GREYOAK.COM IPA
>> CA" are the same certificate and the rest are there just for completeness.
>>
>>>
>>> I'll ACK for now since this doesn't materially change anything and
>>> shouldn't break any installs but it begs the question of why it is
>>> acceptable now but not acceptable to make ipa-certupdate do the same.
>>
>> I have changed the NSS database used by the RPC code from /etc/pki/nssdb to
>> /etc/ipa/nssdb. What the RPM update does is a necessary configuration update to
>> keep RPC working. ipa-certupdate on the other hand fetches new policy from the
>> server, which is quite a different thing IMO.
>>
>>>
>>> So ACK for 319, 324-331, 340.
>>
>> Thanks!
>>
>>>
>>>>
>>>> The LDAP update happens in renew_ca_cert. Are there any relevant errors
>>>> in /var/log/messages? Is there caRenewalMaster in ipaConfigString of the
>>>> master entry of the master where you run ipa-cacert-manage renew?
>>>
>>> Sep 25 16:06:44 grindle renew_ca_cert: Traceback (most recent call last):
>>>     File "/usr/lib64/ipa/certmonger/renew_ca_cert", line 214, in <module>
>>>       main()
>>>     File "/usr/lib64/ipa/certmonger/renew_ca_cert", line 79, in main
>>>       ca = cainstance.CAInstance(host_name=api.env.host, ldapi=False)
>>>     File
>>> "/usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py", line
>>> 357, in __init__
>>>       self.ra_agent_pwd = self.ra_agent_db + "/pwdfile.txt"
>>> TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'
>>> Sep 25 16:06:44 grindle certmonger: Certificate named "caSigningCert
>>> cert-pki-ca" in token "NSS Certificate DB" in database
>>> "/etc/pki/pki-tomcat/alias" issued by CA and saved.
>>
>> One of the KRA patches broke this. I assumed you'd be testing on top of
>> ipa-4-1, so I didn't fix it, sorry. See the attached patch 342 for a fix.
>> Martin, can you please review it?
>>
>>>
>>> rob
>>>
>>
>> The patches needed a rebase, attached.
>>
>
> Thanks to both for this work! I pushed all patches that Rob reviewer to master
> and ipa-4-1.
>
> I also checked Jan's fix up patch 342 and it looks good to me as well.
>
> ACK for that and pushed to master.
>
> Martin
>

I did a lousy job at rebasing the patches, sorry. The attached patch 
fixes it.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-343-Add-missing-imports-to-ipapython.certdb.patch
Type: text/x-patch
Size: 791 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140930/8472e1bc/attachment.bin>