From 21b7cec6993b81227e17e43ebb32c095e0bd20ed Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Sat, 4 Sep 2021 12:28:20 +1200 Subject: [PATCH 1/6] selftest: Split up targets for samba_tool_drs from samba_tool_drs_showrepl These now run in the disconnected sets schema_dc/schema_pair_dc and ad_dc/vampire_dc/promoted_dc. By aiming at different sets ofservers we can't cause cross-contamination in terms of which servers are listed as outbound connections. Also, by running the tests only once we reduce the chaces of trouble by half. RN: Address flapping samba_tool_drs_showrepl test BUG: https://bugzilla.samba.org/show_bug.cgi?id=14818 Signed-off-by: Andrew Bartlett Reviewed-by: Jeremy Allison (cherry picked from commit e8b4599e0935290c5e59df9fd4f695ad8d6f361c) --- source4/selftest/tests.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 8938754d0fc..a011d50a768 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1295,6 +1295,18 @@ planoldpythontestsuite(env, "ridalloc_exop", environ={'DC1': "$DC_SERVER", 'DC2': '$SERVER'}, extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD']) +# This test can pollute the environment a little by creating and +# deleting DCs which can get into the replication state for a while. +# +# The setting of DC1 to $DC_SERVER means that it will join towards and +# operate on schema_dc. This matters most when running +# test_samba_tool_replicate_local as this sets up a full temp DC and +# does new replication to it, which can show up in the replication +# topology. +# +# That is why this test is run on the isolated environment and not on +# those connected with ad_dc (vampiredc/promoteddc) + env = 'schema_pair_dc' planoldpythontestsuite("%s:local" % env, "samba_tool_drs", extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')], @@ -1309,11 +1321,6 @@ planoldpythontestsuite(env, "getnc_schema", extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD']) for env in ['vampire_dc', 'promoted_dc']: - planoldpythontestsuite("%s:local" % env, "samba_tool_drs", - extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')], - name="samba4.drs.samba_tool_drs.python(%s)" % env, - environ={'DC1': '$DC_SERVER', 'DC2': '$SERVER'}, - extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD']) planoldpythontestsuite("%s:local" % env, "samba_tool_drs_showrepl", extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')], name="samba4.drs.samba_tool_drs_showrepl.python(%s)" % env, -- 2.25.1 From 46ad637584149328e60463aec65593c42bcdc602 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Sat, 4 Sep 2021 13:11:08 +1200 Subject: [PATCH 2/6] selftest: Only run samba_tool_drs_showrepl test once This test is not slow, but there is no value running it twice. Running this test twice just increases the chances we might loose a race as it shows and validates live replication data. Signed-off-by: Andrew Bartlett Reviewed-by: Jeremy Allison (cherry picked from commit 75a5ed66731e947fa16af81aab7649d1fddec45f) --- source4/selftest/tests.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index a011d50a768..32059461831 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1320,12 +1320,18 @@ planoldpythontestsuite(env, "getnc_schema", "PLEASE_BREAK_MY_WINDOWS": "1"}, extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD']) +# This test can be sensitive to the DC joins and replications don in +# "samba_tool_drs" so run this is run against scheam_pair_dc/schema_dc +# not the set of environments connected with ad_dc. + +# This will show the replication state of ad_dc +planoldpythontestsuite("promoted_dc:local", "samba_tool_drs_showrepl", + extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')], + name="samba4.drs.samba_tool_drs_showrepl.python(%s)" % env, + environ={'DC1': '$DC_SERVER', 'DC2': '$SERVER'}, + extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD']) + for env in ['vampire_dc', 'promoted_dc']: - planoldpythontestsuite("%s:local" % env, "samba_tool_drs_showrepl", - extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')], - name="samba4.drs.samba_tool_drs_showrepl.python(%s)" % env, - environ={'DC1': '$DC_SERVER', 'DC2': '$SERVER'}, - extra_args=['-U$DOMAIN/$DC_USERNAME%$DC_PASSWORD']) planoldpythontestsuite("%s:local" % env, "replica_sync", extra_path=[os.path.join(samba4srcdir, 'torture/drs/python')], name="samba4.drs.replica_sync.python(%s)" % env, -- 2.25.1 From e1cef55699cc912c932fc3198f0eec7ed78e8a3d Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 25 Aug 2021 09:41:11 +1200 Subject: [PATCH 3/6] dsdb: Be careful to avoid use of the expensive talloc_is_parent() The wrong talloc API was selected while addressing a memory leak. commit ee2fe56ba0ef6626b634376e8dc2185aa89f8c99 Author: Aaron Haslett Date: Tue Nov 27 11:07:44 2018 +1300 drepl: memory leak fix Fixes a memory leak where schema reference attached to ldb instance is lost before it can be freed. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14042 Signed-off-by: Aaron Haslett Reviewed-by: Andrew Bartlett Reviewed-by: Garming Sam Autobuild-User(master): Garming Sam Autobuild-Date(master): Wed Jul 17 06:17:10 UTC 2019 on sn-devel-184 By using talloc_get_parent() walking the entire talloc tree is avoided. RN: Address a signifcant performance regression in database access in the AD DC since Samba 4.12 BUG: https://bugzilla.samba.org/show_bug.cgi?id=14806 Signed-off-by: Andrew Bartlett Reviewed-by: Jeremy Allison (cherry picked from commit 8affe4a1e625104de4ca024fdc3e9cd96498aff3) --- source4/dsdb/schema/schema_set.c | 41 ++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c index f4934917c7c..45faa0912ec 100644 --- a/source4/dsdb/schema/schema_set.c +++ b/source4/dsdb/schema/schema_set.c @@ -698,6 +698,8 @@ int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema, { int ret; void *ptr; + void *schema_parent = NULL; + bool is_already_parent; struct dsdb_schema *old_schema; old_schema = ldb_get_opaque(ldb, "dsdb_schema"); ret = ldb_set_opaque(ldb, "dsdb_schema", schema); @@ -710,8 +712,9 @@ int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema, talloc_unlink(ldb, old_schema); /* Reference schema on ldb if it wasn't done already */ - ret = talloc_is_parent(ldb, schema); - if (ret == 0) { + schema_parent = talloc_parent(schema); + is_already_parent = (schema_parent == ldb); + if (!is_already_parent) { ptr = talloc_reference(ldb, schema); if (ptr == NULL) { return ldb_oom(ldb); @@ -775,10 +778,10 @@ int dsdb_set_global_schema(struct ldb_context *ldb) /* Don't write indices and attributes, it's expensive */ ret = dsdb_schema_set_indices_and_attributes(ldb, global_schema, SCHEMA_MEMORY_ONLY); if (ret == LDB_SUCCESS) { - /* If ldb doesn't have a reference to the schema, make one, - * just in case the original copy is replaced */ - ret = talloc_is_parent(ldb, global_schema); - if (ret == 0) { + void *schema_parent = talloc_parent(global_schema); + bool is_already_parent = + (schema_parent == ldb); + if (!is_already_parent) { ptr = talloc_reference(ldb, global_schema); if (ptr == NULL) { return ldb_oom(ldb); @@ -809,7 +812,6 @@ struct dsdb_schema *dsdb_get_schema(struct ldb_context *ldb, TALLOC_CTX *referen dsdb_schema_refresh_fn refresh_fn; struct ldb_module *loaded_from_module; bool use_global_schema; - int ret; TALLOC_CTX *tmp_ctx = talloc_new(reference_ctx); if (tmp_ctx == NULL) { return NULL; @@ -860,13 +862,28 @@ struct dsdb_schema *dsdb_get_schema(struct ldb_context *ldb, TALLOC_CTX *referen /* This removes the extra reference above */ talloc_free(tmp_ctx); - /* If ref ctx exists and doesn't already reference schema, then add - * a reference. Otherwise, just return schema.*/ - ret = talloc_is_parent(reference_ctx, schema_out); - if ((ret == 1) || (!reference_ctx)) { + /* + * If ref ctx exists and doesn't already reference schema, then add + * a reference. Otherwise, just return schema. + * + * We must use talloc_parent(), which is not quite free (there + * is no direct parent pointer in talloc, only one on the + * first child within a linked list), but is much cheaper than + * talloc_is_parent() which walks the whole tree up to the top + * looking for a potential grand-grand(etc)-parent. + */ + if (reference_ctx == NULL) { return schema_out; } else { - return talloc_reference(reference_ctx, schema_out); + void *schema_parent = talloc_parent(schema_out); + bool is_already_parent = + schema_parent == reference_ctx; + if (is_already_parent) { + return schema_out; + } else { + return talloc_reference(reference_ctx, + schema_out); + } } } -- 2.25.1 From fa6f8b6e68037dded768efa735cea8c47d0e9daa Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 25 Aug 2021 09:54:04 +0000 Subject: [PATCH 4/6] selftest: Add a test for LookupSids3 and LookupNames4 in python BUG: https://bugzilla.samba.org/show_bug.cgi?id=14807 Signed-off-by: Andrew Bartlett Reviewed-by: Jeremy Allison (cherry picked from commit b40761b42e889369599c5eb355028ba377c43b49) --- python/samba/tests/dcerpc/lsa.py | 333 +++++++++++++++++++++++++++++++ source4/selftest/tests.py | 1 + 2 files changed, 334 insertions(+) create mode 100644 python/samba/tests/dcerpc/lsa.py diff --git a/python/samba/tests/dcerpc/lsa.py b/python/samba/tests/dcerpc/lsa.py new file mode 100644 index 00000000000..4377c42e9b8 --- /dev/null +++ b/python/samba/tests/dcerpc/lsa.py @@ -0,0 +1,333 @@ +# -*- coding: utf-8 -*- +# +# Unix SMB/CIFS implementation. +# Copyright © Andrew Bartlett 2021 +# Copyright (C) Catalyst IT Ltd. 2017 +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# + +"""Tests for samba.dcerpc.sam.""" + +from samba.dcerpc import samr, security, lsa +from samba.credentials import Credentials +from samba.tests import TestCase +from samba.dcerpc.security import dom_sid +from samba import NTSTATUSError +from samba.ntstatus import NT_STATUS_ACCESS_DENIED +import samba.tests + +class LsaTests(TestCase): + + def setUp(self): + self.lp = self.get_loadparm() + self.server = samba.tests.env_get_var_value('SERVER') + + def test_lsa_LookupSids3_multiple(self): + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c = lsa.lsarpc( + "ncacn_ip_tcp:%s[schannel,seal]" % self.server, + self.lp, + machine_creds) + + sids = lsa.SidArray() + sid = lsa.SidPtr() + # Need a set + x = dom_sid("S-1-5-7") + sid.sid = x + sids.sids = [sid] + sids.num_sids = 1 + names = lsa.TransNameArray2() + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + + # We want to run LookupSids3 multiple times on the same + # connection as we have code to re-use the sam.ldb and we need + # to check things work for the second request. + (domains, names, count) = c.LookupSids3(sids, names, level, count, lookup_options, client_revision) + self.assertEqual(count, 1) + self.assertEqual(names.count, 1) + self.assertEqual(names.names[0].name.string, + "ANONYMOUS LOGON") + (domains2, names2, count2) = c.LookupSids3(sids, names, level, count, lookup_options, client_revision) + self.assertEqual(count2, 1) + self.assertEqual(names2.count, 1) + self.assertEqual(names2.names[0].name.string, + "ANONYMOUS LOGON") + + # Just looking for any exceptions in the last couple of loops + c.LookupSids3(sids, names, level, count, lookup_options, client_revision) + c.LookupSids3(sids, names, level, count, lookup_options, client_revision) + + def test_lsa_LookupSids3_multiple_conns(self): + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c = lsa.lsarpc( + "ncacn_ip_tcp:%s[schannel,seal]" % self.server, + self.lp, + machine_creds) + + sids = lsa.SidArray() + sid = lsa.SidPtr() + # Need a set + x = dom_sid("S-1-5-7") + sid.sid = x + sids.sids = [sid] + sids.num_sids = 1 + names = lsa.TransNameArray2() + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + + # We want to run LookupSids3, and then again on a new + # connection to show that we don't have an issue with the DB + # being tied to the wrong connection. + (domains, names, count) = c.LookupSids3(sids, + names, + level, + count, + lookup_options, + client_revision) + self.assertEqual(count, 1) + self.assertEqual(names.count, 1) + self.assertEqual(names.names[0].name.string, + "ANONYMOUS LOGON") + + c = lsa.lsarpc( + "ncacn_ip_tcp:%s[schannel,seal]" % self.server, + self.lp, + machine_creds) + + (domains, names, count) = c.LookupSids3(sids, + names, + level, + count, + lookup_options, + client_revision) + self.assertEqual(count, 1) + self.assertEqual(names.count, 1) + self.assertEqual(names.names[0].name.string, + "ANONYMOUS LOGON") + + + def test_lsa_LookupNames4_LookupSids3_multiple(self): + """ + Test by going back and forward between real DB lookups + name->sid->name to ensure the sam.ldb handle is fine once + shared + """ + + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c_normal = lsa.lsarpc( + "ncacn_np:%s[seal]" % self.server, + self.lp, + machine_creds) + + username, domain = c_normal.GetUserName(None, None, None) + + c = lsa.lsarpc( + "ncacn_ip_tcp:%s[schannel,seal]" % self.server, + self.lp, + machine_creds) + + sids = lsa.TransSidArray3() + names = [username] + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + (domains, sids, count) = c.LookupNames4(names, + sids, + level, + count, + lookup_options, + client_revision) + + # Another lookup on the same connection, will re-used the + # server-side implicit state handle on the connection + (domains, sids, count) = c.LookupNames4(names, + sids, + level, + count, + lookup_options, + client_revision) + + self.assertEqual(count, 1) + self.assertEqual(sids.count, 1) + + # Now look the SIDs back up + names = lsa.TransNameArray2() + sid = lsa.SidPtr() + sid.sid = sids.sids[0].sid + lookup_sids = lsa.SidArray() + lookup_sids.sids = [sid] + lookup_sids.num_sids = 1 + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 1 + lookup_options = 0 + client_revision = lsa.LSA_CLIENT_REVISION_2 + + (domains, names, count) = c.LookupSids3(lookup_sids, + names, + level, + count, + lookup_options, + client_revision) + self.assertEqual(count, 1) + self.assertEqual(names.count, 1) + self.assertEqual(names.names[0].name.string, + username.string) + + # And once more just to be sure, just checking for a fault + sids = lsa.TransSidArray3() + names = [username] + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + (domains, sids, count) = c.LookupNames4(names, + sids, + level, + count, + lookup_options, + client_revision) + + + def test_lsa_LookupNames4_multiple_conns(self): + """ + Test by going back and forward between real DB lookups + name->sid->name to ensure the sam.ldb handle is fine once + shared + """ + + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c_normal = lsa.lsarpc( + "ncacn_np:%s[seal]" % self.server, + self.lp, + machine_creds) + + username, domain = c_normal.GetUserName(None, None, None) + + c = lsa.lsarpc( + "ncacn_ip_tcp:%s[schannel,seal]" % self.server, + self.lp, + machine_creds) + + sids = lsa.TransSidArray3() + names = [username] + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + (domains, sids, count) = c.LookupNames4(names, + sids, + level, + count, + lookup_options, + client_revision) + + c = lsa.lsarpc( + "ncacn_ip_tcp:%s[schannel,seal]" % self.server, + self.lp, + machine_creds) + + sids = lsa.TransSidArray3() + names = [username] + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + (domains, sids, count) = c.LookupNames4(names, + sids, + level, + count, + lookup_options, + client_revision) + + def test_lsa_LookupNames4_without_schannel(self): + + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c_normal = lsa.lsarpc( + "ncacn_np:%s[seal]" % self.server, + self.lp, + machine_creds) + + username, domain = c_normal.GetUserName(None, None, None) + + sids = lsa.TransSidArray3() + names = [username] + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + + with self.assertRaises(NTSTATUSError) as e: + c_normal.LookupNames4(names, + sids, + level, + count, + lookup_options, + client_revision) + if (e.exception.args[0] != NT_STATUS_ACCESS_DENIED): + raise AssertionError("LookupNames4 without schannel must fail with ACCESS_DENIED") + + def test_lsa_LookupSids3_without_schannel(self): + machine_creds = Credentials() + machine_creds.guess(self.lp) + machine_creds.set_machine_account() + + c = lsa.lsarpc( + "ncacn_ip_tcp:%s[seal]" % self.server, + self.lp, + machine_creds) + + sids = lsa.SidArray() + sid = lsa.SidPtr() + # Need a set + x = dom_sid("S-1-5-7") + sid.sid = x + sids.sids = [sid] + sids.num_sids = 1 + names = lsa.TransNameArray2() + level = lsa.LSA_LOOKUP_NAMES_ALL + count = 0 + lookup_options = lsa.LSA_LOOKUP_OPTION_SEARCH_ISOLATED_NAMES + client_revision = lsa.LSA_CLIENT_REVISION_2 + + with self.assertRaises(NTSTATUSError) as e: + c.LookupSids3(sids, + names, + level, + count, + lookup_options, + client_revision) + if (e.exception.args[0] != NT_STATUS_ACCESS_DENIED): + raise AssertionError("LookupSids3 without schannel must fail with ACCESS_DENIED") diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 32059461831..638bd57334e 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -770,6 +770,7 @@ planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.sam") planpythontestsuite("ad_dc_default:local", "samba.tests.dsdb") planpythontestsuite("none", "samba.tests.dsdb_lock") planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.bare") +planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.lsa") planpythontestsuite("ad_dc_default:local", "samba.tests.dcerpc.unix") planpythontestsuite("ad_dc_ntvfs:local", "samba.tests.dcerpc.srvsvc") planpythontestsuite("ad_dc_default:local", "samba.tests.samba_tool.timecmd") -- 2.25.1 From 3d88c735ee87311db27871550fe58fa676fe3122 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 25 Aug 2021 12:03:08 +1200 Subject: [PATCH 5/6] s4-lsa: Cache sam.ldb handle in lsa_LookupSids3/LookupNames4 Since 5c0345ea9bb34695dcd7be6c913748323bebe937 this would not have been implicitly cached via the ldb_wrap cache, due to the recording of the remote IP address (which is a good thing). This creates a more explicit and direct correct cache on the connection. The common code, including the SCHANNEL check is placed into a helper function. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14807 RN: Fix performance regression in lsa_LookupSids3/LookupNames4 since Samba 4.9 by using an explicit database handle cache Signed-off-by: Andrew Bartlett Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Sun Sep 5 03:19:26 UTC 2021 on sn-devel-184 (cherry picked from commit ae57d22e45b33537e9fca5969e9b68abd1ad633f) --- source4/rpc_server/lsa/lsa_lookup.c | 131 ++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 37 deletions(-) diff --git a/source4/rpc_server/lsa/lsa_lookup.c b/source4/rpc_server/lsa/lsa_lookup.c index d41997d4b3d..61cb8a10a23 100644 --- a/source4/rpc_server/lsa/lsa_lookup.c +++ b/source4/rpc_server/lsa/lsa_lookup.c @@ -663,25 +663,24 @@ NTSTATUS dcesrv_lsa_LookupSids2(struct dcesrv_call_state *dce_call, return status; } +/* A random hexidecimal number (honest!) */ +#define LSA_SERVER_IMPLICIT_POLICY_STATE_MAGIC 0xc0c99e00 /* - lsa_LookupSids3 - - Identical to LookupSids2, but doesn't take a policy handle - + Ensure we're operating on an schannel connection, + and use a lsa_policy_state cache on the connection. */ -NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call, - TALLOC_CTX *mem_ctx, - struct lsa_LookupSids3 *r) +static NTSTATUS schannel_call_setup(struct dcesrv_call_state *dce_call, + struct lsa_policy_state **_policy_state) { + struct lsa_policy_state *policy_state = NULL; enum dcerpc_transport_t transport = dcerpc_binding_get_transport(dce_call->conn->endpoint->ep_description); enum dcerpc_AuthType auth_type = DCERPC_AUTH_TYPE_NONE; - struct dcesrv_lsa_LookupSids_base_state *state = NULL; - NTSTATUS status; - if (transport != NCACN_IP_TCP) { - DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED); + /* We can't call DCESRV_FAULT() in the sub-function */ + dce_call->fault_code = DCERPC_FAULT_ACCESS_DENIED; + return NT_STATUS_ACCESS_DENIED; } /* @@ -693,8 +692,59 @@ NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call, */ dcesrv_call_auth_info(dce_call, &auth_type, NULL); if (auth_type != DCERPC_AUTH_TYPE_SCHANNEL) { - DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED); + /* We can't call DCESRV_FAULT() in the sub-function */ + dce_call->fault_code = DCERPC_FAULT_ACCESS_DENIED; + return NT_STATUS_ACCESS_DENIED; + } + + /* + * We don't have a policy handle on this call, so we want to + * make a policy state and cache it for the life of the + * connection, to avoid re-opening the DB each call. + */ + policy_state + = dcesrv_iface_state_find_conn(dce_call, + LSA_SERVER_IMPLICIT_POLICY_STATE_MAGIC, + struct lsa_policy_state); + + if (policy_state == NULL) { + NTSTATUS status + = dcesrv_lsa_get_policy_state(dce_call, + dce_call /* mem_ctx */, + 0, /* we skip access checks */ + &policy_state); + if (!NT_STATUS_IS_OK(status)) { + return status; + } + + /* + * This will talloc_steal() policy_state onto the + * connection, which has longer lifetime than the + * immidiate caller requires + */ + status = dcesrv_iface_state_store_conn(dce_call, + LSA_SERVER_IMPLICIT_POLICY_STATE_MAGIC, + policy_state); + if (!NT_STATUS_IS_OK(status)) { + return status; + } } + *_policy_state = policy_state; + return NT_STATUS_OK; +} + +/* + lsa_LookupSids3 + + Identical to LookupSids2, but doesn't take a policy handle + +*/ +NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call, + TALLOC_CTX *mem_ctx, + struct lsa_LookupSids3 *r) +{ + struct dcesrv_lsa_LookupSids_base_state *state = NULL; + NTSTATUS status; *r->out.domains = NULL; r->out.names->count = 0; @@ -706,16 +756,27 @@ NTSTATUS dcesrv_lsa_LookupSids3(struct dcesrv_call_state *dce_call, return NT_STATUS_NO_MEMORY; } - state->dce_call = dce_call; - state->mem_ctx = mem_ctx; - - status = dcesrv_lsa_get_policy_state(state->dce_call, mem_ctx, - 0, /* we skip access checks */ - &state->policy_state); + /* + * We don't have a policy handle on this call, so we want to + * make a policy state and cache it for the life of the + * connection, to avoid re-opening the DB each call. + * + * This also enforces that this is only available over + * ncacn_ip_tcp and with SCHANNEL. + * + * schannel_call_setup may also set the fault state. + * + * state->policy_state is shared between all calls on this + * connection and is moved with talloc_steal() under the + * connection, not dce_call or state. + */ + status = schannel_call_setup(dce_call, &state->policy_state); if (!NT_STATUS_IS_OK(status)) { return status; } + state->dce_call = dce_call; + state->mem_ctx = mem_ctx; state->r.in.sids = r->in.sids; state->r.in.level = r->in.level; state->r.in.lookup_options = r->in.lookup_options; @@ -1296,25 +1357,9 @@ NTSTATUS dcesrv_lsa_LookupNames3(struct dcesrv_call_state *dce_call, NTSTATUS dcesrv_lsa_LookupNames4(struct dcesrv_call_state *dce_call, TALLOC_CTX *mem_ctx, struct lsa_LookupNames4 *r) { - enum dcerpc_transport_t transport = - dcerpc_binding_get_transport(dce_call->conn->endpoint->ep_description); - enum dcerpc_AuthType auth_type = DCERPC_AUTH_TYPE_NONE; struct dcesrv_lsa_LookupNames_base_state *state = NULL; NTSTATUS status; - if (transport != NCACN_IP_TCP) { - DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED); - } - - /* - * We don't have policy handles on this call. So this must be restricted - * to crypto connections only. - */ - dcesrv_call_auth_info(dce_call, &auth_type, NULL); - if (auth_type != DCERPC_AUTH_TYPE_SCHANNEL) { - DCESRV_FAULT(DCERPC_FAULT_ACCESS_DENIED); - } - *r->out.domains = NULL; r->out.sids->count = 0; r->out.sids->sids = NULL; @@ -1328,9 +1373,21 @@ NTSTATUS dcesrv_lsa_LookupNames4(struct dcesrv_call_state *dce_call, TALLOC_CTX state->dce_call = dce_call; state->mem_ctx = mem_ctx; - status = dcesrv_lsa_get_policy_state(state->dce_call, state, - 0, /* we skip access checks */ - &state->policy_state); + /* + * We don't have a policy handle on this call, so we want to + * make a policy state and cache it for the life of the + * connection, to avoid re-opening the DB each call. + * + * This also enforces that this is only available over + * ncacn_ip_tcp and with SCHANNEL. + * + * schannel_call_setup may also set the fault state. + * + * state->policy_state is shared between all calls on this + * connection and is moved with talloc_steal() under the + * connection, not dce_call or state. + */ + status = schannel_call_setup(dce_call, &state->policy_state); if (!NT_STATUS_IS_OK(status)) { return status; } -- 2.25.1 From 0037c931bdd40ccddde2428e5be7edcb0febaebd Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Mon, 6 Sep 2021 08:52:21 +1200 Subject: [PATCH 6/6] selftest: Add prefix to new schema attributes to avoid flapping dsdb_schema_attributes If two of these unit tests run in the same second they could select the same name, as the name was only based on the time and a common prefix. As observed by Jeremy Allison. Thanks for the report! RN: Address flapping dsdb_schema_attributes test BUG: https://bugzilla.samba.org/show_bug.cgi?id=14819 Signed-off-by: Andrew Bartlett Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Mon Sep 6 02:32:51 UTC 2021 on sn-devel-184 (cherry picked from commit 6590bb0b77c641f0d4686b39c713c1405ffb64f5) --- python/samba/tests/dsdb_schema_attributes.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/python/samba/tests/dsdb_schema_attributes.py b/python/samba/tests/dsdb_schema_attributes.py index bb4603d4703..d9b538d4af4 100644 --- a/python/samba/tests/dsdb_schema_attributes.py +++ b/python/samba/tests/dsdb_schema_attributes.py @@ -87,7 +87,7 @@ systemOnly: FALSE def test_AddIndexedAttribute(self): # create names for an attribute to add - (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-Attr-") + (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-IdxAttr-") ldif = self._make_attr_ldif(attr_name, attr_dn, 1, "searchFlags: %d" % samba.dsdb.SEARCH_FLAG_ATTINDEX) @@ -111,7 +111,7 @@ systemOnly: FALSE def test_AddUnIndexedAttribute(self): # create names for an attribute to add - (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-Attr-") + (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-UnIdxAttr-") ldif = self._make_attr_ldif(attr_name, attr_dn, 2) # add the new attribute @@ -134,7 +134,7 @@ systemOnly: FALSE def test_AddTwoIndexedAttributes(self): # create names for an attribute to add - (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-Attr-") + (attr_name, attr_ldap_name, attr_dn) = self._make_obj_names("schemaAttributes-2IdxAttr-") ldif = self._make_attr_ldif(attr_name, attr_dn, 3, "searchFlags: %d" % samba.dsdb.SEARCH_FLAG_ATTINDEX) -- 2.25.1