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