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

Yassir Elley yelley at redhat.com
Sun Jun 15 01:01:53 UTC 2014



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

Is there an easy way to get the name of the DC that 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.

That's fine. I wanted to facilitate the review of the basic gpo-smb code, by not introducing the complexities of a child process. Now that this review is complete, I'll modify the code per the review comments, and then integrate the code into a gpo_child process patch.

> 
> > 
> > > 
> > > 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 theory, we could support just the Allow rules in the policy file (and not support the Deny rules), but I think this would be confusing for AD admins. We should support both Allow rules and Deny rules.

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

We are initially supporting console login as a feature. If we later decide to support network login as a feature (in a subsequent release), what is the concern regarding 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.

OK.

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

OK.

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

OK.

> 
> bye,
> Sumit
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> 

Regards,
Yassir.



More information about the sssd-devel mailing list