Hi, I'm sending all patches implementing support for SELinux user maps. Some support patches are included as well.
#0001: Implemented support for multiple search bases in HBAC rules and services. As discussed before, this is not strictly needed, but I did it anyway to unify the approach to multiple search bases. Just a reminder: the plan is to use these structures and then limit maximal number of search bases to 1 since there is no support in IPA server for more bases anyway.
#0002: This fixes minor regression brought by my previous patch which is already pushed (multiple search bases in IPA hosts).
#0003: Add generic routines to retrieve IPA configuration object. These routines will be used in other parts of the code.
#0004: Rewrite retrieval of password migration flag from IPA config to user previously implemented generic IPA config interface.
#0005: Some sysdb netgroup attributes will be used in SELinux user maps. They will also have the same semantics, therefore they should be renamed and then re- used.
#0006: Some sysdb routines for SELinux support. Please note that some routines are written in very generic way - I'd like to use them also elsewhere in the current code, perhaps as a part of some sysdb refactoring.
#0007: Utility functions for SELinux map matching against information about current user and host.
#0008: SELinux user maps support in IPA provider. Also generig data provider related code is here. I'm considering splitting this patch in two or three. Let me know your opinion.
#0009: Responder support of SELinux user maps - retrieve all applicable maps from sysbd and create content of the user mapping file /etc/selinux/<policy>/logins/<usernale>
#0010: Get the file content from PAM responder and write it to the file. I'm not completely sure whether or not to implement some kind of locking to prevent possible race conditions when reading/writing to this file.
Thanks in advance for the review. Any advices how to improve the code will be appreciated. Jan
Jan Zelený jzeleny@redhat.com wrote:
Hi, I'm sending all patches implementing support for SELinux user maps. Some support patches are included as well.
#0001: Implemented support for multiple search bases in HBAC rules and services. As discussed before, this is not strictly needed, but I did it anyway to unify the approach to multiple search bases. Just a reminder: the plan is to use these structures and then limit maximal number of search bases to 1 since there is no support in IPA server for more bases anyway.
#0002: This fixes minor regression brought by my previous patch which is already pushed (multiple search bases in IPA hosts).
#0003: Add generic routines to retrieve IPA configuration object. These routines will be used in other parts of the code.
#0004: Rewrite retrieval of password migration flag from IPA config to user previously implemented generic IPA config interface.
#0005: Some sysdb netgroup attributes will be used in SELinux user maps. They will also have the same semantics, therefore they should be renamed and then re- used.
#0006: Some sysdb routines for SELinux support. Please note that some routines are written in very generic way - I'd like to use them also elsewhere in the current code, perhaps as a part of some sysdb refactoring.
#0007: Utility functions for SELinux map matching against information about current user and host.
#0008: SELinux user maps support in IPA provider. Also generig data provider related code is here. I'm considering splitting this patch in two or three. Let me know your opinion.
#0009: Responder support of SELinux user maps - retrieve all applicable maps from sysbd and create content of the user mapping file /etc/selinux/<policy>/logins/<usernale>
#0010: Get the file content from PAM responder and write it to the file. I'm not completely sure whether or not to implement some kind of locking to prevent possible race conditions when reading/writing to this file.
Thanks in advance for the review. Any advices how to improve the code will be appreciated. Jan
Patches rebased on top of current master.
Jan
On Tue, 2012-01-31 at 22:15 +0100, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
Hi, I'm sending all patches implementing support for SELinux user maps. Some support patches are included as well.
#0001: Implemented support for multiple search bases in HBAC rules and services. As discussed before, this is not strictly needed, but I did it anyway to unify the approach to multiple search bases. Just a reminder: the plan is to use these structures and then limit maximal number of search bases to 1 since there is no support in IPA server for more bases anyway.
Ack. This is good for maintaining code consistency, as well as being able to support filtering if necessary.
#0002: This fixes minor regression brought by my previous patch which is already pushed (multiple search bases in IPA hosts).
Nack. I fail to see the point of the misdirection through "target" here.
Isn't it simpler to do:
state->hostgroups[state->hostgroup_count] = talloc_steal(state->hostgroups, hostgroups[i]);
#0003: Add generic routines to retrieve IPA configuration object. These routines will be used in other parts of the code.
Ack.
#0004: Rewrite retrieval of password migration flag from IPA config to user previously implemented generic IPA config interface.
Ack.
#0005: Some sysdb netgroup attributes will be used in SELinux user maps. They will also have the same semantics, therefore they should be renamed and then re- used.
Ack.
#0006: Some sysdb routines for SELinux support. Please note that some routines are written in very generic way - I'd like to use them also elsewhere in the current code, perhaps as a part of some sysdb refactoring.
Nack.
(This patch and the next one should be applied in the reverse order. This patch depends on Patch 0007 but the reverse is not true).
The in_transaction construct in sysdb_store_selinux_entity() is wrong. You should not be doing sysdb_transaction_commit() after the done: label. The point of saving the in_transaction value is so that if you 'goto done' after the transaction has started, you will know to cancel the transaction. In the current code, if you failed get_entity(), sysdb_add_selinux_entity(), sysdb_set_selinux_entity_attr() or get_rm_msg(), you're going to be calling sysdb_transaction_commit() on incomplete data. OUCH.
sysdb_search_selinux_config_int() is improperly named. That name implies that it is the internal implementation of sysdb_search_selinux_config(), when it is in fact a wrapper function. How about just calling it get_selinux_config_entity()?
Thinking more about this, I don't think there's really any reason to create and use entity_by_name_fn at all. It would be easier and more readable (I think) to just use an enum for whether it's a user or config lookup. Then you can just call sysdb_search_selinux_usermap_by_name() or sysdb_search_selinux_config() directly in sysdb_store_selinux_entity(). If there were a lot of possible entities to store, it would be one thing. But for just two of them, let's not make the code needlessly complicated.
Don't call the DN argument of sysdb_store_selinux_entity() "dn_template". It's not a template, it has already been constructed. Further, I think it would be better to carry around an ldb_dn rather than just the string representation of the DN. Right now it's being separately constructed in both sysdb_add_selinux_entity() and sysdb_set_selinux_entity_attr(). Beyond that, if you construct the DN in sysdb_store_selinux_entity() and sysdb_store_selinux_usermap(), you don't need to have sysdb_set_selinux_entity_attr() at all, since all it is doing is calling sysdb_set_entry_attr().
Don't "goto done" directly after sysdb_add_selinux_entity(). As mentioned previously, you need to call sysdb_transaction_complete() and (if it succeeds) set in_transaction = false.
sysdb_add_selinux_entity() and sysdb_set_selinux_entity_attr() should set and update the SYSDB_LAST_UPDATE. sysdb_add_selinux_entity() should also set SYSDB_CREATE_TIME. For debugging purposes, it's important to know when the entries were created and updated.
There's no explanation for what the difference is between sysdb_search_selinux_usermap_by_name() and sysdb_search_selinux_usermap_by_user(). Perhaps sysdb_search_selinux_usermap_by_mapname() and sysdb_search_selinux_usermap_by_username() would be more clear?
Please don't call the return value "_msg" in sysdb_search_selinux_usermap_by_name(). It's not descriptive. Name it after the data it should contain (which I think is the contents of the map, expected to be a single entry). If the map name is intended to be unique (which I gather it must be, since it's a base search and you're only returning the first result), you should probably check that sysdb_search_entry returns zero or one for msgs_count. Please make the same change for sysdb_search_selinux_config().
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?
Also, please free each message that doesn't match as we go through the loop. There's no good reason to hang on to memory any longer than we have to. I don't want to tempt the OOM killer.
I know we do the "assume the largest" array allocation in a lot of places, but that's generally where we can reasonably assume array sizes of < 100 entries. With the user maps, there's no limitation on how large it can grow.
sysdb_search_selinux_usermap_by_user() should return ENOENT and free "usermaps" if after the filtering usermaps[0] is NULL. This will make the check in get_selinux_string() in Patch 0009 more clear.
#0007: Utility functions for SELinux map matching against information about current user and host.
Nack.
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.
sss_selinux_match() should probably check for usermap == NULL.
sss_selinux_match() is in desperate need of comments. It's not clear what exactly you're comparing.
sss_selinux_extract_user() doesn't have a matching entry in sss_selinux.h. Looks like you changed plans?
#0008: SELinux user maps support in IPA provider. Also generig data provider related code is here. I'm considering splitting this patch in two or three. Let me know your opinion.
Too much code to look over at 10pm. Will get to it tomorrow.
#0009: Responder support of SELinux user maps - retrieve all applicable maps from sysbd and create content of the user mapping file /etc/selinux/<policy>/logins/<usernale>
Nack.
Please add comments to get_selinux_string().
You need to initialize "order" to NULL in case you get through all of the config elements without finding it. Right now, the if (default_user == NULL || order == NULL) { could be doing a comparison against uninitialized "order".
Don't call strlen(order). You already have this information from the ldb_val.length.
As mentioned in the review for patch 0006, sysdb_search_selinux_usermap_by_user() should be returning ENOENT, which you should check for here instead of NULL usermaps[0].
Did I mention the need for comments in get_selinux_string()? The loop through the usermaps is ugly and hard to read. Please identify why you're writing each piece. (A description comment above describing the file format of the SELinux user map would be great).
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.
#0010: Get the file content from PAM responder and write it to the file. I'm not completely sure whether or not to implement some kind of locking to prevent possible race conditions when reading/writing to this file.
Nack.
As you suggest, you need to handle the file writing safely. Use mkstemp() (properly with umask) to create the contents in a randomly-generated temp file, then move it atop the old file with rename() when it's complete (which is an atomic action).
Thanks in advance for the review. Any advices how to improve the code will be appreciated.
Be careful what you wish for ;-)
On Tue, 2012-01-31 at 22:04 -0500, Stephen Gallagher wrote:
On Tue, 2012-01-31 at 22:15 +0100, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
#0008: SELinux user maps support in IPA provider. Also generig data provider related code is here. I'm considering splitting this patch in two or three. Let me know your opinion.
Too much code to look over at 10pm. Will get to it tomorrow.
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.
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().
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.
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.
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).
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; }
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.
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.
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();
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.
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.
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 sending corrected patches (without the largest one - #0008).
On Tue, 2012-01-31 at 22:15 +0100, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
Hi, I'm sending all patches implementing support for SELinux user maps. Some support patches are included as well.
#0001: Implemented support for multiple search bases in HBAC rules and services. As discussed before, this is not strictly needed, but I did it anyway to unify the approach to multiple search bases. Just a reminder: the plan is to use these structures and then limit maximal number of search bases to 1 since there is no support in IPA server for more bases anyway.
Ack. This is good for maintaining code consistency, as well as being able to support filtering if necessary.
#0002: This fixes minor regression brought by my previous patch which is already pushed (multiple search bases in IPA hosts).
Nack. I fail to see the point of the misdirection through "target" here.
Isn't it simpler to do:
state->hostgroups[state->hostgroup_count] = talloc_steal(state->hostgroups, hostgroups[i]);
Done. The reason here was originally to make the code easier to read. I'm afraid I achieved exactly the opposite in the end.
#0003: Add generic routines to retrieve IPA configuration object. These routines will be used in other parts of the code.
Ack.
#0004: Rewrite retrieval of password migration flag from IPA config to user previously implemented generic IPA config interface.
Ack.
#0005: Some sysdb netgroup attributes will be used in SELinux user maps. They will also have the same semantics, therefore they should be renamed and then re- used.
Ack.
#0006: Some sysdb routines for SELinux support. Please note that some routines are written in very generic way - I'd like to use them also elsewhere in the current code, perhaps as a part of some sysdb refactoring.
Nack.
(This patch and the next one should be applied in the reverse order. This patch depends on Patch 0007 but the reverse is not true).
Done. Although the commend is not quite valid but that's not much important.
The in_transaction construct in sysdb_store_selinux_entity() is wrong. You should not be doing sysdb_transaction_commit() after the done: label. The point of saving the in_transaction value is so that if you 'goto done' after the transaction has started, you will know to cancel the transaction. In the current code, if you failed get_entity(), sysdb_add_selinux_entity(), sysdb_set_selinux_entity_attr() or get_rm_msg(), you're going to be calling sysdb_transaction_commit() on incomplete data. OUCH.
In fact, I copy-pasted this very constuct from sysdb_store_user(). It can be found in sysdb_remove_attrs() as well. I also think the original thought behind this construct was a bit different: to determine if commit/cancel is needed. If it will be commit or cancel is determined by the return code.
sysdb_search_selinux_config_int() is improperly named. That name implies that it is the internal implementation of sysdb_search_selinux_config(), when it is in fact a wrapper function. How about just calling it get_selinux_config_entity()?
Done, it was removed entirely based on your recommendation below.
Thinking more about this, I don't think there's really any reason to create and use entity_by_name_fn at all. It would be easier and more readable (I think) to just use an enum for whether it's a user or config lookup. Then you can just call sysdb_search_selinux_usermap_by_name() or sysdb_search_selinux_config() directly in sysdb_store_selinux_entity(). If there were a lot of possible entities to store, it would be one thing. But for just two of them, let's not make the code needlessly complicated.
I was hoping that this implementation could be later adapted in other parts of sysdb to unify the approach to storing various entities. But looking more closely at the code, it seems that it would require great deal of other changes. Hence I agree with your opinion, the change is included in the new patch.
Don't call the DN argument of sysdb_store_selinux_entity() "dn_template". It's not a template, it has already been constructed. Further, I think it would be better to carry around an ldb_dn rather than just the string representation of the DN. Right now it's being separately constructed in both sysdb_add_selinux_entity() and sysdb_set_selinux_entity_attr(). Beyond that, if you construct the DN in sysdb_store_selinux_entity() and sysdb_store_selinux_usermap(), you don't need to have sysdb_set_selinux_entity_attr() at all, since all it is doing is calling sysdb_set_entry_attr().
Done. I just left the dn_template since it really is a template at the time the function is called.
Don't "goto done" directly after sysdb_add_selinux_entity(). As mentioned previously, you need to call sysdb_transaction_complete() and (if it succeeds) set in_transaction = false.
As commented above.
sysdb_add_selinux_entity() and sysdb_set_selinux_entity_attr() should set and update the SYSDB_LAST_UPDATE. sysdb_add_selinux_entity() should also set SYSDB_CREATE_TIME. For debugging purposes, it's important to know when the entries were created and updated.
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? ;-)
There's no explanation for what the difference is between sysdb_search_selinux_usermap_by_name() and sysdb_search_selinux_usermap_by_user(). Perhaps sysdb_search_selinux_usermap_by_mapname() and sysdb_search_selinux_usermap_by_username() would be more clear?
Done
Please don't call the return value "_msg" in sysdb_search_selinux_usermap_by_name(). It's not descriptive. Name it after the data it should contain (which I think is the contents of the map, expected to be a single entry). If the map name is intended to be unique (which I gather it must be, since it's a base search and you're only returning the first result), you should probably check that sysdb_search_entry returns zero or one for msgs_count. Please make the same change for sysdb_search_selinux_config().
Renaming the _msg: done. Checking the count: I don't think it's necessary. I don't see how it would be possible to get more than one result from base search. Also, I just checked source code of LDB - it explicitly sets count = 1 every time you perform base search.
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).
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.
Also, please free each message that doesn't match as we go through the loop. There's no good reason to hang on to memory any longer than we have to. I don't want to tempt the OOM killer.
Done
I know we do the "assume the largest" array allocation in a lot of places, but that's generally where we can reasonably assume array sizes of < 100 entries. With the user maps, there's no limitation on how large it can grow.
That's true but I'm really not sure how to optimize this further. It would need some design changes on the server side. I will consult my idea with Rob.
sysdb_search_selinux_usermap_by_user() should return ENOENT and free "usermaps" if after the filtering usermaps[0] is NULL. This will make the check in get_selinux_string() in Patch 0009 more clear.
Done
#0007: Utility functions for SELinux map matching against information about current user and host.
Nack.
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.
sss_selinux_match() should probably check for usermap == NULL.
Done.
sss_selinux_match() is in desperate need of comments. It's not clear what exactly you're comparing.
Done.
sss_selinux_extract_user() doesn't have a matching entry in sss_selinux.h. Looks like you changed plans?
Actually, it does have a matching entry in that file.
#0008: SELinux user maps support in IPA provider. Also generig data provider related code is here. I'm considering splitting this patch in two or three. Let me know your opinion.
Too much code to look over at 10pm. Will get to it tomorrow.
#0009: Responder support of SELinux user maps - retrieve all applicable maps from sysbd and create content of the user mapping file /etc/selinux/<policy>/logins/<usernale>
Nack.
Please add comments to get_selinux_string().
Done.
You need to initialize "order" to NULL in case you get through all of the config elements without finding it. Right now, the if (default_user == NULL || order == NULL) { could be doing a comparison against uninitialized "order".
Done. Also the default_user needed the same fix.
Don't call strlen(order). You already have this information from the ldb_val.length.
Done.
As mentioned in the review for patch 0006, sysdb_search_selinux_usermap_by_user() should be returning ENOENT, which you should check for here instead of NULL usermaps[0].
Done.
Did I mention the need for comments in get_selinux_string()? The loop through the usermaps is ugly and hard to read. Please identify why you're writing each piece. (A description comment above describing the file format of the SELinux user map would be great).
Done.
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
#0010: Get the file content from PAM responder and write it to the file. I'm not completely sure whether or not to implement some kind of locking to prevent possible race conditions when reading/writing to this file.
Nack.
As you suggest, you need to handle the file writing safely. Use mkstemp() (properly with umask) to create the contents in a randomly-generated temp file, then move it atop the old file with rename() when it's complete (which is an atomic action).
Done.
Thanks in advance for the review. Any advices how to improve the code will be appreciated.
Be careful what you wish for ;-)
Thanks for the review Jan
On Wed, 2012-02-01 at 18:29 +0100, Jan Zelený wrote:
I'm sending corrected patches (without the largest one - #0008).
On Tue, 2012-01-31 at 22:15 +0100, Jan Zeleny wrote:
Jan Zelený jzeleny@redhat.com wrote:
Hi, I'm sending all patches implementing support for SELinux user maps. Some support patches are included as well.
#0001: Implemented support for multiple search bases in HBAC rules and services. As discussed before, this is not strictly needed, but I did it anyway to unify the approach to multiple search bases. Just a reminder: the plan is to use these structures and then limit maximal number of search bases to 1 since there is no support in IPA server for more bases anyway.
Ack. This is good for maintaining code consistency, as well as being able to support filtering if necessary.
Still ack
#0002: This fixes minor regression brought by my previous patch which is already pushed (multiple search bases in IPA hosts).
Nack. I fail to see the point of the misdirection through "target" here.
Isn't it simpler to do:
state->hostgroups[state->hostgroup_count] = talloc_steal(state->hostgroups, hostgroups[i]);
Done. The reason here was originally to make the code easier to read. I'm afraid I achieved exactly the opposite in the end.
Ack.
#0003: Add generic routines to retrieve IPA configuration object. These routines will be used in other parts of the code.
Ack.
Still ack.
#0004: Rewrite retrieval of password migration flag from IPA config to user previously implemented generic IPA config interface.
Ack.
Still ack.
#0005: Some sysdb netgroup attributes will be used in SELinux user maps. They will also have the same semantics, therefore they should be renamed and then re- used.
Ack.
Still ack.
#0006: Some sysdb routines for SELinux support. Please note that some routines are written in very generic way - I'd like to use them also elsewhere in the current code, perhaps as a part of some sysdb refactoring.
Nack.
(This patch and the next one should be applied in the reverse order. This patch depends on Patch 0007 but the reverse is not true).
Done. Although the commend is not quite valid but that's not much important.
The in_transaction construct in sysdb_store_selinux_entity() is wrong. You should not be doing sysdb_transaction_commit() after the done: label. The point of saving the in_transaction value is so that if you 'goto done' after the transaction has started, you will know to cancel the transaction. In the current code, if you failed get_entity(), sysdb_add_selinux_entity(), sysdb_set_selinux_entity_attr() or get_rm_msg(), you're going to be calling sysdb_transaction_commit() on incomplete data. OUCH.
In fact, I copy-pasted this very constuct from sysdb_store_user(). It can be found in sysdb_remove_attrs() as well. I also think the original thought behind this construct was a bit different: to determine if commit/cancel is needed. If it will be commit or cancel is determined by the return code.
Ok, on a further read, this is safe (though I find it a bit ugly). Sorry for the confusion.
sysdb_search_selinux_config_int() is improperly named. That name implies that it is the internal implementation of sysdb_search_selinux_config(), when it is in fact a wrapper function. How about just calling it get_selinux_config_entity()?
Done, it was removed entirely based on your recommendation below.
Thinking more about this, I don't think there's really any reason to create and use entity_by_name_fn at all. It would be easier and more readable (I think) to just use an enum for whether it's a user or config lookup. Then you can just call sysdb_search_selinux_usermap_by_name() or sysdb_search_selinux_config() directly in sysdb_store_selinux_entity(). If there were a lot of possible entities to store, it would be one thing. But for just two of them, let's not make the code needlessly complicated.
I was hoping that this implementation could be later adapted in other parts of sysdb to unify the approach to storing various entities. But looking more closely at the code, it seems that it would require great deal of other changes. Hence I agree with your opinion, the change is included in the new patch.
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.
Don't call the DN argument of sysdb_store_selinux_entity() "dn_template". It's not a template, it has already been constructed. Further, I think it would be better to carry around an ldb_dn rather than just the string representation of the DN. Right now it's being separately constructed in both sysdb_add_selinux_entity() and sysdb_set_selinux_entity_attr(). Beyond that, if you construct the DN in sysdb_store_selinux_entity() and sysdb_store_selinux_usermap(), you don't need to have sysdb_set_selinux_entity_attr() at all, since all it is doing is calling sysdb_set_entry_attr().
Done. I just left the dn_template since it really is a template at the time the function is called.
Looks good.
Don't "goto done" directly after sysdb_add_selinux_entity(). As mentioned previously, you need to call sysdb_transaction_complete() and (if it succeeds) set in_transaction = false.
As commented above.
Acknowledged.
sysdb_add_selinux_entity() and sysdb_set_selinux_entity_attr() should set and update the SYSDB_LAST_UPDATE. sysdb_add_selinux_entity() should also set SYSDB_CREATE_TIME. For debugging purposes, it's important to know when the entries were created and updated.
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.
There's no explanation for what the difference is between sysdb_search_selinux_usermap_by_name() and sysdb_search_selinux_usermap_by_user(). Perhaps sysdb_search_selinux_usermap_by_mapname() and sysdb_search_selinux_usermap_by_username() would be more clear?
Done
Please don't call the return value "_msg" in sysdb_search_selinux_usermap_by_name(). It's not descriptive. Name it after the data it should contain (which I think is the contents of the map, expected to be a single entry). If the map name is intended to be unique (which I gather it must be, since it's a base search and you're only returning the first result), you should probably check that sysdb_search_entry returns zero or one for msgs_count. Please make the same change for sysdb_search_selinux_config().
Renaming the _msg: done.
Thanks.
Checking the count: I don't think it's necessary. I don't see how it would be possible to get more than one result from base search. Also, I just checked source code of LDB - it explicitly sets count = 1 every time you perform base search.
Yes, you're right.
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.
Also, please free each message that doesn't match as we go through the loop. There's no good reason to hang on to memory any longer than we have to. I don't want to tempt the OOM killer.
Done
Thanks
I know we do the "assume the largest" array allocation in a lot of places, but that's generally where we can reasonably assume array sizes of < 100 entries. With the user maps, there's no limitation on how large it can grow.
That's true but I'm really not sure how to optimize this further. It would need some design changes on the server side. I will consult my idea with Rob.
I'd like to hear this idea too, please :)
sysdb_search_selinux_usermap_by_user() should return ENOENT and free "usermaps" if after the filtering usermaps[0] is NULL. This will make the check in get_selinux_string() in Patch 0009 more clear.
Done
Thanks
#0007: Utility functions for SELinux map matching against information about current user and host.
Nack.
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.
sss_selinux_match() should probably check for usermap == NULL.
Done.
Thanks
sss_selinux_match() is in desperate need of comments. It's not clear what exactly you're comparing.
Done.
Thanks
sss_selinux_extract_user() doesn't have a matching entry in sss_selinux.h. Looks like you changed plans?
Actually, it does have a matching entry in that file.
Sorry, my eyes must have been glazing over by this point.
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.
#0008: SELinux user maps support in IPA provider. Also generig data provider related code is here. I'm considering splitting this patch in two or three. Let me know your opinion.
Too much code to look over at 10pm. Will get to it tomorrow.
See my other reply for changes needed here.
#0009: Responder support of SELinux user maps - retrieve all applicable maps from sysbd and create content of the user mapping file /etc/selinux/<policy>/logins/<usernale>
Nack.
Please add comments to get_selinux_string().
Done.
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).
You need to initialize "order" to NULL in case you get through all of the config elements without finding it. Right now, the if (default_user == NULL || order == NULL) { could be doing a comparison against uninitialized "order".
Done. Also the default_user needed the same fix.
Thanks
Don't call strlen(order). You already have this information from the ldb_val.length.
Done.
As mentioned in the review for patch 0006, sysdb_search_selinux_usermap_by_user() should be returning ENOENT, which you should check for here instead of NULL usermaps[0].
Done.
Thanks
Did I mention the need for comments in get_selinux_string()? The loop through the usermaps is ugly and hard to read. Please identify why you're writing each piece. (A description comment above describing the file format of the SELinux user map would be great).
Done.
Thanks, that's much more clear now.
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.
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.
#0010: Get the file content from PAM responder and write it to the file. I'm not completely sure whether or not to implement some kind of locking to prevent possible race conditions when reading/writing to this file.
Nack.
As you suggest, you need to handle the file writing safely. Use mkstemp() (properly with umask) to create the contents in a randomly-generated temp file, then move it atop the old file with rename() when it's complete (which is an atomic action).
Done.
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().
Otherwise, this looks good to me.
Thanks in advance for the review. Any advices how to improve the code will be appreciated.
Be careful what you wish for ;-)
Thanks for the review Jan _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org
-
Jan Zelený -
Stephen Gallagher