[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

Dne 3.4.2015 v 05:37 Endi Sukma Dewata napsal(a):
> Hi,
>
> Attached are new patches replacing all old ones. Please take a look at
> them. They should applied in this order: 365, 353-8, 355-6, 357-3,
> 359-2, 360-1, 364-1, 361-1.

Thanks for squashing patches 362-364 into the original patches, it's 
much more digestible this way.

>
> I'm planning to merge the vault and vault container object and use the
> vault type attribute to distinguish between the two. See more discussion
> about that below.

OK.

>>>> 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.
>
> The cn=vaults itself is actually a vault container (i.e.
> ipaVaultContainer). Theoretically you could store a vault in any
> container including cn=vaults, but we just don't want people to use it
> that way.
>
> I think this is consistent with other plugins. For example, the
> container_user points to cn=users, which is an nsContainer. There is no
> concept of 'user container' other than the cn=users itself. But even if
> there is one, the container_user will still be stored in the user class.

In fact it is not consistent with other plugins. All entries managed by 
the user plugin are stored *directly* under cn=users. Entries managed by 
the vault plugin are not stored directly under cn=vaults, but rather 
anywhere in the cn=vaults subtree and their DN is derived from the DN of 
the parent vault container. For such objects, we don't set 
<plugin>.container_dn and don't have container_<plugin> constant, but 
rather define them as child objects of their container objects.

>
> When the vault & vaultcontainer is merged, this will no longer be an issue.

OK.

>
>>>> 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.
>
> I think the current framework is rather limiting. It's kind of hard to
> build an interface that looks exactly what you want then add the
> implementation later to match the interface because many things are
> interrelated. In this particular case the object hierarchy on the server
> side would affect how the vault ID will be represented on the client side.

It indeed is limiting and that's a good thing. We don't want people to 
be able to create any crazy interfaces they can imagine, inconsistent 
with everything else in IPA.

>
>>> 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>
>
> This is actually now done in the latest patch. Internally the ID is
> still split into name & parent 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
>
> I think a "." will be confusing because there's no concept of "current
> vaultcontainer" like "current directory".
>
>> or this:
>>
>>      $ ipa vaultcontainer-add '' a b c  # absolute
>>      $ ipa vaultcontainer-add c         # relative
>
> An empty string is also confusing and can be problematic to distinguish
> with missing argument.

I didn't mean empty string specifically, it could have been any special 
value.

>
>> 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
>
> Per discussion in the IPA-CS meeting, we'd rather keep the "/" for vault
> ID delimiters because the spaces will be confusing to users, but we'll
> not use absolute ID anymore.

I'm sorry if I gave you the impression that this is up for discussion, 
but it is not. You either follow the convention without doing ugly hacks 
or your patch will not be accepted.

It won't be confusing to users, because they are used to the convention.

>
> It's not implemented yet, but here is the plan. By default the vault
> will be created in the user's private container:
>
>    $ ipa vault-add PrivateVault
>
> For shared vaults, instead of specifying an absolute ID we can specify a
> --shared option:
>
>    $ ipa vault-add --shared projects/IPA
>
> Same thing with service vaults:
>
>    $ ipa vault-add --service server.example.com/LDAP
>
> To access a vault in another user's private container:
>
>    $ ipa vault-show --user testuser PrivateVault

Fine by me, as long as you follow the convention.

>
>>>> 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.
>
> Actually I think it's better to just merge the add and archive, reducing
> the number of RPC calls. The vault_archive now will support two types of
> operations:
>
> (a) Archive data into a new vault (it will create the vault just before
> archiving the data):
>
>    $ ipa vault-archive testvault --create --in data ...
>
> (b) Archive data into an existing vault:
>
>    $ ipa vault-archive testvault --in data ...
>
> The vault_add is now just a wrapper for the vault_archive(a).

If that's just an implementation detail, OK.

If it's possible to modify existing vault objects using vault-add or 
create new objects using vault-archive, then NACK.

>
>> 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}.
>
> I don't think we want to provide a separate command of every possible
> data type & operation combination. Users would get confused. The archive
> & retrieve commands should be able to handle all current & future data
> types with options.

A command with two sets of mutually exclusive options is really two 
commands in disguise, which is a good sign it should be divided into two 
actual commands.

Who are you to say users would get confused? I say they would be more 
confused by a command with a plethora of mutually exclusive "options".

What other possible data types are there?

>
>>> 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).
>
> The vault data is similar to group description, not group members. When
> creating a group we can supply the description. If not specified it will
> be blank. Archiving vault data is similar to updating the group
> description.

It's similar to group members because there are separate commands to 
manipulate them.

You have to choose one of:

   a) vault data is settable using vault-add and vault-mod and gettable 
using vault-mod, vault-show and vault-find

   b) vault data is settable using vault-archive and gettable using 
vault-retrieve

Anything in between is not permitted.

>
> Vault secrets on the other hand is similar to group members. You will
> see that in the other patch.

>
>>>> 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,
>>              }
>>          )
>
> It will not work because vault doesn't have a parent object. I'm adding
> handle_not_found() into LDAPCreate and LDAPSearch in the first patch.

NACK, this change exists for the sole reason of supporting your hacks. 
Follow IPA convetions and this change won't be necessary.

>
>>>> 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.
>
> It's been changed to inherit from LDAPQuery instead.

NACK, it's not a LDAPQuery operation, because it has different 
semantics. There is more to a command than executing code, so you should 
use a correct base class.

>
>>>> 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().
>
> The unicode(s, encoding) is actually equivalent to s.decode(encoding),
> so the following code will not solve the problem:
>
>    vault_data[u'data'] = data.decode()
>
> As you said, decode() will only work if the data being decoded actually
> follows the encoding rules (i.e. already UTF-8 encoded).
>
>>> It needs to be a Unicode because json.dumps() doesn't work with binary
>>> data. Fixed by adding base-64 encoding.
>
> The base-64 encoding is necessary to convert random binaries into ASCII
> so it can be decoded into Unicode. Here is the current code:
>
>    vault_data[u'data'] = unicode(base64.b64encode(data))
>
> which is equivalent to:
>
>    vault_data[u'data'] = base64.b64encode(data).decode()

If you read a little bit further, you would get to the point, which is 
certainly not calling .decode() without arguments, but *always 
explicitly specify the 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.
>
> Since we know this is ASCII data we can now specify UTF-8 encoding.
>
>    vault_data[u'data'] = base64.b64encode(data).decode('utf-8')
>
> But for anything that comes from user input (e.g. filenames, passwords),
> it's better to use the default encoding because that can be configured
> by the user.

You are confusing user's configured encoding with Python's default 
encoding. Default encoding in Python isn't derived from user's 
localization settings.

Anyway, anything that comes from user input is already decoded using 
user's configured encoding when it enters the framework so I don't know 
why are you even bringing it up here.

>
>> Anyway, I think a better solution than base64 would be to use the
>> "raw_unicode_escape" encoding:
>
> As explained above, base-64 encoding is necessary because random
> binaries don't follow any encoding rules. I'd rather not use
> raw_unicode_escape because it's not really a text data.

The result of decoding binary data using raw_unicode_escape is perfectly 
valid unicode data which doesn't eat 33% more space like base64 encoded 
binary does, hence my suggestion.

Anyway, it turns out when encoded in JSON, raw_unicode_escape string 
generally takes more space than base64 encoded string because of JSON 
escaping, so base64 is indeed better.

> Here's how it's
> now implemented:
>
>>      if data:
>>          data = data.decode('raw_unicode_escape')
>
> Input data is already in binaries, no conversion needed.
>
>>      elif text:
>>          data = text
>
> Input text will be converted to binaries with default encoding:
>
>    data = text.encode()

See what the default encoding actually is and why you shouldn't rely on 
it above.

>
>>      elif input_file:
>>          with open(input_file, 'rb') as f:
>>              data = f.read()
>>          data = data.decode('raw_unicode_escape')
>
> Input contains binary data, no conversion needed.
>
>>      else:
>>          data = u''
>
> If not specified, the data will be empty string:
>
>    data = ''
>
> The data needs to be converted into binaries so it can be encrypted
> before transport (depending on the vault type):
>
>    data = self.obj.encrypt(data, ...)
>
>>      vault_data[u'data'] = data
>
> Then for transport the data is base-64 encoded first, then converted
> into Unicode:
>
>    vault_data[u'data'] = base64.b64encode(data).decode('utf-8')

>
>>>> 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.
>
> The refactoring has also been moved into a separate patch. Just a note,
> I still don't think a plugin should do a search and maybe generate a
> NotAllowedOnLeaf exception on each delete operation. The exception
> should have been generated automatically by the DS. But we can discuss
> that separately.

NACK, turns out there is a better (and preferable) solution I didn't 
remember before, you can use exception callback in vaultcontainer_del:

     def exc_callback(self, keys, options, exc, call_func, *call_args, 
**call_kwargs):
         if call_func.func_name == 'delete_entry':
             if isinstance(exc, errors.NotAllowedOnLeaf):
                 if not options.get('force', False):
                     raise errors.DatabaseError(...)
         raise exc

>
>>>> 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.
>
> As mentioned above, I'm considering merging the vault & vault container
> classes, so no need to refactor the common code out of these classes.
> This will be delivered as a separate patch later.

OK.

>
> Thanks.
>


-- 
Jan Cholasta