#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.
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).
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.
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.
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
#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.
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.
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@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
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 (jcholast@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
On Fri, Feb 03, 2012 at 09:44:13PM +0100, Sumit Bose 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.
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 (jcholast@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
Can you also move the sysdb.h hunk from the last patch into the sysdb patch? It belongs there and makes cherry-picking patches harder.
Next round of patches attached:
Sumit Bose sbose@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@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.
Thank you both for the review Jan
On Sun, 2012-02-05 at 22:20 +0100, Jan Zeleny wrote:
Next round of patches attached:
Sumit Bose sbose@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@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.
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@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@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.
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@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@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
On Mon, 2012-02-06 at 10:25 +0100, Jan Zelený wrote:
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.
Ack to all patches, with one minor change.
I'm pushing a fix to the first patch in the series (you created ipa_hbac_rules.h but didn't add it to Makefile.am, so 'make dist' wasn't including it).
Pushed to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Stephen Gallagher -
Sumit Bose