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

[Fraser Tweedale]

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