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

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