1:37 a.m.
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/pki-devel