[Samba] User's groups issue

[Ephi Dror]

OK,

In two places in winbindd_pam.c  winbindd_dual_pam_auth() is called:

1. winbindd_dual_pam_auth
2. winbindd_dual_pam_auth_crap

You are 10000% RIGHT that it does not make sense to do:

netsamlogon_cache_store(name_user, &info3);
                wcache_invalidate_samlogon(find_domain_from_name(name_domain), &info3);

Since of course they cancel each other.

I think we should remove the call to wcache_invalidate_samlogon() in those cases.

Now, the code I suggested does make sense and does the right job of actually deleting the user entry from netsamlogon tdb. Everything in samlogon_cache.c must operate on that tdb file and not on any other one to my opinion. (Just to make the code clean)

But it looks like the regular SAMBA code does not have a need to clear the user's info3 from the netsamlogon cache since it simply update it on new login.

However, for us, we have separate application in which we need to look for user's group membership information and we need it to be accurate for users that have not logged in yet and also even if they logged in before, we don't want them to ever have the info in the tdb be there until they logon again since the info is not up to date.

Similar to id command.

With today's SAMBA code, when you do "id" on a domain's user and that user was logged in in the past and you adding that user to couple of more groups on the domain and you do "id username" again, you will not get the right info until this user logout and login again.

Now, I tested it with existing netsamlogon with a user that already logged in once, then I added that user to few more groups in the domain and it worked well for me since in my application, I go ahead and get up to date groups info from the domain for that user.

I tested it by simply making sure that user that was there before was really deleted (using netsamlogon_cache_have() to verify after delete)

Of course my netsamlogon.tdb was not empty since there are other places that netsamlogon_cache_store() is called from.

To conclude:
===============

1. I think the code I suggest should be the right code in samlogon_cache.c
2. We should remove the call to wcache_invalidate_samlogon()
3. Additional function has to be added to take care of U/sid and UG/sid cleanup.

Thanks,
Ephi




-----Original Message-----
From: Jeremy Allison [mailto:jra at samba.org]
Sent: Wednesday, August 27, 2008 3:55 PM
To: Jeremy Allison
Cc: Ephi Dror; Gerald (Jerry) Carter; Hames at samba.org; samba-technical at lists.samba.org; Hames Kurma
Subject: Re: [Samba] User's groups issue

On Wed, Aug 27, 2008 at 03:25:36PM -0700, Jeremy Allison wrote:
> On Wed, Aug 27, 2008 at 02:41:25PM -0700, Ephi Dror wrote:
> > Hi Jerry,
> >
> > I'm moving our discussion to technical list.
> >
> > I just wanted to point out that I think the code in samlogon_cache.c is incorrect.
> >
> > It uses the wrong tdb pointer.
> >
> > Here is the right code to my opinion:
> >
> > void netsamlogon_clear_cached_user(NET_USER_INFO_3 *user)
> > {
> >         DOM_SID sid;
> >         fstring key_str, sid_string;
> >
> >         if (!netsamlogon_cache_init()) {
> >                 DEBUG(0,("netsamlogon_clear_cached_user: cannot open %s for write!\n", NETSAMLOGON_TDB));
> >                 return;
> >         }
> >         sid_copy(&sid, &user->dom_sid.sid);
> >         sid_append_rid(&sid, user->user_rid);
> >
> >         fstr_sprintf(key_str, "%s", sid_to_string(sid_string, &sid));
> >         DEBUG(10, ("netsamlogon_clear_cached_user: clearing %s\n", key_str));
> >
> >         tdb_delete_bystring(netsamlogon_tdb, key_str);
> > }
> >
> > I tested it and it works like a Swiss Watch.
> >
> > The reason this bug was hidden is because when we store we use TDB_REPLACE
> >            tdb_store_bystring(netsamlogon_tdb, keystr, data, TDB_REPLACE)
>
> Yeah, this looks right to me. Looks like the code in
> netsamlogon_clear_cached_user() didn't get converted over
> when the interface changed. Thanks for the fix.

Hmmmm. Ok, *looks* right but may not be :-).

Looking closely at winbindd/winbindd_pam.c I find :

                netsamlogon_cache_store(name_user, info3);
                wcache_invalidate_samlogon(find_domain_from_name(name_domain), info3);

in two places (on successful logon). wcache_invalidate_samlogon()
calls netsamlogon_clear_cached_user() which is meant to scrub the
"UG/<sid>" and "U/<sid>" entries from the winbindd cache, not
delete the netsamlogon cache entry.

If we change to your code the wcache_invalidate_samlogon()
call would call netsamlogon_clear_cached_user() which would delete
the entry just added by netsamlogon_cache_store(), making these
two calls essentially a no-op.

Now I'm pretty sure wcache_invalidate_samlogon() should do
the deletion of the "UG/<sid>" and "U/<sid>" entries from the winbindd
cache itself, rather than leaving that code inside samlogon_cache.c
(where it is now) - especially as the reason for it being
there (might be called by smbd) is no longer valid.

But as I said, making the function look the way you have it
appears to never store anything inside NETSAMLOGON_TDB
(as we add then delete again).

How are you testing this ?

Jeremy