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.

OK.

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)
   else:
       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=<username>,
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
used
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
effort.

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 container.

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.

5. In create_container() why do you need to reconstruct the
container_dn
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?

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.

OK.

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, <suffix>
doesn't exist yet. The first add_entry() invocation will fail as
expected, but instead of adding the parent entry the whole method will
fail.

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 are
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
fine.

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 operation.

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

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.

Do you still want to use the loop?

--
Endi S. Dewata

--
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

Reply via email to