10:16 a.m.
On Thu, 2016-03-31 at 16:37 +1000, Fraser Tweedale wrote:
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
0087
1. In SigningUnit.java -- you catch an ObjectNotFound exception and
rethrow that as a CAMissingKey exception. Is that the only way the
ObjectNotFound exception can be thrown? What if the key is present but
the cert is not? Can we refactor here to ensure that the correct
exception is thrown?
0088:
2. What does dbFactory.reset() do and does it need to be called in a
cleanup routine somewhere? Are we leaking resources?
Answered I think on IRC. It just terminates any current connections -
but do we need to call it on CA shutdown?
0089: ACK
0090: ACK
0091: ACK (with proviso below)
3. Not super-crazy about the names of the methods commitAuthority(),
commitModifyAuthority and deleteAuthorityEntry(). They are not very
consistent. I would suggest addAuthorityEntry(),
modifyAuthorityEntry() and deleteAuthorityEntry() instead.
0092: ACK (with following proviso)
4. Talking with Nathan about this, he suggested that syncrepl is then
more modern and preferred method to perform persistent searches. In
fact, I see IPA tickets to replace persistent searches with syncrepl
instead.
We could replace the persistent search with a separate follow-on patch
if you prefer, or just do it now.
0093 : ACK
0094: ACK
0095: ACK
0096: Looks good in general.
5. One thing to keep in mind though. It is perfectly possible to have
more than one dogtag instance on a host. What determines the
uniqueness of the instance is the host:port.
Ade
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