Dne 19.5.2015 v 16:40 Endi Sukma Dewata napsal(a):
Before I send another patch I have some questions below.

On 5/19/2015 3:27 AM, Jan Cholasta wrote:
I changed the 'host vaults' to become 'service vaults'. The interface
will look like this:

$ ipa vault-find --service HTTP/server.example.com
$ ipa vault-add test --service HTTP/server.example.com

I also added user vaults:

$ ipa vault-find --user testuser
$ ipa vault-add test --user testuser

Private vaults is a special case of user vaults where username=<you>.

Host vaults can be added later once we define the use case.


I suppose you meant you're OK with not adding host vaults now?


4. The following code in get_dn() is incorrect:

   principal = getattr(context, 'principal')
   (name, realm) = split_principal(principal)
   name = name.split('/')
   if len(name) == 1:
       container_dn = DN(('cn', 'users'), container_dn)
       container_dn = DN(('cn', 'services'), container_dn)
   container_dn = DN(('cn', name[-1]), container_dn)

A service does not have a private container like users
cn=users, cn=vaults). The entry cn=<name>, cn=<host>, cn=services,
cn=vaults is a service vault, not a container. The service vault is
by the admin to provide a secret for a service.

I'm not sure what the behavior should be if a service is executing a
vault command that uses a private container such as:

   $ ipa vault-add test

Maybe it should just generate an error.

Users, hosts and services are all user-like objects, is there a reason
not to support private vaults for all of them?

As mentioned above, it's not required in the design doc, but we can add
it if there's a clear use case. I agree that at least for now we can
change the service vault into a service container to store multiple
service's private vaults.

I don't really care about having a clear use case, I would prefer if the
design was elegant enough to handle *all* the cases without any extra

The only way to know if the design will be future proof is if we have at
least some idea how it will be used. Without that there is no guarantee.

Host principals have this form: host/<hostname>@<realm>, so with the
current code they will be considered a service and will have a service

Do you want to add a new cn=hosts container just for hosts? Unless we
have a specific reason (i.e. use case) I don't see a need to add
specific code for hosts now, or at least until we get the core vault
functionality working.

The reason is consistency. Private vaults should be available for all identities, because anything else would be an arbitrary limitation (which is not elegant). If private vaults were available for all identities, we would need a container for host vaults. I'm not saying the container has to be added now, but there should at least be a check to reject requests when the authenticated identity is a host (i.e. context.principal.startswith('host/')).

5. In create_container() why do you need to reconstruct the
on each invocation even though the value is fixed?

   container_dn = DN(self.container_dn, self.api.env.basedn)

Because self.api may not necessarily be the same as ipalib.api.

Under what scenario would that be a problem?

When someone uses the plugin with a different API object than ipalib.api.

The original code seems to
be working fine with ipalib.api.

The current best practice is to use self.api and *all* new plugin code
should do that.

If it is a problem, why do we still use ipalib.api to initialize
container_dn vault class attribute?

   container_dn = api.env.container_vault

Then in get_dn() we basically construct the container_dn variable with
values from both self.api and ipalib.api:

   container_dn = DN(self.container_dn, self.api.env.basedn)

When is the self.api actually initialized? Can we initialize the
container_dn (or base_dn as in the original code) attribute immediately
after that?

Not yet, but this will be fixed in the future. (Also, container_dn is
part of the LDAPObject API, unlike base_dn used in the original code.)

Is there a ticket for this?

I don't think there is a ticket for this particular issue.

This change is not included. The code will now obtain the values from
apilib.api.env at init time and store it in class attributes so it can
be reused.

     container_dn = api.env.container_vault
     base_dn = DN(container_dn, api.env.basedn)

Sorry, but no. Please just follow the best practice instead of trying to
invent something new. This is not the right time and place to discuss
this. We should be discussing the vault, not framework idiosyncracies.


Thanks for understanding.

6. The loop in create_container() is incorrect. Suppose we're
creating a
container cn=A, cn=B, <suffix> and the parent container cn=B,
doesn't exist yet. The first add_entry() invocation will fail as
expected, but instead of adding the parent entry the whole method

Right, fixed.

It's still not working. The new code is trying to add cn=vaults first,
and it stops because it already exists, but the intermediary entries
still not added. Also the DN displayed in the message misleading:

$ ipa vault-add test
ipa: ERROR: container entry (cn=vaults) not found

$ ipa vault-add test --host server.example.com
ipa: ERROR: container entry (cn=vaults) not found

The unit tests are failing because of this.

I forgot to remove the break statement in the for loop.

This change is not included. The original code and the tests work just

I would prefer if it was done without recursion, like my code does:

+    def create_container(self, dn):
+        """
+        Creates vault container and its parents.
+        """
+        container_dn = DN(self.container_dn, self.api.env.basedn)
+        assert dn.endswith(container_dn)
+        dns = []
+        while dn.endswith(container_dn):
+            dns.insert(0, dn)
+            dn = DN(*dn[1:])
+        for dn in dns:
+            entry = self.backend.make_entry(
+                dn,
+                objectclass=['nsContainer'],
+                cn=[dn[0]['cn']],
+            )
+            try:
+                self.backend.add_entry(entry)
+            except errors.DuplicateEntry:
+                pass

There is an obvious inefficiency here: all containers in the path
(including the built-in ones) will be re-added on every vault-add

I don't see anything wrong with recursions, especially if it works more
efficiently since only the immediate parent will be re-added.

I tend to stick to the "don't use recursion in production code" rule, epsecially in simple cases like this one.

So for example, with the loop every time you add a private vault you're
guaranteed to have three failed LDAP Add operations whereas with the
recursion there's only one failed operation.

This is not about loop vs. recursion, this is about the used approach. Your code can be easily transformed into a loop while keeping the same approach:

    entries = []

    while dn:
        assert dn.endswith(self.base_dn)

        rdn = dn[0]
        entry = self.backend.make_entry(
                'objectclass': ['nsContainer'],
                'cn': rdn['cn'],

        # if entry can be added, return

        except errors.NotFound:

        # otherwise, create parent entry first
        dn = DN(*dn[1:])
        entries.insert(0, entry)

    for entry in entries:
        # then create the entry itself again

Do you still want to use the loop?

Yes please.

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