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

[Rob Crittenden]

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?

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.

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.

So ACK for 319, 324-331, 340.

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

rob