[PATCH] winbindd: error handling in rpc_try_lookup_sids3()
Ralph Böhme
slow at samba.org
Fri Apr 7 17:53:25 UTC 2017
On Wed, Apr 05, 2017 at 07:47:02PM +0200, Ralph Böhme wrote:
> On Tue, Apr 04, 2017 at 02:13:37PM +0200, Stefan Metzmacher wrote:
> > Am 04.04.2017 um 12:50 schrieb Ralph Böhme via samba-technical:
> > > On Mon, Apr 03, 2017 at 11:49:36AM -0700, Jeremy Allison wrote:
> > >> Ralph, trying to push and I'm getting:
> > >>
> > >> [655(3442)/2083 at 1h12s] idmap.alloc(ad_member_rfc2307)
> > >> Domain SAMBADOMAIN has SID S-1-5-21-3583314470-906957706-621714972
> > >> id: 66666: no such user
> > >> failed to call wbcStringToSid: WBC_ERR_INVALID_SID
> > >> Could not lookup sid S-1-5-21-3583314470-906957706-621714972\66666
> > >> Using non-existing SID S-1-5-21-3583314470-906957706-621714972-66666 to check no id allocation is done by the backend
> > >> wbinfo returned: S-1-5-21-3583314470-906957706-621714972-66666 -> uid/gid 1166666
> > >> UNEXPECTED(failure): idmap.alloc.wbinfo SID to xid returns unmapped for unknown SID(ad_member_rfc2307)
> > >> REASON: Exception: Exception:
> > >>
> > >> Can you take a look (sorry).
> > >
> > > sorry for the hassle! Was only running make test TESTS=wbinfo on the last
> > > incarnation as I already have full private autobuilds running with additional
> > > sids2xids patches on top.
> > >
> > > The good thing is, that this actually found another hidden small bug. Patch 4/5
> > > fixes it.
> >
> > Please don't push this, it's the wrong fix.
>
> Well, I'd still argue it's the correct fix, just for a different problem, but
> anyway. :)
>
> I'll resend this patchset once the below patchset, which is for a different bug,
> is in master.
it is, so I'm adding the current version of this patchset. It just passed a
private autobuild.
> > The fundamental problem is that I didn't understood that
> > find_lookup_domain_from_sid() always returns the primary domain
> > for all remote domains when we discussed commit
> > 9be918116e356c358ef77cc2933e471090088293.
> >
> > We need to use find_domain_from_sid_noinit() to get a possible fallback
> > for the domain name if wb_lookupsid_recv() fails.
> > state->single_domains[state->single_sids_done] is most likely wrong.
>
> yup, and for that the proper fix is attached.
>
> Original bug:
> <https://bugzilla.samba.org/show_bug.cgi?id=11961>
>
> Attempted fix: 9be918116e356c358ef77cc2933e471090088293
>
> Another user found it doesn't work with a different idmap setup:
> <https://bugzilla.samba.org/show_bug.cgi?id=12597>
>
> (Hopefully) correct patch, including tests, attached. It did pass a private
> autobuild.
This is in master.
Cheerio!
-slow
-------------- next part --------------
From 3440645d2c2f09d41faa92d978f3791c08f99dd9 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 1 Apr 2017 16:44:45 +0200
Subject: [PATCH 1/4] s3/include: add NT_STATUS_LOOKUP_ERR
Useful helper macro to check the return value of LSA and SAMR
translations.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/include/lsa.h | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/source3/include/lsa.h b/source3/include/lsa.h
index 7681aed..c23e942 100644
--- a/source3/include/lsa.h
+++ b/source3/include/lsa.h
@@ -22,4 +22,8 @@ int init_lsa_ref_domain_list(TALLOC_CTX *mem_ctx,
const char *dom_name,
struct dom_sid *dom_sid);
+#define NT_STATUS_LOOKUP_ERR(status) \
+ (!NT_STATUS_IS_OK(status) && \
+ !NT_STATUS_EQUAL(status, STATUS_SOME_UNMAPPED) && \
+ !NT_STATUS_EQUAL(status, NT_STATUS_NONE_MAPPED))
#endif
--
2.9.3
From 4eba91a05b57550ea43409352289406abb68a4dc Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 1 Apr 2017 16:56:39 +0200
Subject: [PATCH 2/4] s3/rpc_client: use NT_STATUS_LOOKUP_ERR
No change in behaviour.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/rpc_client/cli_lsarpc.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/source3/rpc_client/cli_lsarpc.c b/source3/rpc_client/cli_lsarpc.c
index 518489a..70952f6 100644
--- a/source3/rpc_client/cli_lsarpc.c
+++ b/source3/rpc_client/cli_lsarpc.c
@@ -256,10 +256,7 @@ static NTSTATUS dcerpc_lsa_lookup_sids_noalloc(struct dcerpc_binding_handle *h,
return status;
}
- if (!NT_STATUS_IS_OK(result) &&
- !NT_STATUS_EQUAL(result, NT_STATUS_NONE_MAPPED) &&
- !NT_STATUS_EQUAL(result, STATUS_SOME_UNMAPPED))
- {
+ if (NT_STATUS_LOOKUP_ERR(result)) {
*presult = result;
return status;
}
--
2.9.3
From 879d521a978a50bb4a25b1c855ec2038e11c905d Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sat, 1 Apr 2017 16:51:07 +0200
Subject: [PATCH 3/4] s3/rpc_client: lookupsids error handling of
NT_STATUS_NONE_MAPPED
NT_STATUS_NONE_MAPPED is not a fatal error, it just means we must return
all lsa_TranslatedName's as type SID_NAME_UNKNOWN.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/rpc_client/cli_lsarpc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/source3/rpc_client/cli_lsarpc.c b/source3/rpc_client/cli_lsarpc.c
index 70952f6..41c1ef4 100644
--- a/source3/rpc_client/cli_lsarpc.c
+++ b/source3/rpc_client/cli_lsarpc.c
@@ -28,6 +28,7 @@
#include "rpc_client/cli_lsarpc.h"
#include "rpc_client/init_lsa.h"
#include "../libcli/security/security.h"
+#include "lsa.h"
/** @defgroup lsa LSA - Local Security Architecture
* @ingroup rpc_client
@@ -221,7 +222,7 @@ static NTSTATUS dcerpc_lsa_lookup_sids_noalloc(struct dcerpc_binding_handle *h,
return status;
}
- if(!NT_STATUS_IS_ERR(result)) {
+ if (!NT_STATUS_LOOKUP_ERR(result)) {
lsa_names.count = lsa_names2.count;
lsa_names.names = talloc_array(mem_ctx,
struct lsa_TranslatedName,
--
2.9.3
From 5f370f95284eadcd0bf249e0364a0897866e8e80 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 26 Mar 2017 08:22:13 +0200
Subject: [PATCH 4/4] winbindd: error handling in rpc_lookup_sids()
NT_STATUS_NONE_MAPPED and NT_STATUS_SOME_NOT_MAPPED should not be
treated as fatal error. We should continue processing the results and
not bail out.
In case we got NT_STATUS_NONE_MAPPED we must have to ensure all
lsa_TranslatedName are of type SID_NAME_UNKNOWN.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=12728
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/winbindd_rpc.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/source3/winbindd/winbindd_rpc.c b/source3/winbindd/winbindd_rpc.c
index 3dd4f77..0023e2a 100644
--- a/source3/winbindd/winbindd_rpc.c
+++ b/source3/winbindd/winbindd_rpc.c
@@ -32,6 +32,7 @@
#include "rpc_client/cli_samr.h"
#include "rpc_client/cli_lsarpc.h"
#include "../libcli/security/security.h"
+#include "lsa.h"
/* Query display info for a domain */
NTSTATUS rpc_query_user_list(TALLOC_CTX *mem_ctx,
@@ -981,7 +982,7 @@ static NTSTATUS rpc_try_lookup_sids3(TALLOC_CTX *mem_ctx,
if (!NT_STATUS_IS_OK(status)) {
return status;
}
- if (NT_STATUS_IS_ERR(result)) {
+ if (NT_STATUS_LOOKUP_ERR(result)) {
return result;
}
if (sids->num_sids != lsa_names2.count) {
@@ -1010,7 +1011,7 @@ static NTSTATUS rpc_try_lookup_sids3(TALLOC_CTX *mem_ctx,
return NT_STATUS_INVALID_NETWORK_RESPONSE;
}
}
- return result;
+ return NT_STATUS_OK;
}
NTSTATUS rpc_lookup_sids(TALLOC_CTX *mem_ctx,
@@ -1043,7 +1044,7 @@ NTSTATUS rpc_lookup_sids(TALLOC_CTX *mem_ctx,
if (!NT_STATUS_IS_OK(status)) {
return status;
}
- if (NT_STATUS_IS_ERR(result)) {
+ if (NT_STATUS_LOOKUP_ERR(result)) {
return result;
}
@@ -1063,5 +1064,5 @@ NTSTATUS rpc_lookup_sids(TALLOC_CTX *mem_ctx,
}
}
- return result;
+ return NT_STATUS_OK;
}
--
2.9.3
More information about the samba-technical
mailing list