On 9.6.2016 08:31, Fraser Tweedale wrote:
On Thu, Jun 09, 2016 at 07:49:51AM +0200, Jan Cholasta wrote:
On 9.6.2016 06:07, Fraser Tweedale wrote:
Updated patches 0053-6 and 0054-6 attached. Comments inline.
Thanks,
Fraser
On Wed, Jun 08, 2016 at 10:31:07AM +0200, Jan Cholasta wrote:
Patch 0052:
The target of the "Dogtag service principals can search Custodia keys" ACI
matches keys in the top-level Custodia container, but not in the Dogtag
container. Is this intentional?
ACI applies to all entries under the 'cn=custodia' tree, not only
the entries directly under it.
You are right:
<https://access.redhat.com/documentation/en-US/Red_Hat_Directory_Server/8.2/html/Administration_Guide/Managing_Access_Control-Creating_ACIs_Manually.html#Defining_Targets-Targeting_a_Directory_Entry>
Mea culpa, I thought the target matching behavior was simpler than this.
Anyway, could the ACI be tightened to allow access only to the Dogtag
subtree?
Dogtag does need access to the host Custodia keys.
I see, the ACI is OK then.
I'm not sure how
to tighten the ACI to prevent access to "deeper" levels. In any
case they're all public keys and it's read only access, so I don't
think it's an issue.
Right.
Patch 0053:
It seems the `servicename`+`host` and `principal` arguments of set_key() are
carrying the same information, could you remove one of them?
OK, just `principal` now.
Patch 0054:
1) Please use ipalib.config to read IPA configuration in
ipa-pki-retrieve-key:
from ipalib.config import Env
env = Env(in_server=True)
hostname = env.host
realm = env.realm
I have implemented this; it was necessary to also call
`env._finalize()`.
Right. I wrote the above snippet off the top of my head, hence the omission.
2) I'm curious why you changed the key name from "ca/$NAME" to
"ca_wrapped/$NAME". Aren't *all* keys in Custodia wrapped?
The payload of the `ca_wrapped` Custodia store is a
PKIArchiveOptions object wrapped by the main CA's private key.
(This payload is then encrypted by Custodia, as all Custodia
payloads are).
See my patch 0056 [1] for implementation details.
[1] https://www.redhat.com/archives/freeipa-devel/2016-May/msg00079.html
Wow, I totally missed patch 0055 and 0056!
No worries; easily done because I sent more emails with fewer,
largely independent patches.
3) Given that Dogtag ExternalProcessKeyRetriever handles JSON *now*, I would
expect a minimum required version bump in the spec file.
Indeed; I added this in latest patches.
Thanks. ACK if the ACI in patch 52 does not need tightening.
Thanks for reviewing!
Pushed to master: b0d9a4728f0dc78e2bbde344beac17ae50b847a9
--
Jan Cholasta
--
Manage your subscription for the Freeipa-devel mailing list:
https://www.redhat.com/mailman/listinfo/freeipa-devel
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
