[sssd PR#5613][opened] ipa: read auto_private_groups from id range if available

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/5613
Author: pbrezina
 Title: #5613: ipa: read auto_private_groups from id range if available
Action: opened

PR body:
"""
The functionality depends on new attribute in IPA:
* https://github.com/freeipa/freeipa/pull/5712 (already merged)

It defaults to current behavior if the attribute is not present.

Resolves: https://github.com/SSSD/sssd/issues/4216

:feature: `auto_private_groups` option can be set centrally through
  ID range setting in IPA (see `ipa idrange` commands family)
"""

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

[sssd PR#5613][+Waiting for review] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: +Waiting for review

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

sumit-bose commented:
"""
Hi,

thank you for the patches. So far I tested the `true` and `false` options in different
trust setups and came across an issue if the trust is created with
`--range-type=ipa-ad-trust-posix`. In this case only a single id-range for the forest root
is created and the settings (basically the range-type) is inherited to all domains in the
forest. This was done because we cannot know which POSIX IDs are used in which domain of
the forest, so there will be an id-range for the whole forest which just blocks the given
range of ID for other to use.

If you now set `--auto-private-groups` to this id-range the patch currently only evaluats
the option for the forest root, but the setting is not inherited to the other domains in
the forest. A workaround is to add an id-range for each other domain in the forest but I
think it would be better if the setting is inherited automatically if
`--range-type=ipa-ad-trust-posix`.

In `test_ipa_idmap.c` some test data is using `struct range_info` and the new `enum
sss_domain_mpg_mode mpg_mode` is not initialized in the test data.

bye,
Sumit
"""

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

[sssd PR#5613][+Changes requested] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: +Changes requested

[sssd PR#5613][-Waiting for review] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: -Waiting for review

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

sumit-bose commented:
"""
Hi,

some additional observations. The setting is inherited but only after multiple
refreshes:

```
[root(a)master.ipa.vm /var/log/sssd]# systemctl stop sssd ; rm -rf /var/log/sssd/*
/var/lib/sss/db/* ; systemctl start sssd                                                  
                                                                  
[root(a)master.ipa.vm /var/log/sssd]# grep 'mpg m' *                                
                                    
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Range mpg
mode for ad.vm: true 
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Domain
mpg mode for ad.vm: true     
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Range mpg
mode for child.ad.vm: default                                                             
                                                        
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Domain
mpg mode for child.ad.vm: false                                                           
                         
[root(a)master.ipa.vm /var/log/sssd]# getent passwd dwqdqw(a)fewfw.fewff                      
                                                                                          
                                                        
[root(a)master.ipa.vm /var/log/sssd]# grep 'mpg m' *                                
                                    
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Range mpg
mode for ad.vm: true
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Domain
mpg mode for ad.vm: true       
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Range mpg
mode for child.ad.vm: default                                                             
                                                        
sssd_ipa.vm.log:(2021-05-05 14:41:22): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Domain
mpg mode for child.ad.vm: false
sssd_ipa.vm.log:(2021-05-05 14:42:30): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Range mpg
mode for ad.vm: true
sssd_ipa.vm.log:(2021-05-05 14:42:30): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Domain
mpg mode for ad.vm: true
sssd_ipa.vm.log:(2021-05-05 14:42:30): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Range mpg
mode for child.ad.vm: true                                                                
                                                        
sssd_ipa.vm.log:(2021-05-05 14:42:30): [be[ipa.vm]] [ipa_subdom_store] (0x0400): Domain
mpg mode for child.ad.vm: true 
```

But even then it looks like a restart is required for the option to start to work.

With `hybrid` it is the same, please note the `hybrid` is special in the sense that it is
completely handled in the responder. While looking at the related commit
2ea38097dc62963403f77c96946a93f8aae11a44 it looks like it is only handled in the nss
responder. Maybe it would be better to move the whole logic into cache_req? But this does
not have to be part of this PR. Here it should be sufficient to make sure the options read
from the server at startup if the cache is empty are available in the nss responder as
well.

bye,
Sumit
"""

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

[sssd PR#5613][synchronized] ipa: read auto_private_groups from id range if available

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/5613
Author: pbrezina
 Title: #5613: ipa: read auto_private_groups from id range if available
Action: synchronized

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

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

pbrezina commented:
"""
Thank you Sumit, please try it now. The problem was that I used `find_domain_by_name` to
get the forest domain but at this time it may exist only in sysdb if it is newly created
and not in the domain objects.
"""

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

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

pbrezina commented:
"""
>  With `hybrid` it is the same, please note the `hybrid` is special in
> the sense that it is completely handled in the responder. While looking at the related
> commit
> [2ea3809](https://github.com/SSSD/sssd/commit/2ea38097dc62963403f77c96946a... it
> looks like it is only handled in the nss responder. Maybe it would be better to move the
> whole logic into cache_req? But this does not have to be part of this PR. Here it should
> be sufficient to make sure the options read from the server at startup if the cache is
> empty are available in the nss responder as well.
>  
This should not be a problem because the mpg mode is stored in the sysdb domain object.
Once the responder starts, it reads the domain list from sysdb so it gets the right mpg
mode.


"""

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

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

sumit-bose commented:
"""
Hi,

thanks for the updates. In the latest version the setting is inherited properly. However
it looks like the `hybrid` mode is still not working as expected. I guess the reason is
that in `nss_get_object_done()`only `state->nss_ctx->rctx->domains->mpg_mode
== MPG_HYBRID` is checked. This works fine in the original use-case where only one domain
with direct integration is configured (even here it will already fail if e.g. the implicit
files provider is used because `state->nss_ctx->rctx->domains` is then the files
provider domain). So I think the check in `nss_get_object_done()` should be made more
flexible.

bye,
Sumit
"""

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

[sssd PR#5613][synchronized] ipa: read auto_private_groups from id range if available

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/5613
Author: pbrezina
 Title: #5613: ipa: read auto_private_groups from id range if available
Action: synchronized

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

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

pbrezina commented:
"""
Thanks,. I added new commit to address this.
"""

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

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

sumit-bose commented:
"""
Hi,

thank you for the updates, I think lookups on the server-side now work as expected.

When testing on a client with POSIX IDs from AD in hybrid mode I came across an issue when
looking up a GID generated via hybrid mode. This fails because the id_type is not set to
`SSS_ID_TYPE_BOTH`. A patch like

```
diff --git a/src/responder/nss/nss_protocol_sid.c b/src/responder/nss/nss_protocol_sid.c
index 96cb8617b..b6292329b 100644
--- a/src/responder/nss/nss_protocol_sid.c
+++ b/src/responder/nss/nss_protocol_sid.c
@@ -76,7 +76,7 @@ nss_get_id_type(struct nss_cmd_ctx *cmd_ctx,
     }
 
     ret = find_sss_id_type(result->msgs[0],
-                           sss_domain_is_mpg(result->domain),
+                           sss_domain_is_mpg(result->domain) ||
sss_domain_is_hybrid(result->domain),
                            _type);
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE,
diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index 26e3ed054..7ceeb4242 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -966,6 +966,11 @@ bool sss_domain_is_mpg(struct sss_domain_info *domain)
     return domain->mpg_mode == MPG_ENABLED;
 }
 
+bool sss_domain_is_hybrid(struct sss_domain_info *domain)
+{
+    return domain->mpg_mode == MPG_HYBRID;
+}
+
 enum sss_domain_mpg_mode get_domain_mpg_mode(struct sss_domain_info *domain)
 {
     return domain->mpg_mode;
diff --git a/src/util/util.h b/src/util/util.h
index e43b2863d..60ad2112f 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -625,6 +625,7 @@ void sss_domain_info_set_output_fqnames(struct sss_domain_info
*domain,
 bool sss_domain_info_get_output_fqnames(struct sss_domain_info *domain);
 
 bool sss_domain_is_mpg(struct sss_domain_info *domain);
+bool sss_domain_is_hybrid(struct sss_domain_info *domain);
 
 enum sss_domain_mpg_mode get_domain_mpg_mode(struct sss_domain_info *domain);
 const char *str_domain_mpg_mode(enum sss_domain_mpg_mode mpg_mode);
```

fixed this for me. As a result all 3 modes with IDs from AD worked on a patched and
un-patched client.

With auto-generated IDs in my testing `--auto-private-groups=false` only worked on a
patched client. On an un-patched client it looks the mpg mode is more or less hardcoded,
even setting `auto_private_groups` didn't help in my testing. As a result looking up a
GID often fails with a 'multiple results found' error in the logs because the
lookup finds the GID of the group but also user with the GID as primary GID.

If you do not have an idea how to make it work on un-patched clients I guess this must be
documented carefully.

Finally, not strictly related to your patches, I think the `hybrid` mode make no sense for
auto-generated IDs (`--range-type=ipa-ad-trust`), it should give the same results as
`false` mode. So it might be worth it not allow to set it in this case at all in the ipa
tools. What do you think?

bye,
Sumit 
"""

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

[sssd PR#5613][synchronized] ipa: read auto_private_groups from id range if available

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/5613
Author: pbrezina
 Title: #5613: ipa: read auto_private_groups from id range if available
Action: synchronized

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

[sssd PR#5613][synchronized] ipa: read auto_private_groups from id range if available

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/5613
Author: pbrezina
 Title: #5613: ipa: read auto_private_groups from id range if available
Action: synchronized

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

[sssd PR#5613][+Waiting for review] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: +Waiting for review

[sssd PR#5613][-Changes requested] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: -Changes requested

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

sumit-bose commented:
"""
>  
>  > Finally, not strictly related to your patches, I think the `hybrid` mode make no
> sense for auto-generated IDs (`--range-type=ipa-ad-trust`), it should give the same
> results as `false` mode. So it might be worth it not allow to set it in this case at all
> in the ipa tools. What do you think?
>  
>  Did you mean same results as `true` mode? It makes sense to disable this option there.
>  
>  
Sorry, yes, I meant `true`. I tested the patch with various id-overrides and didn't
find any oddness which isn't present in the original code as well, so ACK.

bye,
Sumit
"""

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

[sssd PR#5613][-Waiting for review] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: -Waiting for review

[sssd PR#5613][+Accepted] ipa: read auto_private_groups from id range if available

[sumit-bose]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: +Accepted

[sssd PR#5613][+Ready to push] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: +Ready to push

[sssd PR#5613][comment] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

pbrezina commented:
"""
Pushed PR: https://github.com/SSSD/sssd/pull/5613

* `master`
    * 706627cf72d611155b2bc6d26a631252325c6095 - cache_req: consider mpg_mode of each
domain
    * b099498f5ce26660badef52b822cf07ce1d9f29a - ipa: read auto_private_groups from id
range if available

"""

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

[sssd PR#5613][+Pushed] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: +Pushed

[sssd PR#5613][-Accepted] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: -Accepted

[sssd PR#5613][-Ready to push] ipa: read auto_private_groups from id range if available

[pbrezina]

  URL: https://github.com/SSSD/sssd/pull/5613
Title: #5613: ipa: read auto_private_groups from id range if available

Label: -Ready to push

[sssd PR#5613][closed] ipa: read auto_private_groups from id range if available

[pbrezina]

   URL: https://github.com/SSSD/sssd/pull/5613
Author: pbrezina
 Title: #5613: ipa: read auto_private_groups from id range if available
Action: closed

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