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

Jakub Hrozek jhrozek at redhat.com
Mon Jul 28 18:48:37 UTC 2014


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..



More information about the sssd-devel mailing list