----- 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(a)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-...
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.