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

Couple of points on 96/97. 1. First off, I'm not sure you followed my concern about being able to distinguish between CA instances. On an IPA system, this is not an issue because there is only one CA on the server. In this case, I imagine there will be a well known directory which custodia would work with. In general though, we have to imagine that someone could end up installing two different dogtag ca instances on the same server. CMS.getEEHost() would result in the same value (the hostname) for both CAs. How does your helper program (or custodia) know which key to retrieve? The way to distinguish Dogtag instances is host AND port. 2. So, we're very careful that the signing keys are never in memory in the server. All accesses to the system certs are through JSS/NSS which essentially provides us handles to the keys. Now, I see a case where we import PKCS12 data AND the password into memory, so that we can import it into NSS? Say it ain't so .. With custodia, we have a secure mechanism of transferring the keys from one server to another. It makes more sense to me to have the server kick off the custodia transfer and then have that process also import into the NSS db. The server would then need to await status from the custodia/retriever process - and then initialize the signing unit from the NSS DB. Or am I completely confused? Ade On Thu, 2016-04-14 at 16:35 -0400, Ade Lee wrote: > Still reviewing .. ACK on 87-95 (inclusive). > > 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/msg0 > > > > > 00 > > > > > 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,