[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

Hi Endi,

Dne 24.2.2015 v 04:09 Endi Sukma Dewata napsal(a):
> On 2/16/2015 2:50 AM, Endi Sukma Dewata wrote:
>> Hi,
>>
>> Attached are the updated patches for the password vault, and some new
>> ones (please disregard previous patch submissions). Please give them a
>> try. Thanks.
>
> New patches attached replacing all previous vault patches. They include
> the new escrow functionality, changes to the asymmetric vault, and some
> cleanups. Thanks.
>

Patch 353:

1) Please follow PEP8 in new code.

The pep8 tool reports these errors in existing files:

./ipalib/constants.py:98:80: E501 line too long (84 > 79 characters)
./ipalib/plugins/baseldap.py:1527:80: E501 line too long (81 > 79 
characters)
./ipalib/plugins/user.py:915:80: E501 line too long (80 > 79 characters)

as well as many errors in the files this patch adds.


2) Pylint reports the following error:

ipatests/test_xmlrpc/test_vault_plugin.py:153: 
[E0102(function-redefined), test_vault] class already defined line 27)


3) The container_vault config option should be renamed to 
container_vaultcontainer, as it is used in the vaultcontainer plugin, 
not the vault plugin.


4) The vault object should be child of the vaultcontainer object.

Not only is this correct from the object model perspective, but it would 
also make all the container_id hacks go away.


5) When specifying param flags, use set literals.

This is especially wrong, because it's not a tuple, but a string in 
parentheses:

+            flags=('virtual_attribute'),


6) The `container` param of vault should actually be an option in 
vault_* commands.

Also it should be renamed to `container_id`, for consistency with 
vaultcontainer.


7) The `vault_id` param of vault should have "no_option" in flags, since 
it is output-only.


8) Don't translate docstrings where not necessary:

+    def get_dn(self, *keys, **options):
+        __doc__ = _("""
+        Generates vault DN from vault ID.
+        """)

Only plugin modules and classes should have translated docstrings.


9) This looks wrong in vault.get_dn() and vaultcontainer.get_dn():

+        name = None
+        if keys:
+            name = keys[0]

Primary key of the object should always be set, so the if statement 
should not be there.

Also, primary key of any given object is always last in "keys", so use 
keys[-1] instead of keys[0].


10) Use "self.api" instead of "api" to access the API in plugins.


11) No clever optimizations like this please:

+        # vault DN cannot be the container base DN
+        if len(dn) == len(api.Object.vaultcontainer.base_dn):
+            raise ValueError('Invalid vault DN: %s' % dn)

Compare the DNs by value instead.


12) vault.split_id() is not used anywhere.


13) Bytes is not base64-encoded data:

+        Bytes('data?',
+            cli_name='data',
+            doc=_('Base-64 encoded binary data to archive'),
+        ),

It is base64-encoded in the CLI, but on the API level it is not. The doc 
should say just "Binary data to archive".


14) Use File instead of Str for input files:

+        Str('in?',
+            cli_name='in',
+            doc=_('File containing data to archive'),
+        ),


15) Use MutuallyExclusiveError instead of ValidationError when there are 
mutually exclusive options specified.


16) You do way too much stuff in vault_add.forward(). Only code that 
must be done on the client needs to be there, i.e. handling of the 
"data", "text" and "in" options.

The vault_archive call must be in vault_add.execute(), otherwise a) we 
will be making 2 RPC calls from the client and b) it won't be called at 
all when api.env.in_server is True.


17) Why are vaultcontainer objects automatically created in vault_add?

If you have to automatically create them, you also have to automatically 
delete them when the command fails. But that's a hassle, so I would just 
not create them automatically.


18) Why are vaultcontainer objects automatically created in vault_find?

This is just plain wrong and has to be removed, now.


19) What is the reason behind all the json stuff in vault_transport_cert?

vault_transport_cert.__json__() is exactly the same as 
Command.__json__() and hence redundant.


20) Are vault_transport_cert, vault_archive and vault_retrieve supposed 
to be runnable by users? If not, add "NO_CLI = True" to the class 
definition.


21) vault_archive is not a retrieve operation, it should be based on 
LDAPUpdate instead of LDAPRetrieve. Or Command actually, since it does 
not do anything with LDAP. The same applies to vault_retrieve.


22) vault_archive will break with binary data that is not UTF-8 encoded 
text.

This is where it occurs:

+        vault_data[u'data'] = unicode(data)

Generally, don't use unicode() on str values and str() on unicode values 
directly, always use .decode() and .encode().


23) Since vault containers are nested, the vaultcontainer object should 
be child of itself.

There is no support for nested objects in the framework, but it 
shouldn't be too hard to do anyway.


24) Instead of:

+        while len(dn) > len(self.base_dn):
+
+            rdn = dn[0]
...
+            dn = DN(*dn[1:])

you can do:

+        for rdn in dn[:-len(self.base_dn)]:
...


25) Why are parent vaultcontainer objects automatically created in 
vaultcontainer_add?


26) Instead of the delete_entry refactoring in baseldap and 
vaultcontainer_add, you can put this in vaultcontainer_add's pre_callback:

     try:
         ldap.get_entries(dn, scope=ldap.SCOPE_ONELEVEL, attrs_list=[])
     except errors.NotFound:
         pass
     else:
         if not options.get('force', False):
             raise errors.NotAllowedOnNonLeaf()

27) Why are parent vaultcontainer objects automatically created in 
vaultcontainer_find?


28) The vault and vaultcontainer plugins seem to be pretty similar, I 
think it would make sense to put common stuff in a base class and 
inherit vault and vaultcontainer from that.


More later.

Honza

-- 
Jan Cholasta