Ade, thanks for the review. I address your comments inline. Updated patches are attached, and there are new patches implementing schema changes and the key acquisition framework. Pursuant to comment (4), I have split patch 0084 into six different patches. To keep things in order I retire patch numbers 0084-0086 and start over from 0087: /-0087 add CAMissingKeyException class / 0088 use static db connection factory 0084 ___/ 0089 avoid repeat definition of authorities DN \ 0090 move host authority creation out of load method \ 0091 extract LDAP commit/delete methods \_0092 monitor database for changes 0085 -> 0093 set DN based on data from LDAP 0086 -> 0094 indicate when CA does not yet have keys (new) -> 0095 authority schema changes (new) -> 0096 add key retrieval framework On Thu, Mar 24, 2016 at 12:29:24PM -0400, Ade Lee wrote:
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.
This was originally the way that LDAPProfileSubsystem was structured, and the review feedback there was that we accessed so many members of the enclosing class that it would be better to make the enclosing class Runnable and migrate the methods there. So... grass is always greener? :) Based on the LDAPProfileSubsystem experience, I'm not inclined to make this change.
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?
It is initialised before other CertificateAuthority instances are instantiated. I added a comment.
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().
Yep, it's used to block only the host CA's 'init' method while lightweight CAs are loaded by the monitor thread. I added a comment.
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.
Split into six patches, as discussed above.
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?
I added detailed commentary about what's going on here.
There are lots of different variables like initialNumAuthorities etc. which could potentially be hidden in an inner class, making this more understandable.
The declarations are grouped together, and hopefully things are made clearer by the split patches. If you still feel the updated patches would benefit substantially from grouping tracking vars in an inner record type, I will make the change. Many thanks again for the review! Cheers, Fraser
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