Hi guys, it took me and Sumit some time but we finally have completed the first stage of support for subdomains. I'm sending all patches in attachment.
This stage has basic support for subdomains but some pieces like PAC support are left out. We agreed that those can wait for the second stage which depends on some server side changes that are not ready yet. We'll keep you posted.
Please note that patches depend on Sumit's previous idmap library patch. I'll get that reviewed either today or tomorrow.
#0001 All sysdby routines that are used in later patches
#0002 Responder routines for asking provider for a list of supported subdomains
#0003 The routine responder_get_domain is modified to look for subdomains as well when looking for the correct domain descriptor
#0004 If a request for fully qualified entity (user/group) comes to the responder and its domain has not been found, it updates its list of domains and supported subdomains in each domain. To update the list, a get_domains call is sent to each provider.
#0005 Create a list of all domains and subdomains when the first request after start comes in. To create the list, a get_domains call is sent to each provider.
#0006 Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
#0007 Basic infrastructure for handling get_subdomains requests in providers.
#0008 Add the infrastructure in IPA provider which fetches a list of handled subdomains from server and stores in in sysdb.
#0009 Currently the connections to the data provider use the same name as the domain. With sub-domains the name of the sub-domain cannot be used to find the right data provider connection, hence we store the name of the connection in a new member.
#0010 Support for subdomain name given to provider when calling get_account_info
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
#0012 The routine expand_homedir_template is now used on more place, therefore it was convenient to move it to util code.
#0013 s2n extended LDAP operation - this is used to ask IPA server for entities from supported subdomains
#0014 Extend IPA ID provider toi support fetching information for entities within subdomains
#0015 Support for routing PAM request from reponder to the right provider. Similar case as patches #3-#6 in NSS responder but much more simple.
#0016 Basic support (or rather un-support) for subdomains in auth providers. Basically it's designed to acknowledge that the request is not for the main domain but for subdomain and in that case it supports only SSS_PAM_ACCT_MGMT and returns error otherwise. Note that similar thing as the IPA backend change in last hunk is probably not necessary for krb5 backend since when using it, there will be no subdomains for that domain.
#0017 Support providing sysdb context and domain info per request, not per backend context. This is necessary for the next patches.
#0018 Accept be_req instead of be_ctx in access providers. Together with the previous patch, this is needed for the next one.
#0019 Detect if the authorization request came for a user in subdomain and if yes, replace the original backend-wide sysdb context and domain info with their subdomain specific replacements.
#0020 Use request-wide sysdb context and domain info instead of their backend-wide counterparts in HBAC.
That's all folks, have fun reviewing :) Thanks and bye, Jan and Sumit
On Tue, 2012-03-20 at 17:42 +0100, Jan Zeleny wrote:
Hi guys, it took me and Sumit some time but we finally have completed the first stage of support for subdomains. I'm sending all patches in attachment.
This stage has basic support for subdomains but some pieces like PAC support are left out. We agreed that those can wait for the second stage which depends on some server side changes that are not ready yet. We'll keep you posted.
Please note that patches depend on Sumit's previous idmap library patch. I'll get that reviewed either today or tomorrow.
#0001 All sysdby routines that are used in later patches
Nack.
Please fix debug levels to use the new format.
Why are you creating a NULL-terminated array *and* returning the number of elements? Usually you would do only one or the other. This isn't the nack, just a curiosity.
I don't care for the inner loop through the elements in sysdb_get_subdomains(). I'd rather see you use ldb_msg_find_attr_as_string() three times. The benefit here is that you can easily detect if one or more the elements are missing. In the current approach, you'll just iterate through, but if the DN existed but had no attributes associated with it, you would be left with a 'struct subdomain_info' that contained only NULL strings... a recipe for disaster.
Though, if you want to be *really* paranoid, you might even want to use ldb_message_find_element() so you can test that they are in fact single-valued as well.
sysdb_update_subdomains() desperately needs comments. It took me ten minutes to figure out the logic there. I also question why you didn'd include sysdb_[add|remove]_subdomain(). An explanation of this lack should be in the source code, if there's a good reason.
The tests added by this patch depend on routines added in Patch 0003 (domain_info_utils.c). Probably this should be reordered.
#0002 Responder routines for asking provider for a list of supported subdomains
Nack. dp_get_domains_callback() should be static.
Please add comments explaining the purpose of the "hint" argument.
Please redesign this routine to use the common sss_dp_issue_request(). This will give us a few things: 1) Bundling of concurrent 'force' lookups, 2) processing closer to the tevent_req style. get_domains_send() must have a matching get_domains_recv() function for consumers to retrieve, if only to be able to find out if there were errors.
Take a look at how sss_dp_get_account_send() and sss_dp_get_sudoers_send() works now with sss_dp_issue_request()
If parsing fails in dp_get_domains_callback(), you're jumping to done: rather than just treating it like other errors.
This patch also depends on domain_info_utils.c
get_domains_done() needs comments on the logic.
The creation of new_sd_list should be its own function.
#0003 The routine responder_get_domain is modified to look for subdomains as well when looking for the correct domain descriptor
Nack
As mentioned above, domain_info_utils.c should appear earlier in the patch series. Probably it should have its own patch.
The logic in this patch looks good to me.
#0004 If a request for fully qualified entity (user/group) comes to the responder and its domain has not been found, it updates its list of domains and supported subdomains in each domain. To update the list, a get_domains call is sent to each provider.
Nack This is going to need quite a bit of modification to handle the changes required for Patch 0002. With that in mind, the logic looks fine.
#0005 Create a list of all domains and subdomains when the first request after start comes in. To create the list, a get_domains call is sent to each provider.
Nack
Similar to 0004, this is going to need to be reworked to account for changes to 0002.
I don't understand why the addition of "if (dctx->domname)" in the callbacks wasn't part of patch 0004. Also, a comment would got far towards explaining why that's being done (I assume it's to handle explicit domain requests)
#0006 Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
Nack Please add comments to get_next_dom_or_subdom() to explain the logic. The code itself looks good.
#0007 Basic infrastructure for handling get_subdomains requests in providers.
Nack
Please fix debug levels to use the new format.
You initialized be_req to NULL twice.
You need to check for whether becli->bectx->bet_info[BET_SUBDOMAINS] is NULL, or else you'll crash on domains that don't support subdomain lookups.
I'll get to the rest of the patches a bit later.
Finishing my review.
On Tue, 2012-03-20 at 17:42 +0100, Jan Zeleny wrote: ...
#0007 Basic infrastructure for handling get_subdomains requests in providers.
One additional point about this patch. I don't like that you're forcing the BET_SUBDOMAINS to get its data from the ID provider. I'd much prefer that we add a subdomain_provider option, which defaults to being the same as the ID provider. I prefer to maintain the distinction in case we ever decide to have alternate mechanisms for determining the subdomains (in IPA or AD).
#0008 Add the infrastructure in IPA provider which fetches a list of handled subdomains from server and stores in in sysdb.
Nack.
Please don't use variables named 'state' if they're not the state for a tevent_req. It gets confusing. Please rename 'struct subdomains_state' to 'struct ipa_subdomains_ctx' and call the variable something like ipa_sd_ctx.
ipa_subdomains_get_conn_done() should follow the multiple search base pattern, not really because we expect to support multiple bases but because we *should* support filters if they've been added. It's possible that a deployment might choose to filter out certain domains from a particular client using this method.
You have the wrong debug message if new_domain_list[c] fails to be allocated in ipa_subdomains_parse_results(). It lists talloc_array.
I'd prefer if you added an ipa_subdomain_map and used that for determining the attribute names to use in ipa_subdomains_parse_results(). Mostly for consistency.
#0009 Currently the connections to the data provider use the same name as the domain. With sub-domains the name of the sub-domain cannot be used to find the right data provider connection, hence we store the name of the connection in a new member.
Ack
#0010 Support for subdomain name given to provider when calling get_account_info
Nack (minor)
Some whitespace issues in be_get_account_info (leading tabs) around the req->domain allocation.
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
Nack.
I don't think this really makes sense at all. In most cases, users will prefer to use the value on the LDAP server. If they choose to override it, they'll do so through the existing override options (in the case of override_homedir, it already has %d available anyway.
We definitely don't need separate handling for shells. I can *kind of* see a value if you wanted to have only subdomains have a non-default location. I'm not sure I like that though. I feel like it's probably more complexity than we need.
#0012 The routine expand_homedir_template is now used on more place, therefore it was convenient to move it to util code.
Ack
#0013 s2n extended LDAP operation - this is used to ask IPA server for entities from supported subdomains
Nack.
This code really needs comments. s2n_encode_request() and s2n_response_to_attrs() are incomprehensible.
Please make ipa_s2n_exop_send() and ipa_s2n_exop_recv() static.
#0014 Extend IPA ID provider toi support fetching information for entities within subdomains
Ack
#0015 Support for routing PAM request from reponder to the right provider. Similar case as patches #3-#6 in NSS responder but much more simple.
Ack
#0016 Basic support (or rather un-support) for subdomains in auth providers. Basically it's designed to acknowledge that the request is not for the main domain but for subdomain and in that case it supports only SSS_PAM_ACCT_MGMT and returns error otherwise. Note that similar thing as the IPA backend change in last hunk is probably not necessary for krb5 backend since when using it, there will be no subdomains for that domain.
Ack
#0017 Support providing sysdb context and domain info per request, not per backend context. This is necessary for the next patches.
Ack
#0018 Accept be_req instead of be_ctx in access providers. Together with the previous patch, this is needed for the next one.
Ack
#0019 Detect if the authorization request came for a user in subdomain and if yes, replace the original backend-wide sysdb context and domain info with their subdomain specific replacements.
Ack
#0020 Use request-wide sysdb context and domain info instead of their backend-wide counterparts in HBAC.
Ack
That's all folks, have fun reviewing :) Thanks and bye, Jan and Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Ok, all those changes took me a while but I'm finally done. Please note that patch #0003 has been split into two and the part with utility functions has been moved right after sysdb routines. There is a circular dependency between the two which I don't think is worth trying to solve, neither patch would work without the other. This patch split caused the numbering to be shifted by one.
On Tue, 2012-03-20 at 17:42 +0100, Jan Zeleny wrote:
Hi guys, it took me and Sumit some time but we finally have completed the first stage of support for subdomains. I'm sending all patches in attachment.
This stage has basic support for subdomains but some pieces like PAC support are left out. We agreed that those can wait for the second stage which depends on some server side changes that are not ready yet. We'll keep you posted.
Please note that patches depend on Sumit's previous idmap library patch. I'll get that reviewed either today or tomorrow.
#0001 All sysdby routines that are used in later patches
Nack.
Please fix debug levels to use the new format.
Done
Why are you creating a NULL-terminated array *and* returning the number of elements? Usually you would do only one or the other. This isn't the nack, just a curiosity.
That was a leftover from times when the design was not clear.
I don't care for the inner loop through the elements in sysdb_get_subdomains(). I'd rather see you use ldb_msg_find_attr_as_string() three times. The benefit here is that you can easily detect if one or more the elements are missing. In the current approach, you'll just iterate through, but if the DN existed but had no attributes associated with it, you would be left with a 'struct subdomain_info' that contained only NULL strings... a recipe for disaster.
Changed
Though, if you want to be *really* paranoid, you might even want to use ldb_message_find_element() so you can test that they are in fact single-valued as well.
I think we don't have to be that paranoid.
sysdb_update_subdomains() desperately needs comments. It took me ten minutes to figure out the logic there. I also question why you didn'd include sysdb_[add|remove]_subdomain(). An explanation of this lack should be in the source code, if there's a good reason.
Hm, I didn't write the routine and I could read it quite well. Nevertheless I added some comments. Those routines functions you are missing would be rather simple (almost aliases), that's why they are missing. Do you think there is a need for them?
The tests added by this patch depend on routines added in Patch 0003 (domain_info_utils.c). Probably this should be reordered.
There is actually a circular dependency, that's why I left it this way. The only thing I could do is split the patch and move the part with tests. However compared to the original state, I have split the "Modified responder_get_domain()" patch and moved the part with utility functions.
#0002 Responder routines for asking provider for a list of supported subdomains
Nack. dp_get_domains_callback() should be static.
Done
Please add comments explaining the purpose of the "hint" argument.
Done
Please redesign this routine to use the common sss_dp_issue_request(). This will give us a few things: 1) Bundling of concurrent 'force' lookups, 2) processing closer to the tevent_req style. get_domains_send() must have a matching get_domains_recv() function for consumers to retrieve, if only to be able to find out if there were errors.
Take a look at how sss_dp_get_account_send() and sss_dp_get_sudoers_send() works now with sss_dp_issue_request()
Done
If parsing fails in dp_get_domains_callback(), you're jumping to done: rather than just treating it like other errors.
The behavior has changed completely
This patch also depends on domain_info_utils.c
Done
get_domains_done() needs comments on the logic.
Done
The creation of new_sd_list should be its own function.
I disagree. The funcion would be called on this one place anyway so there is probably only a little gain in doing that.
#0003 The routine responder_get_domain is modified to look for subdomains as well when looking for the correct domain descriptor
Nack
As mentioned above, domain_info_utils.c should appear earlier in the patch series. Probably it should have its own patch.
Done
The logic in this patch looks good to me.
#0004 If a request for fully qualified entity (user/group) comes to the responder and its domain has not been found, it updates its list of domains and supported subdomains in each domain. To update the list, a get_domains call is sent to each provider.
Nack This is going to need quite a bit of modification to handle the changes required for Patch 0002. With that in mind, the logic looks fine.
Not really, only renamed the entry procedure and everything works. Most of the changes were introduced in the updated patch #0002
#0005 Create a list of all domains and subdomains when the first request after start comes in. To create the list, a get_domains call is sent to each provider.
Nack
Similar to 0004, this is going to need to be reworked to account for changes to 0002.
Same as previous comment
I don't understand why the addition of "if (dctx->domname)" in the callbacks wasn't part of patch 0004. Also, a comment would got far towards explaining why that's being done (I assume it's to handle explicit domain requests)
Yes, that was fix that should have been part of the previous patch. Fixed now. The reason is obvious IMO - the same behavior was and still is in the original code of functions like nss_cmd_getgrnam().
#0006 Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
Nack Please add comments to get_next_dom_or_subdom() to explain the logic. The code itself looks good.
IMO the logic is straightforward (for the record I didn't write the original code ;-)) but I still added some comments just to be sure.
#0007 Basic infrastructure for handling get_subdomains requests in providers.
Nack
Please fix debug levels to use the new format.
Done
You initialized be_req to NULL twice.
Fixed
You need to check for whether becli->bectx->bet_info[BET_SUBDOMAINS] is NULL, or else you'll crash on domains that don't support subdomain lookups.
I'm not sure what do you mean here, how could it be null when it's not a pointer?
One additional point about this patch. I don't like that you're forcing the BET_SUBDOMAINS to get its data from the ID provider. I'd much prefer that we add a subdomain_provider option, which defaults to being the same as the ID provider. I prefer to maintain the distinction in case we ever decide to have alternate mechanisms for determining the subdomains (in IPA or AD).
I discussed this with Sumit to be sure and we agreed that subdomains are supposed to be tightly bound to the ID provider. One of the reasons is that ID provider has to sort out all following requests so it will also needs to keep track of all subdomains.
#0008 Add the infrastructure in IPA provider which fetches a list of handled subdomains from server and stores in in sysdb.
Nack.
Please don't use variables named 'state' if they're not the state for a tevent_req. It gets confusing. Please rename 'struct subdomains_state' to 'struct ipa_subdomains_ctx' and call the variable something like ipa_sd_ctx.
Actually the name you suggest is already used so I came up with another one: struct ipa_subdomains_req_ctx
ipa_subdomains_get_conn_done() should follow the multiple search base pattern, not really because we expect to support multiple bases but because we *should* support filters if they've been added. It's possible that a deployment might choose to filter out certain domains from a particular client using this method.
Done
You have the wrong debug message if new_domain_list[c] fails to be allocated in ipa_subdomains_parse_results(). It lists talloc_array.
Done
I'd prefer if you added an ipa_subdomain_map and used that for determining the attribute names to use in ipa_subdomains_parse_results(). Mostly for consistency.
I don't think there would be much gain, the code almost wouldn't change, only names of attributes would be different.
#0009 Currently the connections to the data provider use the same name as the domain. With sub-domains the name of the sub-domain cannot be used to find the right data provider connection, hence we store the name of the connection in a new member.
Ack
#0010 Support for subdomain name given to provider when calling get_account_info
Nack (minor)
Some whitespace issues in be_get_account_info (leading tabs) around the req->domain allocation.
Done
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
Nack.
I don't think this really makes sense at all. In most cases, users will prefer to use the value on the LDAP server. If they choose to override it, they'll do so through the existing override options (in the case of override_homedir, it already has %d available anyway.
We definitely don't need separate handling for shells. I can *kind of* see a value if you wanted to have only subdomains have a non-default location. I'm not sure I like that though. I feel like it's probably more complexity than we need.
I think you possibly missed the point. The point is that this information is NOT on the server, therefore we need a value that will fill it in. Otherwise only a blank field will be stored in sysdb and returned to the client
#0012 The routine expand_homedir_template is now used on more place, therefore it was convenient to move it to util code.
Ack
#0013 s2n extended LDAP operation - this is used to ask IPA server for entities from supported subdomains
Nack.
This code really needs comments. s2n_encode_request() and s2n_response_to_attrs() are incomprehensible.
Please make ipa_s2n_exop_send() and ipa_s2n_exop_recv() static.
This should be all done, my thanks go to Sumit
#0014 Extend IPA ID provider toi support fetching information for entities within subdomains
Ack
#0015 Support for routing PAM request from reponder to the right provider. Similar case as patches #3-#6 in NSS responder but much more simple.
Ack
#0016 Basic support (or rather un-support) for subdomains in auth providers. Basically it's designed to acknowledge that the request is not for the main domain but for subdomain and in that case it supports only SSS_PAM_ACCT_MGMT and returns error otherwise. Note that similar thing as the IPA backend change in last hunk is probably not necessary for krb5 backend since when using it, there will be no subdomains for that domain.
Ack
#0017 Support providing sysdb context and domain info per request, not per backend context. This is necessary for the next patches.
Ack
#0018 Accept be_req instead of be_ctx in access providers. Together with the previous patch, this is needed for the next one.
Ack
#0019 Detect if the authorization request came for a user in subdomain and if yes, replace the original backend-wide sysdb context and domain info with their subdomain specific replacements.
Ack
#0020 Use request-wide sysdb context and domain info instead of their backend-wide counterparts in HBAC.
Ack
Thank you Jan
On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
Once more, this time rebased on top of current master.
Thanks Jan
Patch 0001: Sysdb routines for subdomains Ack, can you just add a DEBUG message in case sysdb_transaction_commit() fails?
Patch 0002: Add some utility functions for subdomains Ack
Patch 0003: Add conn_name to allow different names for domains and connections Ack
Patch 0004: Responder part of the subdomain retrieval work "get_domains_timeout" is not mentioned in neither configAPI nor manpages
+ if (ret == EOK) { + DEBUG(SSSDBG_TRACE_FUNC, + ("Last call was too recent, nothing to do!\n")); + ret = EOK; ^^ this is redundant + goto done; + } else if (ret != EAGAIN) {
Please stick to either goto or return in get_domains_next()
Two review items from the original review are not addressed yet: There is no explanation of the "hint" parameter (and without one, it's hard to say what to do with the FIXME) and there is no sss_dp_get_domains_recv()
Patch 0005: Modified responder_get_domain() It is a convention to put a TALLOC_CTX we allocate on as the first parameter (responder_get_domain())
I think that it might be useful to store CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from confdb on every request. The case where confdb_get_int either errors out or the timeout is negative should at least trigger a DEBUG message.
As a general question, why are the domain names case sensitive but subdomain names seem to be case insensitive? For example check_last_request() is doing a case-sensitive strcmp but responder_get_domain is doing a strcasecmp.
Also please split the long DEBUG() statement in responder_get_domain() into two lines, it doesn't fit on my screen :-)
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
Patch 0007: Ask for subdomains in responder in the first request after startup Ack, with the same comment about services and SSH as with patch #6.
Patch 0008: Check sub-domains in nss_cmd_get{pwuid|grgid}_search() Ack
Patch 0009: data provider: added subdomains If the bandaid patch I posted on the list the other day is accepted, there should be a check for the result of the sbus_get_connection() call.
The comment about fast reply in be_get_subdomains() seems misleading to me, there is no check for BE_REQ_FAST and the whole code seems like it can handle online requests only.
As stated in the original review, there needs to be a check for "!be_cli->bectx->bet_info[BET_SUBDOMAINS].bet_ops != NULL" before calling be_file_request, otherwise you'll get crashes on domains with no subdomain support.
That's about halfway into the patchset. I'll continue the review later.
On Mon, Apr 02, 2012 at 02:54:08PM -0400, Jakub Hrozek wrote:
That's about halfway into the patchset. I'll continue the review later.
Patch 0010: IPA: Add get-domains target Please use the new DEBUG level macros.
You should use ipa_parse_search_base(), not sdap_parse_search_base() in the IPA code.
ipa_subdomains_search_base is not documented and not present in the configAPI.
ipa_subdomains_parse_results(): please use goto here instead of return + if (new_domain_list[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n")); + return ENOMEM; + }
The original review suggested to add a ipa_subdomain_map. If you don't want to go that route (I would prefer the map, too, there is a map for every option family in SSSD), please at least #define the attributes like ipaNTTrustedDomain and ipaNTFlatName to ease changing them later if needed. Hardcoding attribute names is bad.
Patch 0011: Add domain name to get_account_info request Ack
Patch 0012: New config options for subdomains See my reply in the other part of this thread. If you are goig forward with these options, add them to the configAPI.
Patch 0013: Moved expand_homedir_template() from NSS responder to utility code Can you change the DEBUG macros to the new format?
Patch 0014: Add s2n extended operation Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of the debug messages says "go [%s]", should be "got [%s]". Otherwise looks fine.
Patch 0015: Add ID operations in subdomains Ack
Patch 0016: Send PAM requests for subdomains to the right provider Ack
Patch 0017: Basic support for subdomains in auth provider Ack
Patch 0018: Carry sysdb context and domain info in be_req structure Ack
Patch 0019: Accept be_req instead if be_ctx in LDAP access provider Ack
Patch 0020: Detect subdomain request in IPA access provider Ack
Patch 0021: Utilize sysdb context within be_req in HBAC Ack
On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
Once more, this time rebased on top of current master.
Thanks Jan
Patch 0001: Sysdb routines for subdomains Ack, can you just add a DEBUG message in case sysdb_transaction_commit() fails?
Done
Patch 0002: Add some utility functions for subdomains Ack
Patch 0003: Add conn_name to allow different names for domains and connections Ack
Patch 0004: Responder part of the subdomain retrieval work "get_domains_timeout" is not mentioned in neither configAPI nor manpages
if (ret == EOK) {DEBUG(SSSDBG_TRACE_FUNC,("Last call was too recent, nothing to do!\n"));ret = EOK; ^^ this is redundantgoto done;} else if (ret != EAGAIN) {
Fixed
Please stick to either goto or return in get_domains_next()
Done
Two review items from the original review are not addressed yet: There is no explanation of the "hint" parameter (and without one, it's hard to say what to do with the FIXME) and there is no sss_dp_get_domains_recv()
The hint is actually described right at the point where it is given as an argument to dbus call.
sss_dp_get_domains_recv() added
Patch 0005: Modified responder_get_domain() It is a convention to put a TALLOC_CTX we allocate on as the first parameter (responder_get_domain())
I know, I mentioned this to Sumit when I was reviewing reviewing the code once but I forgot to fix it. Fixed now.
I think that it might be useful to store CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from confdb on every request. The case where confdb_get_int either errors out or the timeout is negative should at least trigger a DEBUG message.
Done and done
As a general question, why are the domain names case sensitive but subdomain names seem to be case insensitive? For example check_last_request() is doing a case-sensitive strcmp but responder_get_domain is doing a strcasecmp.
I wrote this when the same question was on my mind (you can notice the FIXME right above it). It will be safer to treat all domains and subdomains case insensitively. Is there any other place where you noticed this?
Also please split the long DEBUG() statement in responder_get_domain() into two lines, it doesn't fit on my screen :-)
Done, sorry for the inconvenience ;-)
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done. About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
Patch 0007: Ask for subdomains in responder in the first request after startup Ack, with the same comment about services and SSH as with patch #6.
Patch 0008: Check sub-domains in nss_cmd_get{pwuid|grgid}_search() Ack
Patch 0009: data provider: added subdomains If the bandaid patch I posted on the list the other day is accepted, there should be a check for the result of the sbus_get_connection() call.
Done
The comment about fast reply in be_get_subdomains() seems misleading to me, there is no check for BE_REQ_FAST and the whole code seems like it can handle online requests only.
Yes, the comment was copied from somewhere else I think. Fixed
As stated in the original review, there needs to be a check for "!be_cli->bectx->bet_info[BET_SUBDOMAINS].bet_ops != NULL" before calling be_file_request, otherwise you'll get crashes on domains with no subdomain support.
Done
Patch 0010: IPA: Add get-domains target Please use the new DEBUG level macros.
Done
You should use ipa_parse_search_base(), not sdap_parse_search_base() in the IPA code.
Done
ipa_subdomains_search_base is not documented and not present in the configAPI.
Done
ipa_subdomains_parse_results(): please use goto here instead of return
if (new_domain_list[c] == NULL) {DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n"));return ENOMEM;}
Done
The original review suggested to add a ipa_subdomain_map. If you don't want to go that route (I would prefer the map, too, there is a map for every option family in SSSD), please at least #define the attributes like ipaNTTrustedDomain and ipaNTFlatName to ease changing them later if needed. Hardcoding attribute names is bad.
Agreed. I don't want to use the map because there is really no reason for that. But defining constants is a good idea.
Patch 0011: Add domain name to get_account_info request Ack
Patch 0012: New config options for subdomains See my reply in the other part of this thread. If you are goig forward with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because override_homedir can't be set per domain. After your comment yesterday, I checked to be sure and IMO the man page is incorrect and the override_homedir only works for responder.
The second reason is that admins might want to separate homedirs for native users from homedirs for trusted users on some systems.
Patch 0013: Moved expand_homedir_template() from NSS responder to utility code Can you change the DEBUG macros to the new format?
Done
Patch 0014: Add s2n extended operation Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of the debug messages says "go [%s]", should be "got [%s]". Otherwise looks fine.
Done
Patch 0015: Add ID operations in subdomains Ack
Patch 0016: Send PAM requests for subdomains to the right provider Ack
Patch 0017: Basic support for subdomains in auth provider Ack
Patch 0018: Carry sysdb context and domain info in be_req structure Ack
Patch 0019: Accept be_req instead if be_ctx in LDAP access provider Ack
Patch 0020: Detect subdomain request in IPA access provider Ack
Patch 0021: Utilize sysdb context within be_req in HBAC Ack
One more thing, I removed the conn_name from patch #2. I intend to keep new_subdomain() in patch #1 - these two are closely bound together and there is a circular dependency between them IIRC. Other than this I believe all patches are compilable.
Thanks Jan
On Thu, Apr 05, 2012 at 10:46:55AM +0200, Jan Zelený wrote:
On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
Once more, this time rebased on top of current master.
Thanks Jan
Patch 0001: Sysdb routines for subdomains Ack, can you just add a DEBUG message in case sysdb_transaction_commit() fails?
Done
Patch 0002: Add some utility functions for subdomains Ack
Patch 0003: Add conn_name to allow different names for domains and connections Ack
Patch 0004: Responder part of the subdomain retrieval work "get_domains_timeout" is not mentioned in neither configAPI nor manpages
if (ret == EOK) {DEBUG(SSSDBG_TRACE_FUNC,("Last call was too recent, nothing to do!\n"));ret = EOK; ^^ this is redundantgoto done;} else if (ret != EAGAIN) {Fixed
Please stick to either goto or return in get_domains_next()
Done
Two review items from the original review are not addressed yet: There is no explanation of the "hint" parameter (and without one, it's hard to say what to do with the FIXME) and there is no sss_dp_get_domains_recv()
The hint is actually described right at the point where it is given as an argument to dbus call.
sss_dp_get_domains_recv() added
Patch 0005: Modified responder_get_domain() It is a convention to put a TALLOC_CTX we allocate on as the first parameter (responder_get_domain())
I know, I mentioned this to Sumit when I was reviewing reviewing the code once but I forgot to fix it. Fixed now.
I think that it might be useful to store CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from confdb on every request. The case where confdb_get_int either errors out or the timeout is negative should at least trigger a DEBUG message.
Done and done
As a general question, why are the domain names case sensitive but subdomain names seem to be case insensitive? For example check_last_request() is doing a case-sensitive strcmp but responder_get_domain is doing a strcasecmp.
I wrote this when the same question was on my mind (you can notice the FIXME right above it). It will be safer to treat all domains and subdomains case insensitively. Is there any other place where you noticed this?
The domain names are defined in sssd.conf and only used as defined. With sub-domains it might be more difficult, because it is not always clear if e.g. the DNS domain name or the realm name is used. Typically both will be the same except for the case.
Also please split the long DEBUG() statement in responder_get_domain() into two lines, it doesn't fit on my screen :-)
Done, sorry for the inconvenience ;-)
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done. About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
ah, sorry, I think I haven't read patch 0006 to the end. We do not need netgroups for sub-domains. Only the user object is need for sub-domains (and a way to authenticate the user :-). All other things like access control and group memberships will be controlled by the local domain.
bye, Sumit
On Thu, Apr 05, 2012 at 10:46:55AM +0200, Jan Zelený wrote:
On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
Once more, this time rebased on top of current master.
Thanks Jan
Patch 0001: Sysdb routines for subdomains Ack, can you just add a DEBUG message in case sysdb_transaction_commit() fails?
Done
Patch 0002: Add some utility functions for subdomains Ack
Patch 0003: Add conn_name to allow different names for domains and connections Ack
Patch 0004: Responder part of the subdomain retrieval work "get_domains_timeout" is not mentioned in neither configAPI nor manpages
if (ret == EOK) {DEBUG(SSSDBG_TRACE_FUNC,("Last call was too recent, nothing to do!\n"));ret = EOK; ^^ this is redundantgoto done;} else if (ret != EAGAIN) {Fixed
Please stick to either goto or return in get_domains_next()
Done
Two review items from the original review are not addressed yet: There is no explanation of the "hint" parameter (and without one, it's hard to say what to do with the FIXME) and there is no sss_dp_get_domains_recv()
The hint is actually described right at the point where it is given as an argument to dbus call.
sss_dp_get_domains_recv() added
Patch 0005: Modified responder_get_domain() It is a convention to put a TALLOC_CTX we allocate on as the first parameter (responder_get_domain())
I know, I mentioned this to Sumit when I was reviewing reviewing the code once but I forgot to fix it. Fixed now.
I think that it might be useful to store CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from confdb on every request. The case where confdb_get_int either errors out or the timeout is negative should at least trigger a DEBUG message.
Done and done
As a general question, why are the domain names case sensitive but subdomain names seem to be case insensitive? For example check_last_request() is doing a case-sensitive strcmp but responder_get_domain is doing a strcasecmp.
I wrote this when the same question was on my mind (you can notice the FIXME right above it). It will be safer to treat all domains and subdomains case insensitively. Is there any other place where you noticed this?
The domain names are defined in sssd.conf and only used as defined. With sub-domains it might be more difficult, because it is not always clear if e.g. the DNS domain name or the realm name is used. Typically both will be the same except for the case.
Ok, that means case-insensititve comparison is the right approach.
Also please split the long DEBUG() statement in responder_get_domain() into two lines, it doesn't fit on my screen :-)
Done, sorry for the inconvenience ;-)
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done. About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
ah, sorry, I think I haven't read patch 0006 to the end. We do not need netgroups for sub-domains. Only the user object is need for sub-domains (and a way to authenticate the user :-). All other things like access control and group memberships will be controlled by the local domain.
Does that mean that I should remove all those parts where we are requesting various entities from subdomain?
Thanks Jan
On Fri, Apr 06, 2012 at 09:38:55AM +0200, Jan Zelený wrote:
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done. About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
ah, sorry, I think I haven't read patch 0006 to the end. We do not need netgroups for sub-domains. Only the user object is need for sub-domains (and a way to authenticate the user :-). All other things like access control and group memberships will be controlled by the local domain.
Does that mean that I should remove all those parts where we are requesting various entities from subdomain?
yes, currently I can see no usage for netgroups here, but please keep the changes in your subdomains-future branch. Maybe someone will find a usecase later.
bye, Sumit
Thanks Jan
On Thu, Apr 05, 2012 at 10:46:55AM +0200, Jan Zelený wrote:
On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
Once more, this time rebased on top of current master.
Thanks Jan
Patch 0001: Sysdb routines for subdomains Ack, can you just add a DEBUG message in case sysdb_transaction_commit() fails?
Done
Ack
Patch 0002: Add some utility functions for subdomains Ack
Ack
Patch 0003: Add conn_name to allow different names for domains and connections Ack
Ack
Patch 0004: Responder part of the subdomain retrieval work "get_domains_timeout" is not mentioned in neither configAPI nor manpages
if (ret == EOK) {DEBUG(SSSDBG_TRACE_FUNC,("Last call was too recent, nothing to do!\n"));ret = EOK; ^^ this is redundantgoto done;} else if (ret != EAGAIN) {Fixed
Please stick to either goto or return in get_domains_next()
Done
Two review items from the original review are not addressed yet: There is no explanation of the "hint" parameter (and without one, it's hard to say what to do with the FIXME) and there is no sss_dp_get_domains_recv()
The hint is actually described right at the point where it is given as an argument to dbus call.
sss_dp_get_domains_recv() added
Added but not used anywhere (and not exported in any header). The point is that the requests that call sss_dp_get_domains_send also need to call sss_dp_get_domains_recv in their callback to be notified of potential failures.
Patch 0005: Modified responder_get_domain() It is a convention to put a TALLOC_CTX we allocate on as the first parameter (responder_get_domain())
I know, I mentioned this to Sumit when I was reviewing reviewing the code once but I forgot to fix it. Fixed now.
I think that it might be useful to store CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from confdb on every request. The case where confdb_get_int either errors out or the timeout is negative should at least trigger a DEBUG message.
Done and done
As a general question, why are the domain names case sensitive but subdomain names seem to be case insensitive? For example check_last_request() is doing a case-sensitive strcmp but responder_get_domain is doing a strcasecmp.
I wrote this when the same question was on my mind (you can notice the FIXME right above it). It will be safer to treat all domains and subdomains case insensitively. Is there any other place where you noticed this?
Only in unit tests where it doesn't really matter.
Also please split the long DEBUG() statement in responder_get_domain() into two lines, it doesn't fit on my screen :-)
Done, sorry for the inconvenience ;-)
OK, ack now
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done.
No, sss_dp_get_domains_recv() is not used anywhere.
About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
Replied elsewhere.
Patch 0007: Ask for subdomains in responder in the first request after startup Ack, with the same comment about services and SSH as with patch #6.
Patch 0008: Check sub-domains in nss_cmd_get{pwuid|grgid}_search() Ack
Patch 0009: data provider: added subdomains If the bandaid patch I posted on the list the other day is accepted, there should be a check for the result of the sbus_get_connection() call.
Done
The comment about fast reply in be_get_subdomains() seems misleading to me, there is no check for BE_REQ_FAST and the whole code seems like it can handle online requests only.
Yes, the comment was copied from somewhere else I think. Fixed
As stated in the original review, there needs to be a check for "!be_cli->bectx->bet_info[BET_SUBDOMAINS].bet_ops != NULL" before calling be_file_request, otherwise you'll get crashes on domains with no subdomain support.
Done
Ack
Patch 0010: IPA: Add get-domains target Please use the new DEBUG level macros.
Done
You should use ipa_parse_search_base(), not sdap_parse_search_base() in the IPA code.
Done
ipa_subdomains_search_base is not documented and not present in the configAPI.
Done
ipa_subdomains_parse_results(): please use goto here instead of return
if (new_domain_list[c] == NULL) {DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n"));return ENOMEM;}Done
The original review suggested to add a ipa_subdomain_map. If you don't want to go that route (I would prefer the map, too, there is a map for every option family in SSSD), please at least #define the attributes like ipaNTTrustedDomain and ipaNTFlatName to ease changing them later if needed. Hardcoding attribute names is bad.
Agreed. I don't want to use the map because there is really no reason for that. But defining constants is a good idea.
Ack
Patch 0011: Add domain name to get_account_info request Ack
Patch 0012: New config options for subdomains See my reply in the other part of this thread. If you are goig forward with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because override_homedir can't be set per domain. After your comment yesterday, I checked to be sure and IMO the man page is incorrect and the override_homedir only works for responder.
What was your testcase and what exactly wasn't working for you?
The option works fine for me in a subdomain and the code seems to be quite clear (get_homedir_override())
The second reason is that admins might want to separate homedirs for native users from homedirs for trusted users on some systems.
Then let's extend the homedir template by a subdomain name or "full domain name" option.
Patch 0013: Moved expand_homedir_template() from NSS responder to utility code Can you change the DEBUG macros to the new format?
Done
Ack
Patch 0014: Add s2n extended operation Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of the debug messages says "go [%s]", should be "got [%s]". Otherwise looks fine.
Done
GCC emits these warnings when compiling with -O2: src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done': src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared here src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared here src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_acct_info_send': src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared here
Patch 0015: Add ID operations in subdomains Ack
Patch 0016: Send PAM requests for subdomains to the right provider Ack
Patch 0017: Basic support for subdomains in auth provider Ack
Patch 0018: Carry sysdb context and domain info in be_req structure Ack
Patch 0019: Accept be_req instead if be_ctx in LDAP access provider Ack
Patch 0020: Detect subdomain request in IPA access provider Ack
Patch 0021: Utilize sysdb context within be_req in HBAC Ack
One more thing, I removed the conn_name from patch #2. I intend to keep new_subdomain() in patch #1 - these two are closely bound together and there is a circular dependency between them IIRC. Other than this I believe all patches are compilable.
They are compilable on their own which is great.
On Thu, Apr 05, 2012 at 10:46:55AM +0200, Jan Zelený wrote:
On Mon, Apr 02, 2012 at 10:23:37AM +0200, Jan Zeleny wrote:
Once more, this time rebased on top of current master.
Thanks Jan
Patch 0001: Sysdb routines for subdomains Ack, can you just add a DEBUG message in case sysdb_transaction_commit() fails?
Done
Ack
Patch 0002: Add some utility functions for subdomains Ack
Ack
Patch 0003: Add conn_name to allow different names for domains and connections Ack
Ack
Patch 0004: Responder part of the subdomain retrieval work "get_domains_timeout" is not mentioned in neither configAPI nor manpages
if (ret == EOK) {DEBUG(SSSDBG_TRACE_FUNC,("Last call was too recent, nothing to do!\n"));ret = EOK; ^^ this is redundantgoto done;} else if (ret != EAGAIN) {Fixed
Please stick to either goto or return in get_domains_next()
Done
Two review items from the original review are not addressed yet: There is no explanation of the "hint" parameter (and without one, it's hard to say what to do with the FIXME) and there is no sss_dp_get_domains_recv()
The hint is actually described right at the point where it is given as an argument to dbus call.
sss_dp_get_domains_recv() added
Added but not used anywhere (and not exported in any header). The point is that the requests that call sss_dp_get_domains_send also need to call sss_dp_get_domains_recv in their callback to be notified of potential failures.
I could have sworn I added that as well. Nevertheless, now it's really there.
Patch 0005: Modified responder_get_domain() It is a convention to put a TALLOC_CTX we allocate on as the first parameter (responder_get_domain())
I know, I mentioned this to Sumit when I was reviewing reviewing the code once but I forgot to fix it. Fixed now.
I think that it might be useful to store CONFDB_RESPONDER_GET_DOMAINS_TIMEOUT in rctx rather than load it from confdb on every request. The case where confdb_get_int either errors out or the timeout is negative should at least trigger a DEBUG message.
Done and done
As a general question, why are the domain names case sensitive but subdomain names seem to be case insensitive? For example check_last_request() is doing a case-sensitive strcmp but responder_get_domain is doing a strcasecmp.
I wrote this when the same question was on my mind (you can notice the FIXME right above it). It will be safer to treat all domains and subdomains case insensitively. Is there any other place where you noticed this?
Only in unit tests where it doesn't really matter.
Also please split the long DEBUG() statement in responder_get_domain() into two lines, it doesn't fit on my screen :-)
Done, sorry for the inconvenience ;-)
OK, ack now
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done.
No, sss_dp_get_domains_recv() is not used anywhere.
Fixed
About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
Replied elsewhere.
Fixed
Patch 0007: Ask for subdomains in responder in the first request after startup Ack, with the same comment about services and SSH as with patch #6.
Patch 0008: Check sub-domains in nss_cmd_get{pwuid|grgid}_search() Ack
Patch 0009: data provider: added subdomains If the bandaid patch I posted on the list the other day is accepted, there should be a check for the result of the sbus_get_connection() call.
Done
The comment about fast reply in be_get_subdomains() seems misleading to me, there is no check for BE_REQ_FAST and the whole code seems like it can handle online requests only.
Yes, the comment was copied from somewhere else I think. Fixed
As stated in the original review, there needs to be a check for "!be_cli->bectx->bet_info[BET_SUBDOMAINS].bet_ops != NULL" before calling be_file_request, otherwise you'll get crashes on domains with no subdomain support.
Done
Ack
Patch 0010: IPA: Add get-domains target Please use the new DEBUG level macros.
Done
You should use ipa_parse_search_base(), not sdap_parse_search_base() in the IPA code.
Done
ipa_subdomains_search_base is not documented and not present in the configAPI.
Done
ipa_subdomains_parse_results(): please use goto here instead of return
if (new_domain_list[c] == NULL) {DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n"));return ENOMEM;}Done
The original review suggested to add a ipa_subdomain_map. If you don't want to go that route (I would prefer the map, too, there is a map for every option family in SSSD), please at least #define the attributes like ipaNTTrustedDomain and ipaNTFlatName to ease changing them later if needed. Hardcoding attribute names is bad.
Agreed. I don't want to use the map because there is really no reason for that. But defining constants is a good idea.
Ack
Patch 0011: Add domain name to get_account_info request Ack
Patch 0012: New config options for subdomains See my reply in the other part of this thread. If you are goig forward with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because override_homedir can't be set per domain. After your comment yesterday, I checked to be sure and IMO the man page is incorrect and the override_homedir only works for responder.
What was your testcase and what exactly wasn't working for you?
The option works fine for me in a subdomain and the code seems to be quite clear (get_homedir_override())
I know it is straightforward, but it didn't work for me. However when looking a bit deeper I discovered a bug in completely different part of the patch set. This is now fixed and the override works as expected.
The second reason is that admins might want to separate homedirs for native users from homedirs for trusted users on some systems.
Then let's extend the homedir template by a subdomain name or "full domain name" option.
I'm not sure about this. For one thing this functionality is already there. But I was considering scenario when the path to subdomain homedirs is completely different. Something like this:
/home/ipa/%u - the main IPA domain /home/ad/%u - AD subdomains
I there is a consensus that it is not necessary, I can remove it. But frankly I can see us having exactly this RFE several months later.
Patch 0013: Moved expand_homedir_template() from NSS responder to utility code Can you change the DEBUG macros to the new format?
Done
Ack
Patch 0014: Add s2n extended operation Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of the debug messages says "go [%s]", should be "got [%s]". Otherwise looks fine.
Done
GCC emits these warnings when compiling with -O2: src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done': src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared here src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared here src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_acct_info_send': src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared here
All fixed
Patch 0015: Add ID operations in subdomains Ack
Patch 0016: Send PAM requests for subdomains to the right provider Ack
Patch 0017: Basic support for subdomains in auth provider Ack
Patch 0018: Carry sysdb context and domain info in be_req structure Ack
Patch 0019: Accept be_req instead if be_ctx in LDAP access provider Ack
Patch 0020: Detect subdomain request in IPA access provider Ack
Patch 0021: Utilize sysdb context within be_req in HBAC Ack
One more thing, I removed the conn_name from patch #2. I intend to keep new_subdomain() in patch #1 - these two are closely bound together and there is a circular dependency between them IIRC. Other than this I believe all patches are compilable.
They are compilable on their own which is great.
Thanks Jan
The mails grow huge. I'm going to trim the response a little and omit patches that were already acked elsewhere.
Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack Patch 0005: Ack
On Fri, Apr 06, 2012 at 01:52:36PM +0200, Jan Zelený wrote:
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done.
No, sss_dp_get_domains_recv() is not used anywhere.
Fixed
About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
Replied elsewhere.
Fixed
Please also remove "subreq" from setnetgrent_send() and the declaration of setnetgrent_cb().
Patch 0007: Ack Patch 0008: Ack Patch 0009: Ack Patch 0010: Ack Patch 0011: Ack
Patch 0012: New config options for subdomains See my reply in the other part of this thread. If you are goig forward with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because override_homedir can't be set per domain. After your comment yesterday, I checked to be sure and IMO the man page is incorrect and the override_homedir only works for responder.
What was your testcase and what exactly wasn't working for you?
The option works fine for me in a subdomain and the code seems to be quite clear (get_homedir_override())
I know it is straightforward, but it didn't work for me.
Can you file a bug if the override is not working in a domain? It should and if something is not clear, then it's most probably a docs bug..
However when looking a bit deeper I discovered a bug in completely different part of the patch set. This is now fixed and the override works as expected.
The second reason is that admins might want to separate homedirs for native users from homedirs for trusted users on some systems.
Then let's extend the homedir template by a subdomain name or "full domain name" option.
I'm not sure about this. For one thing this functionality is already there. But I was considering scenario when the path to subdomain homedirs is completely different. Something like this:
/home/ipa/%u - the main IPA domain /home/ad/%u - AD subdomains
I there is a consensus that it is not necessary, I can remove it. But frankly I can see us having exactly this RFE several months later.
If the AD users are to be treated differently than native IPA users the new option is OK.
Patch 0013: Ack
Patch 0014: Add s2n extended operation Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of the debug messages says "go [%s]", should be "got [%s]". Otherwise looks fine.
Done
GCC emits these warnings when compiling with -O2: src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done': src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared here src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared here src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_acct_info_send': src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared here
All fixed
Ack
Patch 0015: Ack Patch 0016: Ack Patch 0017: Ack Patch 0018: Ack Patch 0019: Ack Patch 0020: Ack Patch 0021: Ack
Becuse the only remaining review issue is a trivial one and I know that Stephen has been reviewing the patches as well, feel free to resend another patch set when/if Stephen sends his comments.
Great work!
On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
The mails grow huge. I'm going to trim the response a little and omit patches that were already acked elsewhere.
Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack Patch 0005: Ack
On Fri, Apr 06, 2012 at 01:52:36PM +0200, Jan Zelený wrote:
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done.
No, sss_dp_get_domains_recv() is not used anywhere.
Fixed
About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
Replied elsewhere.
Fixed
Please also remove "subreq" from setnetgrent_send() and the declaration of setnetgrent_cb().
Patch 0007: Ack Patch 0008: Ack Patch 0009: Ack Patch 0010: Ack Patch 0011: Ack
Patch 0012: New config options for subdomains See my reply in the other part of this thread. If you are goig forward with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because override_homedir can't be set per domain. After your comment yesterday, I checked to be sure and IMO the man page is incorrect and the override_homedir only works for responder.
What was your testcase and what exactly wasn't working for you?
The option works fine for me in a subdomain and the code seems to be quite clear (get_homedir_override())
I know it is straightforward, but it didn't work for me.
Can you file a bug if the override is not working in a domain? It should and if something is not clear, then it's most probably a docs bug..
However when looking a bit deeper I discovered a bug in completely different part of the patch set. This is now fixed and the override works as expected.
The second reason is that admins might want to separate homedirs for native users from homedirs for trusted users on some systems.
Then let's extend the homedir template by a subdomain name or "full domain name" option.
I'm not sure about this. For one thing this functionality is already there. But I was considering scenario when the path to subdomain homedirs is completely different. Something like this:
/home/ipa/%u - the main IPA domain /home/ad/%u - AD subdomains
I there is a consensus that it is not necessary, I can remove it. But frankly I can see us having exactly this RFE several months later.
If the AD users are to be treated differently than native IPA users the new option is OK.
Patch 0013: Ack
Patch 0014: Add s2n extended operation Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of the debug messages says "go [%s]", should be "got [%s]". Otherwise looks fine.
Done
GCC emits these warnings when compiling with -O2: src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done': src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared here src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared here src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_acct_info_send': src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared here
All fixed
Ack
Patch 0015: Ack Patch 0016: Ack Patch 0017: Ack Patch 0018: Ack Patch 0019: Ack Patch 0020: Ack Patch 0021: Ack
Becuse the only remaining review issue is a trivial one and I know that Stephen has been reviewing the patches as well, feel free to resend another patch set when/if Stephen sends his comments.
Great work!
There seems to be a startup error with non-IPA back ends:
(Tue Apr 10 17:54:11 2012) [sssd[be[ldap]]] [load_backend_module] (0x0010): Unable to load init fn sssm_ldap_subdomains_init from module ldap, error: /usr/lib64/sssd/libsss_ldap.so: undefined symbol: sssm_ldap_subdomains_init
Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
The mails grow huge. I'm going to trim the response a little and omit patches that were already acked elsewhere.
Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack Patch 0005: Ack
On Fri, Apr 06, 2012 at 01:52:36PM +0200, Jan Zelený wrote:
Patch 0006: Retrieve subdomains if there is a request for fully qualified user Looks OK, will just need amendment when sss_dp_get_domains_recv() is implemented. Currently the patch supports users and netgroups, are there plans to also support services and the SSH responder? I think we can leave out the sudo responder for now, sudo doesn't support domains (yet?). Autofs in theory can send a request for a fully-qualified map, but it doesn't so the work can be done later, although I would prefer a ticket to show that we are aware of the issue.
The amendment has been done.
No, sss_dp_get_domains_recv() is not used anywhere.
Fixed
About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
Replied elsewhere.
Fixed
Please also remove "subreq" from setnetgrent_send() and the declaration of setnetgrent_cb().
Patch 0007: Ack Patch 0008: Ack Patch 0009: Ack Patch 0010: Ack Patch 0011: Ack
Patch 0012: New config options for subdomains See my reply in the other part of this thread. If you are goig forward with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because override_homedir can't be set per domain. After your comment yesterday, I checked to be sure and IMO the man page is incorrect and the override_homedir only works for responder.
What was your testcase and what exactly wasn't working for you?
The option works fine for me in a subdomain and the code seems to be quite clear (get_homedir_override())
I know it is straightforward, but it didn't work for me.
Can you file a bug if the override is not working in a domain? It should and if something is not clear, then it's most probably a docs bug..
However when looking a bit deeper I discovered a bug in completely different part of the patch set. This is now fixed and the override works as expected.
The second reason is that admins might want to separate homedirs for native users from homedirs for trusted users on some systems.
Then let's extend the homedir template by a subdomain name or "full domain name" option.
I'm not sure about this. For one thing this functionality is already there. But I was considering scenario when the path to subdomain homedirs is completely different. Something like this:
/home/ipa/%u - the main IPA domain /home/ad/%u - AD subdomains
I there is a consensus that it is not necessary, I can remove it. But frankly I can see us having exactly this RFE several months later.
If the AD users are to be treated differently than native IPA users the new option is OK.
Patch 0013: Ack
Patch 0014: Add s2n extended operation Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one of the debug messages says "go [%s]", should be "got [%s]". Otherwise looks fine.
Done
GCC emits these warnings when compiling with -O2: src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done': src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared here src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared here src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_acct_info_send': src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared here
All fixed
Ack
Patch 0015: Ack Patch 0016: Ack Patch 0017: Ack Patch 0018: Ack Patch 0019: Ack Patch 0020: Ack Patch 0021: Ack
Becuse the only remaining review issue is a trivial one and I know that Stephen has been reviewing the patches as well, feel free to resend another patch set when/if Stephen sends his comments.
Great work!
There seems to be a startup error with non-IPA back ends:
(Tue Apr 10 17:54:11 2012) [sssd[be[ldap]]] [load_backend_module] (0x0010): Unable to load init fn sssm_ldap_subdomains_init from module ldap, error: /usr/lib64/sssd/libsss_ldap.so: undefined symbol: sssm_ldap_subdomains_init
Here is the patch set with both things fixed. In the end I had to add the subdomains_provider option, it was the only solution for the linking issue.
Thanks Jan
On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
The mails grow huge. I'm going to trim the response a little and omit patches that were already acked elsewhere.
Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack Patch 0005: Ack
On Fri, Apr 06, 2012 at 01:52:36PM +0200, Jan Zelený wrote:
> Patch 0006: Retrieve subdomains if there is a request for fully > qualified user > Looks OK, will just need amendment when sss_dp_get_domains_recv() > is implemented. Currently the patch supports users and > netgroups, are there plans to also support services and the SSH > responder? I think we can leave out the sudo responder for now, > sudo doesn't support domains (yet?). Autofs in theory can send a > request for a fully-qualified map, but it doesn't so the work > can be done later, although I would prefer a ticket to show that > we are aware of the issue.
The amendment has been done.
No, sss_dp_get_domains_recv() is not used anywhere.
Fixed
About support of another responders - I honestly don't know. I wasn't aware that a request for fully qualified service/ssh entity can be given to SSSD. Perhaps Sumit will know more.
Replied elsewhere.
Fixed
Please also remove "subreq" from setnetgrent_send() and the declaration of setnetgrent_cb().
Patch 0007: Ack Patch 0008: Ack Patch 0009: Ack Patch 0010: Ack Patch 0011: Ack
> Patch 0012: New config options for subdomains > See my reply in the other part of this thread. If you are goig > forward with these options, add them to the configAPI.
Based on previous discussion I removed the part which is setting default shell. I intend to keep the homedir for two reasons:
I recalled why I created the config option in the first place. It's because override_homedir can't be set per domain. After your comment yesterday, I checked to be sure and IMO the man page is incorrect and the override_homedir only works for responder.
What was your testcase and what exactly wasn't working for you?
The option works fine for me in a subdomain and the code seems to be quite clear (get_homedir_override())
I know it is straightforward, but it didn't work for me.
Can you file a bug if the override is not working in a domain? It should and if something is not clear, then it's most probably a docs bug..
However when looking a bit deeper I discovered a bug in completely different part of the patch set. This is now fixed and the override works as expected.
The second reason is that admins might want to separate homedirs for native users from homedirs for trusted users on some systems.
Then let's extend the homedir template by a subdomain name or "full domain name" option.
I'm not sure about this. For one thing this functionality is already there. But I was considering scenario when the path to subdomain homedirs is completely different. Something like this:
/home/ipa/%u - the main IPA domain /home/ad/%u - AD subdomains
I there is a consensus that it is not necessary, I can remove it. But frankly I can see us having exactly this RFE several months later.
If the AD users are to be treated differently than native IPA users the new option is OK.
Patch 0013: Ack
> Patch 0014: Add s2n extended operation > Old DEBUG macros again. There is a typo in s2n_response_to_attrs, > one of the debug messages says "go [%s]", should be "got [%s]". > Otherwise looks fine.
Done
GCC emits these warnings when compiling with -O2: src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done': src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared here src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared here src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_acct_info_send': src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared here
All fixed
Ack
Patch 0015: Ack Patch 0016: Ack Patch 0017: Ack Patch 0018: Ack Patch 0019: Ack Patch 0020: Ack Patch 0021: Ack
Becuse the only remaining review issue is a trivial one and I know that Stephen has been reviewing the patches as well, feel free to resend another patch set when/if Stephen sends his comments.
Great work!
There seems to be a startup error with non-IPA back ends:
(Tue Apr 10 17:54:11 2012) [sssd[be[ldap]]] [load_backend_module] (0x0010): Unable to load init fn sssm_ldap_subdomains_init from module ldap, error: /usr/lib64/sssd/libsss_ldap.so: undefined symbol: sssm_ldap_subdomains_init
Here is the patch set with both things fixed. In the end I had to add the subdomains_provider option, it was the only solution for the linking issue.
Thanks Jan
This works fine, but the new option should be added to the configAPI (sorry, I hope this would be the last iteration).
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems quite high because this DEBUG message is emited for any back end which does not support subdomains which is everything but IPA. This should be a MINOR failure at best, maybe even lower.
Also, instead of defaulting to "not set", would it be a good idea to default to "ipa" in the IPA provider and maybe check during the first request if subdomains are enabled on the server -- provided that runtime detection is possible which I honestly don't know. That way the clients wouldn't have to be reconfigured if trusts are added on the server.
On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
The mails grow huge. I'm going to trim the response a little and omit patches that were already acked elsewhere.
Patch 0001: Ack Patch 0002: Ack Patch 0003: Ack Patch 0004: Ack Patch 0005: Ack
On Fri, Apr 06, 2012 at 01:52:36PM +0200, Jan Zelený wrote:
> > Patch 0006: Retrieve subdomains if there is a request for > > fully qualified user > > Looks OK, will just need amendment when > > sss_dp_get_domains_recv() is implemented. Currently the > > patch supports users and netgroups, are there plans to also > > support services and the SSH responder? I think we can leave > > out the sudo responder for now, sudo doesn't support domains > > (yet?). Autofs in theory can send a request for a > > fully-qualified map, but it doesn't so the work can be done > > later, although I would prefer a ticket to show that we are > > aware of the issue. > > The amendment has been done.
No, sss_dp_get_domains_recv() is not used anywhere.
Fixed
> About support of another responders - I honestly > don't know. I wasn't aware that a request for fully qualified > service/ssh entity can be given to SSSD. Perhaps Sumit will > know more.
Replied elsewhere.
Fixed
Please also remove "subreq" from setnetgrent_send() and the declaration of setnetgrent_cb().
Patch 0007: Ack Patch 0008: Ack Patch 0009: Ack Patch 0010: Ack Patch 0011: Ack
> > Patch 0012: New config options for subdomains > > See my reply in the other part of this thread. If you are > > goig forward with these options, add them to the configAPI. > > Based on previous discussion I removed the part which is > setting default shell. I intend to keep the homedir for two > reasons: > > I recalled why I created the config option in the first place. > It's because override_homedir can't be set per domain. After > your comment yesterday, I checked to be sure and IMO the man > page is incorrect and the override_homedir only works for > responder.
What was your testcase and what exactly wasn't working for you?
The option works fine for me in a subdomain and the code seems to be quite clear (get_homedir_override())
I know it is straightforward, but it didn't work for me.
Can you file a bug if the override is not working in a domain? It should and if something is not clear, then it's most probably a docs bug..
However when looking a bit deeper I discovered a bug in completely different part of the patch set. This is now fixed and the override works as expected.
> The second reason is that admins might want to separate > homedirs for native users from homedirs for trusted users on > some systems.
Then let's extend the homedir template by a subdomain name or "full domain name" option.
I'm not sure about this. For one thing this functionality is already there. But I was considering scenario when the path to subdomain homedirs is completely different. Something like this:
/home/ipa/%u - the main IPA domain /home/ad/%u - AD subdomains
I there is a consensus that it is not necessary, I can remove it. But frankly I can see us having exactly this RFE several months later.
If the AD users are to be treated differently than native IPA users the new option is OK.
Patch 0013: Ack
> > Patch 0014: Add s2n extended operation > > Old DEBUG macros again. There is a typo in > > s2n_response_to_attrs, one of the debug messages says "go > > [%s]", should be "got [%s]". Otherwise looks fine. > > Done
GCC emits these warnings when compiling with -O2: src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done': src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared here src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared here src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_acct_info_send': src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used uninitialized in this function [-Wuninitialized] src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared here
All fixed
Ack
Patch 0015: Ack Patch 0016: Ack Patch 0017: Ack Patch 0018: Ack Patch 0019: Ack Patch 0020: Ack Patch 0021: Ack
Becuse the only remaining review issue is a trivial one and I know that Stephen has been reviewing the patches as well, feel free to resend another patch set when/if Stephen sends his comments.
Great work!
There seems to be a startup error with non-IPA back ends:
(Tue Apr 10 17:54:11 2012) [sssd[be[ldap]]] [load_backend_module] (0x0010): Unable to load init fn sssm_ldap_subdomains_init from module ldap, error: /usr/lib64/sssd/libsss_ldap.so: undefined symbol: sssm_ldap_subdomains_init
Here is the patch set with both things fixed. In the end I had to add the subdomains_provider option, it was the only solution for the linking issue.
Thanks Jan
This works fine, but the new option should be added to the configAPI (sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my will to re-check everything.
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems quite high because this DEBUG message is emited for any back end which does not support subdomains which is everything but IPA. This should be a MINOR failure at best, maybe even lower.
I will change this in separate patch
Also, instead of defaulting to "not set", would it be a good idea to default to "ipa" in the IPA provider and maybe check during the first request if subdomains are enabled on the server -- provided that runtime detection is possible which I honestly don't know. That way the clients wouldn't have to be reconfigured if trusts are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD communicates with IPA. I will discuss it with him once he is back from vacation. I'll send the patch set then.
Jan
On Fri, Apr 13, 2012 at 08:24:18AM +0200, Jan Zelený wrote:
On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
This works fine, but the new option should be added to the configAPI (sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my will to re-check everything.
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems quite high because this DEBUG message is emited for any back end which does not support subdomains which is everything but IPA. This should be a MINOR failure at best, maybe even lower.
I will change this in separate patch
Also, instead of defaulting to "not set", would it be a good idea to default to "ipa" in the IPA provider and maybe check during the first request if subdomains are enabled on the server -- provided that runtime detection is possible which I honestly don't know. That way the clients wouldn't have to be reconfigured if trusts are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD communicates with IPA. I will discuss it with him once he is back from vacation. I'll send the patch set then.
yes, please activate subdomains for the IPA provider by default, the detection if subdomains are available is always done at runtime.
bye, Sumit
Jan
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Apr 13, 2012 at 08:24:18AM +0200, Jan Zelený wrote:
On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
This works fine, but the new option should be added to the configAPI (sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my will to re-check everything.
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems quite high because this DEBUG message is emited for any back end which does not support subdomains which is everything but IPA. This should be a MINOR failure at best, maybe even lower.
I will change this in separate patch
Also, instead of defaulting to "not set", would it be a good idea to default to "ipa" in the IPA provider and maybe check during the first request if subdomains are enabled on the server -- provided that runtime detection is possible which I honestly don't know. That way the clients wouldn't have to be reconfigured if trusts are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD communicates with IPA. I will discuss it with him once he is back from vacation. I'll send the patch set then.
yes, please activate subdomains for the IPA provider by default, the detection if subdomains are available is always done at runtime.
After consultation with Jakub, we agreed to move this task to separate ticket.
https://fedorahosted.org/sssd/ticket/1308
I suppose that the change of debug level stated above won't be necessary after this change.
I'm sending the (hopefully) last iteration of patches in attachment.
Thanks Jan
On Wed, Apr 18, 2012 at 03:22:03PM +0200, Jan Zelený wrote:
On Fri, Apr 13, 2012 at 08:24:18AM +0200, Jan Zelený wrote:
On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
Jakub Hrozek jhrozek@redhat.com wrote:
On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
This works fine, but the new option should be added to the configAPI (sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my will to re-check everything.
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems quite high because this DEBUG message is emited for any back end which does not support subdomains which is everything but IPA. This should be a MINOR failure at best, maybe even lower.
I will change this in separate patch
Also, instead of defaulting to "not set", would it be a good idea to default to "ipa" in the IPA provider and maybe check during the first request if subdomains are enabled on the server -- provided that runtime detection is possible which I honestly don't know. That way the clients wouldn't have to be reconfigured if trusts are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD communicates with IPA. I will discuss it with him once he is back from vacation. I'll send the patch set then.
yes, please activate subdomains for the IPA provider by default, the detection if subdomains are available is always done at runtime.
After consultation with Jakub, we agreed to move this task to separate ticket.
https://fedorahosted.org/sssd/ticket/1308
I suppose that the change of debug level stated above won't be necessary after this change.
I'm sending the (hopefully) last iteration of patches in attachment.
Thanks Jan
There were two merge problems in patches 02 and 09.
====================================================================== FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain) ---------------------------------------------------------------------- Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 518, in testListOptions option) AssertionError: Option [subdomains_provider] unexpectedly found
On Wed, Apr 18, 2012 at 03:22:03PM +0200, Jan Zelený wrote:
On Fri, Apr 13, 2012 at 08:24:18AM +0200, Jan Zelený wrote:
On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
Jakub Hrozek jhrozek@redhat.com wrote: > On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote:
This works fine, but the new option should be added to the configAPI (sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my will to re-check everything.
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems quite high because this DEBUG message is emited for any back end which does not support subdomains which is everything but IPA. This should be a MINOR failure at best, maybe even lower.
I will change this in separate patch
Also, instead of defaulting to "not set", would it be a good idea to default to "ipa" in the IPA provider and maybe check during the first request if subdomains are enabled on the server -- provided that runtime detection is possible which I honestly don't know. That way the clients wouldn't have to be reconfigured if trusts are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD communicates with IPA. I will discuss it with him once he is back from vacation. I'll send the patch set then.
yes, please activate subdomains for the IPA provider by default, the detection if subdomains are available is always done at runtime.
After consultation with Jakub, we agreed to move this task to separate ticket.
https://fedorahosted.org/sssd/ticket/1308
I suppose that the change of debug level stated above won't be necessary after this change.
I'm sending the (hopefully) last iteration of patches in attachment.
Thanks Jan
There were two merge problems in patches 02 and 09.
======================================================================
FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain)
Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 518, in testListOptions option) AssertionError: Option [subdomains_provider] unexpectedly found
I encountered only one, the other one was resolved automatically. Attached is the patch set based on top of current master.
Thanks Jan
On Tue, 2012-04-24 at 13:23 +0200, Jan Zelený wrote:
On Wed, Apr 18, 2012 at 03:22:03PM +0200, Jan Zelený wrote:
On Fri, Apr 13, 2012 at 08:24:18AM +0200, Jan Zelený wrote:
On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote: > Jakub Hrozek jhrozek@redhat.com wrote: > > On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote: This works fine, but the new option should be added to the configAPI (sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my will to re-check everything.
The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems quite high because this DEBUG message is emited for any back end which does not support subdomains which is everything but IPA. This should be a MINOR failure at best, maybe even lower.
I will change this in separate patch
Also, instead of defaulting to "not set", would it be a good idea to default to "ipa" in the IPA provider and maybe check during the first request if subdomains are enabled on the server -- provided that runtime detection is possible which I honestly don't know. That way the clients wouldn't have to be reconfigured if trusts are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD communicates with IPA. I will discuss it with him once he is back from vacation. I'll send the patch set then.
yes, please activate subdomains for the IPA provider by default, the detection if subdomains are available is always done at runtime.
After consultation with Jakub, we agreed to move this task to separate ticket.
https://fedorahosted.org/sssd/ticket/1308
I suppose that the change of debug level stated above won't be necessary after this change.
I'm sending the (hopefully) last iteration of patches in attachment.
Thanks Jan
There were two merge problems in patches 02 and 09.
======================================================================
FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain)
Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 518, in testListOptions option) AssertionError: Option [subdomains_provider] unexpectedly found
I encountered only one, the other one was resolved automatically. Attached is the patch set based on top of current master.
Thanks Jan
Nack:
make check-TESTS make[3]: Entering directory `/dev/shm/build/rpmbuild/BUILD/sssd-1.8.90' ....... ---------------------------------------------------------------------- Ran 7 tests in 0.034s
OK .....F...F... ====================================================================== FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain) ---------------------------------------------------------------------- Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 518, in testListOptions option) AssertionError: Option [subdomains_provider] unexpectedly found
====================================================================== FAIL: testRemoveProvider (__main__.SSSDConfigTestSSSDDomain) ---------------------------------------------------------------------- Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 845, in testRemoveProvider option) AssertionError: Option [subdomains_provider] unexpectedly found
---------------------------------------------------------------------- Ran 13 tests in 0.068s
FAILED (failures=2)
On Tue, 2012-04-24 at 08:51 -0400, Stephen Gallagher wrote:
On Tue, 2012-04-24 at 13:23 +0200, Jan Zelený wrote:
On Wed, Apr 18, 2012 at 03:22:03PM +0200, Jan Zelený wrote:
On Fri, Apr 13, 2012 at 08:24:18AM +0200, Jan Zelený wrote:
> On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote: > > Jakub Hrozek jhrozek@redhat.com wrote: > > > On Tue, Apr 10, 2012 at 12:38:31AM -0400, Jakub Hrozek wrote: > This works fine, but the new option should be added to the > configAPI (sorry, I hope this would be the last iteration).
My mistake, I knew I'm missing something but late hour was stronger than my will to re-check everything.
> The DEBUG message level is set to SSSDBG_CRIT_FAILURE, which seems > quite high because this DEBUG message is emited for any back end > which does not support subdomains which is everything but IPA. > This should be a MINOR failure at best, maybe even lower.
I will change this in separate patch
> Also, instead of defaulting to "not set", would it be a good idea > to default to "ipa" in the IPA provider and maybe check during the > first request if subdomains are enabled on the server -- provided > that runtime detection is possible which I honestly don't know. > That way the clients wouldn't have to be reconfigured if trusts > are added on the server.
I'm not completely sure about this since Sumit did the part where SSSD communicates with IPA. I will discuss it with him once he is back from vacation. I'll send the patch set then.
yes, please activate subdomains for the IPA provider by default, the detection if subdomains are available is always done at runtime.
After consultation with Jakub, we agreed to move this task to separate ticket.
https://fedorahosted.org/sssd/ticket/1308
I suppose that the change of debug level stated above won't be necessary after this change.
I'm sending the (hopefully) last iteration of patches in attachment.
Thanks Jan
There were two merge problems in patches 02 and 09.
======================================================================
FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain)
Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 518, in testListOptions option) AssertionError: Option [subdomains_provider] unexpectedly found
I encountered only one, the other one was resolved automatically. Attached is the patch set based on top of current master.
Thanks Jan
Nack:
make check-TESTS make[3]: Entering directory `/dev/shm/build/rpmbuild/BUILD/sssd-1.8.90' .......
Ran 7 tests in 0.034s
OK .....F...F... ====================================================================== FAIL: testListOptions (__main__.SSSDConfigTestSSSDDomain)
Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 518, in testListOptions option) AssertionError: Option [subdomains_provider] unexpectedly found
====================================================================== FAIL: testRemoveProvider (__main__.SSSDConfigTestSSSDDomain)
Traceback (most recent call last): File "././src/config/SSSDConfigTest.py", line 845, in testRemoveProvider option) AssertionError: Option [subdomains_provider] unexpectedly found
Ran 13 tests in 0.068s
FAILED (failures=2)
I fixed the above SSSDConfigTest.py issue (squashed into patch 0009, attached) and pushed them all to master.
Great work Jan and Sumit!
On Mon, Apr 02, 2012 at 10:07:00AM +0200, Jan Zeleny wrote:
Ok, all those changes took me a while but I'm finally done. Please note that patch #0003 has been split into two and the part with utility functions has been moved right after sysdb routines. There is a circular dependency between the two which I don't think is worth trying to solve, neither patch would work without the other. This patch split caused the numbering to be shifted by one.
Yes, patch #1 uses new_subdomain() in tests, patch #2 uses sss_domain_info->conn_name.
In general, I think each patch should compile on its own for two reasons: 1) to ease cherry-picking 2) to allow git-bisect
No 1 is a moot point with a patch set like this because we would be cherry-picking the entire patch set anyway. However, I use git-bisect from time to time, and I'd like to avoid breaking it if possible.
This is not (yet) a review. Just replying to a few questions in the email, inline.
On Mon, 2012-04-02 at 10:07 +0200, Jan Zeleny wrote: ...
sysdb_update_subdomains() desperately needs comments. It took me ten minutes to figure out the logic there. I also question why you didn'd include sysdb_[add|remove]_subdomain(). An explanation of this lack should be in the source code, if there's a good reason.
Hm, I didn't write the routine and I could read it quite well. Nevertheless I added some comments. Those routines functions you are missing would be rather simple (almost aliases), that's why they are missing. Do you think there is a need for them?
When dealing with Active Directory directly, it may be possible that we will only discover a new domain by requesting it the first time (as opposed to acquiring a complete list ahead of time). If that's the case, it would be handy to have a routine to simply add one to the list. I agree that removal is probably unnecessary in most situations.
The creation of new_sd_list should be its own function.
I disagree. The funcion would be called on this one place anyway so there is probably only a little gain in doing that.
The gain is mostly in readability. The creation of the new_sd_list is moderately complex and slightly distracts from the main purpose of the function. It's not worth nacking on, just my preference. Keep it if you wish.
...
You need to check for whether becli->bectx->bet_info[BET_SUBDOMAINS] is NULL, or else you'll crash on domains that don't support subdomain lookups.
I'm not sure what do you mean here, how could it be null when it's not a pointer?
As Jakub noted in his review, I meant to say becli->bectx->bet_info[BET_SUBDOMAINS].bet_ops here.
One additional point about this patch. I don't like that you're forcing the BET_SUBDOMAINS to get its data from the ID provider. I'd much prefer that we add a subdomain_provider option, which defaults to being the same as the ID provider. I prefer to maintain the distinction in case we ever decide to have alternate mechanisms for determining the subdomains (in IPA or AD).
I discussed this with Sumit to be sure and we agreed that subdomains are supposed to be tightly bound to the ID provider. One of the reasons is that ID provider has to sort out all following requests so it will also needs to keep track of all subdomains.
Sure, I'm fine with that, then.
...
I'd prefer if you added an ipa_subdomain_map and used that for determining the attribute names to use in ipa_subdomains_parse_results(). Mostly for consistency.
I don't think there would be much gain, the code almost wouldn't change, only names of attributes would be different.
Again, code-readibility is a good thing. I'd like to have a simple map that we can glance at.
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
Nack.
I don't think this really makes sense at all. In most cases, users will prefer to use the value on the LDAP server. If they choose to override it, they'll do so through the existing override options (in the case of override_homedir, it already has %d available anyway.
We definitely don't need separate handling for shells. I can *kind of* see a value if you wanted to have only subdomains have a non-default location. I'm not sure I like that though. I feel like it's probably more complexity than we need.
I think you possibly missed the point. The point is that this information is NOT on the server, therefore we need a value that will fill it in. Otherwise only a blank field will be stored in sysdb and returned to the client
Ok, I think I understand now, but the manpages need to be MUCH more clear. It sounds like you're adding this option to always override subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles this internally by translating a NULL value for the shell into "the system default shell" (usually /bin/sh). This is handled by glibc and isn't our concern.
Hi,
Ok, I think I understand now, but the manpages need to be MUCH more clear. It sounds like you're adding this option to always override subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles this internally by translating a NULL value for the shell into "the system default shell" (usually /bin/sh). This is handled by glibc and isn't our concern.
as with regular domains also with subdomains you might have the situation where different domains and users have different values for shell (e.g., NULL, /bin/bash, and /bin/tcsh) which in turn will cause users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
Thanks,
On Tue, 2012-04-03 at 16:36 +0300, Marko Myllynen wrote:
Hi,
Ok, I think I understand now, but the manpages need to be MUCH more clear. It sounds like you're adding this option to always override subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles this internally by translating a NULL value for the shell into "the system default shell" (usually /bin/sh). This is handled by glibc and isn't our concern.
as with regular domains also with subdomains you might have the situation where different domains and users have different values for shell (e.g., NULL, /bin/bash, and /bin/tcsh) which in turn will cause users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
Thanks,
The reason we deferred 1087 is because we feel that if the user has a preferred shell set on the central server and the same shell is available on the client machine, then the user should be granted access to that shell on any machine they log into. We have the 'vetoed_shells' option in the event that a particular client machine wants to prevent a user from using a specific shell. Otherwise, if the administrator really wants to force users to use a particular shell, then the most expedient solution is to make that change to the central LDAP attribute, thus percolating it out to all machines trivially.
Hi,
users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
The reason we deferred 1087 is because we feel that if the user has a preferred shell set on the central server and the same shell is available on the client machine, then the user should be granted access to that shell on any machine they log into.
I agree that this is usually the case.
We have the 'vetoed_shells' option in the event that a particular client machine wants to prevent a user from using a specific shell. Otherwise, if the administrator really wants to force users to use a particular shell, then the most expedient solution is to make that change to the central LDAP attribute, thus percolating it out to all machines trivially.
I'm not convinced that this would be trivial when you have AD subdomains in play, some of those potentially without UNIX attributes enabled.
Cheers,
On Tue, 2012-04-03 at 16:54 +0300, Marko Myllynen wrote:
Hi,
users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
The reason we deferred 1087 is because we feel that if the user has a preferred shell set on the central server and the same shell is available on the client machine, then the user should be granted access to that shell on any machine they log into.
I agree that this is usually the case.
We have the 'vetoed_shells' option in the event that a particular client machine wants to prevent a user from using a specific shell. Otherwise, if the administrator really wants to force users to use a particular shell, then the most expedient solution is to make that change to the central LDAP attribute, thus percolating it out to all machines trivially.
I'm not convinced that this would be trivial when you have AD subdomains in play, some of those potentially without UNIX attributes enabled.
Cheers,
You make a good argument. I've pulled 1087 back into NEEDS_TRIAGE with the recommendation that we should implement it as part of the AD Provider effort I'm currently working on for SSSD 1.9.
On Tue, Apr 03, 2012 at 06:43:47AM -0700, Stephen Gallagher wrote:
On Tue, 2012-04-03 at 16:36 +0300, Marko Myllynen wrote:
Hi,
Ok, I think I understand now, but the manpages need to be MUCH more clear. It sounds like you're adding this option to always override subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles this internally by translating a NULL value for the shell into "the system default shell" (usually /bin/sh). This is handled by glibc and isn't our concern.
as with regular domains also with subdomains you might have the situation where different domains and users have different values for shell (e.g., NULL, /bin/bash, and /bin/tcsh) which in turn will cause users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
Thanks,
The reason we deferred 1087 is because we feel that if the user has a preferred shell set on the central server and the same shell is available on the client machine, then the user should be granted access to that shell on any machine they log into. We have the 'vetoed_shells' option in the event that a particular client machine wants to prevent a user from using a specific shell. Otherwise, if the administrator really wants to force users to use a particular shell, then the most expedient solution is to make that change to the central LDAP attribute, thus percolating it out to all machines trivially.
At least for trusts to AD we won't use any LDAP attributes, because they might not be set or it might be difficult to access the LDAP server at all.
bye, Sumit
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
Hi,
Ok, I think I understand now, but the manpages need to be MUCH more clear. It sounds like you're adding this option to always override subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles this internally by translating a NULL value for the shell into "the system default shell" (usually /bin/sh). This is handled by glibc and isn't our concern.
as with regular domains also with subdomains you might have the situation where different domains and users have different values for shell (e.g., NULL, /bin/bash, and /bin/tcsh) which in turn will cause users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
I think a combination of allowed_shells and shell_fallback might be also helpful in this case. It's not exactly correct solution but it's a solution.
Anyway, the reason why I would like to keep this is the possibility to set default shell per domain. But if you think that this won't be useful, I can simply delete it.
Jan
On Tue, 2012-04-03 at 15:51 +0200, Jan Zelený wrote:
Hi,
Ok, I think I understand now, but the manpages need to be MUCH more clear. It sounds like you're adding this option to always override subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles this internally by translating a NULL value for the shell into "the system default shell" (usually /bin/sh). This is handled by glibc and isn't our concern.
as with regular domains also with subdomains you might have the situation where different domains and users have different values for shell (e.g., NULL, /bin/bash, and /bin/tcsh) which in turn will cause users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
I think a combination of allowed_shells and shell_fallback might be also helpful in this case. It's not exactly correct solution but it's a solution.
Anyway, the reason why I would like to keep this is the possibility to set default shell per domain. But if you think that this won't be useful, I can simply delete it.
Let's pull it out of the current patches and consider it more fully under the umbrella of ticket 1087, which I just pulled back into NEEDS_TRIAGE. Sound reasonable?
On Tue, 2012-04-03 at 15:51 +0200, Jan Zelený wrote:
Hi,
Ok, I think I understand now, but the manpages need to be MUCH more clear. It sounds like you're adding this option to always override subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles this internally by translating a NULL value for the shell into "the system default shell" (usually /bin/sh). This is handled by glibc and isn't our concern.
as with regular domains also with subdomains you might have the situation where different domains and users have different values for shell (e.g., NULL, /bin/bash, and /bin/tcsh) which in turn will cause users logging into a system to have different environment. And the case where bash (and other shells) behave differently depending whether invoked as /bin/bash or /bin/sh might be something administrators will hit especially with subdomains as not all AD domains have UNIX attributes enabled.
It would help administrators if SSSD would provide a method to force a shell to all users regardless of domain/libc/user configuration but unfortunately the RFE requesting this functionality has been deferred (#1087). Even though you could state that the shell users will get is not of your concern, it is much of a concern for system administrators and the subdomain_shell option would seem to be helpful with that regard.
I think a combination of allowed_shells and shell_fallback might be also helpful in this case. It's not exactly correct solution but it's a solution.
Anyway, the reason why I would like to keep this is the possibility to set default shell per domain. But if you think that this won't be useful, I can simply delete it.
Let's pull it out of the current patches and consider it more fully under the umbrella of ticket 1087, which I just pulled back into NEEDS_TRIAGE. Sound reasonable?
Yes, let's discuss it on the next meeting.
Thanks Jan
On Mon, Apr 02, 2012 at 10:07:00AM +0200, Jan Zeleny wrote:
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
Nack.
I don't think this really makes sense at all. In most cases, users will prefer to use the value on the LDAP server. If they choose to override it, they'll do so through the existing override options (in the case of override_homedir, it already has %d available anyway.
We definitely don't need separate handling for shells. I can *kind of* see a value if you wanted to have only subdomains have a non-default location. I'm not sure I like that though. I feel like it's probably more complexity than we need.
I think you possibly missed the point. The point is that this information is NOT on the server, therefore we need a value that will fill it in. Otherwise only a blank field will be stored in sysdb and returned to the client
Returning an empty string for user shell is perfectly valid, the shell then defaults to system default (/bin/sh usually). But we already reopened the override ticket anyway..
Doesn't the existing homedir override work even with no shell? I just tested the override work when ldap_user_home_directory is set to an attribute that doesn't exist.
On Mon, Apr 02, 2012 at 10:07:00AM +0200, Jan Zeleny wrote:
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
Nack.
I don't think this really makes sense at all. In most cases, users will prefer to use the value on the LDAP server. If they choose to override it, they'll do so through the existing override options (in the case of override_homedir, it already has %d available anyway.
We definitely don't need separate handling for shells. I can *kind of* see a value if you wanted to have only subdomains have a non-default location. I'm not sure I like that though. I feel like it's probably more complexity than we need.
I think you possibly missed the point. The point is that this information is NOT on the server, therefore we need a value that will fill it in. Otherwise only a blank field will be stored in sysdb and returned to the client
Returning an empty string for user shell is perfectly valid, the shell then defaults to system default (/bin/sh usually). But we already reopened the override ticket anyway..
Doesn't the existing homedir override work even with no shell? I just tested the override work when ldap_user_home_directory is set to an attribute that doesn't exist.
The override works, but the problem I have with it is that it's not for a single domain but for the entire responder. That might not be acceptable in some environments.
Thanks Jan
On Wed, Apr 04, 2012 at 09:08:11AM +0200, Jan Zelený wrote:
On Mon, Apr 02, 2012 at 10:07:00AM +0200, Jan Zeleny wrote:
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
Nack.
I don't think this really makes sense at all. In most cases, users will prefer to use the value on the LDAP server. If they choose to override it, they'll do so through the existing override options (in the case of override_homedir, it already has %d available anyway.
We definitely don't need separate handling for shells. I can *kind of* see a value if you wanted to have only subdomains have a non-default location. I'm not sure I like that though. I feel like it's probably more complexity than we need.
I think you possibly missed the point. The point is that this information is NOT on the server, therefore we need a value that will fill it in. Otherwise only a blank field will be stored in sysdb and returned to the client
Returning an empty string for user shell is perfectly valid, the shell then defaults to system default (/bin/sh usually). But we already reopened the override ticket anyway..
Doesn't the existing homedir override work even with no shell? I just tested the override work when ldap_user_home_directory is set to an attribute that doesn't exist.
The override works, but the problem I have with it is that it's not for a single domain but for the entire responder. That might not be acceptable in some environments.
Thanks Jan
It is also per-domain, see man sssd.conf
On Wed, Apr 04, 2012 at 09:08:11AM +0200, Jan Zelený wrote:
On Mon, Apr 02, 2012 at 10:07:00AM +0200, Jan Zeleny wrote:
#0011 New ID-related config options for subdomains, these have to be present because IPA provider doesn't provide these values and defaults need to be implemented. Having defaults on the responder level didn't seem right since the policy might differ for each domain.
Nack.
I don't think this really makes sense at all. In most cases, users will prefer to use the value on the LDAP server. If they choose to override it, they'll do so through the existing override options (in the case of override_homedir, it already has %d available anyway.
We definitely don't need separate handling for shells. I can *kind of* see a value if you wanted to have only subdomains have a non-default location. I'm not sure I like that though. I feel like it's probably more complexity than we need.
I think you possibly missed the point. The point is that this information is NOT on the server, therefore we need a value that will fill it in. Otherwise only a blank field will be stored in sysdb and returned to the client
Returning an empty string for user shell is perfectly valid, the shell then defaults to system default (/bin/sh usually). But we already reopened the override ticket anyway..
Doesn't the existing homedir override work even with no shell? I just tested the override work when ldap_user_home_directory is set to an attribute that doesn't exist.
The override works, but the problem I have with it is that it's not for a single domain but for the entire responder. That might not be acceptable in some environments.
Thanks Jan
It is also per-domain, see man sssd.conf
Ah, I missed the note at the end before. I will consider if we can use it for this purpose and if yes, I will drop the patch.
Thanks Jan
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Zelený -
Marko Myllynen -
Stephen Gallagher -
Sumit Bose