[PATCH] IPA: do not fail if view name lookup failed on older versions

[Sumit Bose]

Hi,

this patch fixes an issue seen when newer idview-aware SSSD clients try
to connect to older IPA server. As mentioned in the commit message it is
due to different error codes returned by different versions of 389ds.
This issue only becomes important when the old IPA server has a trust to
AD because the issue prevents SSSD from reading the SID of the IPA
domain. Without trust the issue can be verified by checking the logs.
ipa_get_view_name_done() should fail with the message "get_view_name
request failed, looks like server does not support views." and continue
to read data about the IPA domain. Without this path you should see
"get_view_name request failed." and the whole request should be
canceled.

bye,
Sumit

[Sumit Bose]

On Mon, May 11, 2015 at 12:01:12PM +0200, Sumit Bose wrote:
>  Hi, 
and now with patch ...

>  
>  this patch fixes an issue seen when newer idview-aware SSSD clients try
>  to connect to older IPA server. As mentioned in the commit message it is
>  due to different error codes returned by different versions of 389ds.
>  This issue only becomes important when the old IPA server has a trust to
>  AD because the issue prevents SSSD from reading the SID of the IPA
>  domain. Without trust the issue can be verified by checking the logs.
>  ipa_get_view_name_done() should fail with the message "get_view_name
>  request failed, looks like server does not support views." and continue
>  to read data about the IPA domain. Without this path you should see
>  "get_view_name request failed." and the whole request should be
>  canceled.
>  
>  bye,
>  Sumit

[Jakub Hrozek]

On Mon, May 11, 2015 at 12:01:52PM +0200, Sumit Bose wrote:
>  On Mon, May 11, 2015 at 12:01:12PM +0200, Sumit Bose wrote:
>  > Hi,
>  
>  and now with patch ...
>  
>  > 
>  > this patch fixes an issue seen when newer idview-aware SSSD clients try
>  > to connect to older IPA server. As mentioned in the commit message it is
>  > due to different error codes returned by different versions of 389ds.
>  > This issue only becomes important when the old IPA server has a trust to
>  > AD because the issue prevents SSSD from reading the SID of the IPA
>  > domain. Without trust the issue can be verified by checking the logs.
>  > ipa_get_view_name_done() should fail with the message "get_view_name
>  > request failed, looks like server does not support views." and continue
>  > to read data about the IPA domain. Without this path you should see
>  > "get_view_name request failed." and the whole request should be
>  > canceled.
>  > 
>  > bye,
>  > Sumit 
>  From 100862c31f92c644868053fe576e4acfcd34a420 Mon Sep 17 00:00:00
> 2001
>  From: Sumit Bose <sbose(a)redhat.com&gt;
>  Date: Thu, 7 May 2015 11:16:54 +0200
>  Subject: [PATCH] IPA: do not fail if view name lookup failed on older versions
>  
>  Depending on the version 389ds return a different error code if the
>  search for the view name failed because our dereference attribute
>  ipaAssignedIDView is not known. Newer version return
>  LDAP_UNAVAILABLE_CRITICAL_EXTENSION(12) which is translated to
>  EOPNOTSUPP and older versions return LDAP_PROTOCOL_ERROR(2) which is
>  returned as EIO. In both cases we have to assume that the server is not
>  view aware and keep the view name unset.
>  
>  Resolves https://fedorahosted.org/sssd/ticket/2650
>  ---
>   src/providers/ipa/ipa_subdomains.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>  
>  diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
>  index 20657ebb556f75108f64faecc6ad1ccc1d7e26fd..f863b806c8fed01f498d18b953832ebeba7a85d3
> 100644
>  --- a/src/providers/ipa/ipa_subdomains.c
>  +++ b/src/providers/ipa/ipa_subdomains.c
>  @@ -1034,7 +1034,14 @@ static void ipa_get_view_name_done(struct tevent_req *req)
>       ret = sdap_deref_search_with_filter_recv(req, ctx, &reply_count, &reply);
>       talloc_zfree(req);
>       if (ret != EOK) {
>  -        if (ret == EOPNOTSUPP) {
>  +        /* Depending on the version 389ds return a different error code if the
>  +         * search for the view name failed because our dereference attribute
>  +         * ipaAssignedIDView is not known. Newer version return
>  +         * LDAP_UNAVAILABLE_CRITICAL_EXTENSION(12) which is translated to
>  +         * EOPNOTSUPP and older versions return LDAP_PROTOCOL_ERROR(2) which
>  +         * is returned as EIO. In both cases we have to assume that the server
>  +         * is not view aware and keep the view name unset. */
>  +        if (ret == EOPNOTSUPP || ret == EIO) { 
Hmm, EIO is quite a generic error, do you know where is it emitted from?
Could we convert that place to a sssd-specific error to avoid shadowing
a legitimate error?

>               DEBUG(SSSDBG_TRACE_FUNC, "get_view_name request
> failed, looks " \
>                                        "like server does not support
> views.\n");
>               ret = ipa_check_master(ctx);
>  -- 
>  2.1.0
>   
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Mon, May 11, 2015 at 07:13:17PM +0200, Jakub Hrozek wrote:
>  On Mon, May 11, 2015 at 12:01:52PM +0200, Sumit Bose wrote:
>  > On Mon, May 11, 2015 at 12:01:12PM +0200, Sumit Bose wrote:
>  > > Hi,
>  > 
>  > and now with patch ...
>  > 
>  > > 
>  > > this patch fixes an issue seen when newer idview-aware SSSD clients try
>  > > to connect to older IPA server. As mentioned in the commit message it is
>  > > due to different error codes returned by different versions of 389ds.
>  > > This issue only becomes important when the old IPA server has a trust to
>  > > AD because the issue prevents SSSD from reading the SID of the IPA
>  > > domain. Without trust the issue can be verified by checking the logs.
>  > > ipa_get_view_name_done() should fail with the message "get_view_name
>  > > request failed, looks like server does not support views." and continue
>  > > to read data about the IPA domain. Without this path you should see
>  > > "get_view_name request failed." and the whole request should be
>  > > canceled.
>  > > 
>  > > bye,
>  > > Sumit
>  
>  > From 100862c31f92c644868053fe576e4acfcd34a420 Mon Sep 17 00:00:00 2001
>  > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > Date: Thu, 7 May 2015 11:16:54 +0200
>  > Subject: [PATCH] IPA: do not fail if view name lookup failed on older versions
>  > 
>  > Depending on the version 389ds return a different error code if the
>  > search for the view name failed because our dereference attribute
>  > ipaAssignedIDView is not known. Newer version return
>  > LDAP_UNAVAILABLE_CRITICAL_EXTENSION(12) which is translated to
>  > EOPNOTSUPP and older versions return LDAP_PROTOCOL_ERROR(2) which is
>  > returned as EIO. In both cases we have to assume that the server is not
>  > view aware and keep the view name unset.
>  > 
>  > Resolves https://fedorahosted.org/sssd/ticket/2650
>  > ---
>  >  src/providers/ipa/ipa_subdomains.c | 9 ++++++++-
>  >  1 file changed, 8 insertions(+), 1 deletion(-)
>  > 
>  > diff --git a/src/providers/ipa/ipa_subdomains.c
> b/src/providers/ipa/ipa_subdomains.c
>  > index
> 20657ebb556f75108f64faecc6ad1ccc1d7e26fd..f863b806c8fed01f498d18b953832ebeba7a85d3 100644
>  > --- a/src/providers/ipa/ipa_subdomains.c
>  > +++ b/src/providers/ipa/ipa_subdomains.c
>  > @@ -1034,7 +1034,14 @@ static void ipa_get_view_name_done(struct tevent_req *req)
>  >      ret = sdap_deref_search_with_filter_recv(req, ctx, &reply_count,
> &reply);
>  >      talloc_zfree(req);
>  >      if (ret != EOK) {
>  > -        if (ret == EOPNOTSUPP) {
>  > +        /* Depending on the version 389ds return a different error code if the
>  > +         * search for the view name failed because our dereference attribute
>  > +         * ipaAssignedIDView is not known. Newer version return
>  > +         * LDAP_UNAVAILABLE_CRITICAL_EXTENSION(12) which is translated to
>  > +         * EOPNOTSUPP and older versions return LDAP_PROTOCOL_ERROR(2) which
>  > +         * is returned as EIO. In both cases we have to assume that the server
>  > +         * is not view aware and keep the view name unset. */
>  > +        if (ret == EOPNOTSUPP || ret == EIO) {
>  
>  Hmm, EIO is quite a generic error, do you know where is it emitted from?
>  Could we convert that place to a sssd-specific error to avoid shadowing
>  a legitimate error? 
It comes from sdap_get_generic_op_finished(). I was thinking about
adding special handling for LDAP_PROTOCOL_ERROR here, but I thought it
might be too risky because it might break some other code which expects
EIO. 

After you comment I did a "grep -r '== EIO' ../src/providers/*/*" and
found that there is only one other chekc for EIO and this is related to
matching rules which are explicitly covered in
sdap_get_generic_op_finished(). So I now think it is safe to add a
special error code for this (and I will open a ticket to fix the code
for the matching rule error as well :-).

bye,
Sumit

>  
>  >              DEBUG(SSSDBG_TRACE_FUNC, "get_view_name request failed, looks
> " \
>  >                                       "like server does not support
> views.\n");
>  >              ret = ipa_check_master(ctx);
>  > -- 
>  > 2.1.0
>  > 
>  
>  > _______________________________________________
>  > sssd-devel mailing list
>  > sssd-devel(a)lists.fedorahosted.org
>  > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>  
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Mon, May 11, 2015 at 08:14:47PM +0200, Sumit Bose wrote:
>  On Mon, May 11, 2015 at 07:13:17PM +0200, Jakub Hrozek wrote:
>  > On Mon, May 11, 2015 at 12:01:52PM +0200, Sumit Bose wrote:
>  > > On Mon, May 11, 2015 at 12:01:12PM +0200, Sumit Bose wrote:
>  > > > Hi,
>  > > 
>  > > and now with patch ...
>  > > 
>  > > > 
>  > > > this patch fixes an issue seen when newer idview-aware SSSD clients try
>  > > > to connect to older IPA server. As mentioned in the commit message it is
>  > > > due to different error codes returned by different versions of 389ds.
>  > > > This issue only becomes important when the old IPA server has a trust to
>  > > > AD because the issue prevents SSSD from reading the SID of the IPA
>  > > > domain. Without trust the issue can be verified by checking the logs.
>  > > > ipa_get_view_name_done() should fail with the message "get_view_name
>  > > > request failed, looks like server does not support views." and
> continue
>  > > > to read data about the IPA domain. Without this path you should see
>  > > > "get_view_name request failed." and the whole request should be
>  > > > canceled.
>  > > > 
>  > > > bye,
>  > > > Sumit
>  > 
>  > > From 100862c31f92c644868053fe576e4acfcd34a420 Mon Sep 17 00:00:00 2001
>  > > From: Sumit Bose <sbose(a)redhat.com&gt;
>  > > Date: Thu, 7 May 2015 11:16:54 +0200
>  > > Subject: [PATCH] IPA: do not fail if view name lookup failed on older versions
>  > > 
>  > > Depending on the version 389ds return a different error code if the
>  > > search for the view name failed because our dereference attribute
>  > > ipaAssignedIDView is not known. Newer version return
>  > > LDAP_UNAVAILABLE_CRITICAL_EXTENSION(12) which is translated to
>  > > EOPNOTSUPP and older versions return LDAP_PROTOCOL_ERROR(2) which is
>  > > returned as EIO. In both cases we have to assume that the server is not
>  > > view aware and keep the view name unset.
>  > > 
>  > > Resolves https://fedorahosted.org/sssd/ticket/2650
>  > > ---
>  > >  src/providers/ipa/ipa_subdomains.c | 9 ++++++++-
>  > >  1 file changed, 8 insertions(+), 1 deletion(-)
>  > > 
>  > > diff --git a/src/providers/ipa/ipa_subdomains.c
> b/src/providers/ipa/ipa_subdomains.c
>  > > index
> 20657ebb556f75108f64faecc6ad1ccc1d7e26fd..f863b806c8fed01f498d18b953832ebeba7a85d3 100644
>  > > --- a/src/providers/ipa/ipa_subdomains.c
>  > > +++ b/src/providers/ipa/ipa_subdomains.c
>  > > @@ -1034,7 +1034,14 @@ static void ipa_get_view_name_done(struct tevent_req
> *req)
>  > >      ret = sdap_deref_search_with_filter_recv(req, ctx, &reply_count,
> &reply);
>  > >      talloc_zfree(req);
>  > >      if (ret != EOK) {
>  > > -        if (ret == EOPNOTSUPP) {
>  > > +        /* Depending on the version 389ds return a different error code if
> the
>  > > +         * search for the view name failed because our dereference attribute
>  > > +         * ipaAssignedIDView is not known. Newer version return
>  > > +         * LDAP_UNAVAILABLE_CRITICAL_EXTENSION(12) which is translated to
>  > > +         * EOPNOTSUPP and older versions return LDAP_PROTOCOL_ERROR(2) which
>  > > +         * is returned as EIO. In both cases we have to assume that the
> server
>  > > +         * is not view aware and keep the view name unset. */
>  > > +        if (ret == EOPNOTSUPP || ret == EIO) {
>  > 
>  > Hmm, EIO is quite a generic error, do you know where is it emitted from?
>  > Could we convert that place to a sssd-specific error to avoid shadowing
>  > a legitimate error?
>  
>  It comes from sdap_get_generic_op_finished(). I was thinking about
>  adding special handling for LDAP_PROTOCOL_ERROR here, but I thought it
>  might be too risky because it might break some other code which expects
>  EIO. 
>  
>  After you comment I did a "grep -r '== EIO' ../src/providers/*/*" and
>  found that there is only one other chekc for EIO and this is related to
>  matching rules which are explicitly covered in
>  sdap_get_generic_op_finished(). So I now think it is safe to add a
>  special error code for this (and I will open a ticket to fix the code
>  for the matching rule error as well :-). 
Thank you for this thorough check, I guess I could have done it as a
part of the patch review myself.

With that covered -- ACK!

[Jakub Hrozek]

On Tue, May 12, 2015 at 09:55:53AM +0200, Jakub Hrozek wrote:
>  > It comes from sdap_get_generic_op_finished(). I was thinking
> about
>  > adding special handling for LDAP_PROTOCOL_ERROR here, but I thought it
>  > might be too risky because it might break some other code which expects
>  > EIO. 
>  > 
>  > After you comment I did a "grep -r '== EIO' ../src/providers/*/*"
> and
>  > found that there is only one other chekc for EIO and this is related to
>  > matching rules which are explicitly covered in
>  > sdap_get_generic_op_finished(). So I now think it is safe to add a
>  > special error code for this (and I will open a ticket to fix the code
>  > for the matching rule error as well :-).
>  
>  Thank you for this thorough check, I guess I could have done it as a
>  part of the patch review myself.
>  
>  With that covered -- ACK! 
* master: a50b229c8ea1e22c9efa677760b94d8c48c3ec89
* sssd-1-12: 0f85298a31beb53375635b82cb274d29eae45774