On 9.6.2016 06:07, Fraser Tweedale wrote:
Updated patches 0053-6 and 0054-6 attached.  Comments inline.


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:


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?

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

    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

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!

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.

Jan Cholasta

Manage your subscription for the Freeipa-devel mailing list:
Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code

Reply via email to