[Pki-devel] [PATCH] 0084..0086 Lightweight CA replication support

[Ade Lee]

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 at redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel