[Freeipa-devel] [PATCH] Password vault

[Endi Sukma Dewata]

Before I send another patch I have some questions below.

On 5/19/2015 3:27 AM, Jan Cholasta wrote:
>> 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.

I suppose you meant you're OK with not adding host vaults now?

>>>>> 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 only way to know if the design will be future proof is if we have at 
least some idea how it will be used. Without that there is no guarantee.

Host principals have this form: host/<hostname>@<realm>, so with the 
current code they will be considered a service and will have a service 
container.

Do you want to add a new cn=hosts container just for hosts? Unless we 
have a specific reason (i.e. use case) I don't see a need to add 
specific code for hosts now, or at least until we get the core vault 
functionality working.

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

Is there a ticket for this?

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

OK.

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

There is an obvious inefficiency here: all containers in the path 
(including the built-in ones) will be re-added on every vault-add operation.

I don't see anything wrong with recursions, especially if it works more 
efficiently since only the immediate parent will be re-added.

So for example, with the loop every time you add a private vault you're 
guaranteed to have three failed LDAP Add operations whereas with the 
recursion there's only one failed operation.

Do you still want to use the loop?

-- 
Endi S. Dewata