Support for subdomains - stage 1

[Jan Zelený]

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

[Stephen Gallagher]

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.

[Stephen Gallagher]

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(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Zelený]

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

[Jan Zelený]

Once more, this time rebased on top of current master.

Thanks
Jan

[Jakub Hrozek]

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.

[Jakub Hrozek]

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

[Jan Zelený]

>  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 redundant
>  +            goto 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

[Sumit Bose]

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 redundant
>  > +            goto 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

[Jan Zelený]

>  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 redundant
>  > > 
>  > > +            goto 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

[Sumit Bose]

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

[Jakub Hrozek]

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 redundant
>  > +            goto 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.

[Jan Zelený]

>  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 redundant
>  > > 
>  > > +            goto 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

[Jakub Hrozek]

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!

[Jakub Hrozek]

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

[Jan Zelený]

Jakub Hrozek <jhrozek(a)redhat.com&gt; 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

[Jakub Hrozek]

On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
>  Jakub Hrozek <jhrozek(a)redhat.com&gt; 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.

[Jan Zelený]

>  On Thu, Apr 12, 2012 at 09:52:14PM +0200, Jan Zeleny wrote:
>  > Jakub Hrozek <jhrozek(a)redhat.com&gt; 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

[Sumit Bose]

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(a)redhat.com&gt; 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(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Zelený]

>  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(a)redhat.com&gt; 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

[Jakub Hrozek]

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(a)redhat.com&gt; 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

[Jan Zelený]

>  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(a)redhat.com&gt; 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

[Stephen Gallagher]

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(a)redhat.com&gt; 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)

[Stephen Gallagher]

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(a)redhat.com&gt; 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!

[Jakub Hrozek]

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.

[Stephen Gallagher]

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.

[Marko Myllynen]

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,

-- 
Marko Myllynen

[Stephen Gallagher]

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.

[Marko Myllynen]

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,

-- 
Marko Myllynen

[Stephen Gallagher]

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.

[Sumit Bose]

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(a)lists.fedorahosted.org
>  https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Zelený]

>  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

[Stephen Gallagher]

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?

[Jan Zelený]

>  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

[Jakub Hrozek]

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.

[Jan Zelený]

>  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

[Jakub Hrozek]

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

[Jan Zelený]

>  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