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.patc
> > > h:+
> > >
> > > DEBUG(6, ("Option %s set to %s\n",
> > >
> > > jzeleny-084-Implemented-support-for-multiple-search-bases-in-HBA.patc
> > > h:+
> > >
> > > DEBUG(1, ("Could not replace attribute names\n"));
> > >
> > > jzeleny-084-Implemented-support-for-multiple-search-bases-in-HBA.patc
> > > h:+
> > >
> > > DEBUG(1, ("Could not replace attribute names\n"));
> > >
> > > jzeleny-093-Separate-the-host-retrieval-code-from-IPA-HBAC-to-co.patc
> > > h:+
> > >
> > > DEBUG(1, ("Could not replace attribute names\n"));
> > >
> > > jzeleny-093-Separate-the-host-retrieval-code-from-IPA-HBAC-to-co.patc
> > > h:+
> > >
> > > 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.
Hopefully the last set of patches. I'm also attaching the man page update as
patch #099. Feel free to review it separately, I just wanted it to be a part
of the whole patch set.
Thanks
Jan