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