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 (jchol...@redhat.com) 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
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
