6:04 p.m.
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.
Cheers,
Fraser
> > 0088:
> >
> > 2. What does dbFactory.reset() do and does it need to be called in
> > a
> > cleanup routine somewhere? Are we leaking resources?
> >
> > Answered I think on IRC. It just terminates any current
> > connections -
> > but do we need to call it on CA shutdown?
> >
> dbFactory.reset() is already called in the shutdown() method. (Only
> the host authority calls it).
>
> > 0089: ACK
> >
> > 0090: ACK
> >
> > 0091: ACK (with proviso below)
> >
> > 3. Not super-crazy about the names of the methods
> > commitAuthority(),
> > commitModifyAuthority and deleteAuthorityEntry(). They are not
> > very
> > consistent. I would suggest addAuthorityEntry(),
> > modifyAuthorityEntry() and deleteAuthorityEntry() instead.
> >
> Done.
>
> > 0092: ACK (with following proviso)
> >
> > 4. Talking with Nathan about this, he suggested that syncrepl is
> > then
> > more modern and preferred method to perform persistent searches.
> > In
> > fact, I see IPA tickets to replace persistent searches with
> > syncrepl
> > instead.
> >
> > We could replace the persistent search with a separate follow-on
> > patch
> > if you prefer, or just do it now.
> >
> Syncrepl is not supported by ldapjdk (iirc). If/when it is, and if
> syncrepl provides a tangible advantage over persistent search in our
> use case (where it can be assumed that disconnections from DS are
> infrequent and brief, and full refresh of local view is tolerable),
> then I am happy to change it - in a separate commit (because
> LDAPProfileSubsystem is also affected).
>
> > 0093 : ACK
> >
> > 0094: ACK
> >
> > 0095: ACK
> >
> > 0096: Looks good in general.
> >
> > 5. One thing to keep in mind though. It is perfectly possible to
> > have
> > more than one dogtag instance on a host. What determines the
> > uniqueness of the instance is the host:port.
> >
> Noted. KeyRetriever implementations can access instance info via
> the `CMS' class.
>
> Cheers,
> Fraser