On Tue, Aug 19, 2014 at 06:34:32PM +0200, Michal Židek wrote:
On 08/19/2014 03:53 PM, Jakub Hrozek wrote:
>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
Oh well, stuff happens.
I reverted them all.