[Pki-devel] [PATCH] 0084..0086 Lightweight CA replication support
Fraser Tweedale
ftweedal at redhat.com
Wed Apr 13 23:04:31 UTC 2016
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
More information about the Pki-devel
mailing list