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 <[email protected]> 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 ([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 > > > > 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.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
