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

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.