On Thu, Apr 14, 2016 at 04:35:10PM -0400, Ade Lee wrote:
Still reviewing .. ACK on 87-95 (inclusive).
Thanks. I pushed 87..94 to master. I left 95 for now, in case of further schema changes. 6d72a9c7fc067df42a3259fc5ea87b65e94f76ad Lightweight CAs: add exceptions for missing signing key or cert 908c75dcefcb5030b2e3328835c506bf4c53704f Lightweight CAs: use static db connection factory 536312af6798ca688556f559f8bdc76e2ba53e4d Lightweight CAs: avoid repeat definition of authorities DN 18ed063edde8608f2ef30f62c118e24b835f1d83 Lightweight CAs: move host authority creation out of load method 6a2195d6dab0dd4d20155177892af88aeb67d517 Lightweight CAs: extract LDAP commit/delete methods a39499f08966a517d52c97ef0cd54d8e6f098fb9 Lightweight CAs: monitor database for changes 28bc4ed903bc9e2618390ec412602d889e28354b Lightweight CAs: set DN based on data from LDAP 8f93e60e0057b0706c5d5ad762d7ff7ce20b7b39 Lightweight CAs: indicate when CA does not yet have keys Cheers, Fraser
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