[Freeipa-devel] [PATCH] Password vault

[Endi Sukma Dewata]

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?

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.

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

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

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.

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)

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:])

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?

-- 
Endi S. Dewata