[SSSD] [sssd PR#475][comment] LDAP: Only add a sdap_domain instance for the current domain when instantiating a new ad_id_ctx

2018-01-16 Thread jhrozek 
  URL: https://github.com/SSSD/sssd/pull/475
Title: #475: LDAP: Only add a sdap_domain instance for the current domain when 
instantiating a new ad_id_ctx

jhrozek commented:
"""
Umm OK, right what I wrote the previous wall of text, I realized the patch 
might have been wrong. But at the same time. I was initially going to say that 
a single `sdap_opts` can only ever contain a single `sdap_domain` for "self", 
but looking at e.g. `sdap_ad_check_domain_local_groups()` or ` 
ad_get_dom_ldap_conn()` it looks like all domains should be represented by 
`sdap_domain` so that we can reach `ad_id_ctx` of another domain..correct? 
@sumit-bose do you maybe remember?

If that's the case, then the right way to solve this would be to call 
`sdap_domain_get()` instead of dereferencing the `opts->sdom`..

We should really get this documented one way or another.. (and we have a ticket 
but..time..)
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/475#issuecomment-358120530
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#475][comment] LDAP: Only add a sdap_domain instance for the current domain when instantiating a new ad_id_ctx

2018-01-16 Thread jhrozek 
  URL: https://github.com/SSSD/sssd/pull/475
Title: #475: LDAP: Only add a sdap_domain instance for the current domain when 
instantiating a new ad_id_ctx

jhrozek commented:
"""
I spent some today trying to understand this issue better so that I can explain 
it better in the PR. I think the patch is correct as it was submitted the first 
time but I'm also afraid that the relation between the different structures in 
SSSD code has reached a critical point where it's really hard to understand how 
the structures were designed..

So first, about the bug. It only happens where the cache is already primed with 
subdomain objects. Then, the AD subdomains code will call `ad_subdom_reinit` 
which reads the subdomains from sysdb. Then `ads_store_sdap_subdom()` calls 
`sdap_domain_subdom_add()` which creates a `sdap_domain` struct for each 
subdomain. Here I think both the linked list of `sdap_domain` objects and the 
`sdap_domain_subdom_add` call are correct.

Then, for each of the `sdap_domain`, we call `ad_subdom_ad_ctx_new()` which 
creates `ad_id_ctx` for every subdomain. This internally used to call 
`sdap_domain_subdom_add()`  which I think was wrong, because inside, 
`sdap_domain_subdom_add()` iterates over all domains and appends a 
`sdap_domain` structure per domain to the `sdap_opts` structure.

I added some DEBUG messages into the code, which show what happens:
```
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] [ad_subdom_ad_ctx_new] 
(0x0040): INFO: Creating ad_ctx for sssdad.com
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] 
[sdap_domain_subdom_add] (0x0040): INFO: Parent: child1.sssdad.com
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] [sss_domain_get_state] 
(0x1000): Domain sssdad_tree.com is Active
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] 
[sdap_domain_subdom_add] (0x0040): INFO: Iterating over domain sssdad_tree.com
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] 
[sdap_domain_subdom_add] (0x0400): subdomain sssdad_tree.com is a new one, will 
create a new sdap domain object
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] [sss_domain_get_state] 
(0x1000): Domain sssdad.com is Active
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] 
[sdap_domain_subdom_add] (0x0040): INFO: Iterating over domain sssdad.com
(Tue Jan 16 16:25:19 2018) [sssd[be[child1.sssdad.com]]] 
[sdap_domain_subdom_add] (0x0400): subdomain sssdad.com is a new one, will 
create a new sdap domain object
```
So here, for domain `sssdad.com`, the linked list of `sdap_domain` structures 
is `sssdad_tree.com->sssdad.com`. And later, the code that tries to look up the 
trusted domains just uses the first one, which in this case is 
`sssdad_tree.com` for the `sssdad.com` domain:
```
 853 subreq = sdap_search_bases_send(state, state->ev, state->opts, 
  
 854 sdap_id_op_handle(state->sdap_op), 
  
 855 state->opts->sdom->search_bases,
```
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/475#issuecomment-358115852
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] [sssd PR#475][comment] LDAP: Only add a sdap_domain instance for the current domain when instantiating a new ad_id_ctx

2018-01-08 Thread jhrozek 
  URL: https://github.com/SSSD/sssd/pull/475
Title: #475: LDAP: Only add a sdap_domain instance for the current domain when 
instantiating a new ad_id_ctx

jhrozek commented:
"""
I'm just adding Changes Requested here myself because as I said on Thursday's 
meeting, I'd like to make it harder to introduce a bug like this in the future.

@lslebodn if it's not too much work for you, could you please prepare me 
another machine that reproduces the bug? Or just tell me where to insert a test 
breakpoint into which downstream test and I'll set up the environment myself.
"""

See the full comment at 
https://github.com/SSSD/sssd/pull/475#issuecomment-356091703
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org