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

[Rob Crittenden]

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.

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?

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.

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

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.

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.

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

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

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

I'd rename ca_certs_nss to ca_certs_trust for clarity.

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

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.

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.

328:

ACK

329:

ACK

330:

I don't understand the reference to ipa.p11-kit (an unowned file, btw).

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

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

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

331:

ACK

332:

What is the reason for this refactoring?

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.

Some tips on testing would be helpful.

rob