[PR PATCH] idmap_rid: default group always set to "Domain Users"

[David Mulder]

On 04/06/2018 04:47 PM, Uri Simchoni via samba-technical wrote:
> On 04/06/2018 11:43 PM, Github bot account via samba-technical wrote:
>> There is a new pull request by dmulder against master on the Samba Samba Github repository
>>
>> https://github.com/dmulder/samba Bug1087931
>> https://github.com/samba-team/samba/pull/159
>>
>> idmap_rid: default group always set to "Domain Users"
>> https://bugzilla.samba.org/show_bug.cgi?id=13371
>>
>> I don't know whether I've taken the right approach to fixing this, but essentially this forward ports some of the code removed in [this commit](https://github.com/samba-team/samba/commit/241c81b2763392439043261cf179cd2c8793faed) which broke primary groups for the rid backend (gecos was broken also).
>>
>> A patch file from https://github.com/samba-team/samba/pull/159.patch is attached
>>
> tl;dr - Ask Volker, but I'm still providing my 2c so that this doesn't
> get accidentally pushed.
>
> Whew, something looks awfully wrong about this, with algorithmic
> id-mapping code making LDAP calls :(. There's no denying the bug though.
I was sort-of following how things are done in idmap_ad_query_user().
The ad backend has it's query_user function which does ldap calls to get
user attributes.
>
> As far as correctness goes, any query to the DC is to be made only if
> the information is not in the netsamlogon cache - that has been the
> behavior before the breaking commit and it doesn't appear to be the case
> with the fix. So NAK at least until this is fixed. It's enough to verify
> the user doesn't exist, because if it exists, we already get all correct
> data from there.
>
> Beyond correctness, perhaps there's a better fix that would not have an
> algorithmic ID-mapping backend make LDAP queries to AD (do it in the
> central place). I would try making an RPC via the domain child to
> determine the primary group SID/user name if the user is not in the
> netsamlogon cache and the id-mapping backend has no clue, and then have
> the id-mapping translate that into a gid. Volker would probably be the
> person to lay out the best solution. I wouldn't NAK a bug fix just
> because it's not clean enough to my taste without me suggesting a
> concrete alternative though.
If not in the id mapping code, where would I put this? The source has
changed enough since that remove commit that I'm not sure where to start.
I'd defer to Volker if he's willing to look at this.
>
> Background - the patch set that broke things had a premise that returned
> user info is correct only for users who logged on, and this principle
> was agreed-upon (acked and not nacked) on the list before submitting,
> reviewing, and pushing this patch set. In a sense this bug is
> as-designed. Then it turned out users cared about this breakage and part
> of it was reverted but apparently this part was missed.
Uri, could you point me to the commit/commits where some of the changes
were reverted?
>
> Thanks,
> Uri.
>
>

-- 
David Mulder
SUSE Labs Software Engineer - Samba
dmulder at suse.com
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)