[PATCHES] rename samba_gpoupdate to gpupdate
David Mulder
dmulder at suse.com
Fri Jul 6 16:45:53 UTC 2018
Douglas, I've touched up all the issues you mentioned, and also the ones
I mentioned to you in my response.
One thing, I checked on needing check_safe_path() in gp_sec_ext.py, and
I was mistaken. It actually calls check_safe_path() on that path in the
parse function, but the sysvol path split was unnecessary (since it's
handled by check_safe_path()), so I removed that.
I've pushed everything to the same branch at
https://gitlab.com/samba-team/devel/samba/tree/david-mulder-gpo
On 07/06/2018 07:57 AM, David Mulder via samba-technical wrote:
>
> On 07/05/2018 10:58 PM, Douglas Bagnall wrote:
>> 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").
> Looks good.
>>> 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.
> I used shutil.move because it can copy across file systems, though
> simply using the same directory makes more sense.
> You have your os.rename nested to the *wrong depth* though. That rename
> should be in the scope of the with statement, otherwise you use 'f'
> after it's been destroyed.
> Also, this fixed needs to be added to the unregister_gp_extension()
> function as well (in the next commit).
>>>
>>> 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:
> Good catch.
>>>>> 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.
> All good points.
>>> 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?
> We package samba in such a way that the DC code is distinct from the
> client code. On a client without the DC package installed, that import
> would cause a crash.
>>> 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.
> Luke originally wrote the gp_ext_setter class to take the ldb arg
> always, which really doesn't make sense on anything but a samba DC. Here
> we make those two child classes which inherit from gp_ext_setter take
> that argument.
>>> 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.
> This has just been pulled from the samba-gpupdate script. Before this,
> we always initialized a samdb, and passed it down to each client side
> extension, but this doesn't make sense if we have multiple extensions,
> and only one of them is using this samdb.
> The idea is, if we don't have SamDB, or if we fail to open the db (we're
> joined as a client machine, not as a DC), then fail and don't apply the
> extension.
> I agree with the points you make though, this could be simplified.
>>
>>> 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
> This loop gets implemented in a follow up patch. It's just stubbed out
> right now because the backend isn't implemented yet (also in follow up
> code).
>> 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))
>>> +
> check_safe_path() gets added here in follow up code.
>> I think I will stop there for now, mainly because it is late on Friday
>> afternoon.
>>
>> Douglas
>>
> Douglas, I'm hoping to get the patches from merge requests 8, 10, and 15
> into 4.9. Merge request 16 can wait. You and Andrew have already
> reviewed 8 and 10, so these are likely ready to merge (and are not
> dependent on 15 or 16 to merge).
>
--
David Mulder
SUSE Labs Software Engineer - Samba
dmulder at suse.com
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
More information about the samba-technical
mailing list