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. > > 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