A few comments. 1. One of the first things that struck me as odd was making CertificateAuthority implement Runnable. I think it would be cleaner to have a static inner class called AuthorityMonitor or similar to which we pass in the CertificateAuthority. 2. I do like the fact that the caMap updates are done through a static database connection factory created by the hostCA. How do you ensure that the database connection factory is created before being used by other CAs? 3. I'm not sure I understand how the initialLoadDone counter is supposed to work. Are all the CA's supposed to stop until the hostCA has completed its initial load? Because it looks like only the hostCA calls await(). 4. There is a lot of code in that initial patch. It would help review to split that off into at least two patches, say one in which you add the functions in CertificateAuthority that handle modifications in the caMap based on persistent search results, and one which adds the new monitor thread. 5. Some in-code documentation would not go amiss. For instance, I have no idea why this code is correct -- String[] objectClasses = entry.getAttribute("objectClass").getStringValueArray(); if (Arrays.asList(objectClasses).contains("organizationalUnit")) { initialNumAuthorities = new Integer( entry.getAttribute("numSubordinates") .getStringValueArray()[0]); checkInitialLoadDone(); continue; } organizationalUnit? There are lots of different variables like initialNumAuthorities etc. which could potentially be hidden in an inner class, making this more understandable. Ade On Tue, 2016-03-22 at 16:00 +1000, Fraser Tweedale wrote:
On Fri, Mar 18, 2016 at 02:30:24PM +1000, Fraser Tweedale wrote:
Hi all,
The attached patches implement replication support for lightweight CAs. These patches do not implement key replication via Custodia (my next task) but they do implement the persistent search thread and appropriate** API behaviour when the signing keys are not yet available.
** In most cases, we respond 503 Service Unavailable; this is open for discussion. ca-authority-find and ca-authority-show include a boolean field indicating whether the CA is ready to sign. There might be (probably are) endpoints I've missed.
Cheers, Fraser
Updated patches attached - small change in patch 0084 to fix a race condition when deleting an authority that can cause NPE.
Thanks, Fraser _______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel