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

[Fraser Tweedale]

On Wed, Apr 13, 2016 at 05:26:44PM -0400, Ade Lee wrote:
> Still reviewing ..
> 
> See comment on 87.  ACK on 88,89,90,91,92,93, 94, 95.
> 
> Ade
> 
> On Mon, 2016-04-11 at 12:32 +1000, Fraser Tweedale wrote:
> > Thanks for review, Ade.  Comments to specific feedback inline.
> > Rebased and updated patches attached.  The substantive changes are:
> > 
> > - KeyRetriever implementations are now required NOT to import the
> >   key themselves.  Instead the API is updated with
> >   KeyRetriever.retrieveKey returning a Result, which contains PKCS
> >   #12 data and password for same.
> > 
> > - KeyRetrieverRunner reads the Result and imports the PKCS #12 into
> >   NSSDB.
> > 
> > - Added new patch 0097 which provides the IPACustodiaKeyRetriever
> >   and assoicated Python helper script.  It depends on an unmerged
> >   FreeIPA patch[1] as well as a particular principal and associated
> >   keytab and Custodia keys existing.  I'm working on FreeIPA updates
> >   to satisfy these requirements automatically on install or upgrade
> >   but if you want to test this patch LMK and I'll provide detailed
> >   instructions.
> > 
> >   [1] https://www.redhat.com/archives/freeipa-devel/2016-April/msg000
> > 55.html
> > 
> > Other comments inline.
> > 
> > Cheers,
> > Fraser
> > 
> > On Fri, Apr 08, 2016 at 11:16:19AM -0400, Ade Lee wrote:
> > > 
> > > 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?
> > > 
> > One can't get additional info out of ObjectNotFound without
> > inspecting the String message, which I'm not comfortable doing.  The
> > key retrieval system should import key and cert at same time so I've
> > renamed the exception to CAMissingKeyOrCert for clarity.
> > 
> 
> Well, you can always nest exceptions like so :
> 
> 	    mToken.login(cb); // ONE_TIME by default.
> 
>             try {
>                 mCert = mManager.findCertByNickname(mNickname);
>                 CMS.debug("Found cert by nickname: '" + mNickname + "' with serial number: " + mCert.getSerialNumber());
> 
>                 mCertImpl = new X509CertImpl(mCert.getEncoded());
>                 CMS.debug("converted to x509CertImpl");
>             } catch (ObjectNotFoundException e) {
>                 throw new CAMissingCertException();
>             }
> 
>             try {
>                 mPrivk = mManager.findPrivKeyByCert(mCert);
>                 CMS.debug("Got private key from cert");
>             } catch (ObjectNotFoundException e) {
>                throw new CAMissingKeyException();
>             }
>             ....
> 
> The only reason that I suggest this is that I could imagine this kind
> of differentiation being useful in debugging failed custodia
> replications.  If you think otherwise, I'm prepare to be convinced
> otherwise.
> 
I think a scenario where we get key but not cert, or vice versa, is
unlikely (Custodia gives us a PKCS #12 file with both).  However,
your suggestion should work and it is a relatively small change.
I'll cut a new patchset with this change today, along with the
rebase.

Cheers,
Fraser

> > > 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?
> > > 
> > dbFactory.reset() is already called in the shutdown() method.  (Only
> > the host authority calls it).
> > 
> > > 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.
> > > 
> > Done.
> > 
> > > 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.
> > > 
> > Syncrepl is not supported by ldapjdk (iirc).  If/when it is, and if
> > syncrepl provides a tangible advantage over persistent search in our
> > use case (where it can be assumed that disconnections from DS are
> > infrequent and brief, and full refresh of local view is tolerable),
> > then I am happy to change it - in a separate commit (because
> > LDAPProfileSubsystem is also affected).
> > 
> > > 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.
> > > 
> > Noted.  KeyRetriever implementations can access instance info via
> > the `CMS' class.
> > 
> > Cheers,
> > Fraser