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

[Endi Sukma Dewata]

Thanks for the review. I have some questions below. I'll post a new 
patch after the issues are addressed.

On 11/4/2014 11:36 AM, Petr Viktorin wrote:
> The new schema can go to 60basev3.ldif, no need for a new file.

Fixed. Also removed nsContainer as suggested by Simo.

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

Actually the command that generates the NotFound exception is the last 
command. The first two commands are preparing the parameters. I moved 
them before the try-block.

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

This is actually needed to construct a parent DN from an existing DN:

   parent_dn = DN(dn[1:])

Note that the parameter is an array of RDNs, not a sequence of RDNs. I 
don't see an existing mechanism to get a parent DN from a DN. Without 
this change it would generate an error:

ValueError: tuple or list must be 2-valued, not 
"[ipapython.dn.RDN('cn=admin'), ipapython.dn.RDN('cn=users'), 
ipapython.dn.RDN('cn=vaults'), ipapython.dn.RDN('dc=idm'), 
ipapython.dn.RDN('dc=lab'), ipapython.dn.RDN('dc=bos'), 
ipapython.dn.RDN('dc=redhat'), ipapython.dn.RDN('dc=com')]"

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

I found 20 star imports in the existing plugins :/

All existing plugins are actually using LDAPObject directly, not 
baseldap.LDAPObject. Is there a concern doing that? I fixed the code to 
do the same thing.

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

Fixed.

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

This is actually copied verbatim from user's uid. Could you give some 
sample values that show the problem?

I think ideally there should be a similar check for parent, but I 
haven't figured out the proper pattern yet. I think we can do this as a 
later enhancement. Or maybe we should remove the validation for cn for now.

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

I'm still considering some options for container ID:
a) use a slash at the end for all containers (e.g. /users/)
b) don't use a trailing slash, except for root container (/)

I think option (a) is more consistent, container ID always ends with 
slash, although in cases like this it can be redundant. This is how the 
code is currently written. We can make an exception for 
vaultcontainer.get_dn() though.

Option (b) will require more careful coding in many places (e.g. 
concatenations) because it needs to handle a special case for root 
container.

Maybe a better way is to use an array of names internally and the slash 
is only used during input/output. The problem is when calling another 
command the array has to be converted into string and parsed back into 
array, so it may even introduce more redundancies. I don't see a way to 
pass an object parameter to a command. Any suggestion?

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

Fixed.

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

Yes, but my assumption is the DN will always be within the namespace 
because it's only used internally, and using len() will be faster than 
endswith(). The error case will never happen, but I was just making sure 
there won't be an infinite loop. Maybe instead of calling the method 
recursively I should just use a loop? That will avoid repetitive 
validations.

> I sugggest writing doctests for the id manipulation methods (get_id,
> get_private_id, ...) – it's not always obvious what exactly they do.

Do you mean sample code in docs? Is it still necessary if we have some 
test code?

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

Yeah, as mentioned above I'm still considering several options. The 
design may need to be adjusted.

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

Hmm.. I'm not sure if we should write a code that will duplicate 
everything done by the LDAPCreate class. Shouldn't server-side 
command-to-command invocation be relatively faster?

The recursion is mainly used to create the user's private container. 
Note that the private container is not created by default. We only 
provide /users top-level container. When a user creates a private vault 
the command will automatically create the /users/<username> private 
container on the user's behalf. This will only happen once for each 
user. In most other cases the parent would already exist.

Another option is to require the user or the admin to create the private 
container manually, but that would be less user-friendly.

Another option is to check the parent container using direct LDAP 
search, but still do the add using vaultcontainer_add. What do you think?

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

This seems to be LDAPDelete's built-in functionality. Is there an option 
to disallow non-empty subtree removal?

For vault purposes, when a user is removed from IPA the server will 
remove the user's entire private container, so the feature would 
actually be useful here. But probably in general we should warn the user 
if they are deleting a non-empty container. If there's an option in 
LDAPDelete we can let the user specify that option if necessary.

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

Actually, no. The primary way to archive data is via vault_archive. The 
vault_add is for creating a vault, but optionally you can specify the 
initial data to be archived. If nothing is specified it will archive an 
empty string. Note that for symmetric vault the encrypted empty string 
can be used to enforce that the same password will be used on on 
subsequent archival operations.

I'm trying to change vault_archive to raise an error if it's missing the 
input data and change vault_add to archive empty string if nothing is 
specified, but it doesn't work since the framework seems to be 
converting the empty string into None, so vault_archive can't tell if 
it's archiving an empty string or missing the input data. Any suggestion?

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

Yes, that's due to a recent 'standard vault' addition. I have not added 
vault_archive yet to the design. The vault_archive is used to archive a 
generic blob of data. The vaultsecret_archive will later be used to add 
secrets into a JSON structure which will then be archived using the 
vault_archive.

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

I think so, but as I mentioned earlier I haven't look very far on 
attribute validations.

The slashes in vault ID is converted into dashes in KRA's client key ID. 
KRA actually accepts slashes, but the slashes will not look pretty in 
REST URL because they will be URL-encoded (in case it's needed for 
troubleshooting KRA via browser) so I changed them to dashes.

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

Right, but I'm not sure how it will work with the IPA framework. The 
first command parameter is the cn (vault name). If we specify a vault ID 
(with slashes) there it will be entered into cn. Should we parse the ID 
and insert the vault name and parent ID back into keys and options 
variables? Which value will eventually be stored into cn in LDAP?

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

See the above explanation. What does primary_key actually mean in 
hierarchical structure like vault? Since cn is only unique locally, is 
cn a primary key? Can a virtual attribute be a primary key?

> There are no tests.

Right, I'll take a look at how to write tests. Is there a way to run 
tests without running the full build?

-- 
Endi S. Dewata