Still reviewing .. ACK on 87-95 (inclusive). On Thu, 2016-04-14 at 16:18 +1000, Fraser Tweedale wrote: > On Thu, Apr 14, 2016 at 09:04:31AM +1000, Fraser Tweedale wrote: > > 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. > > > Updated and rebased patches attached. > > The suggested changes were made to 0087. This also resulted in > changes to patch 0094 (indicate when CA does not yet have keys). > > No substantive changes to any other patches. > > Cheers, > Fraser
_______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel