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>
> > 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!