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

[Ade Lee]

96-2 looks like it does not apply.  Please rebase .

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.
> 
> > 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