On Mon, Aug 18, 2014 at 01:53:58PM +0200, Michal Židek wrote:
> On 08/14/2014 01:20 PM, Pavel Reichl wrote:
>>
>> On 08/05/2014 05:18 PM, Michal Židek wrote:
>>> On 07/16/2014 05:18 PM, Pavel Reichl wrote:
>>>> Hello,
>>>>
>>>> please see attached patches.
>>>>
>>>> Regards,
>>>>
>>>> Pavel Reichl
>>>>
>>>
>>> Hi,
>>>
>>> Patch 1: LGTM
>>>
>>> Patch 2:
>>>
>>>> @@ -962,9 +977,40 @@ static void sdap_initgr_nested_search(struct
>>>> tevent_req *subreq)
>>>> groups[0]);
>>>> state->groups_cur++;
>>>> } else {
>>>> - DEBUG(SSSDBG_OP_FAILURE,
>>>> - "Search for group %s, returned %zu results.
Skipping\n",
>>>> - state->group_dns[state->cur], count);
>>>> + if (state->try_as_non_posix || count != 0) {
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>> This condition was a little difficult for me to understand
>>> without additional explanation. Could you add some more descriptive
>>> comments to this (and the else) branch?
>>>
>>>> + /* We can't get the group as posix nor non-posix.
>>>> Succumb on
>>>> + * this one and try to continue with another.
>>>> + */
>>>> + DEBUG(SSSDBG_OP_FAILURE,
>>>> + "Search for group %s, returned %zu results.
>>>> Skipping\n",
>>>> + state->group_dns[state->cur], count);
>>>> + } else {
>>>> + /* Try to get the group as non-posix. */
>>>> + state->try_as_non_posix = true;
>>>> +
>>>> + DEBUG(SSSDBG_OP_FAILURE,
>>>> + "Search for group %s, returned %zu results.
"
>>>> + "Trying to get it as non-posix\n",
>>>> + state->group_dns[state->cur], count);
>>>> +
>>>> + subreq = sdap_get_generic_send(state, state->ev,
>>>> + state->opts,
state->sh,
>>>> + state->group_dns[state->cur],
>>>> + LDAP_SCOPE_BASE,
>>>> + state->np_grp_filter,
>>>> + state->np_grp_attrs,
>>>> + state->opts->np_group_map,
>>>> + SDAP_OPTS_NP_GROUP,
>>>> + dp_opt_get_int(state->opts->basic,
>>>> + SDAP_SEARCH_TIMEOUT),
>>>> + false);
>>>> + if (!subreq) {
>>>> + tevent_req_error(req, ENOMEM);
>>>> + return;
>>>> + }
>>>> + tevent_req_set_callback(subreq,
>>>> sdap_initgr_nested_search, req);
>>>> + return;
>>>> + }
>>>> }
>>>>
>>>> state->cur++;
>>>> @@ -972,6 +1018,7 @@ static void sdap_initgr_nested_search(struct
>>>> tevent_req *subreq)
>>>> * memberOf which might not be only groups, but permissions, etc.
>>>> * Use state->groups_cur for group index cap */
>>>> if (state->cur < state->memberof->num_values) {
>>>> + state->try_as_non_posix = false;
>>>> subreq = sdap_get_generic_send(state, state->ev,
>>>> state->opts, state->sh,
>>>> state->group_dns[state->cur],
>>>> @@ -2656,6 +2703,7 @@ struct tevent_req
>>>> *sdap_get_initgr_send(TALLOC_CTX *memctx,
>>>> state->conn = conn;
>>>> state->name = name;
>>>> state->grp_attrs = grp_attrs;
>>>> + state->np_grp_attrs = np_grp_attrs;
>>>> state->orig_user = NULL;
>>>> state->timeout = dp_opt_get_int(state->opts->basic,
>>>> SDAP_SEARCH_TIMEOUT);
>>>> state->user_base_iter = 0;
>>>> -- 1.9.3
>>>>
>>>>
>>>
>>> I found some problems while testing these patches.
>>> Let us have hierarchy like this:
>>>
>>> GROUP | MEMBERS (users and groups)
>>> -------------------------------------
>>> pos1 | ipauser1, nonpos2
>>> nonpos2 | ipauser2, pos3
>>> pos3 | ipauser3,
>>>
>>> Groups pos1 and pos3 are posix groups, nonpos2 is normal group.
>>>
>>> (note: output is sanitized uid, gid and non relevant groups are deleted)
>>>
>>> This is correct (these command are given in order as shown):
>>>
>>> # id ipauser3
>>> groups=234600001(pos1),234600003(pos3)
>>>
>>> # id ipauser2
>>> groups=234600001(pos1)
>>>
>>> # getent group pos1
>>> pos1:*:234600001:ipauser3,ipauser2,ipauser1
>>>
>>> Now I clear the cache and do:
>>>
>>> # getent group pos1
>>> pos1:*:234600001:ipauser1
>>>
>>> Users ipauser2 and ipauser3 are not shown, which is
>>> wrong.
>>>
>>> When I try hierarchy like this:
>>>
>>> GROUP | MEMBERS (users and groups)
>>> -------------------------------------
>>> ppos1 | pnonpos, ppos2
>>> pnonpos | ipauser4,
>>> ppos2 | ipauser5,
>>>
>>> Groups ppos1 and ppos2 are posix groups, pnonpos is normal group.
>>>
>>> # getent group ppos1
>>> ppos1:*:234600007:ipauser5
>>>
>>> So ipauser4 was not returned and it should be. It is only
>>> returned when I do 'id ipauser4' first.
>>>
>>> So when searching for users of groups (getgrnam) the
>>> non-posix group cuts the hierarchy if the user data is
>>> not cached already. This is still potential
>>> security issue IMO. If you deny access to group
>>> pos1, the user ipauser2 and ipauser3 will be able
>>> to enter.
>>>
>>> So the bug is still there. On top of that we have
>>> inconsistent behaviour when commands like 'id'
>>> precede 'getent group', (in other words, we have
>>> different results of getgrnam with clear and
>>> filled cache) which is weird.
>>>
>>> Michal
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel(a)lists.fedorahosted.org
>>>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>>
>> Hello,
>>
>> please see updated patches.
>>
>
> Thanks for adding the comments to the 2nd patch. It
> helped a lot.
>
> Ack to all 3 patches.
>
> I just have one more comment, but this should not block these patches.
> When looking at the functions in 3rd patch, the new added functions
> are mostly copies of their posix alternativces. I wonder if it would
> be possible to create one generic set of functions to deal with
> this particular problem.
>
> But this does not need to be done now, and frankly I am not sure if it
> is worth the effort, but removing duplicities might make us a little
> more happy in the long term.
>
> But again, it does not need to be done now. These patches passed
> my testing and work good. So ack again.
>
> Michal
Pushed to master:
* 08145755f66e83c304e11228c2b610a09576dd81
* 5197ac634572a2e0f8c7cacad68d5e5336064744
* 4c560e7b98e7ab71d22be24d2fbc468396cb634f
sssd-1-11:
* 1fe677614603cb57784dbc03f60dbe4c1ba2db44
* 4417c874595600cd93e12822fab54aa5753df74a
* b7afe5caaaeae1e92479284a7f555aee4ba23422
Sorry this was premature ACK from my side. The patches actually broke
the LDAP provider. I think (I am actually sure) that we should
revert these patches and use a different approach to solve this
problem (not so IPA specific).
Jakub, will you please revert these patches from both 1-11 and
master?
Sorry for the inconvenience.
Michal