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

[Ade Lee]

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