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

Wed Sep 24 17:39:14 UTC 2014

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.

>
> As a side note, it was rather difficult to review parts of this as
> different patches modify exactly the same code in different ways.
>
> General:
>
> These patches don't replace /etc/pki/nssdb, they just add a new
> location. Do we need to keep using the shared db because of p11-kit?

It's for backward compatibility.

>
> 319:
>
> The post-install script tries to get a cert with nickname 'IPA CA'. It
> should also look for '$REALM IPA CA'. I'd use
> ipapythhon.certdb.py:CA_NICKNAME_FMT. Having a similar option for
> External CA is probably not a bad idea.

I included 'IPA CA' and 'External CA cert', because they are the only 
nicknames used for the CA certificate in /etc/pki/nssdb before 4.1 (I 
checked in git). I did not include '$REALM IPA CA', as it is used only 
in unreleased code.

>
> You don't need the password file to import a cert into an NSS database.

Fixed.

>
> It may be too complex in a spec file but would it be better to try to
> fetch things out of LDAP? This nearly screams for a separate script to
> do the work.

Sorry, you have to run ipa-certupdate manually for now to update the 
database. Automatic updates from LDAP will be included in 4.2.

We definitely need a client update script, for other stuff as well.

>
> Are these operations important enough to be logged to
> /var/log/ipaupgrade.log? I suspect it's the kind of thing that might
> fail and never be noticed.

It couldn't hurt.

>
> At some point we'll upgrade to sql NSS databases. Is this going to
> complicate things?

I don't think so. When do we want to do the upgrade?

>
> There are some places you do str(e[2]) that aren't necessary.

Fixed.

>
> Might be worthwhile to clean up the comments regarding XML-RPC while
> you're at it and just refer to RPC.

Done.

>
> I'd rename ca_certs_nss to ca_certs_trust for clarity.

OK.

>
> Is the separate helper create_ipa_nssdb() necessary? Should create_db()
> be extended to do this extra work?

It is easier to call from the spec file this way.

>
> 324:
>
> ACK
>
> 325:
>
> The exception from get_cert() is very rough so wouldn't cover the case
> of file permissions, missing database, etc, but since the overall
> behavior isn't changing, ACK. Might be worth a comment though indicating
> a potential source of oddness for the uninitiated.

Comment added.

>
> 326:
>
> ACK
>
> 327:
>
> @@ -133,6 +116,7 @@ class CertUpdate(admintool.AdminTool):
>           criteria = {
>               'cert-database': dogtag_constants.ALIAS_DIR,
>               'cert-nickname': nickname,
> +            'ca-name': 'dogtag-ipa-ca-renew-agent',
>           }
>           request_id = certmonger.get_request_id(criteria)
>           if request_id is not None:
>
> Doesn't seem related to this change.

Moved to patch 340, along with similar change in ipa-cacert-manage in 
patch 335.

>
> 328:
>
> ACK
>
> 329:
>
> ACK
>
> 330:
>
> I don't understand the reference to ipa.p11-kit (an unowned file, btw).

Added the file to the spec file.

>
> Are you removing any cert that may be left over from some previous install?

Yes. I believe it might what was causing the NSS failures you saw in 
<http://www.redhat.com/archives/freeipa-devel/2014-July/msg00112.html>.

>
> If the file were to exist it could cause update-ca-trust to be executed
> twice. Is that needed?

Where?

>
> I think more clarity in the commit message will clear things up.

OK.

>
> 331:
>
> ACK

Updated the patch with a simpler fix and a ticket number.

>
> 332:
>
> What is the reason for this refactoring?

It makes it easier to make changes such as the one in patch 333. Also it 
makes dogtag-ipa-ca-renew-agent more pythonic and easy to read IMHO.

>
> 333:
>
> Seems reasonable but why would a CA profile change in the middle of a
> request?
>
> 334:
>
> Same boat, why?
>
> 335:
>
> I kinda get the picture, along with previous patches, but need more info.

Updated commit messages of the above 3 patches with more info.

>
> Some tips on testing would be helpful.

Testing is basically the same as for the original CA management patches 
- do some server installs, upgrades and uninstalls and run 
ipa-certupdate and ipa-cacert-manage with different options.

>
> rob
>

Updated patches attached.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-319.2-Introduce-NSS-database-etc-ipa-nssdb.patch
Type: text/x-patch
Size: 15996 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-324.1-Move-NSSDatabase-from-ipaserver.certs-to-ipapython.c.patch
Type: text/x-patch
Size: 20092 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-325.1-Add-NSSDatabase.has_nickname-for-checking-nickname-p.patch
Type: text/x-patch
Size: 1030 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-326.1-Use-NSSDatabase-instead-of-direct-certutil-calls-in-.patch
Type: text/x-patch
Size: 9167 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-327.1-Use-etc-ipa-nssdb-to-get-nicknames-of-IPA-certs-inst.patch
Type: text/x-patch
Size: 8849 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-328.1-Check-if-IPA-client-is-configured-in-ipa-certupdate.patch
Type: text/x-patch
Size: 1081 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-329.1-Get-server-hostname-from-jsonrpc_uri-in-ipa-certupda.patch
Type: text/x-patch
Size: 1164 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-330.1-Remove-ipa-ca.crt-from-systemwide-CA-store-on-client.patch
Type: text/x-patch
Size: 4415 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-331.1-Fix-certmonger.wait_for_request.patch
Type: text/x-patch
Size: 960 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-332.1-Refactor-dogtag-ipa-ca-renew-agent.patch
Type: text/x-patch
Size: 20633 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-333.1-Restart-request-in-dogtag-ipa-ca-renew-agent-if-prof.patch
Type: text/x-patch
Size: 2237 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-334.1-Do-not-wait-for-new-CA-certificate-to-appear-in-LDAP.patch
Type: text/x-patch
Size: 5181 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-335.1-Fail-if-certmonger-can-t-see-new-CA-certificate-in-L.patch
Type: text/x-patch
Size: 2606 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-jcholast-340-Fix-certmonger-search-for-the-CA-cert-in-ipa-certupd.patch
Type: text/x-patch
Size: 1852 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140924/2acecacf/attachment-0013.bin>