3:35 p.m.
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/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