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

Jakub Hrozek jhrozek at redhat.com
Tue Aug 5 09:28:25 UTC 2014


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.



More information about the sssd-devel mailing list