On Sun, 2012-02-05 at 22:20 +0100, Jan Zeleny wrote:
Next round of patches attached:
Sumit Bose <sbose(a)redhat.com> 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.
Done
> > > 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.
I added an "if" statement detecting whether or not is the value ok.
> > > > 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'
Ok, the function has been modified to count with UTF-8 strings.
> > > > > 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"));
Fixed
> > > 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 know. I think the code is safe in this area. If you want me to, I can re-
check to be sure.
> > > 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 (jcholast(a)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
I thought about the other solution a bit more and it won't probably be
unacceptable due to large data redundancy on the server. Therefore I'm
inclined towards leaving the code as is and file a ticket for its memory
optimization. What do you think?
> > > 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.
Also Jakub's note has been taken into account.
Please rebase these patches atop the current master. I tried to do it
myself, but I couldn't get it to work.