The branch, master has been updated via 59b5abbe8ce gp: Test PAM Access with DENY_ALL via ca5f8072a4c gp: PAM Access should implicitly deny ALL w/ allow via 9f6cf276e22 gp: samba-tool manage gpo access add don't fail w/out upn via 8d0d79ba3b3 gp: Make samba-tool gpo manage sudoers remove backward compatible via d0c4aebb0ef gp: Test that samba-tool gpo manage removes gpme sudoers via cc0c784d3ab gp: Make samba-tool gpo manage sudoers list backward compatible via 4c2b418882e gp: Test that samba-tool gpo manage lists gpme sudoers from f03665bb7e8 s3:rpc_server: Fix include directive substitution when enumerating shares
https://git.samba.org/?p=samba.git;a=shortlog;h=master - Log ----------------------------------------------------------------- commit 59b5abbe8cec52d7cf1197a91f32d832670284d5 Author: David Mulder <dmul...@samba.org> Date: Fri Nov 18 11:42:15 2022 -0700 gp: Test PAM Access with DENY_ALL Signed-off-by: David Mulder <dmul...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> Autobuild-User(master): Jeremy Allison <j...@samba.org> Autobuild-Date(master): Mon Nov 21 22:05:01 UTC 2022 on sn-devel-184 commit ca5f8072a4c7be6fdebef494664a27bbd73340ff Author: David Mulder <dmul...@samba.org> Date: Thu Nov 17 16:33:24 2022 -0700 gp: PAM Access should implicitly deny ALL w/ allow If an allow entry is specified, the PAM Access CSE should implicitly deny ALL (everyone other than the explicit allow entries). Signed-off-by: David Mulder <dmul...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> commit 9f6cf276e22b82601a81286fabeae5303f58339c Author: David Mulder <dmul...@samba.org> Date: Thu Nov 17 12:37:20 2022 -0700 gp: samba-tool manage gpo access add don't fail w/out upn The search response for the user could possibly not include a upn (this happens with Administrator for example). Signed-off-by: David Mulder <dmul...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> commit 8d0d79ba3b317401cfe089c0df871189bb79c9dc Author: David Mulder <dmul...@samba.org> Date: Wed Nov 16 15:04:16 2022 -0700 gp: Make samba-tool gpo manage sudoers remove backward compatible Ensure `samba-tool gpo manage sudoers remove` is backward compatible with the GPME sudo rules. Signed-off-by: David Mulder <dmul...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> commit d0c4aebb0eff59716cfc51d86eec26a52f6913c5 Author: David Mulder <dmul...@samba.org> Date: Wed Nov 16 15:03:18 2022 -0700 gp: Test that samba-tool gpo manage removes gpme sudoers The file format for storing the sudo rules changed in samba-tool, but these can still be added via the GPME. We should still include them here. Signed-off-by: David Mulder <dmul...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> commit cc0c784d3ab914593356b4b1a0ca924c9dc4b9fa Author: David Mulder <dmul...@samba.org> Date: Wed Nov 16 10:46:11 2022 -0700 gp: Make samba-tool gpo manage sudoers list backward compatible Ensure `samba-tool gpo manage sudoers list` is backward compatible with the GPME sudo rules. Signed-off-by: David Mulder <dmul...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> commit 4c2b418882ecdb6293cc1d033c33685ada684c2e Author: David Mulder <dmul...@samba.org> Date: Wed Nov 16 10:44:22 2022 -0700 gp: Test that samba-tool gpo manage lists gpme sudoers The file format for storing the sudo rules changed in samba-tool, but these can still be added via the GPME. We should still include them here. Signed-off-by: David Mulder <dmul...@samba.org> Reviewed-by: Jeremy Allison <j...@samba.org> ----------------------------------------------------------------------- Summary of changes: python/samba/gp/vgp_access_ext.py | 54 +++++++++++--- python/samba/netcmd/gpo.py | 135 ++++++++++++++++++++++++----------- python/samba/tests/gpo.py | 7 +- python/samba/tests/samba_tool/gpo.py | 53 ++++++++++++++ 4 files changed, 197 insertions(+), 52 deletions(-) Changeset truncated at 500 lines: diff --git a/python/samba/gp/vgp_access_ext.py b/python/samba/gp/vgp_access_ext.py index efd91ef93fb..474d5e278ee 100644 --- a/python/samba/gp/vgp_access_ext.py +++ b/python/samba/gp/vgp_access_ext.py @@ -19,6 +19,7 @@ from samba.gp.gpclass import gp_xml_ext from hashlib import blake2b from tempfile import NamedTemporaryFile from samba.common import get_bytes +from samba.gp.util.logging import log intro = ''' ### autogenerated by samba @@ -31,11 +32,34 @@ intro = ''' ''' +# The deny all file is implicit any time an allow entry is used +DENY_BOUND = 9000000000 +DENY_FILE = '_gp_DENY_ALL.conf' + +# Each policy MUST create it's own DENY_ALL file if an allow entry exists, +# otherwise policies will conflict and one could remove a DENY_ALL when another +# one still requires it. +def deny_file(access): + deny_filename = os.path.join(access, + '%d%s' % (select_next_deny(access), DENY_FILE)) + with NamedTemporaryFile(delete=False, dir=access) as f: + with open(f.name, 'w') as w: + w.write(intro) + w.write('-:ALL:ALL') + os.chmod(f.name, 0o644) + os.rename(f.name, deny_filename) + return deny_filename + +def select_next_deny(directory): + configs = [re.match(r'(\d+)', f) for f in os.listdir(directory) if DENY_FILE in f] + return max([int(m.group(1)) for m in configs if m]+[DENY_BOUND])+1 + # Access files in /etc/security/access.d are read in the order of the system # locale. Here we number the conf files to ensure they are read in the correct -# order. +# order. Ignore the deny file, since allow entries should always come before +# the implicit deny ALL. def select_next_conf(directory): - configs = [re.match(r'(\d+)', f) for f in os.listdir(directory)] + configs = [re.match(r'(\d+)', f) for f in os.listdir(directory) if DENY_FILE not in f] return max([int(m.group(1)) for m in configs if m]+[0])+1 class vgp_access_ext(gp_xml_ext): @@ -47,9 +71,10 @@ class vgp_access_ext(gp_xml_ext): for guid, settings in deleted_gpo_list: self.gp_db.set_guid(guid) if str(self) in settings: - for attribute, access_file in settings[str(self)].items(): - if os.path.exists(access_file): - os.unlink(access_file) + for attribute, policy_files in settings[str(self)].items(): + for access_file in policy_files.split(':'): + if os.path.exists(access_file): + os.unlink(access_file) self.gp_db.delete(str(self), attribute) self.gp_db.commit() @@ -63,14 +88,20 @@ class vgp_access_ext(gp_xml_ext): path = os.path.join(gpo.file_sys_path, deny) deny_conf = self.parse(path) entries = [] + policy_files = [] if allow_conf: policy = allow_conf.find('policysetting') data = policy.find('data') - for listelement in data.findall('listelement'): + allow_listelements = data.findall('listelement') + for listelement in allow_listelements: adobject = listelement.find('adobject') name = adobject.find('name').text domain = adobject.find('domain').text entries.append('+:%s\\%s:ALL' % (domain, name)) + if len(allow_listelements) > 0: + log.info('Adding an implicit deny ALL because an allow' + ' entry is present') + policy_files.append(deny_file(access)) if deny_conf: policy = deny_conf.find('policysetting') data = policy.find('data') @@ -79,10 +110,14 @@ class vgp_access_ext(gp_xml_ext): name = adobject.find('name').text domain = adobject.find('domain').text entries.append('-:%s\\%s:ALL' % (domain, name)) + if len(allow_listelements) > 0: + log.warn("Deny entry '%s' is meaningless with " + "allow present" % entries[-1]) if len(entries) == 0: continue conf_id = select_next_conf(access) access_file = os.path.join(access, '%010d_gp.conf' % conf_id) + policy_files.append(access_file) access_contents = '\n'.join(entries) attribute = blake2b(get_bytes(access_contents)).hexdigest() old_val = self.gp_db.retrieve(str(self), attribute) @@ -96,7 +131,7 @@ class vgp_access_ext(gp_xml_ext): w.write(access_contents) os.chmod(f.name, 0o644) os.rename(f.name, access_file) - self.gp_db.store(str(self), attribute, access_file) + self.gp_db.store(str(self), attribute, ':'.join(policy_files)) self.gp_db.commit() def rsop(self, gpo): @@ -113,13 +148,16 @@ class vgp_access_ext(gp_xml_ext): if allow_conf: policy = allow_conf.find('policysetting') data = policy.find('data') - for listelement in data.findall('listelement'): + allow_listelements = data.findall('listelement') + for listelement in allow_listelements: adobject = listelement.find('adobject') name = adobject.find('name').text domain = adobject.find('domain').text if str(self) not in output.keys(): output[str(self)] = [] output[str(self)].append('+:%s\\%s:ALL' % (name, domain)) + if len(allow_listelements) > 0: + output[str(self)].append('-:ALL:ALL') if deny_conf: policy = deny_conf.find('policysetting') data = policy.find('data') diff --git a/python/samba/netcmd/gpo.py b/python/samba/netcmd/gpo.py index eefce9d8d0e..9cd08273aaa 100644 --- a/python/samba/netcmd/gpo.py +++ b/python/samba/netcmd/gpo.py @@ -1847,6 +1847,42 @@ samba-tool gpo manage sudoers list {31B2F340-016D-11D2-945F-00C04FB984F9} 'SudoersConfiguration\\manifest.xml']) try: xml_data = ET.fromstring(conn.loadfile(vgp_xml)) + except NTSTATUSError as e: + # STATUS_OBJECT_NAME_INVALID, STATUS_OBJECT_NAME_NOT_FOUND, + # STATUS_OBJECT_PATH_NOT_FOUND + if e.args[0] in [0xC0000033, 0xC0000034, 0xC000003A]: + # The file doesn't exist, so there is nothing to list + xml_data = None + elif e.args[0] == 0xC0000022: # STATUS_ACCESS_DENIED + raise CommandError("The authenticated user does " + "not have sufficient privileges") + else: + raise + + if xml_data is not None: + policy = xml_data.find('policysetting') + data = policy.find('data') + for entry in data.findall('sudoers_entry'): + command = entry.find('command').text + user = entry.find('user').text + listelements = entry.findall('listelement') + principals = [] + for listelement in listelements: + principals.extend(listelement.findall('principal')) + if len(principals) > 0: + uname = ','.join([u.text if u.attrib['type'] == 'user' \ + else '%s%%' % u.text for u in principals]) + else: + uname = 'ALL' + nopassword = entry.find('password') is None + np_entry = ' NOPASSWD:' if nopassword else '' + p = '%s ALL=(%s)%s %s' % (uname, user, np_entry, command) + self.outf.write('%s\n' % p) + + pol_file = '\\'.join([realm.lower(), 'Policies', gpo, + 'MACHINE\\Registry.pol']) + try: + pol_data = ndr_unpack(preg.file, conn.loadfile(pol_file)) except NTSTATUSError as e: # STATUS_OBJECT_NAME_INVALID, STATUS_OBJECT_NAME_NOT_FOUND, # STATUS_OBJECT_PATH_NOT_FOUND @@ -1857,24 +1893,12 @@ samba-tool gpo manage sudoers list {31B2F340-016D-11D2-945F-00C04FB984F9} "not have sufficient privileges") raise - policy = xml_data.find('policysetting') - data = policy.find('data') - for entry in data.findall('sudoers_entry'): - command = entry.find('command').text - user = entry.find('user').text - listelements = entry.findall('listelement') - principals = [] - for listelement in listelements: - principals.extend(listelement.findall('principal')) - if len(principals) > 0: - uname = ','.join([u.text if u.attrib['type'] == 'user' \ - else '%s%%' % u.text for u in principals]) - else: - uname = 'ALL' - nopassword = entry.find('password') is None - np_entry = ' NOPASSWD:' if nopassword else '' - p = '%s ALL=(%s)%s %s' % (uname, user, np_entry, command) - self.outf.write('%s\n' % p) + # Also list the policies set from the GPME + keyname = b'Software\\Policies\\Samba\\Unix Settings\\Sudo Rights' + for entry in pol_data.entries: + if get_bytes(entry.keyname) == keyname and \ + get_string(entry.data).strip(): + self.outf.write('%s\n' % entry.data) class cmd_remove_sudoers(Command): """Removes a Samba Sudoers Group Policy from the sysvol @@ -1931,14 +1955,30 @@ samba-tool gpo manage sudoers remove {31B2F340-016D-11D2-945F-00C04FB984F9} 'fak # STATUS_OBJECT_NAME_INVALID, STATUS_OBJECT_NAME_NOT_FOUND, # STATUS_OBJECT_PATH_NOT_FOUND if e.args[0] in [0xC0000033, 0xC0000034, 0xC000003A]: - raise CommandError("The specified entry does not exist") + data = None elif e.args[0] == 0xC0000022: # STATUS_ACCESS_DENIED raise CommandError("The authenticated user does " "not have sufficient privileges") - raise + else: + raise + + pol_file = '\\'.join([realm.lower(), 'Policies', gpo, + 'MACHINE\\Registry.pol']) + try: + pol_data = ndr_unpack(preg.file, conn.loadfile(pol_file)) + except NTSTATUSError as e: + # STATUS_OBJECT_NAME_INVALID, STATUS_OBJECT_NAME_NOT_FOUND, + # STATUS_OBJECT_PATH_NOT_FOUND + if e.args[0] in [0xC0000033, 0xC0000034, 0xC000003A]: + pol_data = None + elif e.args[0] == 0xC0000022: # STATUS_ACCESS_DENIED + raise CommandError("The authenticated user does " + "not have sufficient privileges") + else: + raise entries = {} - for e in data.findall('sudoers_entry'): + for e in data.findall('sudoers_entry') if data else []: command = e.find('command').text user = e.find('user').text listelements = e.findall('listelement') @@ -1955,23 +1995,35 @@ samba-tool gpo manage sudoers remove {31B2F340-016D-11D2-945F-00C04FB984F9} 'fak p = '%s ALL=(%s)%s %s' % (uname, user, np_entry, command) entries[p] = e - if entry not in entries.keys(): - raise CommandError("Cannot remove '%s' because it does not exist" % - entry) + if entry in entries.keys(): + data.remove(entries[entry]) - data.remove(entries[entry]) + out = BytesIO() + xml_data.write(out, encoding='UTF-8', xml_declaration=True) + out.seek(0) + try: + create_directory_hier(conn, vgp_dir) + conn.savefile(vgp_xml, out.read()) + except NTSTATUSError as e: + if e.args[0] == 0xC0000022: # STATUS_ACCESS_DENIED + raise CommandError("The authenticated user does " + "not have sufficient privileges") + raise + elif entry in ([e.data for e in pol_data.entries] if pol_data else []): + entries = [e for e in pol_data.entries if e.data != entry] + pol_data.num_entries = len(entries) + pol_data.entries = entries - out = BytesIO() - xml_data.write(out, encoding='UTF-8', xml_declaration=True) - out.seek(0) - try: - create_directory_hier(conn, vgp_dir) - conn.savefile(vgp_xml, out.read()) - except NTSTATUSError as e: - if e.args[0] == 0xC0000022: # STATUS_ACCESS_DENIED - raise CommandError("The authenticated user does " - "not have sufficient privileges") - raise + try: + conn.savefile(pol_file, ndr_pack(pol_data)) + except NTSTATUSError as e: + if e.args[0] == 0xC0000022: # STATUS_ACCESS_DENIED + raise CommandError("The authenticated user does " + "not have sufficient privileges") + raise + else: + raise CommandError("Cannot remove '%s' because it does not exist" % + entry) class cmd_sudoers(SuperCommand): """Manage Sudoers Group Policy Objects""" @@ -3763,7 +3815,8 @@ class cmd_add_access(Command): """Adds a VGP Host Access Group Policy to the sysvol This command adds a host access setting to the sysvol for applying to winbind -clients. +clients. Any time an allow entry is detected by the client, an implicit deny +ALL will be assumed. Example: samba-tool gpo manage access add {31B2F340-016D-11D2-945F-00C04FB984F9} allow goodguy example.com @@ -3864,13 +3917,11 @@ samba-tool gpo manage access add {31B2F340-016D-11D2-945F-00C04FB984F9} allow go etype = ET.SubElement(listelement, 'type') etype.text = objectclass.upper() entry = ET.SubElement(listelement, 'entry') - if objectclass == 'user': - entry.text = get_string(res[0]['userPrincipalName'][-1]) - else: + entry.text = '%s\\%s' % (samdb.domain_netbios_name(), + get_string(res[0]['samaccountname'][-1])) + if objectclass == 'group': groupattr = ET.SubElement(data, 'groupattr') groupattr.text = 'samAccountName' - entry.text = '%s\\%s' % (domain, - get_string(res[0]['samaccountname'][-1])) adobject = ET.SubElement(listelement, 'adobject') name = ET.SubElement(adobject, 'name') name.text = get_string(res[0]['samaccountname'][-1]) diff --git a/python/samba/tests/gpo.py b/python/samba/tests/gpo.py index 7ba193884a1..9873f13c1e3 100644 --- a/python/samba/tests/gpo.py +++ b/python/samba/tests/gpo.py @@ -8543,8 +8543,11 @@ class GPOTests(tests.TestCase): with TemporaryDirectory() as dname: ext.process_group_policy([], gpos, dname) conf = os.listdir(dname) - self.assertEquals(len(conf), 1, 'The conf file was not created') - gp_cfg = os.path.join(dname, conf[0]) + # There will be 2 files, the policy file and the deny file + self.assertEquals(len(conf), 2, 'The conf file was not created') + # Ignore the DENY_ALL conf file + gp_cfg = os.path.join(dname, + [c for c in conf if '_gp_DENY_ALL.conf' not in c][0]) # Check the access config for the correct access.conf entries print('Config file %s found' % gp_cfg) diff --git a/python/samba/tests/samba_tool/gpo.py b/python/samba/tests/samba_tool/gpo.py index d60e5b96c34..78ed5d493af 100644 --- a/python/samba/tests/samba_tool/gpo.py +++ b/python/samba/tests/samba_tool/gpo.py @@ -730,6 +730,24 @@ class GpoCmdTestCase(SambaToolCmdTest): self.assertFalse(inf_data.has_section('Kerberos Policy')) def test_sudoers_add(self): + lp = LoadParm() + lp.load(os.environ['SERVERCONFFILE']) + local_path = lp.get('path', 'sysvol') + reg_pol = os.path.join(local_path, lp.get('realm').lower(), 'Policies', + self.gpo_guid, 'Machine/Registry.pol') + + # Stage the Registry.pol file with test data + stage = preg.file() + e = preg.entry() + e.keyname = b'Software\\Policies\\Samba\\Unix Settings\\Sudo Rights' + e.valuename = b'Software\\Policies\\Samba\\Unix Settings' + e.type = 1 + e.data = b'fakeu ALL=(ALL) NOPASSWD: ALL' + stage.num_entries = 1 + stage.entries = [e] + ret = stage_file(reg_pol, ndr_pack(stage)) + self.assertTrue(ret, 'Could not create the target %s' % reg_pol) + (result, out, err) = self.runsublevelcmd("gpo", ("manage", "sudoers", "add"), self.gpo_guid, 'ALL', 'ALL', @@ -751,6 +769,7 @@ class GpoCmdTestCase(SambaToolCmdTest): (os.environ["USERNAME"], os.environ["PASSWORD"])) self.assertIn(sudoer, out, 'The test entry was not found!') + self.assertIn(get_string(e.data), out, 'The test entry was not found!') (result, out, err) = self.runsublevelcmd("gpo", ("manage", "sudoers", "remove"), @@ -762,6 +781,17 @@ class GpoCmdTestCase(SambaToolCmdTest): os.environ["PASSWORD"])) self.assertCmdSuccess(result, out, err, 'Sudoers remove failed') + (result, out, err) = self.runsublevelcmd("gpo", ("manage", + "sudoers", "remove"), + self.gpo_guid, + get_string(e.data), + "-H", "ldap://%s" % + os.environ["SERVER"], + "-U%s%%%s" % + (os.environ["USERNAME"], + os.environ["PASSWORD"])) + self.assertCmdSuccess(result, out, err, 'Sudoers remove failed') + (result, out, err) = self.runsublevelcmd("gpo", ("manage", "sudoers", "list"), self.gpo_guid, "-H", @@ -771,6 +801,11 @@ class GpoCmdTestCase(SambaToolCmdTest): (os.environ["USERNAME"], os.environ["PASSWORD"])) self.assertNotIn(sudoer, out, 'The test entry was still found!') + self.assertNotIn(get_string(e.data), out, + 'The test entry was still found!') + + # Unstage the Registry.pol file + unstage_file(reg_pol) def test_sudoers_list(self): lp = LoadParm() @@ -825,6 +860,21 @@ class GpoCmdTestCase(SambaToolCmdTest): ret = stage_file(vgp_xml, etree.tostring(stage, 'utf-8')) self.assertTrue(ret, 'Could not create the target %s' % vgp_xml) + reg_pol = os.path.join(local_path, lp.get('realm').lower(), 'Policies', + self.gpo_guid, 'Machine/Registry.pol') + + # Stage the Registry.pol file with test data + stage = preg.file() + e = preg.entry() + e.keyname = b'Software\\Policies\\Samba\\Unix Settings\\Sudo Rights' + e.valuename = b'Software\\Policies\\Samba\\Unix Settings' + e.type = 1 + e.data = b'fakeu3 ALL=(ALL) NOPASSWD: ALL' + stage.num_entries = 1 + stage.entries = [e] + ret = stage_file(reg_pol, ndr_pack(stage)) + self.assertTrue(ret, 'Could not create the target %s' % reg_pol) + sudoer = 'fakeu ALL=(ALL) NOPASSWD: ALL' sudoer2 = 'fakeu2,fakeg2% ALL=(ALL) NOPASSWD: ALL' sudoer_no_principal = 'ALL ALL=(ALL) NOPASSWD: ALL' @@ -839,6 +889,7 @@ class GpoCmdTestCase(SambaToolCmdTest): self.assertCmdSuccess(result, out, err, 'Sudoers list failed') self.assertIn(sudoer, out, 'The test entry was not found!') self.assertIn(sudoer2, out, 'The test entry was not found!') + self.assertIn(get_string(e.data), out, 'The test entry was not found!') self.assertIn(sudoer_no_principal, out, 'The test entry was not found!') @@ -877,6 +928,8 @@ class GpoCmdTestCase(SambaToolCmdTest): # Unstage the manifest.xml file unstage_file(vgp_xml) + # Unstage the Registry.pol file + unstage_file(reg_pol) def test_symlink_list(self): lp = LoadParm() -- Samba Shared Repository