[Freeipa-devel] [PATCH] Password vault

[Jan Cholasta]

Dne 21.5.2015 v 17:45 Endi Sukma Dewata napsal(a):
> Please take a look at the new patch.
>
> On 5/20/2015 1:53 AM, Jan Cholasta wrote:
>>> I suppose you meant you're OK with not adding host vaults now?
>>
>> Yes.
>>
>>> 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.
>>
>> The reason is consistency. Private vaults should be available for all
>> identities, because anything else would be an arbitrary limitation
>> (which is not elegant). If private vaults were available for all
>> identities, we would need a container for host vaults. I'm not saying
>> the container has to be added now, but there should at least be a check
>> to reject requests when the authenticated identity is a host (i.e.
>> context.principal.startswith('host/')).
>
> In the previous patch, since a host was considered a service it could
> have private vaults too. But anyway, I added the code to reject host
> principals as you requested.
>
>>>>>> 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?
>>
>> I don't think there is a ticket for this particular issue.
>
> Added TODOs in the 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.
>>>
>>> OK.
>>
>> Thanks for understanding.
>
> Changed the code as requested.
>
>>> 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.
>>
>> I tend to stick to the "don't use recursion in production code" rule,
>> epsecially in simple cases like this one.
>
> I think the rule is kind of misguided. Recursion might be considered bad
> if it goes very deep thus consuming so much stack space, which is not
> the case here given the short and flat tree topology. Or, if it's
> unnecessary such as a tail-recursion, which is not the case here either.
>
>>> 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.
>>
>> This is not about loop vs. recursion, this is about the used approach.
>> Your code can be easily transformed into a loop while keeping the same
>> approach:
>>
>>      entries = []
>>
>>      while dn:
>>          assert dn.endswith(self.base_dn)
>>
>>          rdn = dn[0]
>>          entry = self.backend.make_entry(
>>              dn,
>>              {
>>                  'objectclass': ['nsContainer'],
>>                  'cn': rdn['cn'],
>>              })
>>
>>          # if entry can be added, return
>>          try:
>>              self.backend.add_entry(entry)
>>              break
>>
>>          except errors.NotFound:
>>              pass
>>
>>          # otherwise, create parent entry first
>>          dn = DN(*dn[1:])
>>          entries.insert(0, entry)
>>
>>      for entry in entries:
>>          # then create the entry itself again
>>          self.backend.add_entry(entry)
>>
>>>
>>> Do you still want to use the loop?
>>
>> Yes please.
>
> This algorithm is recursive by nature (start at the bottom, but add the
> parent first). The above code basically emulates recursion with two
> loops and an explicit stack of entries, but the memory requirement is
> pretty much the same because it's still using a stack. With real
> recursion there is no loops and the stack is implicit, so the code is at
> least cleaner.

I thought you cared about efficiency in the first place, given our 
discussion about container_dn. The looped variant is faster, even when 
it has 2 for loops, and consumes less memory, because of the function 
call overhead in the recursive variant.

>
> If you really want to avoid recursion you probably should use an RDN
> iterator instead of a stack, but the code would be either even uglier or
> less efficient.
>
> Anyway, it's not a big deal, I included this change.
>

Thanks, LGTM.

Pushed to master: fde21adcbd62b9a300740d9ba237ca9e89a905e4

-- 
Jan Cholasta