libwbclient: duplicate primary group in returned sids array (was github PR #102)

[Isaac Boukris]

On Tue, Jan 8, 2019 at 10:17 PM Volker Lendecke
<Volker.Lendecke at sernet.de> wrote:
>
> On Tue, Jan 08, 2019 at 12:36:59PM +0200, Isaac Boukris via samba-technical wrote:
> > A while ego, I was testing wbcAuthenticateUserEx with NTLM against
> > Windows and noticed that the primary group is being referenced twice
> > in returned wbcAuthUserInfo.sids array.
> >
> > The way it happens as far as I recall, is because the DC returns the
> > primary group in NETLOGON_VALIDATION_SAM_INFO, both as PrimaryGroupId
> > and in GroupIds. Then libwbclient copies it to a single array where
> > array[0] is the user SID, array[1] is the PrimaryGroupId and then it
> > adds the primary group again as a part of the groups from GroupIds.
> >
> > This patch skips the second addition, as it seems odd to have
> > duplicate SIDs in a flat array (even though the first two elements are
> > fixed). If this looks right, I'll try to add a test and resubmit on
> > gitlab.
>
> Of course this looks functionally right. One small nit-pick: We always
> do { } even if there's only one statement in the "if"-clause.

Thanks, I'll fix in next round.

> The other question is: Is this too specific? Should we filter all
> duplicate SIDs in case they happen?

To my understanding, there are no real duplicates SIDs in the DC
response or in winbind, they just use a different data structure,
like:
struct {
    UserId,
    PrimaryGroupId,
    GroupIds[],  (all groups, including the primary but not including uid)
    ...
}

So yes, the primary group is referenced twice but in different
contexts. It only becomes an issue when libwbclient stacks all these
SIDs in a single array as now it looks like a duplicate.

> And -- is this the right place? My
> gut feeling would be to keep the winbind clients as simple as possible
> and put complexity into winbind itself. Should we filter the primary
> group RID inside winbind? Really a genuine question, I don't have a
> firm opinion yet.

The transition from info3 struct to a single array (with reserved
positions) happens in wbclient, hence it seemed the right place to
skip the primary group id.

On Tue, Jan 8, 2019 at 11:02 PM Jeremy Allison <jra at samba.org> wrote:
>
> This looks right to me, but shouldn't you also scan for and
> ignore user_rid as well, so we only get the first two entries
> once ? Not sure if this can happen, but just makes it impossible
> anyway.

It should not happen as the uid is not referenced in GroupIds (because
it's not a group, I guess). So I don't think we should filter it,
instead, I'll try to add a test that shows there are no duplicates in
the returned array.

Thanks!