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

Yassir Elley yelley at redhat.com
Sun Jun 15 23:08:55 UTC 2014



----- Original Message -----
> 
> 
> ----- 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.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

I have attached a patch, which integrates the gpo-smb code into a gpo_child process. You may have additional code review comments on the gpo_child part of the code, but I think I have addressed all of the initial gpo-smb code review comments in this patch, with the following two exceptions (for which I need some more guidance from you):
* You suggested adding the new minimal version (libini_config-devel >= 1.1) to the configure check. Can you clarify what you mean by this, or preferably provide a diff? We probably only want to enforce this minimal version configure check for the case in which the gpo code is being used, right? For example, if the ipa-provider is being used, then we wouldn't want this configure check.
* You suggested using the name of the DC that SSSD is currently connected to in the smb uri (rather than the domain.name, which will require libsmbclient to perform a DNS resolution). Is there an easy way to get the name of the DC that SSSD is currently connected to? I am having trouble finding it.

Regards,
Yassir.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-AD-GPO-Add-gpo-smb-implementation-in-gpo_child-proce.patch
Type: text/x-patch
Size: 51355 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140615/49187bcb/attachment.bin>


More information about the sssd-devel mailing list