[PATCH] winbindd: error handling in rpc_try_lookup_sids3()
Ralph Böhme
slow at samba.org
Wed Apr 5 17:47:02 UTC 2017
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.
> 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.
-slow
-------------- next part --------------
From 99f81d0d6ef18a791b6019cf880f2a6585aff740 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 4 Apr 2017 14:21:25 +0200
Subject: [PATCH 1/4] winbindd: use correct domain name for failed lookupsids
What we want here is, for failed lookupsids, pass the domain name of the
SID we were trying to lookup to the idmap backend.
But as a domain member, using
state->single_domains[state->single_sids_done]
for this purpose will always be use our primary domain name (for S-1-5-21
SIDs that are not in our local SAM).
So for now use find_domain_from_sid_noinit() to find the domain from the
domain list. This can be removed when we switch idmap backend
determination to be based on domain SIDs, not names.
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Bug: https://bugzilla.samba.org/show_bug.cgi?id=11961
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_lookupsids.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index 858616b..e690730 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -556,12 +556,13 @@ static void wb_lookupsids_single_done(struct tevent_req *subreq)
&domain_name, &name);
TALLOC_FREE(subreq);
if (!NT_STATUS_IS_OK(status)) {
- struct wb_lookupsids_domain *wb_domain;
+ struct winbindd_domain *wb_domain = NULL;
const char *tmpname;
type = SID_NAME_UNKNOWN;
- wb_domain = state->single_domains[state->single_sids_done];
+ 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
@@ -573,7 +574,7 @@ static void wb_lookupsids_single_done(struct tevent_req *subreq)
* name in the idmap backend to figure out
* which domain to use in processing.
*/
- tmpname = wb_domain->domain->name;
+ tmpname = wb_domain->name;
} else {
tmpname = "";
}
--
2.9.3
From f1be9637cd0fc8cf8452b24ef036404c7091558c Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Tue, 4 Apr 2017 14:23:03 +0200
Subject: [PATCH 2/4] winbindd: remove unused single_domains array
This was added as part of 9be918116e356c358ef77cc2933e471090088293, but
is not needed anymore as the previous commit changed the logic.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=11961
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Ralph Boehme <slow at samba.org>
---
source3/winbindd/wb_lookupsids.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/source3/winbindd/wb_lookupsids.c b/source3/winbindd/wb_lookupsids.c
index e690730..c13bd5b 100644
--- a/source3/winbindd/wb_lookupsids.c
+++ b/source3/winbindd/wb_lookupsids.c
@@ -72,8 +72,6 @@ struct wb_lookupsids_state {
* wbint_LookupSid. Preallocated with num_sids.
*/
uint32_t *single_sids;
- /* Pointer into the "domains" array above*/
- struct wb_lookupsids_domain **single_domains;
uint32_t num_single_sids;
uint32_t single_sids_done;
@@ -129,12 +127,6 @@ struct tevent_req *wb_lookupsids_send(TALLOC_CTX *mem_ctx,
if (tevent_req_nomem(state->single_sids, req)) {
return tevent_req_post(req, ev);
}
- state->single_domains = talloc_zero_array(state,
- struct wb_lookupsids_domain *,
- num_sids);
- if (tevent_req_nomem(state->single_domains, req)) {
- return tevent_req_post(req, ev);
- }
state->res_domains = talloc_zero(state, struct lsa_RefDomainList);
if (tevent_req_nomem(state->res_domains, req)) {
@@ -496,7 +488,6 @@ static void wb_lookupsids_done(struct tevent_req *subreq)
state->single_sids[state->num_single_sids] =
res_sid_index;
- state->single_domains[state->num_single_sids] = d;
state->num_single_sids += 1;
}
state->domains_done += 1;
--
2.9.3
From 4dc155bf379806c18bb2fe00ee31f4ed80136a81 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 5 Apr 2017 13:27:14 +0200
Subject: [PATCH 3/4] selftest: new environment "ad_member_idmap_rid"
This uses idmap_rid for the primary domain.
Bug: https://bugzilla.samba.org/show_bug.cgi?id=11961
Signed-off-by: Ralph Boehme <slow at samba.org>
---
selftest/target/Samba.pm | 1 +
selftest/target/Samba3.pm | 88 +++++++++++++++++++++++++++++++++++++++++++++++
selftest/target/Samba4.pm | 6 ++++
3 files changed, 95 insertions(+)
diff --git a/selftest/target/Samba.pm b/selftest/target/Samba.pm
index 9fc84b5..4cf1179 100644
--- a/selftest/target/Samba.pm
+++ b/selftest/target/Samba.pm
@@ -309,6 +309,7 @@ sub get_interface($)
$interfaces{"fakednsforwarder2"} = 37;
$interfaces{"s4member_dflt"} = 38;
$interfaces{"vampire2000dc"} = 39;
+ $interfaces{"idmapridmember"} = 40;
# update lib/socket_wrapper/socket_wrapper.c
# #define MAX_WRAPPED_INTERFACES 40
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 92031a1..0aa88ee 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -584,6 +584,94 @@ sub setup_admember_rfc2307($$$$)
return $ret;
}
+sub setup_ad_member_idmap_rid($$$$)
+{
+ my ($self, $prefix, $dcvars) = @_;
+
+ # If we didn't build with ADS, pretend this env was never available
+ if (not $self->have_ads()) {
+ return "UNKNOWN";
+ }
+
+ print "PROVISIONING S3 AD MEMBER WITH idmap_rid config...";
+
+ my $member_options = "
+ security = ads
+ workgroup = $dcvars->{DOMAIN}
+ realm = $dcvars->{REALM}
+ idmap config * : backend = tdb
+ idmap config * : range = 1000000-1999999
+ idmap config $dcvars->{DOMAIN} : backend = rid
+ idmap config $dcvars->{DOMAIN} : range = 2000000-2999999
+";
+
+ my $ret = $self->provision($prefix,
+ "IDMAPRIDMEMBER",
+ "loCalMemberPass",
+ $member_options,
+ $dcvars->{SERVER_IP},
+ $dcvars->{SERVER_IPV6});
+
+ $ret or return undef;
+
+ close(USERMAP);
+ $ret->{DOMAIN} = $dcvars->{DOMAIN};
+ $ret->{REALM} = $dcvars->{REALM};
+
+ my $ctx;
+ my $prefix_abs = abs_path($prefix);
+ $ctx = {};
+ $ctx->{krb5_conf} = "$prefix_abs/lib/krb5.conf";
+ $ctx->{domain} = $dcvars->{DOMAIN};
+ $ctx->{realm} = $dcvars->{REALM};
+ $ctx->{dnsname} = lc($dcvars->{REALM});
+ $ctx->{kdc_ipv4} = $dcvars->{SERVER_IP};
+ $ctx->{kdc_ipv6} = $dcvars->{SERVER_IPV6};
+ $ctx->{krb5_ccname} = "$prefix_abs/krb5cc_%{uid}";
+ Samba::mk_krb5_conf($ctx, "");
+
+ $ret->{KRB5_CONFIG} = $ctx->{krb5_conf};
+
+ my $net = Samba::bindir_path($self, "net");
+ my $cmd = "";
+ $cmd .= "SOCKET_WRAPPER_DEFAULT_IFACE=\"$ret->{SOCKET_WRAPPER_DEFAULT_IFACE}\" ";
+ if (defined($ret->{RESOLV_WRAPPER_CONF})) {
+ $cmd .= "RESOLV_WRAPPER_CONF=\"$ret->{RESOLV_WRAPPER_CONF}\" ";
+ } else {
+ $cmd .= "RESOLV_WRAPPER_HOSTS=\"$ret->{RESOLV_WRAPPER_HOSTS}\" ";
+ }
+ $cmd .= "KRB5_CONFIG=\"$ret->{KRB5_CONFIG}\" ";
+ $cmd .= "SELFTEST_WINBINDD_SOCKET_DIR=\"$ret->{SELFTEST_WINBINDD_SOCKET_DIR}\" ";
+ $cmd .= "$net join $ret->{CONFIGURATION}";
+ $cmd .= " -U$dcvars->{USERNAME}\%$dcvars->{PASSWORD}";
+
+ if (system($cmd) != 0) {
+ warn("Join failed\n$cmd");
+ return undef;
+ }
+
+ # We need world access to this share, as otherwise the domain
+ # administrator from the AD domain provided by Samba4 can't
+ # access the share for tests.
+ chmod 0777, "$prefix/share";
+
+ if (not $self->check_or_start($ret, "yes", "yes", "yes")) {
+ return undef;
+ }
+
+ $ret->{DC_SERVER} = $dcvars->{SERVER};
+ $ret->{DC_SERVER_IP} = $dcvars->{SERVER_IP};
+ $ret->{DC_SERVER_IPV6} = $dcvars->{SERVER_IPV6};
+ $ret->{DC_NETBIOSNAME} = $dcvars->{NETBIOSNAME};
+ $ret->{DC_USERNAME} = $dcvars->{USERNAME};
+ $ret->{DC_PASSWORD} = $dcvars->{PASSWORD};
+
+ # Special case, this is called from Samba4.pm but needs to use the Samba3 check_env and get_log_env
+ $ret->{target} = $self;
+
+ return $ret;
+}
+
sub setup_simpleserver($$)
{
my ($self, $path) = @_;
diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm
index 0d7e6b5..30bb255 100755
--- a/selftest/target/Samba4.pm
+++ b/selftest/target/Samba4.pm
@@ -2088,6 +2088,12 @@ sub setup_env($$$)
}
return $target3->setup_admember_rfc2307("$path/ad_member_rfc2307",
$self->{vars}->{ad_dc_ntvfs}, 34);
+ } elsif ($envname eq "ad_member_idmap_rid") {
+ if (not defined($self->{vars}->{ad_dc})) {
+ $self->setup_ad_dc("$path/ad_dc");
+ }
+ return $target3->setup_ad_member_idmap_rid("$path/ad_member_idmap_rid",
+ $self->{vars}->{ad_dc});
} elsif ($envname eq "none") {
return $self->setup_none("$path/none");
} else {
--
2.9.3
From faae9c4188563a8b8c2f5e8e216504aa2b4697f1 Mon Sep 17 00:00:00 2001
From: Ralph Boehme <slow at samba.org>
Date: Wed, 5 Apr 2017 13:27:51 +0200
Subject: [PATCH 4/4] selftest: tests idmap mapping with idmap_rid
This adds two blackbox tests that run wbinfo --sids-to-unix-ids:
o a non-existing SID from the primary domain should return a mapping
o a SID with a bogus (and therefor unknown) domain must not return a mapping
Bug: https://bugzilla.samba.org/show_bug.cgi?id=11961
Signed-off-by: Ralph Boehme <slow at samba.org>
---
nsswitch/tests/test_idmap_rid.sh | 66 ++++++++++++++++++++++++++++++++++++++++
source3/selftest/tests.py | 4 ++-
2 files changed, 69 insertions(+), 1 deletion(-)
create mode 100755 nsswitch/tests/test_idmap_rid.sh
diff --git a/nsswitch/tests/test_idmap_rid.sh b/nsswitch/tests/test_idmap_rid.sh
new file mode 100755
index 0000000..7fb5985
--- /dev/null
+++ b/nsswitch/tests/test_idmap_rid.sh
@@ -0,0 +1,66 @@
+#!/bin/sh
+#
+# Test id mapping with various SIDs and idmap_rid
+#
+
+if [ $# -lt 1 ]; then
+ echo Usage: $0 DOMAIN RANGE_START
+ exit 1
+fi
+
+DOMAIN="$1"
+RANGE_START="$2"
+
+wbinfo="$VALGRIND $BINDIR/wbinfo"
+failed=0
+
+. `dirname $0`/../../testprogs/blackbox/subunit.sh
+
+DOMAIN_SID=$($wbinfo -n "@$DOMAIN" | cut -f 1 -d " ")
+if [ $? -ne 0 ] ; then
+ echo "Could not find domain SID" | subunit_fail_test "test_idmap_rid"
+ exit 1
+fi
+
+# Find an unused uid and SID
+RID=66666
+MAX_RID=77777
+while true ; do
+ id $RID
+ if [ $? -ne 0 ] ; then
+ SID="$DOMAIN_SID-$RID"
+ $wbinfo -s $SID
+ if [ $? -ne 0 ] ; then
+ break
+ fi
+ fi
+ RID=$(expr $RID + 1)
+ if [ $RID -eq $MAX_RID ] ; then
+ echo "Could not find free SID" | subunit_fail_test "test_idmap_rid"
+ exit 1
+ fi
+done
+
+#
+# Test 1: Using non-existing SID to check backend returns a mapping
+#
+
+EXPECTED_ID=$(expr $RID + $RANGE_START)
+out="$($wbinfo --sids-to-unix-ids=$SID)"
+echo "wbinfo returned: \"$out\", expecting \"$SID -> uid/gid $EXPECTED_ID\""
+test "$out" = "$SID -> uid/gid $EXPECTED_ID"
+ret=$?
+testit "Unknown RID from primary domain returns a mapping" test $ret -eq 0 || failed=$(expr $failed + 1)
+
+#
+# Test 2: Using bogus SID with bad domain part to check idmap backend does not generate a mapping
+#
+
+SID=S-1-5-21-1111-2222-3333-666
+out="$($wbinfo --sids-to-unix-ids=$SID)"
+echo "wbinfo returned: \"$out\", expecting \"$SID -> unmapped\""
+test "$out" = "$SID -> unmapped"
+ret=$?
+testit "Bogus SID returns unmapped" test $ret -eq 0 || failed=$(expr $failed + 1)
+
+exit $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 336ec92..d0e2ae6 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -330,7 +330,7 @@ rpc = ["rpc.authcontext", "rpc.samba3.bind", "rpc.samba3.srvsvc", "rpc.samba3.sh
local = ["local.nss"]
-idmap = ["idmap.rfc2307", "idmap.alloc"]
+idmap = ["idmap.rfc2307", "idmap.alloc", "idmap.rid"]
rap = ["rap.basic", "rap.rpc", "rap.printing", "rap.sam"]
@@ -400,6 +400,8 @@ for t in tests:
plantestsuite(t, "ad_member_rfc2307", [os.path.join(samba3srcdir, "../nsswitch/tests/test_idmap_rfc2307.sh"), '$DOMAIN', 'Administrator', '2000000', 'Guest', '2000001', '"Domain Users"', '2000002', 'DnsAdmins', '2000003', 'ou=idmap,dc=samba,dc=example,dc=com', '$DC_SERVER', '$DC_USERNAME', '$DC_PASSWORD'])
elif t == "idmap.alloc":
plantestsuite(t, "ad_member_rfc2307", [os.path.join(samba3srcdir, "../nsswitch/tests/test_idmap_nss.sh"), '$DOMAIN'])
+ elif t == "idmap.rid":
+ plantestsuite(t, "ad_member_idmap_rid", [os.path.join(samba3srcdir, "../nsswitch/tests/test_idmap_rid.sh"), '$DOMAIN', '2000000'])
elif t == "raw.acls":
plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/nfs4acl_simple -U$USERNAME%$PASSWORD', description='nfs4acl_xattr-simple')
--
2.9.3
More information about the samba-technical
mailing list