[Pki-devel] [PATCH] 0084..0086 Lightweight CA replication support
Ade Lee
alee at redhat.com
Wed Apr 13 21:30:21 UTC 2016
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
More information about the Pki-devel
mailing list