[sssd PR#820][opened] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/820
Author: pbrezina
 Title: #820: ad: delete domains disabled through ad_enabled_domains from cache
Action: opened

PR body:
"""
Steps to reproduce:
1. Have at least one subdomain in ad domain (e.g. child.ad.vm is subdomain of ad.vm).
2. Enable all domains, set ad_enabled_domains =
  [ad.vm]
  ...
  ad_enabled_domains =
3. Look up 'administrator(a)child.ad.vm'
  $ id administrator(a)child.ad.vm
  uid=1678800500(administrator(a)child.ad.vm) ...
4. Disable the subdomain by setting 'ad_enabled_domains = ad.vm'
5. Restart sssd without clearing the cache
6. Request for *(a)child.ad.vm will go to data provider and try to lookup the user in
child.ad.vm domain which will yield 'domain not found'. However if the user is
cached it will return the user.
  $ id administrator(a)child.ad.vm
  uid=1678800500(administrator(a)child.ad.vm) ...


Subdomains that are not root domains are removed from cache. Root domains are
disabled in sysdb with new `enabled` attribute.

Resolves:
https://pagure.io/SSSD/sssd/issue/4009
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/820/head:pr820
git checkout pr820

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

pbrezina commented:
"""
I did not test the "root" domain case because I was not able to establish trust
with a non-root domain so far. But the pull request is straightforward, so it does not
necessarily blocks review.

```
[root(a)master.client.vm /home/vagrant]# realm join child.ad.vm
Password for Administrator:
See: journalctl REALMD_OPERATION=r1521.5100
realm: Couldn't join realm: Insufficient permissions to join the domain
[root(a)master.client.vm /home/vagrant]# journalctl REALMD_OPERATION=r1521.5100
-- Logs begin at Sun 2019-05-26 19:54:19 UTC, end at Thu 2019-05-30 09:40:15 UTC. --
May 30 09:40:13 master.client.vm realmd[5103]:  * Resolving: _ldap._tcp.child.ad.vm
May 30 09:40:13 master.client.vm realmd[5103]:  * Performing LDAP DSE lookup on:
192.168.100.120
May 30 09:40:13 master.client.vm realmd[5103]:  * Performing LDAP DSE lookup on:
192.168.121.248
May 30 09:40:13 master.client.vm realmd[5103]:  * Successfully discovered: child.ad.vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Required files: /usr/sbin/oddjobd,
/usr/libexec/oddjob/mkhomedir, /usr/sbin/sssd, /usr/sbin/adcli
May 30 09:40:15 master.client.vm realmd[5103]:  * LANG=C /usr/sbin/adcli join --verbose
--domain child.ad.vm --domain-realm CHILD.AD.VM --domain-controller 192.168.100.120
--login-type user --login-user Administrator --stdin-password
May 30 09:40:15 master.client.vm realmd[5103]:  * Using domain name: child.ad.vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Calculated computer account name from
fqdn: MASTER
May 30 09:40:15 master.client.vm realmd[5103]:  * Using domain realm: child.ad.vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Sending netlogon pings to domain
controller: cldap://192.168.100.120
May 30 09:40:15 master.client.vm realmd[5103]:  * Received NetLogon info from:
child-dc.child.ad.vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Wrote out krb5.conf snippet to
/var/cache/realmd/adcli-krb5-uxaCvi/krb5.d/adcli-krb5-conf-iAtYIJ
May 30 09:40:15 master.client.vm realmd[5103]:  * Authenticated as user:
Administrator(a)CHILD.AD.VM
May 30 09:40:15 master.client.vm realmd[5103]:  * Looked up short domain name: ADCHILD
May 30 09:40:15 master.client.vm realmd[5103]:  * Looked up domain SID:
S-1-5-21-2624477844-534582034-2536808417
May 30 09:40:15 master.client.vm realmd[5103]:  * Using fully qualified name:
master.client.vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Using domain name: child.ad.vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Using computer account name: MASTER
May 30 09:40:15 master.client.vm realmd[5103]:  * Using domain realm: child.ad.vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Calculated computer account name from
fqdn: MASTER
May 30 09:40:15 master.client.vm realmd[5103]:  * Generated 120 character computer
password
May 30 09:40:15 master.client.vm realmd[5103]:  * Using keytab: FILE:/etc/krb5.keytab
May 30 09:40:15 master.client.vm realmd[5103]:  * Computer account for MASTER$ does not
exist
May 30 09:40:15 master.client.vm realmd[5103]:  * Found well known computer container at:
CN=Computers,DC=child,DC=ad,DC=vm
May 30 09:40:15 master.client.vm realmd[5103]:  * Calculated computer account:
CN=MASTER,CN=Computers,DC=child,DC=ad,DC=vm
May 30 09:40:15 master.client.vm realmd[5103]:  ! Insufficient permissions to modify
computer account: CN=MASTER,CN=Computers,DC=child,DC=ad,DC=vm: 000021C7: AtrErr:
DSID-03200BBC, #1:
May 30 09:40:15 master.client.vm realmd[5103]:         0: 000021C7: DSID-03200BBC, problem
1005 (CONSTRAINT_ATT_TYPE), data 0, Att 90303 (servicePrincipalName)
May 30 09:40:15 master.client.vm realmd[5103]:
May 30 09:40:15 master.client.vm realmd[5103]: adcli: joining domain child.ad.vm failed:
Insufficient permissions to modify computer account:
CN=MASTER,CN=Computers,DC=child,DC=ad,DC=vm: 000021C7: AtrErr: DSID-03200BBC, #1:
May 30 09:40:15 master.client.vm realmd[5103]:         0: 000021C7: DSID-03200BBC, problem
1005 (CONSTRAINT_ATT_TYPE), data 0, Att 90303 (servicePrincipalName)
May 30 09:40:15 master.client.vm realmd[5103]:
May 30 09:40:15 master.client.vm realmd[5103]:  ! Insufficient permissions to join the
domain
``` 
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-497294985

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

pbrezina commented:
"""
Pull request:
https://github.com/SSSD/sssd/pull/820
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-497295105

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

pbrezina commented:
"""
Ok, I managed to setup trust with the child domain (it was necessary to change
client's hostname because it was already enrolled to the root domain) and it works
correctly.

There is one corner case when the master domain is the only enabled domain, we hit
`ad_subdomains.c:1837` and the subdomains are not refresh. @sumit-bose Is it OK to
recursively delete all cached subdomains (including the root domian) here? Or should it be
only disabled?
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-499843260

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
>  Ok, I managed to setup trust with the child domain (it was necessary
> to change client's hostname because it was already enrolled to the root domain) and it
> works correctly.
>  
>  There is one corner case when the master domain is the only enabled domain, we hit
> `ad_subdomains.c:1837` and the subdomains are not refresh. @sumit-bose Is it OK to
> recursively delete all cached subdomains (including the root domian) here? Or should it be
> only disabled?
>  
Hi,

I think it would be more elegant to just set the disable flag for the domain object in the
cache. But iirc when starting with an empty cache we do not create a domain object if the
domain is not listed in ad_enabled_domains, only for the forest root and I guess for the
domain we are joined to as well. In this case it might be more consistent to just remove
the domain and only set disable flag for the forest root and the domain we are joined
to.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-499856160

[sssd PR#820][synchronized] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/820
Author: pbrezina
 Title: #820: ad: delete domains disabled through ad_enabled_domains from cache
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/820/head:pr820
git checkout pr820

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

pbrezina commented:
"""
Thank you Sumit. Your answer was not clear so I chose to delete all subdomains from cache
in case when only master domain is enabled.

Now:
* If only master domain is enabled: all subdomains are removed.
* If non-root subdomain is disabled: it is removed from cache.
* If root subdomain is disabled: it is marked as disabled in cache.

Patches are ready for review.
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-500781109

[sssd PR#820][synchronized] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/820
Author: pbrezina
 Title: #820: ad: delete domains disabled through ad_enabled_domains from cache
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/820/head:pr820
git checkout pr820

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
Hi Pavel,

sorry for not being clear but your 3 points are exactly what I meant.

My tests went well. I had one issue where I'm not sure if should be fixed or not and
if this is an issue in your patches or with 'ad_enabled_domains' in general. I
have an AD domain with a mixed case name 'ChIlD.ad.devel' and if I add

    ad_enabled_domains = child.ad.devel

(lower case is recommended by the man page), it does not work after the first restart only
after the second I guess because the subdomains were re-discovered during the first
restart and the domain was disabled here.

But if I add

    ad_enabled_domains = ChIlD.ad.devel

it already works after the first restart. Do you have an idea where a strcmp() should be
replaced with a strcasecmp()?

bye,
Sumit

"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-519468024

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

pbrezina commented:
"""
Perhaps `ad_subdomains.c:193`? All other interactions with the list that I foud is case
insensitive.

```c
    is_ad_in_domains = false;
    for (int i = 0; i < count; i++) {
        is_ad_in_domains += strcmp(ad_domain, domains[i]) == 0 ? true : false;
    }
```
I don't have a test environment handy, would you mind trying it out?
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-519833290

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
>  Perhaps `ad_subdomains.c:193`? All other interactions with the list
> that I foud is case insensitive.
>  
>  ```c
>      is_ad_in_domains = false;
>      for (int i = 0; i < count; i++) {
>          is_ad_in_domains += strcmp(ad_domain, domains[i]) == 0 ? true : false;
>      }
>  ```
>  
>  I don't have a test environment handy, would you mind trying it out?
>  
Yes, that did the trick, good catch. Can you include this into your patchset?

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-519996331

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
Hi Pavel,

please check if all patches are added there. It looks like 'sysdb: add
sysdb_list_subdomains()' and 'ad: remove all subdomains if only master domain is
enabled' are missing.
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-521662619

[sssd PR#820][synchronized] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/820
Author: pbrezina
 Title: #820: ad: delete domains disabled through ad_enabled_domains from cache
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/820/head:pr820
git checkout pr820

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

pbrezina commented:
"""
Thank you. I just pushed to correct version.
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-521941787

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
Hi,

this version now works well in my tests and Coverity didn't find an issue. ACK.

bye,
Sumit
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-522070347

[sssd PR#820][+Accepted] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

Label: +Accepted

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
Master:
 - b3c3542188e50770b431942c0b603e6f2733cb33
 - d0bdaabbc95bc9ee3253e1376d849e6a8bd6c6f0
 - c7e6530d642f746982c5306cf3455608d1980d1f
 - d278704d85fea74c229b67e6a63b650b0d776c88
 - 6882bc5f5c8805abff3511d55c0ed60cad84faab
 - 7a03e99890806257df1ed8a126673d6a032fee6a
 - 815957cd10a82aca6742b0bd56c7e7f199596cd4
"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-524355419

[sssd PR#820][+Pushed] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

Label: +Pushed

[sssd PR#820][-Accepted] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

Label: -Accepted

[sssd PR#820][closed] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

   URL: https://github.com/SSSD/sssd/pull/820
Author: pbrezina
 Title: #820: ad: delete domains disabled through ad_enabled_domains from cache
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/820/head:pr820
git checkout pr820

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
sssd-1-16:
 -0b6f144084ec3ed96eb2c60bed7bea5d6c15f15c
 - 5605fa5f8adf79fa60286f5427aa2f989e663de0
 - 3c6c9d4d939bb2f1f629421e347285bea9a59341
 - b2cd4a74e231611f7862a8bb39a655c5194a035a
 - 800d24dccbf655b2c65521727256c4e6c4a540d5
 - 0e16ec74c380b35fc201ded15434184d88413dc7
 - c9c2b60128b7faa29615123de79ed206491396a9

"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-524361036

[sssd PR#820][comment] ad: delete domains disabled through ad_enabled_domains from cache

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/820
Title: #820: ad: delete domains disabled through ad_enabled_domains from cache

sumit-bose commented:
"""
sssd-1-16:
 - 0b6f144084ec3ed96eb2c60bed7bea5d6c15f15c
 - 5605fa5f8adf79fa60286f5427aa2f989e663de0
 - 3c6c9d4d939bb2f1f629421e347285bea9a59341
 - b2cd4a74e231611f7862a8bb39a655c5194a035a
 - 800d24dccbf655b2c65521727256c4e6c4a540d5
 - 0e16ec74c380b35fc201ded15434184d88413dc7
 - c9c2b60128b7faa29615123de79ed206491396a9

"""

See the full comment at https://github.com/SSSD/sssd/pull/820#issuecomment-524361036