[PATCH] LDAP: tokengroups do not work with id_provider=ldap

[Pavel Reichl]

Hello,

please see attached patch.

Regards,

PR

[Jakub Hrozek]

On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> Hello,
>
> please see attached patch.
>
> Regards,
>
> PR

The patch solves the problem, but I think one part should be improved:
> @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
>          domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
>          if (domain == NULL) {
>              DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
>        continue;
>        if (state->domain->parent == NULL &&
>            state->domain->subdomains == NULL) {
>            domain = state->domain;
>            DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
>        } else {
>            continue;
>        }
>    }

I think this is a bit dangerous. I wonder if we should have some
modification of find_subdomain_by_sid that would return the first
configured domain if no subdomain provider was configured or if no
domains had a SID. This could be a separate function.

Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
to find the primary domain.

[Sumit Bose]

On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > Hello,
> >
> > please see attached patch.
> >
> > Regards,
> >
> > PR
>
> The patch solves the problem, but I think one part should be improved:
> > @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> >          domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
> >          if (domain == NULL) {
> >              DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
> >        continue;
> >        if (state->domain->parent == NULL &&
> >            state->domain->subdomains == NULL) {
> >            domain = state->domain;
> >            DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
> >        } else {
> >            continue;
> >        }
> >    }
>
> I think this is a bit dangerous. I wonder if we should have some
> modification of find_subdomain_by_sid that would return the first
> configured domain if no subdomain provider was configured or if no
> domains had a SID. This could be a separate function.

This sounds even more dangerous to me.
> Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> to find the primary domain.

I think find_subdomain_by_sid() does what the name says and of course it
can return the primary domain as long as the SID of the domain is know
which is the case for the IPA and AD provider.

What about adding an explicit check if the running id provider is the
plain LDAP provider? As an alternative the LDAP provider can add a
special value in the id member of the sss_dom_info struct and then
find_subdomain_by_sid can handle this case specially?

bye,
Sumit
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
> On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see attached patch.
> > >
> > > Regards,
> > >
> > > PR
> >
> > The patch solves the problem, but I think one part should be improved:
> > > @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> > >          domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
> > >          if (domain == NULL) {
> > >              DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
> > >        continue;
> > >        if (state->domain->parent == NULL &&
> > >            state->domain->subdomains == NULL) {
> > >            domain = state->domain;
> > >            DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
> > >        } else {
> > >            continue;
> > >        }
> > >    }
> >
> > I think this is a bit dangerous. I wonder if we should have some
> > modification of find_subdomain_by_sid that would return the first
> > configured domain if no subdomain provider was configured or if no
> > domains had a SID. This could be a separate function.
>
> This sounds even more dangerous to me.
> > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > to find the primary domain.
>
> I think find_subdomain_by_sid() does what the name says and of course it
> can return the primary domain as long as the SID of the domain is know
> which is the case for the IPA and AD provider.
> What about adding an explicit check if the running id provider is the
> plain LDAP provider?

Would that be acceptable with you Jakub?
> As an alternative the LDAP provider can add a
> special value in the id member of the sss_dom_info struct and then
> find_subdomain_by_sid can handle this case specially?
>
> bye,
> Sumit
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
> On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
> > On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> > > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see attached patch.
> > > >
> > > > Regards,
> > > >
> > > > PR
> > >
> > > The patch solves the problem, but I think one part should be improved:
> > > > @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> > > >          domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
> > > >          if (domain == NULL) {
> > > >              DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
> > > >        continue;
> > > >        if (state->domain->parent == NULL &&
> > > >            state->domain->subdomains == NULL) {
> > > >            domain = state->domain;
> > > >            DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
> > > >        } else {
> > > >            continue;
> > > >        }
> > > >    }
> > >
> > > I think this is a bit dangerous. I wonder if we should have some
> > > modification of find_subdomain_by_sid that would return the first
> > > configured domain if no subdomain provider was configured or if no
> > > domains had a SID. This could be a separate function.
> >
> > This sounds even more dangerous to me.
> > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > to find the primary domain.
> >
> > I think find_subdomain_by_sid() does what the name says and of course it
> > can return the primary domain as long as the SID of the domain is know

^^^^^^
fwiw, this was my concern, the function is named "find_subdomain" yet it
can find both main domain and subdomain. But I won't bikeshed any further.
> > which is the case for the IPA and AD provider.
> > What about adding an explicit check if the running id provider is the
> > plain LDAP provider?
>
> Would that be acceptable with you Jakub?

It's a bit hackish but I don't see any other way.
> > As an alternative the LDAP provider can add a
> > special value in the id member of the sss_dom_info struct and then
> > find_subdomain_by_sid can handle this case specially?
> >
> > bye,
> > Sumit
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Mon, Jul 07, 2014 at 10:25:23PM +0200, Jakub Hrozek wrote:
> On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
> > On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
> > > On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> > > > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > > > > Hello,
> > > > >
> > > > > please see attached patch.
> > > > >
> > > > > Regards,
> > > > >
> > > > > PR
> > > >
> > > > The patch solves the problem, but I think one part should be improved:
> > > > > @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> > > > >          domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
> > > > >          if (domain == NULL) {
> > > > >              DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
> > > > >        continue;
> > > > >        if (state->domain->parent == NULL &&
> > > > >            state->domain->subdomains == NULL) {
> > > > >            domain = state->domain;
> > > > >            DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
> > > > >        } else {
> > > > >            continue;
> > > > >        }
> > > > >    }
> > > >
> > > > I think this is a bit dangerous. I wonder if we should have some
> > > > modification of find_subdomain_by_sid that would return the first
> > > > configured domain if no subdomain provider was configured or if no
> > > > domains had a SID. This could be a separate function.
> > >
> > > This sounds even more dangerous to me.
> > > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > to find the primary domain.
> > >
> > > I think find_subdomain_by_sid() does what the name says and of course it
> > > can return the primary domain as long as the SID of the domain is know
>
> ^^^^^^
> fwiw, this was my concern, the function is named "find_subdomain" yet it
> can find both main domain and subdomain. But I won't bikeshed any further.

ah, sorry, now I see your point. I agree that the name misleading but I
think this can be fixed after the release.

bye,
Sumit
> > > which is the case for the IPA and AD provider.
> > > What about adding an explicit check if the running id provider is the
> > > plain LDAP provider?
> >
> > Would that be acceptable with you Jakub?
>
> It's a bit hackish but I don't see any other way.
> > > As an alternative the LDAP provider can add a
> > > special value in the id member of the sss_dom_info struct and then
> > > find_subdomain_by_sid can handle this case specially?
> > >
> > > bye,
> > > Sumit
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

Hello,

please see attached patches.

I found out that if we go with approach introduced in previous version
(in case of LDAP provider assume SID comes from default domain) this can
lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
groups which in case of disabled id mapping leads to failure which will
end request prematurely. 2nd patch should make SID resolution more
resilient to handle this.

Regards,
Pavel Reichl

On Tue, 2014-07-08 at 09:57 +0200, Sumit Bose wrote:
> On Mon, Jul 07, 2014 at 10:25:23PM +0200, Jakub Hrozek wrote:
> > On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
> > > On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
> > > > On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> > > > > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > > > > > Hello,
> > > > > >
> > > > > > please see attached patch.
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > PR
> > > > >
> > > > > The patch solves the problem, but I think one part should be improved:
> > > > > > @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> > > > > >          domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
> > > > > >          if (domain == NULL) {
> > > > > >              DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
> > > > > >        continue;
> > > > > >        if (state->domain->parent == NULL &&
> > > > > >            state->domain->subdomains == NULL) {
> > > > > >            domain = state->domain;
> > > > > >            DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
> > > > > >        } else {
> > > > > >            continue;
> > > > > >        }
> > > > > >    }
> > > > >
> > > > > I think this is a bit dangerous. I wonder if we should have some
> > > > > modification of find_subdomain_by_sid that would return the first
> > > > > configured domain if no subdomain provider was configured or if no
> > > > > domains had a SID. This could be a separate function.
> > > >
> > > > This sounds even more dangerous to me.
> > > > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > > to find the primary domain.
> > > >
> > > > I think find_subdomain_by_sid() does what the name says and of course it
> > > > can return the primary domain as long as the SID of the domain is know
> >
> > ^^^^^^
> > fwiw, this was my concern, the function is named "find_subdomain" yet it
> > can find both main domain and subdomain. But I won't bikeshed any further.
>
> ah, sorry, now I see your point. I agree that the name misleading but I
> think this can be fixed after the release.

Would 's/find_subdomain_by_sid/find_domain_by_sid/' be sufficient
solution?
> bye,
> Sumit
> > > > which is the case for the IPA and AD provider.
> > > > What about adding an explicit check if the running id provider is the
> > > > plain LDAP provider?
> > >
> > > Would that be acceptable with you Jakub?
> >
> > It's a bit hackish but I don't see any other way.
> > > > As an alternative the LDAP provider can add a
> > > > special value in the id member of the sss_dom_info struct and then
> > > > find_subdomain_by_sid can handle this case specially?
> > > >
> > > > bye,
> > > > Sumit
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > sssd-devel mailing list
> > > > [e-mail]
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On 10 Jul 2014, at 16:38, Pavel Reichl  wrote:
> Hello,
>
> please see attached patches.
>
> I found out that if we go with approach introduced in previous version
> (in case of LDAP provider assume SID comes from default domain) this can
> lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
> groups which in case of disabled id mapping leads to failure which will
> end request prematurely. 2nd patch should make SID resolution more
> resilient to handle this.

The only complaint I have about the code is that the domain NULL check is not needed, I actually think we should fail if there is a NULL domain in the provider code, the provider handler should already take care of finding the right domain. If not, we’re in big trouble anyway.

Otherwise the code solves the problem — I can’t say it looks great. That’s not your fault Pavel, just yet another reminder that we need to work on the LDAP provider refactoring no later than 1.13.

I tested with both id_provider=ldap and =ad using both POSIX attributes and ID mapping. Both worked fine in my testing.

Can you re-send the patches without the domain check? Then I’ll ack.
> Regards,
> Pavel Reichl
>
> On Tue, 2014-07-08 at 09:57 +0200, Sumit Bose wrote:
> > On Mon, Jul 07, 2014 at 10:25:23PM +0200, Jakub Hrozek wrote:
> > > On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
> > > > On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
> > > > > On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> > > > > > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > please see attached patch.
> > > > > > > 
> > > > > > > Regards,
> > > > > > > 
> > > > > > > PR
> > > > > >
> > > > > > The patch solves the problem, but I think one part should be improved:
> > > > > >
> > > > > > > @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> > > > > > >         domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
> > > > > > >         if (domain == NULL) {
> > > > > > >             DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
> > > > > > > -            continue;
> > > > > > > +            if (state->domain->parent == NULL &&
> > > > > > > +                state->domain->subdomains == NULL) {
> > > > > > > +                domain = state->domain;
> > > > > > > +                DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
> > > > > > > +            } else {
> > > > > > > +                continue;
> > > > > > > +            }
> > > > > > >         }
> > > > > >
> > > > > > I think this is a bit dangerous. I wonder if we should have some
> > > > > > modification of find_subdomain_by_sid that would return the first
> > > > > > configured domain if no subdomain provider was configured or if no
> > > > > > domains had a SID. This could be a separate function.
> > > > >
> > > > > This sounds even more dangerous to me.
> > > > > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > > > to find the primary domain.
> > > > >
> > > > > I think find_subdomain_by_sid() does what the name says and of course it
> > > > > can return the primary domain as long as the SID of the domain is know
> > >
> > > ^^^^^^
> > > fwiw, this was my concern, the function is named "find_subdomain" yet it
> > > can find both main domain and subdomain. But I won't bikeshed any further.
> >
> > ah, sorry, now I see your point. I agree that the name misleading but I
> > think this can be fixed after the release.
>
> Would 's/find_subdomain_by_sid/find_domain_by_sid/' be sufficient
> solution?
> > bye,
> > Sumit
> > > > > which is the case for the IPA and AD provider.
> > > > > What about adding an explicit check if the running id provider is the
> > > > > plain LDAP provider?
> > > >
> > > > Would that be acceptable with you Jakub?
> > >
> > > It's a bit hackish but I don't see any other way.
> > > > > As an alternative the LDAP provider can add a
> > > > > special value in the id member of the sss_dom_info struct and then
> > > > > find_subdomain_by_sid can handle this case specially?
> > > > >
> > > > > bye,
> > > > > Sumit
> > > > > > sssd-devel mailing list
> > > > > > [e-mail]
> > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > > > > sssd-devel mailing list
> > > > > [e-mail]
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > > sssd-devel mailing list
> > > [e-mail]
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> <0001-LDAP-tokengroups-do-not-work-with-id_provider-ldap.patch><0002-SDAP-Continue-resolving-SID-even-if-some-fail.patch>_______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

On Thu, 2014-07-17 at 07:59 +0200, Jakub Hrozek wrote:
> On 10 Jul 2014, at 16:38, Pavel Reichl  wrote:
> > Hello,
> >
> > please see attached patches.
> >
> > I found out that if we go with approach introduced in previous version
> > (in case of LDAP provider assume SID comes from default domain) this can
> > lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
> > groups which in case of disabled id mapping leads to failure which will
> > end request prematurely. 2nd patch should make SID resolution more
> > resilient to handle this.
>
> The only complaint I have about the code is that the domain NULL check is not needed, I actually think we should fail if there is a NULL domain in the provider code, the provider handler should already take care of finding the right domain. If not, we’re in big trouble anyway.
>
> Otherwise the code solves the problem — I can’t say it looks great. That’s not your fault Pavel, just yet another reminder that we need to work on the LDAP provider refactoring no later than 1.13.
>
> I tested with both id_provider=ldap and =ad using both POSIX attributes and ID mapping. Both worked fine in my testing.
>
> Can you re-send the patches without the domain check? Then I’ll ack.

Sure, attached patches address your and Lukas' comments.

[snip]
> > > > > > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > > > > to find the primary domain.
> > > > > >
> > > > > > I think find_subdomain_by_sid() does what the name says and of course it
> > > > > > can return the primary domain as long as the SID of the domain is know
> > > >
> > > > ^^^^^^
> > > > fwiw, this was my concern, the function is named "find_subdomain" yet it
> > > > can find both main domain and subdomain. But I won't bikeshed any further.
> > >
> > > ah, sorry, now I see your point. I agree that the name misleading but I
> > > think this can be fixed after the release.

Would 's/find_subdomain_by_sid/find_domain_by_sid/' be a sufficient
solution?

[snip]

[Jakub Hrozek]

On Thu, Jul 17, 2014 at 05:41:23PM +0200, Pavel Reichl wrote:
> On Thu, 2014-07-17 at 07:59 +0200, Jakub Hrozek wrote:
> > On 10 Jul 2014, at 16:38, Pavel Reichl  wrote:
> > > Hello,
> > >
> > > please see attached patches.
> > >
> > > I found out that if we go with approach introduced in previous version
> > > (in case of LDAP provider assume SID comes from default domain) this can
> > > lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
> > > groups which in case of disabled id mapping leads to failure which will
> > > end request prematurely. 2nd patch should make SID resolution more
> > > resilient to handle this.
> >
> > The only complaint I have about the code is that the domain NULL check is not needed, I actually think we should fail if there is a NULL domain in the provider code, the provider handler should already take care of finding the right domain. If not, we’re in big trouble anyway.
> >
> > Otherwise the code solves the problem — I can’t say it looks great. That’s not your fault Pavel, just yet another reminder that we need to work on the LDAP provider refactoring no later than 1.13.
> >
> > I tested with both id_provider=ldap and =ad using both POSIX attributes and ID mapping. Both worked fine in my testing.
> >
> > Can you re-send the patches without the domain check? Then I’ll ack.
>
> Sure, attached patches address your and Lukas' comments.

They indeed do, ACK
> [snip]
> > > > > > >> Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > > > >> to find the primary domain.
> > > > > > > 
> > > > > > > I think find_subdomain_by_sid() does what the name says and of course it
> > > > > > > can return the primary domain as long as the SID of the domain is know
> > > > >
> > > > > ^^^^^^
> > > > > fwiw, this was my concern, the function is named "find_subdomain" yet it
> > > > > can find both main domain and subdomain. But I won't bikeshed any further.
> > > >
> > > > ah, sorry, now I see your point. I agree that the name misleading but I
> > > > think this can be fixed after the release.
>
> Would 's/find_subdomain_by_sid/find_domain_by_sid/' be a sufficient
> solution?

Yes, but in a separate patch, please.

[Jakub Hrozek]

On Fri, Jul 18, 2014 at 05:10:53PM +0200, Jakub Hrozek wrote:
> On Thu, Jul 17, 2014 at 05:41:23PM +0200, Pavel Reichl wrote:
> > On Thu, 2014-07-17 at 07:59 +0200, Jakub Hrozek wrote:
> > > On 10 Jul 2014, at 16:38, Pavel Reichl  wrote:
> > > > Hello,
> > > >
> > > > please see attached patches.
> > > >
> > > > I found out that if we go with approach introduced in previous version
> > > > (in case of LDAP provider assume SID comes from default domain) this can
> > > > lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
> > > > groups which in case of disabled id mapping leads to failure which will
> > > > end request prematurely. 2nd patch should make SID resolution more
> > > > resilient to handle this.
> > >
> > > The only complaint I have about the code is that the domain NULL check is not needed, I actually think we should fail if there is a NULL domain in the provider code, the provider handler should already take care of finding the right domain. If not, we’re in big trouble anyway.
> > >
> > > Otherwise the code solves the problem — I can’t say it looks great. That’s not your fault Pavel, just yet another reminder that we need to work on the LDAP provider refactoring no later than 1.13.
> > >
> > > I tested with both id_provider=ldap and =ad using both POSIX attributes and ID mapping. Both worked fine in my testing.
> > >
> > > Can you re-send the patches without the domain check? Then I’ll ack.
> >
> > Sure, attached patches address your and Lukas' comments.
>
> They indeed do, ACK
> > [snip]
> > > > > > >>> Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > > > >>> to find the primary domain.
> > > > > > >> 
> > > > > > >> I think find_subdomain_by_sid() does what the name says and of course it
> > > > > > >> can return the primary domain as long as the SID of the domain is know
> > > > > >  ^^^^^^
> > > > > > fwiw, this was my concern, the function is named "find_subdomain" yet it
> > > > > > can find both main domain and subdomain. But I won't bikeshed any further.
> > > > >
> > > > > ah, sorry, now I see your point. I agree that the name misleading but I
> > > > > think this can be fixed after the release.
> >
> > Would 's/find_subdomain_by_sid/find_domain_by_sid/' be a sufficient
> > solution?
>
> Yes, but in a separate patch, please.

Sorry Pavel, I pushed your rename patch, because it was simple and
applied on origin/master and didn't realize this one should took
precedence...can you rebase it one last time?

btw it's fine to say patch A depends on patch B, especially if B is
significantly more complex than A ;_)

[Jakub Hrozek]

On Sun, Jul 20, 2014 at 10:05:37PM +0200, Jakub Hrozek wrote:
> Sorry Pavel, I pushed your rename patch

I didn't actually, I was still on review branch. Ack restored.

[Pavel Reichl]

On 07/20/2014 10:05 PM, Jakub Hrozek wrote:
> On Fri, Jul 18, 2014 at 05:10:53PM +0200, Jakub Hrozek wrote:
> > On Thu, Jul 17, 2014 at 05:41:23PM +0200, Pavel Reichl wrote:
> > > On Thu, 2014-07-17 at 07:59 +0200, Jakub Hrozek wrote:
> > > > On 10 Jul 2014, at 16:38, Pavel Reichl  wrote:
> > > > > Hello,
> > > > >
> > > > > please see attached patches.
> > > > >
> > > > > I found out that if we go with approach introduced in previous version
> > > > > (in case of LDAP provider assume SID comes from default domain) this can
> > > > > lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
> > > > > groups which in case of disabled id mapping leads to failure which will
> > > > > end request prematurely. 2nd patch should make SID resolution more
> > > > > resilient to handle this.
> > > >
> > > > The only complaint I have about the code is that the domain NULL check is not needed, I actually think we should fail if there is a NULL domain in the provider code, the provider handler should already take care of finding the right domain. If not, we’re in big trouble anyway.
> > > >
> > > > Otherwise the code solves the problem — I can’t say it looks great. That’s not your fault Pavel, just yet another reminder that we need to work on the LDAP provider refactoring no later than 1.13.
> > > >
> > > > I tested with both id_provider=ldap and =ad using both POSIX attributes and ID mapping. Both worked fine in my testing.
> > > >
> > > > Can you re-send the patches without the domain check? Then I’ll ack.
> > >
> > > Sure, attached patches address your and Lukas' comments.
> >
> > They indeed do, ACK
> > > [snip]
> > > > > > >>>> Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > > > >>>> to find the primary domain.
> > > > > > >>> I think find_subdomain_by_sid() does what the name says and of course it
> > > > > > >>> can return the primary domain as long as the SID of the domain is know
> > > > > > >   ^^^^^^
> > > > > > > fwiw, this was my concern, the function is named "find_subdomain" yet it
> > > > > > > can find both main domain and subdomain. But I won't bikeshed any further.
> > > > > > ah, sorry, now I see your point. I agree that the name misleading but I
> > > > > > think this can be fixed after the release.
> > >
> > > Would 's/find_subdomain_by_sid/find_domain_by_sid/' be a sufficient
> > > solution?
> >
> > Yes, but in a separate patch, please.
>
> Sorry Pavel, I pushed your rename patch, because it was simple and
> applied on origin/master and didn't realize this one should took
> precedence...can you rebase it one last time?
>
> btw it's fine to say patch A depends on patch B, especially if B is
> significantly more complex than A ;_)

Sorry, the patch dependence somehow slipped my mind, I'll try to be more 
careful next time.

[Jakub Hrozek]

On Fri, Jul 18, 2014 at 05:10:53PM +0200, Jakub Hrozek wrote:
> > Sure, attached patches address your and Lukas' comments.
>
> They indeed do, ACK

master:
    * 1614e1b25a98ff2f03648c4bf61d750fb688285a
    * b12e2500237f33c44807d7e5b377ec06007c7252
sssd-1-11:
    * 5001bab712149a27ab37697d487b3f51082df26d
    * deb0cc874606db31f454531c03d381fe0de76bd6

[Lukas Slebodnik]

On (10/07/14 16:38), Pavel Reichl wrote:
> Hello,
>
> please see attached patches.
>
> I found out that if we go with approach introduced in previous version
> (in case of LDAP provider assume SID comes from default domain) this can
> lead to resolutions of SIDs like S-1-5-32-545 and also SIDs of non-posix
> groups which in case of disabled id mapping leads to failure which will
> end request prematurely. 2nd patch should make SID resolution more
> resilient to handle this.
>
> Regards,
> Pavel Reichl
>
> On Tue, 2014-07-08 at 09:57 +0200, Sumit Bose wrote:
> > On Mon, Jul 07, 2014 at 10:25:23PM +0200, Jakub Hrozek wrote:
> > > On Mon, Jul 07, 2014 at 07:03:01PM +0200, Pavel Reichl wrote:
> > > > On Mon, 2014-06-23 at 11:41 +0200, Sumit Bose wrote:
> > > > > On Mon, Jun 23, 2014 at 11:20:52AM +0200, Jakub Hrozek wrote:
> > > > > > On Tue, Jun 17, 2014 at 06:30:31PM +0200, Pavel Reichl wrote:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > please see attached patch.
> > > > > > > 
> > > > > > > Regards,
> > > > > > > 
> > > > > > > PR
> > > > > >
> > > > > > The patch solves the problem, but I think one part should be improved:
> > > > > >
> > > > > > > @@ -875,7 +893,13 @@ static void sdap_ad_tokengroups_initgr_mapping_done(struct tevent_req *subreq)
> > > > > > >          domain = find_subdomain_by_sid(get_domains_head(state->domain), sid);
> > > > > > >          if (domain == NULL) {
> > > > > > >              DEBUG(SSSDBG_MINOR_FAILURE, "Domain not found for SID %s\n", sid);
> > > > > > > -            continue;
> > > > > > > +            if (state->domain->parent == NULL &&
> > > > > > > +                state->domain->subdomains == NULL) {
> > > > > > > +                domain = state->domain;
> > > > > > > +                DEBUG(SSSDBG_TRACE_FUNC, "Using domain %s\n", domain->name);
> > > > > > > +            } else {
> > > > > > > +                continue;
> > > > > > > +            }
> > > > > > >          }
> > > > > >
> > > > > > I think this is a bit dangerous. I wonder if we should have some
> > > > > > modification of find_subdomain_by_sid that would return the first
> > > > > > configured domain if no subdomain provider was configured or if no
> > > > > > domains had a SID. This could be a separate function.
> > > > >
> > > > > This sounds even more dangerous to me.
> > > > > > Anyhow, find_subdomain_by_sid is misnamed, we routinely use the function
> > > > > > to find the primary domain.
> > > > >
> > > > > I think find_subdomain_by_sid() does what the name says and of course it
> > > > > can return the primary domain as long as the SID of the domain is know
> > >
> > > ^^^^^^
> > > fwiw, this was my concern, the function is named "find_subdomain" yet it
> > > can find both main domain and subdomain. But I won't bikeshed any further.
> >
> > ah, sorry, now I see your point. I agree that the name misleading but I
> > think this can be fixed after the release.
>
> Would 's/find_subdomain_by_sid/find_domain_by_sid/' be sufficient
> solution?
> > bye,
> > Sumit
> > > > > which is the case for the IPA and AD provider.
> > > > > What about adding an explicit check if the running id provider is the
> > > > > plain LDAP provider?
> > > >
> > > > Would that be acceptable with you Jakub?
> > >
> > > It's a bit hackish but I don't see any other way.
> > > > > As an alternative the LDAP provider can add a
> > > > > special value in the id member of the sss_dom_info struct and then
> > > > > find_subdomain_by_sid can handle this case specially?
> > > > >
> > > > > bye,
> > > > > Sumit

You will change patch to address Jakub comments. So, you can also change
few nitpicks ;-) I would ignore them if patches were already ACK-ed.
> From 77f41aea31afd642d26057018181394c65d81000 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Tue, 17 Jun 2014 17:16:14 +0100
> Subject: [PATCH 1/2] LDAP: tokengroups do not work with id_provider=ldap
>
> With plain LDAP provider we already have a sdap_handle, so it should be possible
> that in the case where sdom->pvt == NULL sdap_id_op_connect_send() can be
> skipped and sdap_get_ad_tokengroups_send() can be already send with the
> sdap_handle passed to sdap_ad_tokengroups_initgr_mapping_send(). So we should
> only fail if sdom->pvt == NULL and sh == NULL.
>
> if find_subdomain_by_sid() failed we can check if there is only one domain in
> the domain list (state->domain) and in this case continue with this domain since
> the LDAP provider does not know about sub-domains and hence can only have one
> configured domain.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2345

//snip
> +static errno_t handle_missing_pvt(TALLOC_CTX *mem_ctx,
>                              struct tevent_context *ev,
>                              struct sdap_options *opts,
>                              const char *orig_dn,
>                              int timeout,
>                              const char *username,
>                              struct sdap_handle *sh,
>                              struct tevent_req *req,
>                              tevent_req_fn callback)
>
> +{
> struct tevent_req *subreq = NULL;
> errno_t ret;
> if (sh != NULL) {
>    /*  plain LDAP provider already has a sdap_handle */
>    subreq = sdap_get_ad_tokengroups_send(mem_ctx, ev, opts, sh, username,
>                                          orig_dn, timeout);
>    if (subreq == NULL) {
>        ret = ENOMEM;
>        tevent_req_error(req, ENOMEM);

^^^^^^
            We call in this way: tevent_req_error(req, ret);
            There are some exceptions, but it this case you can use it.
>        goto done;
>    }
>    tevent_req_set_callback(subreq, callback, req);
>    ret = EOK;
>    goto done;
> } else {
>    ret = EINVAL;
>    goto done;
> }
>
> +done:
> return ret;
>
> +}
> From 2056ad183feaf41423610f7f1705dad827bb8470 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Thu, 10 Jul 2014 10:48:42 +0100
> Subject: [PATCH 2/2] SDAP: Continue resolving SID even if some fail
>
> Resolving groups obtained via Token-Groups in case of disabled ID mapping may
> lead to failure as non-posix groups are not resolved. This patch amends
> sdap_ad_resolve_sids_done() not to abruptly finish request if ENOENT is
> returned.
>
> Resolves:
> https://fedorahosted.org/sssd/ticket/2345
>
> src/providers/ldap/sdap_async_initgroups_ad.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
> index 1550f6ceab7fe7ad6c4aed3af674407b92802752..f6a45fb70173355010289dfae30b0a898c61f166 100644
> --- a/src/providers/ldap/sdap_async_initgroups_ad.c
> +++ b/src/providers/ldap/sdap_async_initgroups_ad.c
> @@ -646,7 +646,12 @@ static void sdap_ad_resolve_sids_done(struct tevent_req *subreq)
> ret = groups_get_recv(subreq, &dp_error, &sdap_error);
> talloc_zfree(subreq);
> if (ret != EOK || sdap_error != EOK || dp_error != DP_ERR_OK) {
> if (ret == EOK && sdap_error == ENOENT && dp_error == DP_ERR_OK) {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>          "Unable to resolve SID %s - will try next sid.\n",
>          state->current_sid);
> } else if (ret != EOK || (sdap_error != EOK) || dp_error != DP_ERR_OK) {

^^^^^^^^^^^^^^^^^^^
                        Could you explain why you added parentheses just here?
                        They weren't in previous code.
>     DEBUG(SSSDBG_CRIT_FAILURE, "Unable to resolve SID %s [dp_error: %d, "
>           "sdap_error: %d, ret: %d]: %s\n", state->current_sid, dp_error,
>           sdap_error, ret, strerror(ret));

LS

[Pavel Reichl]

On Thu, 2014-07-17 at 09:56 +0200, Lukas Slebodnik wrote:
[snip]
> You will change patch to address Jakub comments.

As you command m'lord. ;-)
> So, you can also change
> few nitpicks ;-) I would ignore them if patches were already ACK-ed.
> > From 77f41aea31afd642d26057018181394c65d81000 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Tue, 17 Jun 2014 17:16:14 +0100
> > Subject: [PATCH 1/2] LDAP: tokengroups do not work with id_provider=ldap
> >
> > With plain LDAP provider we already have a sdap_handle, so it should be possible
> > that in the case where sdom->pvt == NULL sdap_id_op_connect_send() can be
> > skipped and sdap_get_ad_tokengroups_send() can be already send with the
> > sdap_handle passed to sdap_ad_tokengroups_initgr_mapping_send(). So we should
> > only fail if sdom->pvt == NULL and sh == NULL.
> >
> > if find_subdomain_by_sid() failed we can check if there is only one domain in
> > the domain list (state->domain) and in this case continue with this domain since
> > the LDAP provider does not know about sub-domains and hence can only have one
> > configured domain.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2345
>
> //snip
> > +static errno_t handle_missing_pvt(TALLOC_CTX *mem_ctx,
> >                              struct tevent_context *ev,
> >                              struct sdap_options *opts,
> >                              const char *orig_dn,
> >                              int timeout,
> >                              const char *username,
> >                              struct sdap_handle *sh,
> >                              struct tevent_req *req,
> >                              tevent_req_fn callback)
> >
> > +{
> > struct tevent_req *subreq = NULL;
> > errno_t ret;
> > if (sh != NULL) {
> >    /*  plain LDAP provider already has a sdap_handle */
> >    subreq = sdap_get_ad_tokengroups_send(mem_ctx, ev, opts, sh, username,
> >                                          orig_dn, timeout);
> >    if (subreq == NULL) {
> >        ret = ENOMEM;
> >        tevent_req_error(req, ENOMEM);
>                                 ^^^^^^
>         We call in this way: tevent_req_error(req, ret);
>         There are some exceptions, but it this case you can use it.

Fixed.
> >        goto done;
> >    }
> >    tevent_req_set_callback(subreq, callback, req);
> >    ret = EOK;
> >    goto done;
> > } else {
> >    ret = EINVAL;
> >    goto done;
> > }
> >
> > +done:
> > return ret;
> >
> > +}
> > From 2056ad183feaf41423610f7f1705dad827bb8470 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Thu, 10 Jul 2014 10:48:42 +0100
> > Subject: [PATCH 2/2] SDAP: Continue resolving SID even if some fail
> >
> > Resolving groups obtained via Token-Groups in case of disabled ID mapping may
> > lead to failure as non-posix groups are not resolved. This patch amends
> > sdap_ad_resolve_sids_done() not to abruptly finish request if ENOENT is
> > returned.
> >
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2345
> >
> > src/providers/ldap/sdap_async_initgroups_ad.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
> > index 1550f6ceab7fe7ad6c4aed3af674407b92802752..f6a45fb70173355010289dfae30b0a898c61f166 100644
> > --- a/src/providers/ldap/sdap_async_initgroups_ad.c
> > +++ b/src/providers/ldap/sdap_async_initgroups_ad.c
> > @@ -646,7 +646,12 @@ static void sdap_ad_resolve_sids_done(struct tevent_req *subreq)
> > ret = groups_get_recv(subreq, &dp_error, &sdap_error);
> > talloc_zfree(subreq);
> > if (ret != EOK || sdap_error != EOK || dp_error != DP_ERR_OK) {
> > if (ret == EOK && sdap_error == ENOENT && dp_error == DP_ERR_OK) {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >          "Unable to resolve SID %s - will try next sid.\n",
> >          state->current_sid);
> > } else if (ret != EOK || (sdap_error != EOK) || dp_error != DP_ERR_OK) {
>                            ^^^^^^^^^^^^^^^^^^^
>                     Could you explain why you added parentheses just here?
>                     They weren't in previous code.

Sorry, I can't explain. Fixed in patch sent today.
> >     DEBUG(SSSDBG_CRIT_FAILURE, "Unable to resolve SID %s [dp_error: %d, "
> >           "sdap_error: %d, ret: %d]: %s\n", state->current_sid, dp_error,
> >           sdap_error, ret, strerror(ret));
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel