[Freeipa-devel] [PATCH] Password vault

[Endi Sukma Dewata]

On 5/14/2015 1:42 PM, Jan Cholasta wrote:
>>> 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.

There's no plan for host vaults in the design doc, but it's not 
impossible to add it in the future. What is the use case?

I suppose we can change the service vault into a container, and possibly 
add generic user vaults too (in addition to private user vaults), the 
interface will look like this:

$ ipa vault-add PrivateVault
$ ipa vault-add ServiceVault --service <name>/<hostname>
$ ipa vault-add SharedVault --shared
$ ipa vault-add UsersVault --user <username>
$ ipa vault-add HostVault --host <hostname>

Basically we'll need a new parameter for each new container. For the 
initial implementation we only need the first 2 or 3.

>> Do we
>> need to support services of different realms?
>
> That depends. Do we want to support vaults for users and services from
> AD trusts?

It wasn't mentioned in the design doc either, but probably in the future 
we can support external vaults too:

$ ipa vault-add ExternalVault --principal <principal>

   cn=vaults
   + cn=external
     + cn=<name>/<hostname>@<realm>
       + cn=<vault 1>
       + cn=<vault 2>
     + cn=<user>@<realm>
       + cn=<vault 3>
       + cn=<vault 4>

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

OK.

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

We can actually ignore all NotFound errors since now all containers are 
either created automatically or already created by default.

   if call_func.__name__ == 'find_entries':
       if isinstance(exc, errors.NotFound):
           shared = options.get('shared')

           # if private or service container has not been created, ignore
           if not shared:
               raise errors.EmptyResult(reason=str(exc))

The original code was only ignoring NotFound errors on user vaults 
because previously only the user containers was supposed to be created 
automatically, and there wasn't a plan to provide service container.

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

OK.

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

As mentioned above, it's not required in the design doc, but we can add 
it if there's a clear use case. I agree that at least for now we can 
change the service vault into a service container to store multiple 
service's private vaults.

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

Under what scenario would that be a problem? The original code seems to 
be working fine with ipalib.api.

If it is a problem, why do we still use ipalib.api to initialize 
container_dn vault class attribute?

   container_dn = api.env.container_vault

Then in get_dn() we basically construct the container_dn variable with 
values from both self.api and ipalib.api:

   container_dn = DN(self.container_dn, self.api.env.basedn)

When is the self.api actually initialized? Can we initialize the 
container_dn (or base_dn as in the original code) attribute immediately 
after that?

>> 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.
>
> Right, fixed.

It's still not working. The new code is trying to add cn=vaults first, 
and it stops because it already exists, but the intermediary entries are 
still not added. Also the DN displayed in the message misleading:

$ ipa vault-add test
ipa: ERROR: container entry (cn=vaults) not found

$ ipa vault-add test --host server.example.com
ipa: ERROR: container entry (cn=vaults) not found

The unit tests are failing because of this.

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

OK. As discussed above, the vault may need to be identified by even more 
parameters, e.g. service, user, principal.

> Updated patch attached.

8. There's a new PEP8 error:

.../ipalib/plugins/vault.py:98:1: E302 expected 2 blank lines, found 1

9. The API.txt needs to be regenerated.

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

Let's circle back to this after we get the essential vault functionality 
checked in. The latest plan was to merge vaultcontainer into vault.

-- 
Endi S. Dewata