[SSSD] [PATCH] AD: Cache gpo version; only download policy files if version changes

Yassir Elley yelley at redhat.com
Thu Aug 7 03:59:47 UTC 2014



----- Original Message -----
> On Wed, 2014-08-06 at 17:34 -0400, Yassir Elley wrote:
> > 
> > ----- Original Message -----
> > > On Tue, Aug 05, 2014 at 06:30:26PM -0400, Yassir Elley wrote:
> > > > 
> > > > 
> > > > ----- Original Message -----
> > > > > On Thu, Jul 31, 2014 at 10:45:57AM -0400, Yassir Elley wrote:
> > > > > > 
> > > > > > 
> > > > > > ----- Original Message -----
> > > > > > > On Wed, Jul 23, 2014 at 08:02:55PM -0400, Yassir Elley wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > ----- Original Message -----
> > > > > > > > > On Wed, Jul 23, 2014 at 03:03:34AM -0400, Yassir Elley wrote:
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > ----- Original Message -----
> > > > > > > > > > > On Tue, Jul 22, 2014 at 03:13:58AM -0400, Yassir Elley
> > > > > > > > > > > wrote:
> > > > > > > > > > > 
> > > > > > > > > > > See some comments about ad_gpo.c inline, I'm still
> > > > > > > > > > > reading
> > > > > > > > > > > ad_gpo_child.c
> > > > > > > > > > > 
> > > > > > > > > > > > From f3f00a78102ad5e389266b28c43d0396caf57592 Mon Sep
> > > > > > > > > > > > 17
> > > > > > > > > > > > 00:00:00
> > > > > > > > > > > > 2001
> > > > > > > > > > > > From: Yassir Elley <yelley at redhat.com>
> > > > > > > > > > > > Date: Tue, 22 Jul 2014 03:03:03 -0400
> > > > > > > > > > > > Subject: [PATCH 2/2] AD-GPO: only download policy files
> > > > > > > > > > > > if
> > > > > > > > > > > > gpo
> > > > > > > > > > > > version
> > > > > > > > > > > > changes
> > > > > > > > > > > 
> > > > > > > > > > > [...]
> > > > > > > > > > > 
> > > > > > > > > > > > @@ -1298,8 +1304,6 @@ ad_gpo_cse_step(struct tevent_req
> > > > > > > > > > > > *req)
> > > > > > > > > > > >  {
> > > > > > > > > > > >      struct tevent_req *subreq;
> > > > > > > > > > > >      struct ad_gpo_access_state *state;
> > > > > > > > > > > > -    char *cse_smb_uri;
> > > > > > > > > > > > -    char *cse_unix_path;
> > > > > > > > > > > >      int i = 0;
> > > > > > > > > > > >  
> > > > > > > > > > > >      state = tevent_req_data(req, struct
> > > > > > > > > > > >      ad_gpo_access_state);
> > > > > > > > > > > 
> > > > > > > > > > > It's not really visible from the diff, but
> > > > > > > > > > > ad_gpo_cse_step
> > > > > > > > > > > increments
> > > > > > > > > > > the index as the first thing it does and only then
> > > > > > > > > > > assigns to
> > > > > > > > > > > cse_filtered_gpo. This reads to me as if you were
> > > > > > > > > > > skipping
> > > > > > > > > > > the
> > > > > > > > > > > first
> > > > > > > > > > > entry...shouldn't the code read the other way around,
> > > > > > > > > > > first
> > > > > > > > > > > assign
> > > > > > > > > > > the
> > > > > > > > > > > GPO and then increment the index?
> > > > > > > > > > 
> > > > > > > > > > I ran into problems when using an index initialized to 0.
> > > > > > > > > > Although
> > > > > > > > > > I
> > > > > > > > > > don't
> > > > > > > > > > remember what those problems were anymore, I was able to
> > > > > > > > > > solve
> > > > > > > > > > them
> > > > > > > > > > by
> > > > > > > > > > using an index initialized to -1, which is what I am using
> > > > > > > > > > now,
> > > > > > > > > > and
> > > > > > > > > > which
> > > > > > > > > > is why the current code works correctly (and doesn't skip
> > > > > > > > > > the
> > > > > > > > > > first
> > > > > > > > > > entry).
> > > > > > > > > 
> > > > > > > > > OK, thank you, I admit I haven't run any tests yet. Can you
> > > > > > > > > add a
> > > > > > > > > comment to that part of code, then? (You don't have to
> > > > > > > > > re-send
> > > > > > > > > the
> > > > > > > > > patches now)
> > > > > > > > 
> > > > > > > > Actually, I agree with you that the code is currently
> > > > > > > > confusing, so
> > > > > > > > I
> > > > > > > > have
> > > > > > > > changed it to use a cse_gpo_index initialized to 0. I have also
> > > > > > > > made
> > > > > > > > similar changes to som_index and gpo_index.
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > > I wonder if you could call either talloc_steal to move
> > > > > > > > > > > the
> > > > > > > > > > > variables
> > > > > > > > > > > like smb_path, smb_share and smb_server to the gp_gpo
> > > > > > > > > > > context
> > > > > > > > > > > or
> > > > > > > > > > > assign
> > > > > > > > > > > to members of gp_gpo right away when calling
> > > > > > > > > > > ad_gpo_extract_smb_components() instead of allocating the
> > > > > > > > > > > result
> > > > > > > > > > > on
> > > > > > > > > > > state and assigning to local variables. Not a big deal
> > > > > > > > > > > though.
> > > > > > > > > > 
> > > > > > > > > > If I store directly to the gp_gpo member variables, they
> > > > > > > > > > are
> > > > > > > > > > initially
> > > > > > > > > > stored correctly. However, later on (in ad_gpo_cse_step),
> > > > > > > > > > the
> > > > > > > > > > member
> > > > > > > > > > variables have corrupted values. Not sure why. As such, I
> > > > > > > > > > have
> > > > > > > > > > left
> > > > > > > > > > this
> > > > > > > > > > part of the code unchanged.
> > > > > > > > > 
> > > > > > > > > I'll try to play with this area of code during my testing.
> > > > > > > > > But
> > > > > > > > > the
> > > > > > > > > strdup shouldn't block the inclusion of the patches to
> > > > > > > > > master.
> > > > > > > > 
> > > > > > > > Actually, I agree with you here as well, so I have changed the
> > > > > > > > code
> > > > > > > > to
> > > > > > > > avoid the double-allocation (of allocating first to state and
> > > > > > > > then
> > > > > > > > strdup'ing to gp_gpo).
> > > > > > > > 
> > > > > > > > In a previous email, I mentioned that I tried to directly
> > > > > > > > assign
> > > > > > > > the
> > > > > > > > output
> > > > > > > > values to member variables of gp->gpo, and that this was
> > > > > > > > leading to
> > > > > > > > corruption. Thanks to Lukas's suggestion, I ran valgrind, which
> > > > > > > > showed
> > > > > > > > a
> > > > > > > > "use-after-free" condition leading to corruption, which made my
> > > > > > > > mistake
> > > > > > > > evident, which was that I was still using state (rather than
> > > > > > > > gp_gpo) as
> > > > > > > > the TALLOC_CTX, thereby disrupting the logical memory
> > > > > > > > hierarchy. In
> > > > > > > > other
> > > > > > > > words, I was doing:
> > > > > > > > 
> > > > > > > >     ret = ad_gpo_extract_smb_components(state,
> > > > > > > >     state->server_hostname,
> > > > > > > >                                         file_sys_path,
> > > > > > > >                                         &gp_gpo->smb_server,
> > > > > > > >                                         &gp_gpo->smb_share,
> > > > > > > >                                         &gp_gpo->smb_path);
> > > > > > > > 
> > > > > > > > instead of the correct solution, which I am now doing, which
> > > > > > > > is:
> > > > > > > > 
> > > > > > > >     ret = ad_gpo_extract_smb_components(gp_gpo,
> > > > > > > >     state->server_hostname,
> > > > > > > >                                         file_sys_path,
> > > > > > > >                                         &gp_gpo->smb_server,
> > > > > > > >                                         &gp_gpo->smb_share,
> > > > > > > >                                         &gp_gpo->smb_path);
> > > > > > > > 
> > > > > > > > 
> > > > > > > > I have attached a revised patch that addresses these two
> > > > > > > > issues. I
> > > > > > > > have
> > > > > > > > also attached the previous (unchanged) sysdb_gpo API patch for
> > > > > > > > your
> > > > > > > > convenience.
> > > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > Yassir.
> > > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > after your helpful instructions I was able to set up the GPOs but
> > > > > > > I'm
> > > > > > > seeing an odd behaviour and I'm not sure which way to fix it..
> > > > > > > 
> > > > > > > So the thing I'm seeing is that the file we fetch from AD
> > > > > > > contains
> > > > > > > the
> > > > > > > user or group names already, but SSSD seems to be looking for
> > > > > > > SIDs in
> > > > > > > ad_gpo_access_check().
> > > > > > > 
> > > > > > > This is the contents of the file as downloaded on my machine.
> > > > > > > Note
> > > > > > > the
> > > > > > > garbage (?) at the beginning of the file:
> > > > > > > 
> > > > > > > # cat
> > > > > > > /var/lib/sss/gpo_cache/win.example.com/Policies/\{94235CE6-B2F2-43C4-8C9F-CB24C0B61E0B\}/Machine/Microsoft/Windows\
> > > > > > > NT/SecEdit/GptTmpl.inf
> > > > > > > ��[Unicode]
> > > > > > > Unicode=yes
> > > > > > > [Version]
> > > > > > > signature="$CHICAGO$"
> > > > > > > Revision=1
> > > > > > > [Privilege Rights]
> > > > > > > SeInteractiveLogonRight =
> > > > > > > tuser,tgroup1,*S-1-5-32-544,administrator
> > > > > > > 
> > > > > > > I'm using Windows server 2012 if that matters.
> > > > > > > 
> > > > > > > When I navigate to the SYSVOL folder on the server, I see the
> > > > > > > same
> > > > > > > contents,
> > > > > > > including user names (except for the extra bytes at the start).
> > > > > > > 
> > > > > > > So I'm pretty sure I must be missing the part that converts the
> > > > > > > names
> > > > > > > to
> > > > > > > SIDs or vice-versa..
> > > > > > 
> > > > > > The gpo code in sssd is indeed only looking for SIDs (not names) in
> > > > > > the
> > > > > > policy files it parses. In this particular case, it wouldn't be
> > > > > > possible
> > > > > > to convert the user/group name to a SID anyway, b/c the name is not
> > > > > > fully-qualified (e.g. tuser).
> > > > > > 
> > > > > > After doing some investigation to figure out why I have never run
> > > > > > into
> > > > > > this
> > > > > > issue, I found that this issue arises if one does not enter a
> > > > > > fully-qualified name for the user/group (in the GPME GUI). If one
> > > > > > specifies the user/group using a fully-qualified name (either by
> > > > > > browsing
> > > > > > for the name, or by manually entering a fully-qualified name in the
> > > > > > text
> > > > > > box provided by GPME), then the name is converted to a SID before
> > > > > > being
> > > > > > written to the GptTmpl.inf file (and the SID is prepended with an
> > > > > > asterisk
> > > > > > character). This is clearly the preferred (and most-used) approach.
> > > > > > However, if one specifies the user/group by entering a
> > > > > > non-fully-qualified
> > > > > > name, then no SID conversion is done and the name itself (e.g.
> > > > > > tuser,
> > > > > > tgroup1) is written to the GptTmpl.inf file. For more details, see:
> > > > > > http://morgansimonsen.wordpress.com/2008/05/26/working-with-group-policy-restricted-groups-policies/
> > > > > > 
> > > > > > I propose that we do *not* support this use case, since it seems
> > > > > > like
> > > > > > such
> > > > > > a corner case (and I'm not sure how we would determine which domain
> > > > > > the
> > > > > > user/group belongs to). In other words, if someone manually enters
> > > > > > a
> > > > > > non-fully-qualified user/group name (or any other non-SID token),
> > > > > > then
> > > > > > the
> > > > > > gpo code should simply continue to ignore that token. One could
> > > > > > argue
> > > > > > that
> > > > > > the gpo code should throw a parsing error when it encounters a
> > > > > > non-SID
> > > > > > token (rather than ignoring it), but I think it is better to simply
> > > > > > ignore
> > > > > > it, b/c it is possible that policy files containing non-SID tokens
> > > > > > exist
> > > > > > in the wild (despite it being considered poor practice).
> > > > > > 
> > > > > > Regards,
> > > > > > Yassir.
> > > > > 
> > > > > I did some more testing and experiments and found out that the dialog
> > > > > to
> > > > > add names (as opposed to browse for names) even lets you add
> > > > > nonexistant
> > > > > users...that's quite bad..
> > > > > 
> > > > > So I propose we act differently with respect to allow and deny lists.
> > > > > If
> > > > > we encounter a non-SID on the allow list, we skip it. If we encounter
> > > > > a
> > > > > non-SID in the deny list, we deny access and log verbosely that the
> > > > > admin needs to go and change his policies.
> > > > > 
> > > > > This would be in line with how we proposed to handle typos in the
> > > > > simple
> > > > > allow list btw.
> > > > 
> > > > I disagree.
> > > > 
> > > > My understanding is that, if an AD admin intentionally places a
> > > > non-qualified name on either the allowed or denied list, then each
> > > > domain
> > > > member (running Windows) will either match that name, or not match that
> > > > name (regardless of which list it's on). It is perfectly valid for some
> > > > domain members to match, and others not to match. Considering that AD
> > > > is a
> > > > centralized system, this is very odd, but that is why it's considered
> > > > such
> > > > poor practice to use non-qualified names on these lists; this is a
> > > > deprecated corner case. Since sssd can't possibly match a non-qualified
> > > > name, I think we should simply never match (i.e. always skip).
> > > > 
> > > > Also, the simple access provider seems to consistently deny access if
> > > > there
> > > > are any typos on either of its allowed/denied lists (not just if there
> > > > are
> > > > typos on the denied list).
> > > > 
> > > > Regards,
> > > > Yassir.
> > > > 
> > > 
> > > Do you know how exactly do windows client behave? I spoke to simo today
> > > on #sssd and he asked us to implement the same behaviour Windows clients
> > > implement:
> > > 
> > > 18:55 < jhrozek> simo: do you have time to chime into a GPO discussion I
> > > was
> > > having with yelley ?
> > > 18:57 < simo> jhrozek: where ?
> > > 18:58 < jhrozek> simo: sssd-devel, the thread was called "[PATCH] AD:
> > > Cache
> > > gpo version; only download policy files if version changes"
> > > 18:59 < simo> jhrozek: ok what is the contentious point ?
> > > 19:00 < jhrozek> simo: what do do with entries that are represented as
> > > user
> > > names and not SIDs
> > > 19:00 < jhrozek> simo: especially in the deny case
> > > 19:01 < jhrozek> simo: AD does no validation so you can have completely
> > > nonexistant entries in the list and the names are not qualified, so ou
> > > have
> > > no idea which domain they apply to
> > > 19:01 < simo> resolve them >
> > > 19:01 < simo> ?
> > > 19:01 < jhrozek> to which domain?
> > > 19:01 < simo> jhrozek: what does AD do ?
> > > 19:02 < jhrozek> umm, I think yelley described that in the e-mail but I'm
> > > not
> > > 100% sure
> > > 19:03 < simo> jhrozek: it is unclear to me what windows does exactly
> > > 19:04 < jhrozek> simo: this is what yassir said and mind you, I haven't
> > > tested this myself:
> > > 19:04 < simo> does it deny if a name does not match ?
> > > 19:04 < jhrozek> then each domain member (running Windows) will either
> > > match
> > > that name, or not match that
> > > 19:04 < jhrozek> name (regardless of which list it's on). It is perfectly
> > > valid for some domain members to match, and others not to match.
> > > Considering
> > > that AD is a centralized system, this is very odd, but that is why it's
> > > 19:04 < jhrozek> considered such poor practice to use non-qualified names
> > > on
> > > these lists; this is a deprecated corner case.
> > > 19:04 < simo> I just read it
> > > 19:04 < simo> it is not clear
> > > 19:04 < jhrozek> ok, sorry
> > > 19:04 < simo> not sufficient to bnase a decision on it
> > > 19:04 < jhrozek> to me it reads like windows matches against its own
> > > domain
> > > only
> > > 19:04 < simo> if unqualified names are normally used in AD then we must
> > > deal
> > > with it
> > > 19:04 < jhrozek> which would be strange, though
> > > 19:04 < simo> and we need to deal with it the same way windows clients do
> > > 19:05 < simo> so testing is necessary
> > > 19:05 < jhrozek> ok
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel at lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > 
> > 
> > I have not tested this empirically, but my analysis is based on the
> > following two sources:
> > * http://support.microsoft.com/kb/890737
> > *
> > http://morgansimonsen.wordpress.com/2008/05/26/working-with-group-policy-restricted-groups-policies/
> > (unofficial blog posting)
> > 
> > As far as I can tell, you would only use non-qualified names in scenarios
> > for which you want to use the same name across multiple domains. This is
> > similar to using well-known SIDs, except that names can be different in
> > different languages, and also that such names would only be "well-known"
> > among your particular domains (not globally). Given these shortcoming,
> > this is considered poor practice and well-known SIDs should be used
> > instead.
> > 
> > As I mentioned initially, if someone browses to the built-in group
> > "Administrators", then the corresponding SID ("*S-1-5-32-544") is placed
> > in GptTmpl.inf, and all domain members recognize that SID (b/c it is
> > well-known). However, manually entering "Administrators" results in
> > "Administrator" being put in GptTmpl.inf, which means that only domain
> > members that have the same account name in their SAM DB will recognize the
> > group name. A domain member that is running a German version of windows
> > will not recognize the group name (b/c it uses "Administratoren" instead
> > of "Administrators"). So, recognizing the non-qualified name comes down to
> > whether the client's locale is compatible with the domain controller's
> > locale, whether the client has renamed one of the default built-in groups,
> > etc.
> > 
> > Based on this, I believe a windows client behaves as follows:
> > if (non-qualified name is encountered in GPO) {
> >     if (non-qualified name in GPO matches non-qualified name in client's
> >     SAM DB) {
> >         process matching name accordingly (depending on whether it is on
> >         allow list or deny list)
> >     } else {
> >         policy processing fails (at least for this client-side extension);
> >         error 1332 reported: "No mapping between account names and security
> >         IDs was done"
> >     }
> > }
> > 
> > In the case of SSSD, since I don't think we store non-qualified names,
> > there could never possibly be a match. If we want to behave the same
> > way as windows clients behave, I think the correct behavior is to
> > always fail (i.e. deny access) in the presence of non-qualified names
> > in the GPO, regardless of whether the non-qualified name appears on
> > the allow list or deny list.
> 
> 
> It is not really hard to match it to Administrators at domain, why
> shouldn't we behave just like windows ?
> 
> Simo.
> 

I think a non-qualified name can be thought of as either a built-in group (such as "Administrators") or as a local group (such as "RandomGroup") which happens to have the same name on multiple clients (a la windows workgroups). Since sssd supports neither built-in groups nor local groups, we aren't going to be able to do exactly what a windows client would do. We should do whatever makes sense in an sssd context.

For example, if a Windows client encounters a non-qualified name (such as RandomGroup) in the GPO policy settings, the client doesn't qualify it with the client's domain (RandomGroup at domain) in order to determine a match. If it used that strategy, and RandomGroup at domain didn't appear among the user's memberships, it would simply ignore that group and process the other users/groups in the policy settings. However, that's not what a Windows client does. Rather, the Windows client consults its SAM database for a match against RandomGroup. If it doesn't find a match, it denies access and logs an error (and stops processing any other users/groups). So, the actions are quite different depending on the strategy used (ignore group vs. deny access).

For sssd, if we think it makes sense to match against RandomGroup at domain when we encounter RandomGroup in policy settings, we can certainly do that. If we think we should always deny access when we encounter non-qualified names, we can also do that. However, I think we should apply whatever strategy we decide upon consistently, regardless of whether the non-qualified name is on an allow list or a deny list. 

Regards,
Yassir.



More information about the sssd-devel mailing list