[Freeipa-devel] [PATCHES] 295-299 Allow changing chaining of the IPA CA certificate

[Rob Crittenden]

Jan Cholasta wrote:
> On 22.7.2014 15:21, Rob Crittenden wrote:
>> Rob Crittenden wrote:
>>> Jan Cholasta wrote:
>>>> On 2.7.2014 19:37, Jan Cholasta wrote:
>>>>> On 2.7.2014 19:08, Rob Crittenden wrote:
>>>>>> Trimming to respond to your questions.
>>>>>>>> Not sure if this is related:
>>>>>>>> # pki cert-find
>>>>>>>> PKIException: Internal Server Error
>>>>>>
>>>>>> I'm pretty sure the cert-find error is related to the fact that I
>>>>>> had a
>>>>>> test build of dogtag installed, so that can be ignored.
>>>>>
>>>>> It does not work for me as well, with the current F20 dogtag packages,
>>>>> but like I said, it worked some time ago.
>>>>
>>>> Still haven't figured this out, unfortunately.
> 
> Fixed. Part of the problem was that the validation code I used on CA
> certificates was too tolerant (fixed in patches 249 and 251). Another
> part was the NSS validation code that Dogtag uses requires the issuing
> CA to be present in the NSS database (fixed in patch 306). Finally,
> Dogtag uses default NSS certificate path validation, which means you
> have to either keep all versions of the CA certificate in the NSS
> database, or enable PKIX path validation in NSS. Certmonger does not
> like having multiple versions of a certificate it is tracking in the
> database, so I have gone the PKIX route (patch 307).
> 
>>>>
>>>> Added patches 304 and 305 to fix /etc/ipa/ca.crt not having all the CA
>>>> certificates on master.
>>>>
>>>> Updated rebased patches attached. The correct order to apply is
>>>> 295-294,
>>>> 303-305, 295-299.
>>>>
>>>
>>> 251 I'm a little confused about the profile names. I see you changed the
>>> renewal profile from ipaCACertRenewal to caCACert which I guess makes
>>> sense. I don't see a ipaCACertRenewal profile. There is still a
>>> reference to a ipaRetrieval profile, what is that?
> 
> Oops, I forgot to mention that, I guess I shouldn't post patches at such
> late hour :) Sorry.
> 
> ipaCACertRenewal should be used only for automatic renewal, not for
> manual. It calls caCACert and ipaRetrieval internally, but there are
> some conditions, which don't apply to manual renewal. It's a change I
> forgot to make before, so I made it now when I noticed it. ipaRetrieval
> fetches the certificate from cn=ca_renewal, i.e. what
> dogtag-ipa-retrieve-agent-submit used to do.
> 
>>>
>>> ACK to the changes in 291
>>>
>>> 299 I guess you added the check for existing certs to avoid conflicts? I
>>> guess it means that a user is hosed if they chose the same name for
>>> their CA that we use? I think you're missing a sys.exit(1) here.
> 
> Yes. It is a poor man's solution, but it would take time to make
> something better. (I can deal with nickname conflicts rather easy by
> renaming the certificates, but handling subject conflict would require
> removing the old certificate from the certificate store, which is not
> yet supported.)
> 
> Fixed missing exit.
> 
>>>
>>> 303 Looks good. The man page is still a little thin
>>>
>>> 304 Not to be too pedantic but if removing the old CACERT fails
>>> (SELinux, immutable file) then the install will blow up and this is the
>>> very end. I think the removal should happen earlier, before anything
>>> else happens. That way at least you don't wait 10 minuts to find out the
>>> install failed.
> 
> I switched to overwriting the file instead. It is created/written a few
> lines above, so if it shall fail, it will fail there.
> 
>>>
>>> 305 ACK
>>>
>>> I didn't have a ton of time to test but a basic install fails with:
>>>
>>> 2014-07-03T21:44:49Z DEBUG stderr=
>>> 2014-07-03T21:44:49Z DEBUG   File
>>> "/usr/lib/python2.7/site-packages/ipaserver/install/installutils.py",
>>> line 640, in run_script
>>>      return_value = main_function()
>>>
>>>    File "/usr/sbin/ipa-server-install", line 1046, in main
>>>      dm_password, subject_base=options.subject)
>>>
>>>    File
>>> "/usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py", line
>>> 489, in configure_instance
>>>      self.start_creation(runtime=210)
>>>
>>>    File "/usr/lib/python2.7/site-packages/ipaserver/install/service.py",
>>> line 382, in start_creation
>>>      method()
>>>
>>>    File
>>> "/usr/lib/python2.7/site-packages/ipaserver/install/cainstance.py", line
>>> 1041, in __import_ca_chain
>>>      (rdn, subject_dn) = certs.get_cert_nickname(certlist[st:en+25])
>>>
>>>    File "/usr/lib/python2.7/site-packages/ipaserver/install/certs.py",
>>> line 79, in get_cert_nickname
>>>      nsscert = x509.load_certificate(cert)
>>>
>>>    File "/usr/lib/python2.7/site-packages/ipalib/x509.py", line 119, in
>>> load_certificate
>>>      return nss.Certificate(buffer(data))
>>>
>>> 2014-07-03T21:44:49Z DEBUG The ipa-server-install command failed,
>>> exception: NSPRError: (SEC_ERROR_REUSED_ISSUER_AND_SERIAL) You are
>>> attempting to import a cert with the same issuer/serial as an existing
>>> cert, but that is not the same cert.
>>
>> I haven't gotten much further than this. I spent some time trying to
>> find the a change that would cause it and came up empty. Once this bug
>> shows, it always shows, but it can go away at times too which is just
>> blowing my little mind.
>>
>> For example, I tried rolling the patches back one at a time (revert,
>> build, install, repeat). It failed even back to the point where I knew
>> things should be working. I installed 3.3.5, then tried the current
>> build, which had failed before, and it worked. So there is some odd
>> transient thing going on that I can't wrap my head around.
> 
> I have not yet seen this failure myself.
> 
> It looks like NSS internally imports the certificate, which conflicts
> with the database NSS is initialized with. Perhaps a well placed
> nss_shutdown() might fix this? Maybe using NSS contexts instead of
> global initialization could help.
> 
>>
>> rob
>>
> 
> I have taken your advice and don't touch trust flags on unknown CA certs
> on upgrades anymore. I have also made a number of little tweaks.
> 
> Updated rebased patches attached.
> 

This is oh-so close. AFAICT it generally does what it should, I think it
is ready for a wider audience. Just a few more things:

306: A while True loop is used for something which AFAICT can only ever
execute once. I'd think something like this is more readable:

for ca_nick, ca_flags in db.list_certs():
    if db.has_nickname(ca_cert):
        try:
            db.delete_cert(ca_nick)
        except ipautil.CalledProcessError:
            syslog.syslog(
                syslog.LOG_ERR,
                "Failed to remove certificate %s" % ca_nick)

+1 on the additional syslogs. It will help figure out what's going on if
things go sideways.

Otherwise things seem to be working. I think that fixing the above is
enough for a +57 with the promise of unit tests to back up some of these
new functions.

rob
rob