----- 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(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
Regards,
Yassir.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)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.