On Sun, 2012-02-05 at 19:28 -0500, Stephen Gallagher wrote:
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.
Sorry, hit send a bit early. Additionally, I'm giving a tentative ack
based on patch inspection for now. So once this is rebased, as long as
it builds and doesn't regress anything, I think it's ready to go for
beta.