[SSSD] [PATCH] AD: Add initial gpo-smb implementation.

Sumit Bose sbose at redhat.com
Fri Jun 13 10:35:04 UTC 2014


On Thu, Jun 12, 2014 at 04:37:25PM -0400, Yassir Elley wrote:
> 
> 
> ----- Original Message -----
> > On Fri, May 30, 2014 at 12:35:32PM -0400, Yassir Elley wrote:
> > > 
> > > 
> > > ----- Original Message -----
> > > > On (29/05/14 09:06), Yassir Elley wrote:
> > > > >----- Original Message -----
> > > 

...

> > 
> > 
> > I didn't had a chance to do functional testings yet, I'll send my
> > results tomorrow. For a start please find my comments on the code
> > inline.
> 
> Please let me know if you have additional comments after functional testing.

Basic functional tests were working fine, I was able to reject access
and grant it again by modifying the GPO on the AD DC.

While watching the logs I've seen the smb uri
smb://domain.name/sysvol/... A DNS query for domain.name will return the
list of all DCs in domain.name and libsmbclient will probably pick the
first which might be in a completely different location than the client.
I would suggest to replace domain.name with the name of the DC SSSD is
currently connected to.

As a general comment, I would prefer to wait until the file transfer is
done in a child process before pushing this patch to master. Because
otherwise imo the risk of blocking the backend for too long is too high.

> 
> > 
> > bye,
> > Sumit
> > 
> > 
> > 

...

> > I have two comments here. First, is there a way to avoid having deny
> > rules? The reason is that we had some troubles in the past here, because
> > when checking deny rules all group memberships of a user must be
> > available to properly deny access. If some are missing access might be
> > granted although it should be rejected. I guess it is not possible to
> > get rid of them because they will be set on the AD side and we have to
> > respect them.
> 
> I agree that relying on deny rules for security is problematic, b/c it can be difficult to exhaustively prove a negative. Don't we have the same challenge with the simple_access_provider's simple_deny_groups?

yes, but I guess there were request to add deny groups as well. But
since this will be set in sssd.conf users are free to use it or not.

> 
> In any case, I also agree with your conclusion that we have to respect the GPO policy settings defined on the AD side. 
> 
> > 
> > Second, you are only checking SeInteractiveLogonRight and
> > SeDenyInteractiveLogonRight. What about
> > Se[Deny][Network|Batch|Service|RemoteInteractive]LogonRight? Are you
> > planning to integrate them as well and relate them to different PAM
> > services?
> 
> The plan for the initial deployment is to only support SeInteractiveLogonRight and SeDenyInteractiveLogonRight.

ok, but please keep in mind that it would be hard to add support for
different login types, e.g. network vs. console, later on without
breaking backward compatibility.

> 

...

> > > +
> > > +  ret = ini_get_config_valueobj(RIGHTS_SECTION, name, ini_config,
> > > +                                INI_GET_FIRST_VALUE, &vobj);
> > > +  if (vobj == NULL) {
> > > +      DEBUG(SSSDBG_CRIT_FAILURE, "section/name not found: [%s][%s]\n",
> > > +            RIGHTS_SECTION, name);
> > > +      return EOK;
> > > +  }
> > > +  sids = ini_get_string_config_array(vobj, NULL, &num_sids, &ret);
> > > +
> > > +  if (ret) {
> > 
> > Please do explicit checks, in this case (ret != 0) because according to
> > the docs ini_get_string_config_array() returns 0 on success.
> 
> Doesn't C define "true" as a non-zero value? Or are you suggesting this just for clarity?

For clarity, see e.g. Andreas' blog post
http://blog.cryptomilk.org/2013/03/28/writing-and-reading-code/ .

I think we have the practice in SSSD to use 'if (a)' and 'if (!a)' only
if 'a' is a bool or a pointer. Personally I prefer '!= NULL' and '==
NULL' with pointers as well.

> 
> > 
> > > +      DEBUG(SSSDBG_CRIT_FAILURE,
> > > +            "ini_get_string_config_array failed [%d][%s]\n", ret,
> > > strerror(ret));
> > > +      return ret;
> > > +  }
> > > +
> > > +  for (i = 0; i <num_sids; i++) {
> > > +    if (sids[i][0] == '*') {
> > > +      sids[i]++;
> > > +    }
> > > +  }
> > 
> > What is this loop good for?
> > 
>  
> The GptTmpl.inf policy files I have seen all include an asterisk before the user_sid or group_sid; this loop removes the asterisk so that the policy file sids can be correctly compared against the cached sids (which don't have asterisks).

Please add this as a comment for the loop.

> 

...

> > > +}
> > > +
> > > +int ad_gpo_process_cse_recv(struct tevent_req *req,
> > > +                            TALLOC_CTX *mem_ctx,
> > > +                            char ***allowed_sids,
> > > +                            int *allowed_size,
> > > +                            char ***denied_sids,
> > > +                            int *denied_size)
> > > +{
> > > +    struct ad_gpo_process_cse_state *state =
> > > +        tevent_req_data(req, struct ad_gpo_process_cse_state);
> > > +
> > > +    TEVENT_REQ_RETURN_ON_ERROR(req);
> > > +
> > > +    *allowed_sids = state->allowed_sids;
> > > +    *allowed_size = state->allowed_size;
> > 
> > I guess talloc_steal is missing here.
> 
> Both allowed_sids and denied_sids are populated using ini_get_string_config_array(), which does not use talloc memory allocation. 

I see. I was confused by mem_ctx in the argument list. I would suggest
to copy the lists to talloc arrays here or even at a lower level because
it is confusing and the lists are currently not freed in
ad_gpo_cse_done(). It would be even more difficult if you start to read
the list from cache, because here talloc arrays would be used.

bye,
Sumit



More information about the sssd-devel mailing list