[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

Dne 18.5.2015 v 21:17 Endi Sukma Dewata napsal(a):
> Please take a look at the attached new patch which includes some of your
> changes you proposed.
>
> On 5/14/2015 7:17 PM, Endi Sukma Dewata wrote:
>> 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.
>
> I changed the 'host vaults' to become 'service vaults'. The interface
> will look like this:
>
> $ ipa vault-find --service HTTP/server.example.com
> $ ipa vault-add test --service HTTP/server.example.com
>
> I also added user vaults:
>
> $ ipa vault-find --user testuser
> $ ipa vault-add test --user testuser
>
> Private vaults is a special case of user vaults where username=<you>.
>
> Host vaults can be added later once we define the use case.

OK.

>
>>>> 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.
>
> This change has been included in this patch.

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.
>
> This change has been included and it will ignore all NotFound errors.

OK.

>
>>>> 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.
>
> Not sure which code you changed, but the new patch doesn't exhibit this
> problem.

I changed Flag('shared') to Flag('shared?').

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

I don't really care about having a clear use case, I would prefer if the 
design was elegant enough to handle *all* the cases without any extra 
effort.

>
> The code has been changed to use the service name as the container:
>
>    cn=vaults
>    + cn=services
>      + cn=<service name>
>        + cn=<vault 1>
>        + cn=<vault 2>

OK.

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

When someone uses the plugin with a different API object than ipalib.api.

> The original code seems to
>> be working fine with ipalib.api.

The current best practice is to use self.api and *all* new plugin code 
should do that.

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

Not yet, but this will be fixed in the future. (Also, container_dn is 
part of the LDAPObject API, unlike base_dn used in the original code.)

>
> This change is not included. The code will now obtain the values from
> apilib.api.env at init time and store it in class attributes so it can
> be reused.
>
>      container_dn = api.env.container_vault
>      base_dn = DN(container_dn, api.env.basedn)

Sorry, but no. Please just follow the best practice instead of trying to 
invent something new. This is not the right time and place to discuss 
this. We should be discussing the vault, not framework idiosyncracies.

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

I forgot to remove the break statement in the for loop.

>
> This change is not included. The original code and the tests work just
> fine.

I would prefer if it was done without recursion, like my code does:

+    def create_container(self, dn):
+        """
+        Creates vault container and its parents.
+        """
+
+        container_dn = DN(self.container_dn, self.api.env.basedn)
+        assert dn.endswith(container_dn)
+
+        dns = []
+        while dn.endswith(container_dn):
+            dns.insert(0, dn)
+            dn = DN(*dn[1:])
+
+        for dn in dns:
+            entry = self.backend.make_entry(
+                dn,
+                objectclass=['nsContainer'],
+                cn=[dn[0]['cn']],
+            )
+
+            try:
+                self.backend.add_entry(entry)
+            except errors.DuplicateEntry:
+                pass

>
>> 8. There's a new PEP8 error:
>>
>> .../ipalib/plugins/vault.py:98:1: E302 expected 2 blank lines, found 1
>
> The new patch doesn't have this issue.

OK.

>
>> 9. The API.txt needs to be regenerated.
>
> The API.txt and VERSION file have been updated.
>

OK.

-- 
Jan Cholasta