[Freeipa-devel] [PATCH] 353 Added initial vault implementation.

[Petr Viktorin]

On 11/04/2014 07:27 AM, Endi Sukma Dewata wrote:
> On 10/28/2014 5:34 PM, Endi Sukma Dewata wrote:
>>>>> The NSSConnection class has to be modified not to shutdown existing
>>>>> database because some of the vault clients (e.g. vault-archive and
>>>>> vault-retrieve) also use a database to encrypt/decrypt the secret.
>>>>
>>>> The problem is described in more detail in this ticket:
>>>> https://fedorahosted.org/freeipa/ticket/4638
>>>>
>>>> The changes to the NSSConnection in the first patch caused the
>>>> installation to fail. Attached is a new patch that uses the solution
>>>> proposed by jdennis.
>>>
>>> New patch attached. It's now using the correct OID's for the schema. It
>>> also has been rebased on top of #352-1 and #354.
>>
>> New patch attached to fix the ticket URL. It depends on #352-2 and
>> #354-1.
>
> New patch attached to fix vault/container ID problems and for some
> cleanups.

The new schema can go to 60basev3.ldif, no need for a new file.

ipalib/plugins/user.py: Make the try: block as small as possible; maybe 
something like this:

      try:
          vaultcontainer_id = ...
      except errors.NotFound:
          pass
      else:
         (vaultcontainer_name, vaultcontainer_parent_id) = ...
         self.api.Command.vaultcontainer_del(...)

ipapython/dn.py: This change is not needed. If you have a sequence of 
RNDs you can do `DN(*seq)`.


ipalib/plugins/vault.py:
Do not use star imports in new code; remove the line
      from ipalib.plugins.baseldap import *
and use e.g. `baseldap.LDAPObject` instead of just `LDAPObject`.
Hopefully the only star-imported used are the base classes?

To make translators' jobs easier, split the help text in __doc__ into 
strings that can be translated individually.
Replace every blank line by:
""") + _("""

The pattern and pattern_errmsg for the 'cn' param don't match. Which one 
is right? Shouldn't a similar check be there for parent?

vaultcontainer.get_dn: Why put '/' at the end of container_id, if the 
empty string is ignored later anyway?

Nitpick: In vaultcontainer.get_dn, prefer the if statement over the 
if-else expressions; it's a bit longer but more readable.

In vaultcontainer.get_id, what's the purpose of the len() check? Did you 
want dn.endswith() instead?

I sugggest writing doctests for the id manipulation methods (get_id, 
get_private_id, ...) – it's not always obvious what exactly they do.
According to the design doc, container IDs shouldn't end in '/'. If you 
did that I think the manipulation would be simpler, and it could be 
shared with the vault equivalents.

vaultcontainer_add: You should use ldap backend directly. Calling 
Commands is costly, most of the call is spent doing validation of what 
here is already validated. You should add a function to recursively 
create vault container using just the ldap client, and call that here 
and in vault_add.

You can delete a container with children; is that expected?

vault_add should complain if it does not get exactly one of data/text/in.

What's the difference between vault_add and vault_archive? I don't see 
vault_archive in the design.

It seems '/' is equivalent to '-' as far as KRA is concerned; should we 
disallow '-' in container/vault names?

You can specify an absolute id by starting it with a slash, but only in 
--parent and not in the name itself. I think this should be possible in 
the name too.

You can't include slashes in the name, so you always need to specify the 
prefix with --parent. I don't think there's a technical reason for this 
limitation.

There are no tests.

-- 
Petr³