Re: [Pki-devel] [PATCH] 0084..0086 Lightweight CA replication support
Thanks Ade. Updated patch 0096 attached. Comments inline.
On Wed, Apr 20, 2016 at 11:30:52AM -0400, Ade Lee wrote:
Comments:
95 - ack
96 -
1. You have made the return type of initSigUnit() to be boolean.
Should you be checking the return value in init()?
It is not needed to check it here; only when re-entering init from
the KeyReplicatorRunner thread.
2. In addInstanceToAuthorityKeyHosts(), you are still using only the
hostname. Should be host:port
Good pickup. Fixed in latest patch.
3. The logic in the KeyRetrieverRunner class looks OK to me, but
I'd
like cfu and/or jmagne to check it and make sure we are calling the
right primitives to wrap/unwrap inside the cryptographic token.
Also I'd like them to confirm that this would wor for an HSM.
Statements like the following make me question that:
CryptoToken token = manager.getInternalKeyStorageToken()
It won't work on HSM. Can I get an HSM to test with? ;) I've filed
a ticket for HSM support[1]. FreeIPA does not yet support HSM[2] so
I think we can put it in 10.4 milestone (I've put it there for now).
[1] https://fedorahosted.org/pki/ticket/2292
[2] https://fedorahosted.org/freeipa/ticket/5608
4. Can you explain what happens if for some reason the script fails
to
retrieve the key? Do we end up retrying later and if so, when?
If the script fails to retrieve the key, it does not retry
automatically. I filed a ticket[3] to implement retry with
backoff (this patchset is big enough already!) and put it in
10.3.1 milestone (that's up for discussion).
[3] https://fedorahosted.org/pki/ticket/2293
Right now, the following events cause authority reinitialisation,
entailing key retrieval if necessary:
- Dogtag is restarted
- LDAP disconnect-reconnect
- LDAP modification of authority replicated from another clone
97- ACK
98 - ACK
Thanks. Any feedback on patch 0099?
Attachments:
- pki-ftweedal-0096-5-Lightweight-CAs-add-key-retrieval-framework.patch (text/plain — 15.6 KB)