[Freeipa-devel] [PATCH] Password vault

Thu May 14 18:42:54 UTC 2015

Dne 14.5.2015 v 05:01 Endi Sukma Dewata napsal(a):
> On 5/13/2015 4:09 AM, Jan Cholasta wrote:
>> Dne 12.5.2015 v 12:52 Endi Sukma Dewata napsal(a):
>>> Please take a look at the attached patch (#353-9). It obsoletes all
>>> previous patches. See comments below.
>>>
>>> On 4/20/2015 1:12 AM, Jan Cholasta wrote:
>>>>> 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.
>>>
>>> The vault container plugin has been removed instead of merged (see
>>> explanation below). Internally the vaults are still stored in built-in
>>> containers in the DS, but there won't be an interface to manage them.
>>> The following containers are available for use: private, shared, and
>>> services, but they are flat, not hierarchical.
>>
>> To speed up the review, I have amended your patch with code and coding
>> style fixes (attached), please review my changes.
>>
>> Question: Services in IPA are identified by Kerberos principal. Why are
>> service vaults identified by hostname alone?
>
> The service vaults are actually identified by the hostname and service
> name assuming the principal is in this format: <name>/<host>@<realm>.
> The vault is stored in cn=<name>, cn=<host>, cn=services, cn=vaults,
> <suffix>. When accessing a vault service you are supposed to specify the
> name and the host (except for vault-find which will return all services
> in the same host):
>
> $ ipa vault-find --host <host>
> $ ipa vault-add <name> --host <host>
>
> Are there other service principal formats that we need to support? Do we
> need to support services of different realms?

Well, users are identified by username (uid attribute), hosts by 
hostname (fqdn attribute) and services by principal name 
(krbprincipalname attribute). Each of them has separate container and is 
also uniquely identified by principal name. I guess it would make sense 
to follow that for vaults as well, in which case service vaults should 
be called host vaults (because they are created in a host-specific 
container, not a service-specific one) and maybe "real" service vaults 
should be added.

> Do we
> need to support services of different realms?

That depends. Do we want to support vaults for users and services from 
AD trusts?

>
> Some comments about your changes:
>
> 1. The following code in get_dn() is causing problems for vault-find.
>
>    dn = super(vault, self).get_dn(*keys, **options)
>    rdn = dn[0]
>    container_dn = DN(*dn[1:])
>
> The super.get_dn() will return cn=vaults, <suffix>, so the rdn will
> become cn=vaults and container_dn will become <suffix>. When combined
> with the subcontainer (private/shared/service) it will produce an
> invalid DN.

Right, fixed.

I should have tested the patch before posting it, my bad, sorry.

>
> 2. Not sure if it'related to #1, the vault-find is failing.
>
> $ ipa vault-find
> ipa: ERROR: an internal error has occurred
>
> The error_log shows TypeError: 'NoneType' object is not iterable

Fixed. It was caused by missing return value in vault_find.exc_callback.

>
> 3. The --shared option in vault-find is now requiring an argument even
> though it's a Flag.
>
> $ ipa vault-find --shared
> Usage: ipa [global-options] vault-find [CRITERIA] [options]
>
> ipa: error: --shared option requires an argument

Fixed.

>
> 4. The following code in get_dn() is incorrect:
>
>    principal = getattr(context, 'principal')
>    (name, realm) = split_principal(principal)
>    name = name.split('/')
>    if len(name) == 1:
>        container_dn = DN(('cn', 'users'), container_dn)
>    else:
>        container_dn = DN(('cn', 'services'), container_dn)
>    container_dn = DN(('cn', name[-1]), container_dn)
>
> A service does not have a private container like users (cn=<username>,
> cn=users, cn=vaults). The entry cn=<name>, cn=<host>, cn=services,
> cn=vaults is a service vault, not a container. The service vault is used
> by the admin to provide a secret for a service.
>
> I'm not sure what the behavior should be if a service is executing a
> vault command that uses a private container such as:
>
>    $ ipa vault-add test
>
> Maybe it should just generate an error.

Users, hosts and services are all user-like objects, is there a reason 
not to support private vaults for all of them?

>
> 5. In create_container() why do you need to reconstruct the container_dn
> on each invocation even though the value is fixed?
>
>    container_dn = DN(self.container_dn, self.api.env.basedn)

Because self.api may not necessarily be the same as ipalib.api.

>
> 6. The loop in create_container() is incorrect. Suppose we're creating a
> container cn=A, cn=B, <suffix> and the parent container cn=B, <suffix>
> doesn't exist yet. The first add_entry() invocation will fail as
> expected, but instead of adding the parent entry the whole method will
> fail.
>
>    while dn.endswith(container_dn):
>        entry = self.backend.make_entry(
>            dn,
>            objectclass=['nsContainer'],
>            cn=[dn[0]['cn']],
>        )
>
>        try:
>            self.backend.add_entry(entry)
>        except errors.DuplicateEntry:
>            break
>
>        dn = DN(*dn[1:])

Right, fixed.

>
> 7. The host and shared parameters are no longer available in vault-show
> and vault-del commands. The unit tests are failing because of that. Why
> do these commands not automatically inherit all parameters from the class?
>

The parameters of Object plugins represent attributes of the object, so 
they are all provided in commands which allow setting values of the 
attributes (-add and -mod). Other commands need to only know which 
object to look up, so they provide only the primary key, which is 
supposed to uniquely identify the object.

The issue with vaults is that they are in fact identified by 
(vault_name, host, shared) tuple, but the primary_key is just 
vault_name. Fixed by reverting to your original approach.

Updated patch attached.

Also attached is a modified patch, which implements hierarchical vault 
container objects. It is very hacky, but functional and self-contained. 
You can use it if you want.

-- 
Jan Cholasta
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-vault-plugin.patch
Type: text/x-patch
Size: 27426 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150514/5f1b5e54/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Added-vault-and-vaultcontainer-plugins.patch
Type: text/x-patch
Size: 35618 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20150514/5f1b5e54/attachment-0001.bin>