[PATCHES] winbindd: fix sid->xid for SID History SIDs

[Uri Simchoni]

Sorry for delaying my answer. This looks reasonable, and I'll manually
test it in a couple of days.

One remark: The find_domain_from_sid_noinit() in the prerequisite patch
is kind of against where we're heading, in that we don't want to rely on
that and get inconsistent results based on whether the domain has been
discovered already or not. In master, this is justified because it moved
from another place (i.e. it was not introduced by a the fix to this bug,
it was part of you dis-entangling lookupsids). Here we just add a new
lookup. The bug could well be fixed without the fallback. I don't mind
if it stays with the prerequisite patch as-is, to make things common, I
just don't want it to appear like we're headed in different directions...

Thanks,
Uri.

On 04/19/2017 12:15 AM, Ralph Böhme via samba-technical wrote:
> On Wed, Apr 12, 2017 at 11:46:19AM +0300, Uri Simchoni wrote:
>> On 04/10/2017 08:24 PM, Ralph Böhme wrote:
>>> On Fri, Apr 07, 2017 at 09:23:43PM +0300, Uri Simchoni wrote:
>>>> On 04/07/2017 08:32 PM, Ralph Böhme wrote:
>>>>> On Mon, Apr 03, 2017 at 07:50:08AM +0300, Uri Simchoni via samba-technical wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Here's a V2 of the patch set.
>>>>>
>>>>> just fyi, I have this on my list.
>>>>>
>>>>> -slow
>>>>>
>>>> Thanks.
>>>> Here's a rebased patch set, with 4/6 adapted to the recent change in
>>>> wb_lookupsids_bulk(). Otherwise the same.
>>>
>>> looks like this still has the O(N^2) performance issue I mentioned or did I miss
>>> anything?
>>>
>>> Besides that, attached is a simple fix for sids2xids on-top of my proposed
>>> cleanup for wb_lookupsids [1] that should already address the SID-history if the
>>> domain is still around.
>>>
>>> Your patch could go on-top as a more complete patch for the case that the
>>> "historic domain" is behind a one-way trust (where we don't have the domain in
>>> our list of trusted domains).
>>>
>>> Thoughts?
>>>
>>> -slow
>>>
>>> [1] <https://lists.samba.org/archive/samba-technical/2017-April/119944.html>
>>>
>>
>> Pushed the simple fix, I think for backports we can use just "no
>> match->default domain", or, if we want the bugfix to be based on
>> backported patches, only backport [1/6] of the recent simplification
>> patch set and the "don_sid_in_donain" fix.
> 
> backport for 4.5 and 4.6 attached. Can you test it?
> 
>> Regarding my patch and O(N^2) - the search is O(NxM) where M is the
>> number of added domain sids in the lookup. Notice that the domain sids
>> are added, unconditionally, to the back of the resolving list, and the
>> subsequent search is over the added stuff only.
>>
>> Please indicate if you'd like me to rebase this patch set on top of
>> recent commits or drop this - as far as I'm concerned, the main
>> objective of not polluting the id-mapping cache is achieved, and
>> anything else is just trying to get it right for more cases.
> 
> More correct is always good, but it comes at a cost: it makes the code slightly
> more complicated. So given the prospect of moving the idmap backend decision to
> be based on SIDs instead of domain names, which will also solve the more exotic
> SID-history use-cases, maybe it makes sense to stop here.
> 
> -slow
>