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