Dne 11.3.2015 v 15:12 Endi Sukma Dewata napsal(a):
Thanks for the review. New patch attached to be applied on top of all
previous patches. Please see comments below.


Thanks. I have replied to some of your comments below.


On 3/6/2015 3:53 PM, Jan Cholasta wrote:
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.

For some reason pylint keeps crashing during build so I cannot run it
for all files. I'm fixing the errors that I can see. If you see other
errors please let me know while I'm still trying to figure out the problem.

Well, I did not use pylint, but pep8: <http://pep8.readthedocs.org/en/latest/>


Is there an existing ticket for fixing PEP8 errors? Let's use that for
fixing the errors in the existing code.

There is no ticket, but we still follow PEP8 in new code, so please do that. It shouldn't be too hard.

...

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

It was named container_vault because it defines the DN for of the
subtree that contains all vault-related entries. I moved the base_dn
variable from vaultcontainer object to the vault object for clarity.

That does not make much sense to me. Vault objects are contained in their respective vaultcontainer objects, not directly in cn=vaults.


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.

It's a bit difficult because it will affect how the container & vault
ID's are represented on the CLI.

Yes, but the API should be done right (without hacks) first. You can tune the CLI after that if you want.


In the design the container ID would be a single value like this:

   $ ipa vault-add /services/server.example.com/HTTP

And if the vault ID is relative (without initial slash), it will be
appended to the user's private container (i.e. /users/<username>/):

   $ ipa vault-add PrivateVault

The implementation is not complete yet. Currently it accepts this format:

   $ ipa vault-add <vault name> [--container <container ID>]

and I'm still planning to add this:

   $ ipa vault-add <vault ID>

If the vault must be a child of vaultcontainer, and the vaultcontainer
must be a child of a vaultcontainer, does it mean the vault ID would
have to be split into separate arguments like this?

   $ ipa vaultcontainer-add services server.example.com HTTP

If that's the case we'd lose the ability to specify a relative vault ID.

Yes, that's the case.

But I don't think relative IDs should be a problem, we can do this:

    $ ipa vaultcontainer-add a b c  # absolute
    $ ipa vaultcontainer-add . c    # relative

or this:

    $ ipa vaultcontainer-add '' a b c  # absolute
    $ ipa vaultcontainer-add c         # relative

or this:

    $ ipa vaultcontainer-add a b c         # absolute
    $ ipa vaultcontainer-add c --relative  # relative

or this:

    $ ipa vaultcontainer-add a b c --absolute  # absolute
    $ ipa vaultcontainer-add c                 # relative

...

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.

Actually the DN values have already been compared in the code right
above it:

     # make sure the DN is a vault DN
     if not dn.endswith(self.api.Object.vaultcontainer.base_dn):
         raise ValueError('Invalid vault DN: %s' % dn)

This code confirms that the incoming vault DN is within the vault
subtree. After that, the DN length comparison above is just to make sure
the incoming vault DN is not the root of the vault subtree itself. It
doesn't need to compare the values again.

I see. You can combine both of the checks into one:

    if not dn.endswith(self.api.Object.vaultcontainer.base_dn, 1):
        raise ValueError(...)

...

14) Use File instead of Str for input files:

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

The File type doesn't work with binary files because it tries to decode
the content.

OK. I know File is broken and plan to fix it in the future. Just add a comment saying that it should be a File, but it's broken, OK?

...

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.

This is done by design. The vault_add.forward() generates the salt and
the keys. The vault_archive.forward() will encrypt the data. These
operations have to be done on the client side to secure the transport of
the data from the client through the server and finally to KRA. This
mechanism prevents the server from looking at the unencrypted data.

OK, but that does not justify that it's broken in server-side API. It can and should be done so that it works the same way on both client and server.

I think the best solution would be to split the command into two commands, server-side vault_archive_raw to archive already encrypted data, and client-side vault_archive to encrypt data and archive them with vault_archive_raw in its .execute(). Same thing for vault_retrieve.

BTW, I also think it would be better if there were 2 separate sets of commands for binary and textual data (vault_{archive,retrieve}_{data,text}) rather than trying to handle everything in vault_{archive,retrieve}.


The add & archive combination was added for convenience, not for
optimization. This way you would be able to archive data into a new
vault using a single command. Without this, you'd have to execute two
separate commands: add & archive, which will result in 2 RPC calls anyway.

I think I would prefer if it was separate, as that would be consistent with other plugins (e.g. for objects with members, we don't allow adding members directly in -add, you have to use -add-member after -add).


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.

The vaultcontainer is created automatically to provide a private
container (i.e. /users/<username>/) for the each user if they need it.
Without this, the admin will have to create the container manually first
before a user can create a vault, which would be an unreasonable
requirement. If the vault_add fails, it's ok to leave the private
container intact because it can be used again if the user tries to
create a vault again later and it will not affect other users. If the
user is deleted, the private container will be deleted too.

The code was fixed to create the container only if they are adding a
vault/vault container into the user's private container. If they are
adding into other container, the container must already exist.

This sounds like a job fit for the managed entries plugin. Have you tried using it for this?


18) Why are vaultcontainer objects automatically created in vault_find?

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

The code was supposed to create the user's private container like in
#17, but the behavior has been changed. If the container being searched
is the user's private container, it will ignore the container not found
error and return zero results as if the private container already
exists. For other containers the container must already exist. For this
to work I had to add a handle_not_found() into LDAPSearch so the plugins
can customize the proper search response for the missing private container.

No ad-hoc refactoring please. If you want to refactor anything, it should be first designed properly and put in a separate patch.

Anyway, what should actually happen here is that if parent object is not found, its object plugin's handle_not_found is called, i.e. something like this:

    parent = self.obj.parent_object
    if parent:
        self.api.Object[parent].handle_not_found(*args[:-1])
    else:
        raise errors.NotFound(
            reason=self.obj.container_not_found_msg % {
                'container': self.obj.container_dn,
            }
        )

...

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.

The vault_archive does not actually modify the LDAP entry because it
stores the data in KRA. It is actually an LDAPRetrieve operation because
it needs to get the vault info before it can perform the archival
operation. Same thing with vault_retrieve.

It is not a LDAPRetrieve operation, because it has different semantics. Please use Command as base class and either use ldap2 for direct LDAP or call vault_show instead of hacking around LDAPRetrieve.


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().

It needs to be a Unicode because json.dumps() doesn't work with binary
data. Fixed by adding base-64 encoding.

If something str needs to be unicode, you should use .decode() to explicitly specify the encoding, instead of relying on unicode() to pick the correct one.

Anyway, I think a better solution than base64 would be to use the "raw_unicode_escape" encoding:

    if data:
        data = data.decode('raw_unicode_escape')
    elif text:
        data = text
    elif input_file:
        with open(input_file, 'rb') as f:
            data = f.read()
        data = data.decode('raw_unicode_escape')
    else:
        data = u''

    ...

    vault_data[u'data'] = data

...

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

I suppose you meant vaultcontainer_del. Fixed, but this will generate an
additional search for each delete.

I'm leaving the changes baseldap because it may be useful later and it
doesn't change the behavior of the current code.

Again, no ad-hoc refactoring please.

...

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.

I plan to refactor the common code later. Right now the focus is to get
the functionality working correctly first.

Please do it now, "later" usually means "never". It shouldn't be too hard and I can give you a hand with it if you want.

--
Jan Cholasta

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