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

Jakub Hrozek jhrozek at redhat.com
Wed Aug 6 18:30:37 UTC 2014


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



More information about the sssd-devel mailing list