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

Sunday, 10 April 2016

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

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

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