7:40 p.m.
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