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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel