[PATCH] Simplify wb_lookupsids
Ralph Böhme
slow at samba.org
Mon Apr 10 16:14:37 UTC 2017
Hi!
Attached is a patch that simplifies wb_lookupsids by
o removing the fallback logic caused every failed lookup to be retried as a
single lookup, and
o fixing the handling of lookups that failed (because the SID was from an unknown
domain) which let us construct a fake lsa_DomainInfo with domain name ""
(empty string)
The former was mostly need to trigger the latter, the latter was needed as
callers like wb_sids2xids we're not dealing with the failed lookups for SIDs
from unknown domains correctly (sid_index in the translated name will is
UINT32_MAX in the response).
Please review & push if happy. Patchset passes (private) autobuild.
Oh, wait! These patches might become prequisite patches for changes dealing with
SID-history Uri's is working on, so we might need to assing a bugnumber to this
patchset so we can backport.
Cheerio!
-slow
-------------- next part --------------
From 314d23f41ebdb7c21abc1603aa214361c6b57e7e Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 4 Apr 2017 14:51:09 +0200
Subject: [PATCH 1/6] winbindd: handling of SIDs without domain reference in
wb_sids2xids_lookupsids_done()
This lets wb_sids2xids_lookupsids_done() deal with wp_lookupsids
returning UINT32_MAX as domain index for SIDs from unknown domains.
Call find_domain_from_sid_noinit() to search our list of known
domains. If a matching domain is found, use it's name, otherwise use the
empty string "". This needed to handle Samba DCs which always returns
sid_index UINT32_MAX for unknown SIDs, even from known domains.
Currently the wb_lookupsids adds these fake domains with an empty string
as domain name, but that's not the correct place to do it. We need the
domain name as it gets passed to the idmap child where the choise of
idmap backend is based on the domain name. This will possibly be changed
in the future to be based on domain SIDs, not the name.
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_sids2xids.c | 33 +++++++++++++++++++++++++++------
1 file changed, 27 insertions(+), 6 deletions(-)
diff --git a/source3/winbindd/wb_sids2xids.c b/source3/winbindd/wb_sids2xids.c
index 9bb8fa8..dc90bdf 100644
--- a/source3/winbindd/wb_sids2xids.c
+++ b/source3/winbindd/wb_sids2xids.c
@@ -185,20 +185,41 @@ static void wb_sids2xids_lookupsids_done(struct tevent_req *subreq)
}
for (i=0; i<state->num_non_cached; i++) {
+ const struct dom_sid *sid = &state->non_cached[i];
struct dom_sid dom_sid;
- struct lsa_DomainInfo *info;
struct lsa_TranslatedName *n = &names->names[i];
struct wbint_TransID *t = &state->ids.ids[i];
int domain_index;
+ const char *domain_name = NULL;
- sid_copy(&dom_sid, &state->non_cached[i]);
- sid_split_rid(&dom_sid, &t->rid);
+ if (n->sid_index != UINT32_MAX) {
+ const struct lsa_DomainInfo *info;
- info = &domains->domains[n->sid_index];
- t->type = lsa_SidType_to_id_type(n->sid_type);
+ info = &domains->domains[n->sid_index];
+ domain_name = info->name.string;
+ }
+ if (domain_name == NULL) {
+ struct winbindd_domain *wb_domain = NULL;
+
+ /*
+ * This is needed to handle Samba DCs
+ * which always return sid_index == UINT32_MAX for
+ * unknown sids.
+ */
+ wb_domain = find_domain_from_sid_noinit(sid);
+ if (wb_domain != NULL) {
+ domain_name = wb_domain->name;
+ }
+ }
+ if (domain_name == NULL) {
+ domain_name = "";
+ }
+ sid_copy(&dom_sid, sid);
+ sid_split_rid(&dom_sid, &t->rid);
+ t->type = lsa_SidType_to_id_type(n->sid_type);
domain_index = init_lsa_ref_domain_list(
- state, &state->idmap_doms, info->name.string, &dom_sid);
+ state, &state->idmap_doms, domain_name, &dom_sid);
if (domain_index == -1) {
tevent_req_oom(req);
return;
--
2.9.3
From 2dbb4cc26ffa861def3698b233072b4b5e334ec8 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 26 Mar 2017 08:34:59 +0200
Subject: [PATCH 2/6] winbindd: let wb_lookupsids_move_name() handle
domain_index UINT32_MAX
If the SID was in an unknown domain, src_name->sid_index will be
UINT32_MAX.
This change allows wb_lookupsids_move_name() to add such names to the
result set. This is not used for now, but will be used in subsequent
commits.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_lookupsids.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index c13bd5b..d7f8820 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -432,17 +432,23 @@ static bool wb_lookupsids_move_name(struct lsa_RefDomainList *src_domains,
{
struct lsa_TranslatedName *dst_name;
struct lsa_DomainInfo *src_domain;
- uint32_t src_domain_index, dst_domain_index;
+ uint32_t src_domain_index;
+ uint32_t dst_domain_index = UINT32_MAX;
+ bool ok;
src_domain_index = src_name->sid_index;
- if (src_domain_index >= src_domains->count) {
- return false;
- }
- src_domain = &src_domains->domains[src_domain_index];
+ if ((src_domain_index != UINT32_MAX) && (src_domains != NULL)) {
+ if (src_domain_index >= src_domains->count) {
+ return false;
+ }
+ src_domain = &src_domains->domains[src_domain_index];
- if (!wb_lookupsids_find_dom_idx(
- src_domain, dst_domains, &dst_domain_index)) {
- return false;
+ ok = wb_lookupsids_find_dom_idx(src_domain,
+ dst_domains,
+ &dst_domain_index);
+ if (!ok) {
+ return false;
+ }
}
dst_name = &dst_names->names[dst_name_index];
--
2.9.3
From 4f87f044a62bd871ecd5e324570c29ffec433495 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 24 Mar 2017 17:06:38 +0100
Subject: [PATCH 3/6] winbindd: handling of failed lookupsids in
wb_lookupsids_single_done()
If lookupsid() failed with NT_STATUS_SOME_NOT_MAPPED or
NT_STATUS_NONE_MAPPED, if we didn't get a domain name, don't add a fake
domain to the lsa_RefDomainList. Just set the domain index in the
translated name to UINT32_MAX.
It's up to callers like wb_sids2xids to handle such failed mappings and
wb_sids2xids_lookupsids_done() has been updated in a previous commit to
deal with it.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_lookupsids.c | 82 +++++++++++++++++-----------------------
1 file changed, 35 insertions(+), 47 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index d7f8820..dbb24fd 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -23,6 +23,7 @@
#include "librpc/gen_ndr/ndr_winbind_c.h"
#include "../libcli/security/security.h"
#include "passdb/machine_sid.h"
+#include "lsa.h"
struct wb_lookupsids_domain {
struct winbindd_domain *domain;
@@ -537,7 +538,8 @@ static void wb_lookupsids_single_done(struct tevent_req *subreq)
subreq, struct tevent_req);
struct wb_lookupsids_state *state = tevent_req_data(
req, struct wb_lookupsids_state);
- const char *domain_name, *name;
+ const char *domain_name = NULL;
+ const char *name = NULL;
enum lsa_SidType type;
uint32_t res_sid_index;
uint32_t src_rid;
@@ -545,67 +547,53 @@ static void wb_lookupsids_single_done(struct tevent_req *subreq)
struct dom_sid src_domain_sid;
struct lsa_DomainInfo src_domain;
struct lsa_RefDomainList src_domains;
+ struct lsa_RefDomainList *psrc_domains = NULL;
struct lsa_TranslatedName src_name;
+ uint32_t domain_idx = UINT32_MAX;
NTSTATUS status;
+ bool ok;
status = wb_lookupsid_recv(subreq, talloc_tos(), &type,
&domain_name, &name);
TALLOC_FREE(subreq);
- if (!NT_STATUS_IS_OK(status)) {
- struct winbindd_domain *wb_domain = NULL;
- const char *tmpname;
-
- type = SID_NAME_UNKNOWN;
-
- res_sid_index = state->single_sids[state->single_sids_done];
- wb_domain = find_domain_from_sid_noinit(&state->sids[res_sid_index]);
- if (wb_domain != NULL) {
- /*
- * If the lookupsid failed because the rid not
- * found in a domain and we have a reference
- * to the lookup domain, use the name from
- * there.
- *
- * Callers like sid2xid will use the domain
- * name in the idmap backend to figure out
- * which domain to use in processing.
- */
- tmpname = wb_domain->name;
- } else {
- tmpname = "";
- }
- domain_name = talloc_strdup(talloc_tos(), tmpname);
- if (tevent_req_nomem(domain_name, req)) {
- return;
- }
- name = talloc_strdup(talloc_tos(), "");
- if (tevent_req_nomem(name, req)) {
- return;
- }
+ if (NT_STATUS_LOOKUP_ERR(status)) {
+ tevent_req_nterror(req, status);
+ return;
}
- /*
- * Fake up structs for wb_lookupsids_move_name
- */
res_sid_index = state->single_sids[state->single_sids_done];
- sid_copy(&src_domain_sid, &state->sids[res_sid_index]);
- sid_split_rid(&src_domain_sid, &src_rid);
- src_domain.name.string = domain_name;
- src_domain.sid = &src_domain_sid;
+ if ((domain_name != NULL) && (domain_name[0] != '\0')) {
+ /*
+ * Build structs with the domain name for
+ * wb_lookupsids_move_name(). If we didn't get a name, we will
+ * pass NULL and UINT32_MAX.
+ */
- src_domains.count = 1;
- src_domains.domains = &src_domain;
+ sid_copy(&src_domain_sid, &state->sids[res_sid_index]);
+ sid_split_rid(&src_domain_sid, &src_rid);
+
+ src_domain.name.string = domain_name;
+ src_domain.sid = &src_domain_sid;
+
+ src_domains.count = 1;
+ src_domains.domains = &src_domain;
+ psrc_domains = &src_domains;
+
+ domain_idx = 0;
+ }
src_name.sid_type = type;
src_name.name.string = name;
- src_name.sid_index = 0;
-
- if (!wb_lookupsids_move_name(
- &src_domains, &src_name,
- state->res_domains, state->res_names,
- res_sid_index)) {
+ src_name.sid_index = domain_idx;
+
+ ok = wb_lookupsids_move_name(psrc_domains,
+ &src_name,
+ state->res_domains,
+ state->res_names,
+ res_sid_index);
+ if (!ok) {
tevent_req_oom(req);
return;
}
--
2.9.3
From 4413b3d268e8dd4628680f3ef3a224bf23fca0c6 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 24 Mar 2017 16:46:40 +0100
Subject: [PATCH 4/6] winbindd: remove fallback to lookupsid for unknown SIDs
In wb_lookupsids_done() if a SID failed with lookupsids(), remove the
hokey retry via lookupsid().
The retry logic with going through the single sids lookup at the end
added a fake domain with an empty string. The wb_lookupsids caller
wb_sids2xids needed this, as it wasn't doing the needed error handling
itself. As wb_sids2xids has been fixed to cope, we can just fail the
lookupsids here.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_lookupsids.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index dbb24fd..219afb3 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -507,19 +507,8 @@ static void wb_lookupsids_done(struct tevent_req *subreq)
*/
for (i=0; i<state->tmp_names.count; i++) {
-
uint32_t res_sid_index = d->sid_indexes[i];
- if (state->tmp_names.names[i].sid_type == SID_NAME_UNKNOWN) {
- /*
- * Make unknown SIDs go through
- * wb_lookupsid. This retries the forest root.
- */
- state->single_sids[state->num_single_sids] =
- res_sid_index;
- state->num_single_sids += 1;
- continue;
- }
if (!wb_lookupsids_move_name(
&state->tmp_domains, &state->tmp_names.names[i],
state->res_domains, state->res_names,
--
2.9.3
From dad030d651e9bc32386f9639ab5b66d791e37b8f Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Fri, 24 Mar 2017 16:54:39 +0100
Subject: [PATCH 5/6] winbindd: remove lookupsid() fallback for a failed
lookupsids()
If lookupsids() returned any other error then OK, SOME_NOT_MAPPED or
NONE_MAPPED we must just bail out.
If some or all SIDs could not be mapped via lookupds(), don't fallback
to lookupsid(), it will just fail again.
The retry logic with going through the single sids lookup at the end
added a fake domain with an empty string. The wb_lookupsids caller
wb_sids2xids needed this, as it wasn't doing the needed error handling
itself. As wb_sids2xids has been fixed to cope, we can just fail the
lookupsids here.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_lookupsids.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index 219afb3..f6052a5 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -471,7 +471,6 @@ static void wb_lookupsids_done(struct tevent_req *subreq)
req, struct wb_lookupsids_state);
struct wb_lookupsids_domain *d;
uint32_t i;
- bool fallback = false;
NTSTATUS status, result;
@@ -480,25 +479,8 @@ static void wb_lookupsids_done(struct tevent_req *subreq)
if (tevent_req_nterror(req, status)) {
return;
}
-
- d = &state->domains[state->domains_done];
-
- if (NT_STATUS_IS_ERR(result)) {
- fallback = true;
- } else if (state->tmp_names.count != d->sids.num_sids) {
- fallback = true;
- }
-
- if (fallback) {
- for (i=0; i < d->sids.num_sids; i++) {
- uint32_t res_sid_index = d->sid_indexes[i];
-
- state->single_sids[state->num_single_sids] =
- res_sid_index;
- state->num_single_sids += 1;
- }
- state->domains_done += 1;
- wb_lookupsids_next(req, state);
+ if (NT_STATUS_LOOKUP_ERR(result)) {
+ tevent_req_nterror(req, result);
return;
}
@@ -506,6 +488,8 @@ static void wb_lookupsids_done(struct tevent_req *subreq)
* Look at the individual states in the translated names.
*/
+ d = &state->domains[state->domains_done];
+
for (i=0; i<state->tmp_names.count; i++) {
uint32_t res_sid_index = d->sid_indexes[i];
--
2.9.3
From 1de6fad0b98ba2fa0e53188a8e7a2bcc3ae3f31d Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Sun, 2 Apr 2017 14:15:33 +0200
Subject: [PATCH 6/6] winbindd: remove fallback from lookuprids
We're only calling lookuprids for our local SAM and BUILTIN domains, if
that results in a failed lookup for some rid, sending it again via
lookupsids() won't help, it will just fail again.
If the caller wrongly had sent any other SID that is not from our SAM or
BUILTIN via lookuprids(), that it is up to the caller to fix that, not
us.
The retry logic with going through the single sids lookup at the end
added a fake domain with an empty string. The wb_lookupsids caller
wb_sids2xids needed this, as it wasn't doing the needed error handling
itself. As wb_sids2xids has been fixed to cope, we can just fail the
lookupsids here.
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_lookupsids.c | 24 ++++--------------------
1 file changed, 4 insertions(+), 20 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index f6052a5..f2b2768 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -586,32 +586,14 @@ static void wb_lookupsids_lookuprids_done(struct tevent_req *subreq)
NTSTATUS status, result;
struct wb_lookupsids_domain *d;
uint32_t i;
- bool fallback = false;
status = dcerpc_wbint_LookupRids_recv(subreq, state, &result);
TALLOC_FREE(subreq);
if (tevent_req_nterror(req, status)) {
return;
}
-
- d = &state->domains[state->domains_done];
-
- if (NT_STATUS_IS_ERR(result)) {
- fallback = true;
- } else if (state->rid_names.num_principals != d->sids.num_sids) {
- fallback = true;
- }
-
- if (fallback) {
- for (i=0; i < d->sids.num_sids; i++) {
- uint32_t res_sid_index = d->sid_indexes[i];
-
- state->single_sids[state->num_single_sids] =
- res_sid_index;
- state->num_single_sids += 1;
- }
- state->domains_done += 1;
- wb_lookupsids_next(req, state);
+ if (NT_STATUS_LOOKUP_ERR(result)) {
+ tevent_req_nterror(req, result);
return;
}
@@ -619,6 +601,8 @@ static void wb_lookupsids_lookuprids_done(struct tevent_req *subreq)
* Look at the individual states in the translated names.
*/
+ d = &state->domains[state->domains_done];
+
sid_copy(&src_domain_sid, get_global_sam_sid());
src_domain.name.string = get_global_sam_name();
src_domain.sid = &src_domain_sid;
--
2.9.3
More information about the samba-technical
mailing list