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

Simo Sorce simo at redhat.com
Thu Aug 7 00:55:38 UTC 2014


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.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list