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

[Uri Simchoni]

On 04/12/2018 05:11 PM, David Mulder wrote:
>>
>> 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.

Originally it wasn't in the id-mapping code (after all, what does unix
id <-> windows ID have to do with user information). The change said the
user info comes from samlogon cache (i.e. from information supplied
along with authentication), but let the id-mapping backend a shot at
overriding it, primarily (AFAICT) to support AD Unix extensions, where
the Unix user info is directly stored in AD and might be different than
the one inferred from translating the authentication token.

So the current algorithm is:
1. Fill the "Windows" info from netsamlogon cache (in wb_queryuser_got_uid)
2. let id-mapping backend have a go at directly supplying "Unix" info -
calling dcerpc_wbint_GetNssInfo_send() which invokes _wbint_GetNssInfo()
3. if the id-mapping backend didn't do it (in wb_queryuser_done()),
translate the group-sid into a primary gid. But the group-sid could be
"domain users" if we didn't have the netsamlogon info.

What I suggest is that in step 3, if the group sid is not something we
obtained from the netsamlogon cache, then we fetch it "somehow" first,
in a manner that's not dependent on id-mapping configuration, and only
then continue to translate the group SID (this fetching can also obtain
the info for gecos field).

The thing I'm less certain about is the "somehow". I'd guess an RPC to
the DC would do it correctly irrespective of the winbindd backend, but I
could be missing something here. In the original code we had a
_wbint_QueryUser to deal with that on a per-backend basis, and it was
removed in the series of commits that ended in
319d60285c92bbf86bc0a3f872f9c9f9d0530129. I'm not sure we really need
this per-backend behavior though - all AD DC's support RPC, and the ad
backend already does lots of RPC, it's far from pure ldap (and rightly so).

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

The revert is the series of 9 commits that end in
6296c32668af60118ae7059772d2f70e58e1f0d1

The original discussion:
https://lists.samba.org/archive/samba-technical/2016-November/116972.html

(notice that it's about removal of user/group enumeration and user group
membership, the issue of primary group / gecos is missed. the issue you
report seems to be obtaining correct user info without a logon,
something that's outside the regular SMB server use case)

The original patch set:
https://lists.samba.org/archive/samba-technical/2017-January/117858.html

and then also:
https://lists.samba.org/archive/samba-technical/2017-January/117895.html

(the latter is the one that actually removed user querying, once it was
no longer being used)

Then the discussion about reverting:
https://lists.samba.org/archive/samba-technical/2017-March/119066.html

The backlash was about user groups, so the other piece of primary
group/gecos was not reverted.

I hope that helps,
Uri.