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 _______________________________________________ Pki-devel mailing list Pki-devel@redhat.com https://www.redhat.com/mailman/listinfo/pki-devel