On Fri, Feb 03, 2012 at 09:44:13PM +0100, Sumit Bose wrote:
> Hi,
>
> the patches apply and compile cleanly. I haven't tested them only looked
> at the code. I'm sorry but there is only #92 which I ACK additionally to
> the already ACKed ones. Please so comments below.
>
> bye,
> Sumit
>
> On Thu, Feb 02, 2012 at 06:02:37PM +0100, Jan Zelený wrote:
> > #084:
> > Original #0001, already Acked
> >
> > #085:
> > Original #0002, already Acked
> >
> > #086:
> > Original #0003, already Acked
> >
> > #087:
> > Original #0004, already Acked
> >
> > #088:
> > Original #0005, already Acked
> >
> > #089:
> > Original #0007, comments:
> >
> > > > > In match_entity() and sss_selinux_match(), do we have a guarantee that
> > > > > the entities being matched are UTF-8 strings? If not, you should
> > > > > probably use the sss_utf8_case_eq() routine.
> > > >
> > > > They are DNs. I'm not quite sure they need to be treated as UTF-8
> > > > strings.
> > >
> > > Sorry, I meant ASCII above, but you answered the question I meant to ask
> > > anyway :)
> > >
> > > That said, I think our better bet would be to actually convert them to
> > > ldb_dn objects with ldb_dn_new() and use ldb_dn_compare() to determine
> > > if they're the same. This will handle any case-sensitivity issues in an
> > > LDB-appropriate way.
> >
> > Actually I already looked into that and it would require to pass ldb
> > context
> > to the matching function. I don't like that, this solution seems to be good
> > enough to me.
>
> I tend to agree, you might want to speed things up by comparing the
> length before comparing the strings. Also please use strncasecmp()
> because I'm not sure if ldb_vals are always zero-terminated.
>
> >
> > > That said, you have a prototype for sss_selinux_extract_usermaps() but
> > > it's not defined anywhere. I think this is where I got confused.
> >
> > Done
> >
> >
> > #090:
> > Original #0006, comments:
> > > When using an enum, prefer a switch statement around 'type' (and skip
> > > the 'default' case). This way the compiler would catch any potential
> > > mistakes here such as adding a new type and not checking for it.
> >
> > Done. Although I'm considering also implementing if after this switch (just
> > in
> > case on some place an ordinary int value will be given instead of the
> > proper
> > enum value).
>
> yes, I'm not sure if skipping 'default' is i good idea in general. The
> compiler can warn you if you have missed some enum value, but cannot
> warn you if the variable was set to some value outside of the enum
> range. But here it would be safe if you set dn = NULL before running
> through the switch statement, because of the if statement which follows.
>
> >
> > > > TODO: I just noticed that it isn't fixed before sending the email and I
> > > > don't have the strength to do it right now. I'll send the fix later. How
> > > > about a conditional ack for the patch until this is done? ;-)
> > >
> > > Well, there are other things left to fix, so we might as well get this
> > > in too.
> >
> > Done
> >
> > > > > The number of usermaps in the cache is theoretically unbounded. It's
> > > > > probably not safe to allocate an array to contain all of them if we're
> > > > > going to be matching and holding onto only a subset of them. I'd
> > > > > rather
> > > > > that you just realloc to fit each one that we filter (or if you
> > > > > believe
> > > > > that this is going to occur computationally-often, write a simple
> > > > > doubling algorithm). As it is, I'm wary of doing ALL of the processing
> > > > > as a post-operation. Is there any way to improve the sysdb search
> > > > > filter to reduce what we have to filter through?
> > > >
> > > > I thought about this for quite some time. About the memory allocation:
> > > > I'm against reallocation of the final array one by one. The allocation
> > > > of the entire array could be solved by two-pass in place filtering
> > > > algorithm (with subsequent realloc), but it won't solve the issue that
> > > > potentially large amount of memory has to be allocated to store all the
> > > > ldb search results (that's why I left the second array - it's quite
> > > > small compared to the memory we need to store all fetched results).
> > >
> > > That's a good point. The array of pointers is going to be negligible in
> > > comparison to the array of sysdb_attrs.
> > >
> > > > I don't believe there's a good solution for this, since the user can be
> > > > member of unlimited number of groups (and our experience says the real
> > > > number is sometimes pretty high). That's why the post-processing is
> > > > necessary. If the goal was to improve memory consumption no matter what,
> > > > the solution would be to do multiple ldb searches, one for each group
> > > > the user is member of. But I don't like that scenario.
> > >
> > > Hmm, it's more work, but it WOULD keep the memory down. This area of the
> > > code really could become a pretty serious memory bottleneck in large
> > > deployments. That said, I'm going to suggest that we go with this for
> > > now and open a ticket to switch to a search per group later on. At the
> > > very least I want to have a ticket to identify a possible pain point in
> > > the future.
> > >
> > > And obviously such an approach would have a much better effect of
> > > keeping the memory down than just limiting the pointer array.
> >
> > I want to consult the alternative approach with Rob first. I will CC you in
> > the
> > conversation.
> >
> >
> > #091:
> > original #0009, comments:
> > > While I recognize that the way you're splitting the order_array is
> > > fairly clever, it's also somewhat difficult to read (and its memory
> > > hierarchy is somewhat wrong, since the array and the contents are both
> > > allocated at the same level on the tmp_ctx).
> > >
> > > I'd prefer if you just walked through the order string until you hit a
> > > '$', set that '$' to a '\0' and then did
> > > order_array[i] = talloc_strdup(order_array, p);
> > >
> > > (Where p is a pointer to the start of the 'word'.
> > >
> > > You can then free 'order' after that and the order_array will have a
> > > clear memory hierarchy (and the code will be more easily read).
> >
> > That approach wouldn't make much difference, I still would need to make it
> > two-
> > pass, it would change just one line of code and add unnecessary overhead.
> > If
> > you insist on the correct memory hierarchy (which isn't particularly useful
> > in
> > that part of the code), I could steal the original string on top of the
> > order
> > array.
>
> I would agree with Jan here, since order_array is not returned to the
> caller. It would be even possible to skip order array and only work on
> order, but this would make the code really hard to read :-)
>
> >
> > > However, I'm pretty sure usernames in PAM can be unicode strings, so you
> > > need to use the utf-8 case-insensitive string compare for matching the
> > > users to the order list.
> >
> > Perhaps, but these are SELinux user identities, not pam usernames. I'm not
> > completely sure about these, but I assume they are ASCII strings.
>
> They can be utf-8, try 'semanage user -a -R xguest_r test_öä_u'
> >
> > > > > Among other things, it might be nicer to write a comparison routine
> > > > > that, if it passes, you append to file_content. This convoluted
> > > > > triply-nested loop is hard to grok.
> > > >
> > > > Done
> > >
> > > Thanks, but see above comment about unicode comparisons.
> >
> > I'm not sure what do you exactly mean? Use UTF-8/ldb_dn comparison for
> > this? I
> > Based on my previous comments I think this is good as it is.
> >
> > #092:
> > Original #0010, comments:
> >
> > > Please issue a syslog message if the mkstemp() or write() fails. This is
> > > probably going to end up being an SELinux or permission issue.
> > >
> > > Also, please test whether this change is going to necessitate a change
> > > to the SELinux policy shipped in Fedora. I don't know offhand if all
> > > applications have privileges to write to mkstemp().
> >
> > Done
>
> ACK
>
> >
> >
> > #093 - #096:
> > Original #0008, comments:
> >
> > > First point: yes, this needs to be at least two patches. The first
> > > should create the common session creation functionality. The second
> > > should implement it in the IPA provider.
> >
> > Done, although the final granularity is a bit finer.
> >
> > > Please reorder ipa_session.c so that the _recv() functions follow the
> > > _send/<steps>/_done functions for a tevent request.
> > > ipa_get_selinux_recv() does not belong between ipa_session_handler() and
> > > ipa_get_selinux_done().
> >
> > Done
> >
> > > Also, your naming of ipa_get_selinux_done() is wrong. Call it something
> > > like ipa_session_handler_done(). It's not part of the
> > > ipa_get_selinux_*() tevent_req, so don't name it like it is.
> >
> > Done
> >
> > > In ipa_get_selinux_done() you have FIXME to add an outer transaction.
> > > Please do this. Both sysdb_store_selinux_config() and
> > > ipa_save_user_maps() are synchronous actions, so it's perfectly safe.
> >
> > Done
> >
> > > Please don't use numeric DEBUG levels any more. Use the appropriate
> > > macro level. For example, in ipa_get_selinux_send(), the connection
> > > status should probably be SSSDBG_TRACE_INTERNAL, not level 9 (AKA
> > > SSSDBG_TRACE_ALL). SSSDBG_TRACE_ALL should be reserved for REALLY
> > > esoteric data (like pointer addresses in the sbus connection code).
> >
> > Sorry, that was copy-paste remainder. Corrected.
>
> I'm afraid there are still some left:
>
> jzeleny-084-Implemented-support-for-multiple-search-bases-in-HBA.patch:+
> DEBUG(6, ("Option %s set to %s\n",
> jzeleny-084-Implemented-support-for-multiple-search-bases-in-HBA.patch:+
> DEBUG(1, ("Could not replace attribute names\n"));
> jzeleny-084-Implemented-support-for-multiple-search-bases-in-HBA.patch:+
> DEBUG(1, ("Could not replace attribute names\n"));
> jzeleny-093-Separate-the-host-retrieval-code-from-IPA-HBAC-to-co.patch:+
> DEBUG(1, ("Could not replace attribute names\n"));
> jzeleny-093-Separate-the-host-retrieval-code-from-IPA-HBAC-to-co.patch:+
> DEBUG(3, ("Error [%d][%s]\n", ret, strerror(ret)));
> jzeleny-095-Add-session-target-in-data-provider.patch:+ DEBUG(0,
> ("fatal error initializing data providers\n"));
> jzeleny-095-Add-session-target-in-data-provider.patch:+ DEBUG(1, ("No
> SUDO module provided for [%s] !!\n",
> jzeleny-095-Add-session-target-in-data-provider.patch:+ DEBUG(9,
> ("SUDO backend target successfully loaded "
> jzeleny-096-Session-target-in-IPA-provider.patch:+ DEBUG(6, ("Option
> %s set to %s\n",
> jzeleny-096-Session-target-in-IPA-provider.patch:+ DEBUG(1,
> ("talloc_zero failed.\n"));
> jzeleny-096-Session-target-in-IPA-provider.patch:+ DEBUG(1,
> ("sssm_ipa_id_init failed.\n"));
>
>
> >
> > > In ipa_get_selinux_send(), I'd rather you handled the label like this:
> > >
> > > ...
> > > return req;
> > >
> > > immediate:
> > > if (ret == EOK) {
> > > tevent_req_done(req);
> > > } else {
> > > tevent_req_error(req, ret);
> > > }
> > > tevent_req_post(req, ev);
> > > return req;
> > > }
> >
> > Done
> >
> > > Furthermore, I don't think we want to return success for offline. You
> > > should add support for dp_err, errnop and err_msg to the
> > > ipa_get_selinux_state and pass those back in ipa_get_selinux_recv().
> > > Then extend ipa_session_reply() to return these values. This way you
> > > will properly return DP_ERR_OFFLINE and how to handle that will belong
> > > to the responder. You should handle this appropriately in each of the
> > > step functions here as well.
> >
> > Done, although I removed the the ipa_session_reply() because it was
> > basically
> > just an alias.
>
> ok, but there shouldn't be any other call in ipa_session_handler_done()
> which can return EAGAIN, otherwise you might get misleading error
> messages.
>
> >
> > > I just made this same comment on a private review of the SSH key patches
> > > in progress:
> > > "Please do not reimplement ipa_get_hosts_send() completely. Please
> > > modify ipa_hbac_host_info_send() so that you can maintain a common
> > > routine. (You should be able to just add a boolean to skip the hostgroup
> > > lookup and whatever new attributes you need)"
> > >
> > > Please work with Honza ([email protected]) to come up with a common
> > > routine that all three features (SELinux, SSH and HBAC) can use.
> >
> > Done, see patch #093
> >
> > > ipa_selinux_get_maps_state should not be public. Proper encapsulation
> > > means that it should only ever be visible to the
> > > ipa_selinux_get_maps_send() tevent_req. If any external application
> > > needs data from it, they should be getting it from
> > > ipa_selinux_get_maps_recv();
> >
> > I was planning to do that, I guess I forgot. Done
> >
> > > If you get to the done: label in ipa_get_selinux_maps_done() via
> > > receiving no selinux maps, you're not calling tevent_req_done(). Thus
> > > the request will never complete.
> >
> > Actually the same error could happen on more occasions. Good catch, thank
> > you.
> > Corrected.
> >
> > > I have the same concern here about the greedy array allocation that I
> > > had in the sysdb code. There could be hundreds (or thousands) of
> > > possible user maps to filter through. I don't want to waste that much
> > > memory on the confirmed and possible matches.
> >
> > TBD later
> >
> > > Wrap the call to ipa_get_config_send() in a function to be used in both
> > > ipa_get_selinux_maps_done() and ipa_get_selinux_hbac_done().
> >
> > I'm not sure what would be the benefit here. I looked into this briefly and
> > it
> > would probably add more redundant code than it would remove.
> >
> > Thanks for the review. I'm sorry for this format of the email, but it still
> > seems more suitable than to respond to two different emails.
> >
> > Jan
Can you also move the sysdb.h hunk from the last patch into the sysdb
patch? It belongs there and makes cherry-picking patches harder.
_______________________________________________
sssd-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/sssd-devel
