[PATCHES] rename samba_gpoupdate to gpupdate
Douglas Bagnall
douglas.bagnall at catalyst.net.nz
Fri Jul 6 04:58:53 UTC 2018
These comments relate to
https://gitlab.com/samba-team/samba/merge_requests/16
which I have modified slightly and pushed back to
https://gitlab.com/samba-team/devel/samba/tree/david-mulder-gpo
David, first I will explain the small changes I have made to the patch
series, then I'll go over the remaining things I have problems with or
don't understand. So this first bit is the difference between two
branches, not the actual revised patches which you can see at the url
above.
> diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
> index c53492e2e4c..b55826ef241 100644
> --- a/python/samba/gpclass.py
> +++ b/python/samba/gpclass.py
> @@ -17,6 +17,7 @@
>
> import sys
> import os
> +import errno
> import tdb
> sys.path.insert(0, "bin/python")
> from samba import NTSTATUSError
> @@ -32,7 +33,6 @@ import samba.gpo as gpo
> from samba.param import LoadParm
> from uuid import UUID
> from tempfile import NamedTemporaryFile
> -from shutil import move as mv
>
> try:
> from enum import Enum
> @@ -365,8 +365,9 @@ def cache_gpo_dir(conn, cache, sub_dir):
> loc_sub_dir = sub_dir.upper()
> try:
> os.makedirs(os.path.join(cache, loc_sub_dir), mode=0o755)
> - except OSError:
> - pass # File Exists
> + except OSError as e:
> + if e.errno == errno.EEXIST:
> + pass # File Exists
Here it is easy and better to ignore only the particular error you
expect (and not e.g. paper over "no space left on device").
> for fdata in conn.list(sub_dir):
> if fdata['attrib'] & FILE_ATTRIBUTE_DIRECTORY:
> cache_gpo_dir(conn, cache, os.path.join(sub_dir, fdata['name']))
> @@ -477,7 +478,7 @@ def register_gp_extension(guid, name, path,
> try:
> uuid = guid[1:-1]
> UUID(uuid, version=4)
> - except:
> + except ValueError:
> return False
And similarly here (and in general) it is better to catch only the
exception you expect. BTW I think uuid.UUID() copes with curly-bracket
guids, so you don't really need the [1:-1]. Also I think
samba.misc.GUID would do it.
> lp = LoadParm()
> @@ -495,12 +496,9 @@ def register_gp_extension(guid, name, path,
> parser.set(guid, 'NoMachinePolicy', 0 if machine else 1)
> parser.set(guid, 'NoUserPolicy', 0 if user else 1)
>
> - with NamedTemporaryFile(delete=False) as f:
> - if os.path.exists(ext_conf):
> - os.remove(ext_conf)
> + with NamedTemporaryFile(delete=False, dir=os.path.dirname(ext_conf)) as f:
> parser.write(f)
> - f.close()
> - mv(f.name, ext_conf)
> + os.rename(f.name, ext_conf)
The way you had this was not atomic. The trick is to make the
replacement on the same file system (which the same directory
hopefully ensures) then rename it over the old one without deletion.
>
> return True
>
> diff --git a/python/samba/tests/gpo.py b/python/samba/tests/gpo.py
> index d66f5761fba..77a2323ad75 100644
> --- a/python/samba/tests/gpo.py
> +++ b/python/samba/tests/gpo.py
> @@ -99,6 +99,9 @@ class GPOTests(tests.TestCase):
> path = '/usr/local/samba/var/locks/sysvol/../../../../../../root/'
> self.assertRaises(OSError, check_safe_path, path)
>
> + self.assertEqual(check_safe_path('/etc/passwd'), 'etc/passwd')
> + self.assertEqual(check_safe_path('\\\\etc/\\passwd'), 'etc/passwd')
> +
These tests ensure leading slashes are removed. That is necessary
because leading slashes restart a path in os.path.join(), which these
paths get fed into. e.g:
>>> import os
>>> os.path.join('/path/to/cache/dir', '/etc/passwd')
'/etc/passwd'
Now onto some issues that I haven't fixed:
> From 7859a8be1a2d9ff04bb1c79e4126a29115152359 Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Wed, 16 May 2018 10:37:09 -0600
> Subject: [PATCH 05/26] gpo: Offline policy application via cache
>
> Read policy files from the cache, rather than
> the sysvol (unless on a dc, then read from the
> local sysvol). This enables offline policy
> apply.
>
This comment is now wrong, because you always read from the local
files?
> From 4c0788f94d30391c67db81a5a4261faba4fe918d Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Wed, 13 Jun 2018 14:45:09 -0600
> Subject: [PATCH 07/26] gpo: add register_gp_extension for registering gp
> extensions
>
> Signed-off-by: David Mulder <dmulder at suse.com>
> ---
> python/samba/gpclass.py | 37 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 37 insertions(+)
>
> diff --git a/python/samba/gpclass.py b/python/samba/gpclass.py
> index bc673a0bb23..0105f270cc2 100644
> --- a/python/samba/gpclass.py
> +++ b/python/samba/gpclass.py
> @@ -30,6 +30,9 @@ from samba.net import Net
> from samba.dcerpc import nbt
> from samba import smb
> import samba.gpo as gpo
> +from samba.param import LoadParm
> +from uuid import UUID
> +from tempfile import NamedTemporaryFile
>
> try:
> from enum import Enum
> @@ -503,3 +506,37 @@ def unapply_gp(lp, creds, test_ldb, logger, store, gp_extensions):
> gp_db.delete(str(attr_obj), attr[0])
> gp_db.commit()
>
> +def register_gp_extension(guid, name, path,
> + smb_conf=None, machine=True, user=True):
> + # Check that the module exists
> + if not os.path.exists(path):
> + return False
> + # Check for valid guid
> + if not guid[0] == '{' or not guid[-1] == '}':
> + return False
> + try:
> + uuid = guid[1:-1]
> + UUID(uuid, version=4)
> + except ValueError:
> + return False
Since you're doing this again and again...
> +
> + lp = LoadParm()
> + if smb_conf is not None:
> + lp.load(smb_conf)
> + else:
> + lp.load_default()
> + ext_conf = lp.state_path('gpext.conf')
> + parser = ConfigParser()
> + parser.read(ext_conf)
...and this, it would be nice to have helper functions.
Something like this:
def parse_gpext_conf(smb_conf):
lp = LoadParm()
if smb_conf is not None:
lp.load(smb_conf)
else:
lp.load_default()
ext_conf = lp.state_path('gpext.conf')
parser = ConfigParser()
parser.read(ext_conf)
return parser
and check_guid() and atomic_write_conf()
which would be easily unittestable.
>
> From d784ee0afc7a779c3217deac77741cb7afd909ab Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Fri, 4 May 2018 14:09:30 -0600
> Subject: [PATCH 13/26] gpo: gp_sec_ext should check whether to apply
>
> Whether an extension should apply should be
> determined by the extension, not by the
> calling script.
>
> Signed-off-by: David Mulder <dmulder at suse.com>
> ---
> python/samba/gp_sec_ext.py | 20 ++++++++++++++++++++
> python/samba/gpclass.py | 14 ++++++--------
> source4/scripting/bin/samba-gpupdate | 20 +++-----------------
> 3 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/python/samba/gp_sec_ext.py b/python/samba/gp_sec_ext.py
> index bbd385f73c6..9e1764b7fb9 100644
> --- a/python/samba/gp_sec_ext.py
> +++ b/python/samba/gp_sec_ext.py
> @@ -17,8 +17,17 @@
>
> import os.path
> from gpclass import gp_ext_setter, gp_inf_ext
> +from samba.auth import system_session
> +try:
> + from samba.samdb import SamDB
> +except:
> + SamDB = None
I don't understand under what circumstances you wouldn't have Samdb.
Can you explain that?
> class inf_to_kdc_tdb(gp_ext_setter):
> + def __init__(self, logger, gp_db, lp, attribute, val, ldb):
> + super(inf_to_kdc_tdb, self).__init__(logger, gp_db, lp, attribute, val)
> + self.ldb = ldb
> +
> def mins_to_hours(self):
> return '%d' % (int(self.val)/60)
>
> @@ -53,6 +62,10 @@ class inf_to_ldb(gp_ext_setter):
> object to update the parameter to Samba4. Not registry oriented whatsoever.
> '''
>
> + def __init__(self, logger, gp_db, lp, attribute, val, ldb):
> + super(inf_to_ldb, self).__init__(logger, gp_db, lp, attribute, val)
> + self.ldb = ldb
> +
> def ch_minPwdAge(self, val):
> old_val = self.ldb.get_minPwdAge()
> self.logger.info('KDC Minimum Password age was changed from %s to %s' \
> @@ -127,6 +140,13 @@ class gp_sec_ext(gp_inf_ext):
> return os.path.join(rootpath, "User/Registry.pol")
>
> def apply_map(self):
> + if SamDB:
> + self.ldb = SamDB(self.lp.samdb_url(),
> + session_info=system_session(),
> + credentials=self.creds,
> + lp=self.lp)
> + else:
> + return {}
and then from here I'm all a bit lost.
> From 889011b57588d80781e2d330743b1da1bc5c13fb Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Mon, 7 May 2018 10:18:00 -0600
> Subject: [PATCH 16/26] gpupdate: Don't fail with dc installed
>
> Don't fail if the dc is installed, but
> we're joined as a client.
>
> Signed-off-by: David Mulder <dmulder at suse.com>
> ---
> python/samba/gp_sec_ext.py | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/python/samba/gp_sec_ext.py b/python/samba/gp_sec_ext.py
> index 4f2092ba403..58f6fb66ecd 100644
> --- a/python/samba/gp_sec_ext.py
> +++ b/python/samba/gp_sec_ext.py
> @@ -141,10 +141,13 @@ class gp_sec_ext(gp_inf_ext):
When it comes to this I just feel things are wrong:
> def apply_map(self):
> if SamDB:
> - self.ldb = SamDB(self.lp.samdb_url(),
> - session_info=system_session(),
> - credentials=self.creds,
> - lp=self.lp)
> + try:
> + self.ldb = SamDB(self.lp.samdb_url(),
> + session_info=system_session(),
> + credentials=self.creds,
> + lp=self.lp)
> + except:
> + return {}
> else:
> return {}
> return {"System Access": {"MinimumPasswordAge": ("minPwdAge",
There are little things I can point to, like the excepts should be
more specific, and there is no need for the if/try nesting -- if SamDB
is None you'll get an exception anyway -- and the whole thing is
repeated enough times to be its own function, but I also need some
convincing about the actual goal here.
>
> From 9824bb9904856a071fa985ff3374cb82af438808 Mon Sep 17 00:00:00 2001
> From: David Mulder <dmulder at suse.com>
> Date: Wed, 9 May 2018 13:16:38 -0600
> Subject: [PATCH 21/26] gpo: Implement process_group_policy() gp_ext func
>
> MS spec describes the policy callback as a
> function called ProcessGroupPolicy which accepts
> a pDeletedGPOList and a pChangedGPOList param.
> The Group Policy Client Side Extension then
> iterates over the deleted, then the changed gpo
> lists and applies/unapplies policy. We should do
> this also.
>
> Signed-off-by: David Mulder <dmulder at suse.com>
> ---
> python/samba/gp_sec_ext.py | 35 ++++++++++++++++++-------------
> python/samba/gpclass.py | 43 ++++++++++++++++++++------------------
> @@ -168,3 +154,24 @@ class gp_sec_ext(gp_inf_ext):
> }
> }
>
> + def process_group_policy(self, deleted_gpo_list, changed_gpo_list):
> + if SamDB:
> + try:
> + ldb = SamDB(self.lp.samdb_url(),
> + session_info=system_session(),
> + credentials=self.creds,
> + lp=self.lp)
> + except:
> + return
> + else:
> + return
> + inf_file = 'MACHINE/Microsoft/Windows NT/SecEdit/GptTmpl.inf'
> + for gpo in deleted_gpo_list:
> + pass
^^^^
This loop does nothing, and the next bit looks like it could use
check_safe_path(), but I might be wrong:
> + for gpo in changed_gpo_list:
> + if gpo.file_sys_path:
> + self.gp_db.set_guid(gpo.name)
> + path = gpo.file_sys_path.split('\\sysvol\\')[-1]
> + self.parse(os.path.join(path, inf_file))
> +
I think I will stop there for now, mainly because it is late on Friday
afternoon.
Douglas
More information about the samba-technical
mailing list